ASoC: soc-core: filter use of 'ignore_machine' field

[plbossart]

The 'ignore_machine' field is currently used to ignore all FE dailinks
statically added by the machine drivers, as well as override the
fixups for the BE dailinks. The motivation for this field was
primarily to reuse the same machine driver on Intel devices, both with
legacy and SOF-based platform drivers.

SOF is now used on Mediatek platforms, where the same card uses
SOF-based dailinks to deal with DSP-managed streams, as well as
'regular' dailinks. The 'ignore_machine' field set by the core SOF
platform driver is too strong, with dailinks not managed by SOF being
modified.

This patch adds a stricter filtering so that only dailinks managed by
a topology-based SOF driver are modified.

Reported-by: YC Hung yc.hung@mediatek.com
Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com

[plbossart]

completely untested proof-of-concept as an alternative to PR #3217

[yaochunhung]

@plbossart Thanks for providing this PR. I tried this patch. It can skip and won't modify regular dailinks but I found sof-based dailinks, dai_link->platform->of_node will be set then it will show "ASoC : Neither/both platform name/of_node are set for xxx, platform yyy" in the so_dai_link_sanity_check function. Do you have any suggestion to avoid this? Thanks!

[plbossart]

@plbossart Thanks for providing this PR. I tried this patch. It can skip and won't modify regular dailinks

Thanks for testing, that's good, I had no idea if it even worked ;-)

but I found sof-based dailinks, dai_link->platform->of_node will be set then it will show "ASoC : Neither/both platform name/of_node are set for xxx, platform yyy" in the so_dai_link_sanity_check function. Do you have any suggestion to avoid this? Thanks!

I don't have a clue unfortunately, but this code was written by @dbaluta so that's something to be discussed between DeviceTree users:

d971400339483 (Daniel Baluta         2021-04-14 13:12:12 +0300 1805)                    if (component->dev->of_node)
d971400339483 (Daniel Baluta         2021-04-14 13:12:12 +0300 1806)                            dai_link->platforms->of_node = component->dev->of_node;
d971400339483 (Daniel Baluta         2021-04-14 13:12:12 +0300 1807)                    else
d971400339483 (Daniel Baluta         2021-04-14 13:12:12 +0300 1808)                            dai_link->platforms->name = component->name;

[yaochunhung]

@dbaluta @plbossart
After reverting this patch below to "dai_link->platforms->name = component->name;" and the sound card can be registered well. @dbaluta Do you have any clue or suggestion for it? Thanks.

d971400339483 (Daniel Baluta         2021-04-14 13:12:12 +0300 1805)                    if (component->dev->of_node)
d971400339483 (Daniel Baluta         2021-04-14 13:12:12 +0300 1806)                            dai_link->platforms->of_node = component->dev->of_node;
d971400339483 (Daniel Baluta         2021-04-14 13:12:12 +0300 1807)                    else
d971400339483 (Daniel Baluta         2021-04-14 13:12:12 +0300 1808)                            dai_link->platforms->name = component->name;

[dbaluta]

@plbossart Thanks for providing this PR. I tried this patch. It can skip and won't modify regular dailinks but I found sof-based dailinks, dai_link->platform->of_node will be set then it will show "ASoC : Neither/both platform name/of_node are set for xxx, platform yyy" in the so_dai_link_sanity_check function. Do you have any suggestion to avoid this? Thanks!

@yaochunhung I got the exact error so for this reason I've added d971400. In your case it means that you have dai_link->platforms->name already set and with d971400 dai_link->plaforms->of_node is also set which upsets SOC core.
I would check who sets dai_link->platforms->name for offending link. Also, understand if that is a regular link and shouldn't have got past the condition introduced by @plbossart in this PR.

[plbossart]

@yaochunhung any comments on @dbaluta's feedback? should we proceed with my change?

[yaochunhung]

@plbossart @dbaluta I check dai_link->platforms->name will be set in sof_link_load function.

	link->platforms->name = dev_name(scomp->dev);

Not sure if it is ok to add additional condition check as below,

	if (component->dev->of_node && !dai_link->platforms->name)
		dai_link->platforms->of_node = component->dev->of_node;
	else
		dai_link->platforms->name = component->name;

[dbaluta]

@plbossart I think your idea it's good.

One existing problem I noticed is when using the same machine driver to create two sound cards. One for a normal sound card and one for a SOF-sound card.

Here is pseudocode for soc_check_tplg_fes:

soc_check_tplg_fes
    for_each_component
          if component->driver->ignore_machine != card->dev->driver_name
              return;
          for_each_card_prelinks
               override platform component
          # here is the important part
          update card name with topology short name

So, this function will change card name no matter what! (normal card or SOF card).

Here is an update to your patch which avoids this problem:

@ -1777,6 +1780,9 @@ static void soc_check_tplg_fes(struct snd_soc_card *card)
        struct snd_soc_component *component;
        const struct snd_soc_component_driver *comp_drv;
        struct snd_soc_dai_link *dai_link;
+       struct snd_soc_dai_link_component *dlc;
+       struct snd_soc_dai *dai;
+       bool update_card_name;
        int i;
 
        for_each_component(component) {
@@ -1792,9 +1798,23 @@ static void soc_check_tplg_fes(struct snd_soc_card *card)
                if (strcmp(component->driver->ignore_machine,
                           dev_name(card->dev)))
                        continue;
+               update_card_name = false;
 match:
                /* machine matches, so override the rtd data */
                for_each_card_prelinks(card, i, dai_link) {
+                       /*
+                        * ignore dailinks exposed by other components, with the
+                        * assumption that all cpu_dais are exposed by the same
+                        * component
+                        */
+                       dlc = asoc_link_to_cpu(dai_link, 0);
+                       dai = snd_soc_find_dai(dlc);
+
+
+                       if (!dai || dai->component != component)
+                               continue;
+
+                       update_card_name = true;
 
                        /* ignore this FE */
                        if (dai_link->dynamic) {
@@ -1854,7 +1874,7 @@ static void soc_check_tplg_fes(struct snd_soc_card *card)
                }
 
                /* Inform userspace we are using alternate topology */
-               if (component->driver->topology_name_prefix) {
+               if (update_card_name && component->driver->topology_name_prefix) {
 
                        /* topology shortname created? */
                        if (!card->topology_shortname_created) {

[plbossart]

@dbaluta I didn't think about the card name at all, but I really wonder what we are supposed to do if there are multiple components used by a card. Clearly it makes no sense to pick the topology prefix from one component or the other. The result would anyway dependent on the order in which components are handled, a complete nightmare.

Put differently, I am not sure if the changes you suggested above would work for the 2 component case.

[dbaluta]

@plbossart indeed i never thought about two components. I'm not sure the initial code was designed with that in mind. Put differently, the code has the same problem w or w/o my changes but at least it doesn't modify the card name for components not intended to be used by that card.

Later edit: I think that when using multiple components they'll need to have some topology prefix.

[yaochunhung]

@plbossart @dbaluta
When I set the SOF BE dai link platform name as empty and just assign of_node to them. This patch will work and not fail in the soc_dai_link_sanity_check function. I think this PR could be merged. Thanks!

[dbaluta]

@plbossart i also think we can go with your patch and fix the card name change later. because this problem exists w or w/o your patch.

[plbossart]

@plbossart i also think we can go with your patch and fix the card name change later. because this problem exists w or w/o your patch.

ok, then let's record your review approval and merge - standard operating procedure :-)

[marc-hb]

Why was this merged with (old and) mostly failing tests? https://sof-ci.01.org/linuxpr/PR3236/build6619/devicetest/

Revert submitted in #3294

[dbaluta]

@marc-hb my fault. for some reason i thought that those were normal to fail

[yaochunhung]

@plbossart Do you have any suggestion about how to fix these testing? Thanks.

[plbossart]

@plbossart Do you have any suggestion about how to fix these testing? Thanks.

Been busy with other things, will try to look into this later this week.

[marc-hb]

@plbossart Do you have any suggestion about how to fix these testing? Thanks.

BTW https://sof-ci.01.org/linuxpr/PR3236/build6619/devicetest/ had many test failures across many platforms, so it should be easy to reproduce at least some failures and start debugging to help. sof-test is entirely open-source and if anything is missing for failure reproduction then we'll make sure it gets fixed.

[yaochunhung]

@plbossart Sorry to bother again. Do you have any idea about the testing fails? Thanks.
Is the conditional checking of !dai required here?

+                       if (!dai || dai->component != component)
+                               continue;

Should it be

+                       if (!dai && dai->component != component)
+                               continue;

[cujomalainey]

@yaochunhung i can hopefully take a look later this week

[cujomalainey]

Seems like this is something to do with SDW skus, was able to repro on one of my TGL devices with soundwire

[yaochunhung]

@cujomalainey Please see the comment #3352 (comment) thanks.