fixup! ASoC: SOF: Intel: hda: make sure DAI widget is set up before IPC

[ranj063]

Applications can free a PCM stream after system suspend without
restarting them. In this scenario, the widget should be prevented
from getting freed for static pipeline widgets and for dynamic
widgets the use_count should not be allowed to be decremented
below 0.

Fixes #2875

[bardliao]

Sorry, I didn't get the logic. How do we know the PCM stream is freed without being restarted? And why we should prevent the use_count from being decremented? Will this prevent widget from being freed in normal cases?

[ranj063]

this is what pulse audio does when the system is suspended during probe. It frees the streams that it opened before suspend without ever restarting them. If the use_count is 0 (or 1 for static widgets), we should not decrement it. The only reason you'd ever hit this is if you have a hw_free without hw_params and in that case, we should do nothing.

[bardliao]

My point is that when will we free the widget? I assume that use_count will be decreased when sof_widget_free is called for any reason. So ideally the use_count will be 4->3->2->1->0 and we will free the widget when it is 0. But with this change, we will do nothing when the use_count is 0 (or 1 for static widgets). Let's take static widgets for example, the use_count will be 4->3->2->1->1->1..., right? Maybe I am wrong but I am confused here.

[ranj063]

For static widgets, you'd never free the widget and thats why they are called static. For dynamic widgets, you will free them when the current use_count is 1 when sof_widget_free() is called

[bardliao]

For static widgets, you'd never free the widget and thats why they are called static. For dynamic widgets, you will free them when the current use_count is 1 when sof_widget_free() is called

Thanks @ranj063 for the explanation. It answered my question.

[lyakh]

I understand the motivation, but this makes me a bit suspicious: use-counts are usually used to trace users and uses and sometimes to verify consistency, but less often as an indication of the system state. They are more "mostly-write" than "mostly-read" if it makes sense. In a way they should stay consistent because the flow is correct, not the flow is corrected to keep them consistent. So it would be better (if possible) here not to use use_count for decision making. Rather some other indication of state.

[ranj063]

@lyakh I gave this a lot of thought but unfortunately the PCM state is the same during hw_free() irrespective of whether it is called after a normal stream stop or after resuming from suspend. The FE hw_free() checks the spcm->prepared[dir] flag during hw_free() to avoid this problem. Its the BE hw_free() that's problematic. Maybe what I can do is introduce a prepapred flag in snd_sof_dai that can be used to check if hw_free() should do anything or not. stay tuned!

[plbossart]

comments below. I agree with @bardliao and @lyakh that this PR isn't completely clear.

[plbossart]

maybe a side question, but sof_widget_free() is also used from intel/hda.c

int hda_ctrl_dai_widget_setup(struct snd_soc_dapm_widget *w, bool setup)
{
	struct snd_sof_widget *swidget = w->dobj.private;
	struct snd_soc_component *component = swidget->scomp;
	struct snd_sof_dev *sdev = snd_soc_component_get_drvdata(component);
	struct sof_ipc_dai_config *config;
	struct snd_sof_dai *sof_dai;
	struct sof_ipc_reply reply;
	int ret;

	sof_dai = swidget->private;

	if (!sof_dai || !sof_dai->dai_config) {
		dev_err(sdev->dev, "No config for DAI %s\n", w->name);
		return -EINVAL;
	}

	config = &sof_dai->dai_config[sof_dai->current_config];

	/*
	 * For static pipelines, the DAI widget would already be set up and calling
	 * sof_widget_setup()/free() simply returns without doing anything.
	 * For dynamic pipelines, the DAI widget will be set up or freed here.
	 */
	if (setup) {
		ret = sof_widget_setup(sdev, swidget);
		if (ret < 0) {
			dev_err(sdev->dev, "error: setting up DAI widget %s\n", w->name);
			return ret;
		}

		/* send DAI_CONFIG IPC */
		return sof_ipc_tx_message(sdev->ipc, config->hdr.cmd, config, config->hdr.size,
					  &reply, sizeof(reply));
	}

	ret = sof_ipc_tx_message(sdev->ipc, config->hdr.cmd, config, config->hdr.size,
				  &reply, sizeof(reply));
	if (ret < 0) {
		dev_err(sdev->dev, "error: updating DAI config for %s\n", w->name);
		return ret;
	}

	return sof_widget_free(sdev, swidget);
}

would the change impact the HDaudio DAI config?

[plbossart]

I personally don't get why the use count is different for dynamic and static pipelines. A comment would not hurt...

[plbossart]

can we just use if (!swidget->use_count). That would prevent the counter from going into negative values in corner cases.

[ranj063]

@plbossart @lyakh @bardliao I've updated this PR now with a new flag for BE hw_params() to mirror what we do in FE hw_params(). Hopefully this is not as controversial as checking the widget use_count.

[kv2019i]

This version looks better. Conditional code around refcount is better avoided. I'm not sure if the implementation is correct though, see inline.

[kv2019i]

Thanks, looks good now. Even better if you update to have unique error messages (Pierre's comment).

[ranj063]

Thanks, looks good now. Even better if you update to have unique error messages (Pierre's comment).

wish granted. Thanks!

[lyakh]

It isn't a huge problem for me, but I think some layering purists would frown on this: a parameter is set to true at one level (in HDA) and to false at a different level, is there no way to do this all at the SOF core level? Not a huge deal for me, but if possible - that would look nicer

[plbossart]

LGTM, much clearer indeed. Thanks @ranj063

[lyakh]

Nice, I think it looks better now! A minor comment can be addressed later.

[lyakh]

this error message is now identical to the one in hda_ctrl_dai_widget_setup(). Also I know that in some cases / files we prefix error messages with "error" in others we don't. Which one is this? :-) This message has no "error," but the one below has one... And in fact there's at least one more "no config for DAI" error case in sdw_dai_config_ipc() below and that one has an "error" word in it. So, it could be a separate patch / PR to improve that.

[kv2019i]

Uh, as this is already a fixup, let's just get this right in this PR. I'm not fan of the additional "error: " prefix, but that's what we do now, so let's stay aligned.

And for the message, maybe change it to "No config to free DAI %s\n". Then it's unique from the setup one.

[plbossart]

holding on the merge, let's give @kv2019i and @bardliao a chance to review.

[plbossart]

@kv2019i can you take a look and merge this if you approve?

[kv2019i]

This is a bit nitpicking, but let's try to avoid fixups of fixups :)

[kv2019i]

Uh, as this is already a fixup, let's just get this right in this PR. I'm not fan of the additional "error: " prefix, but that's what we do now, so let's stay aligned.

And for the message, maybe change it to "No config to free DAI %s\n". Then it's unique from the setup one.

[ranj063]

This is a bit nitpicking, but let's try to avoid fixups of fixups :)

@kv2019i @lyakh fixed the error message now.

[kv2019i]

Thanks @ranj063 !