ASoC: SOF: Intel: test w before processing the params_stream callback

[bardliao]

The DAI widget is set by the topology. And it will not be set if the widget is not defined in the topology. The case happens if the topology doesn't provide enough DAI widgets of the amp in the amp aggregation case.
Test w to report the error and avoid kernel NULL pointer dereference.

[ujfalusi]

there is no way to catch this during probe?

[bardliao]

I don't think we can catch this during probe since we need substream->stream. What do you think? @ranj063

[ranj063]

There's one possibility of making sure that all cpu DAI's associated with all runtimes in the card have their respective stream widgets set. What we can do is in the topology complete op, we can iterate though all of the rtd's and check if they're set and fail if they aren't. Worth a try. Something like this in sof_complete()

struct snd_soc_card *card = scomp->card;
list_for_each_entry(rtd, &card->rtd_list, list)
		for_each_rtd_cpu_dais(rtd, i, cpu_dai)
			if (!snd_soc_dai_get_widget(cpu_dai, stream))
                              return -EINVAL;

A CPU DAI must have atleast the playback/capture widget set. so you'd have to check both directions above

[bardliao]

Right, I can add

        list_for_each_entry(rtd, &card->rtd_list, list)
                for_each_rtd_cpu_dais(rtd, i, cpu_dai)
                        if (!snd_soc_dai_get_widget(cpu_dai, SNDRV_PCM_STREAM_PLAYBACK) &&
                            !snd_soc_dai_get_widget(cpu_dai, SNDRV_PCM_STREAM_CAPTURE)) {
                                dev_err(sdev->dev, "no associated widgets for dai %s\n",
                                        cpu_dai->name);
                                return -EINVAL;
                        }

in sof_complete().
However, it will not return error if a dai has both directions and one direction isn't associated with a widget.

[ranj063]

so if we know what directions the CPU DAI has set, can we not check both or either based on that?

[bardliao]

The main issue is that the dai links may not always be used by the topology. I.e. topology could be a subset of the dai links. In that case the cpu dai will not associate with any widget and it is still valid.

[ranj063]

fair enough. this change looks good then

[ujfalusi]

@bardliao, the w is unused here, do we really need this check added to this specific function?

[bardliao]

Checking here is to be consistent as sdw_params_stream(). Both sdw_params_stream() and sdw_ace2x_params_stream() are .params_stream callback. We can also check in sdw_hda_dai_hw_params(). What do you prefer?

[ujfalusi]

imho you should check for NULL pointer where it can cause issues. You will catch this problem anyways, but if you insist of doing it here, then do a simple

if (!snd_soc_dai_get_widget(params_data->dai, params_data->substream->stream)) {
}

perhaps as forward looking check, but again, in this function the w is not used, so why check?

[ujfalusi]

I see, so yes, you should check in sdw_hda_dai_hw_params(), not here.

[bardliao]

Updated, thanks.

[lgirdwood]

Can we say in the commit message why aggregated amps are special here so that upstream reviewers understand why we have to be more cautious. Thanks !

[bardliao]

Can we say in the commit message why aggregated amps are special here so that upstream reviewers understand why we have to be more cautious. Thanks !

Commit message updated.