| Message ID | 20240304190536.1783332-3-cezary.rojewski@intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | [1/5] ASoC: Intel: Disable route checks for Skylake boards | expand |
On 2024-03-04 10:25 PM, Pierre-Louis Bossart wrote: > On 3/4/24 14:50, Cezary Rojewski wrote: >> On 2024-03-04 8:32 PM, Pierre-Louis Bossart wrote: >>> On 3/4/24 13:05, Cezary Rojewski wrote: >>>> One of the framework responsibilities is to ensure that the enumerated >>>> DPCMs are valid i.e.: a valid BE is connected to a valid FE DAI. While >>>> the are checks in soc-core.c and soc-pcm.c that verify this, a component >>>> driver may attempt to workaround this by loading an invalid graph >>>> through the topology file. >>>> >>>> Be strict and fail topology loading when invalid graph is encountered. >>> >>> This is very invasive, it's perfectly possible that we have a number of >>> 'broken' topologies where one path is 'invalid' but it doesn't impact >>> functionality. >>> >>> This should be an opt-in behavior IMHO, not a blanket change. >> >> To my best knowledge, soc-topology.c' first "customer" was the >> skylake-driver and the final details were cloudy at best back then. >> >> Right now sound-drivers utilizing the topology feature do so in more >> refined fashion. Next, in ASoC we have three locations where >> snd_soc_dapm_add_routes() is called but error-checks are done only in >> 2/3 of them. This is bogus. > > I don't disagree that it was a mistake to omit the check on the returned > value, but now that we have topologies in the wild we can't change the > error handling without a risk of breaking "working" solutions. Exhibit A > is what happened in the other places where this error check was added... > >> If the intended way of using snd_soc_dapm_add_routes() is to ignore the >> return, it should be converted to void and flag ->disable_route_checks >> removed. > > Now that would go back to an "anything goes" mode, not necessarily a > great step. > >>>> Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com> >>>> --- >>>> sound/soc/soc-topology.c | 5 +++-- >>>> 1 file changed, 3 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/sound/soc/soc-topology.c b/sound/soc/soc-topology.c >>>> index d6d368837235..778f539d9ff5 100644 >>>> --- a/sound/soc/soc-topology.c >>>> +++ b/sound/soc/soc-topology.c >>>> @@ -1083,8 +1083,9 @@ static int >>>> soc_tplg_dapm_graph_elems_load(struct soc_tplg *tplg, >>>> break; >>>> } >>>> - /* add route, but keep going if some fail */ >>>> - snd_soc_dapm_add_routes(dapm, route, 1); >>>> + ret = snd_soc_dapm_add_routes(dapm, route, 1); >>>> + if (ret && !dapm->card->disable_route_checks) >>>> + break; > > you could alternatively follow the example in soc-core.c, with a > dev_info() thrown if the route_checks is disabled and a dev_err() thrown > otherwise. At least this would expose the reason for the failure after a > change in error handling, and a means to 'restore' functionality for > specific cards if the topology cannot be updated. Sure, in the next revision I'll mimic the behaviour found in soc-core.c.
diff --git a/sound/soc/soc-topology.c b/sound/soc/soc-topology.c index d6d368837235..778f539d9ff5 100644 --- a/sound/soc/soc-topology.c +++ b/sound/soc/soc-topology.c @@ -1083,8 +1083,9 @@ static int soc_tplg_dapm_graph_elems_load(struct soc_tplg *tplg, break; } - /* add route, but keep going if some fail */ - snd_soc_dapm_add_routes(dapm, route, 1); + ret = snd_soc_dapm_add_routes(dapm, route, 1); + if (ret && !dapm->card->disable_route_checks) + break; } return ret;
One of the framework responsibilities is to ensure that the enumerated DPCMs are valid i.e.: a valid BE is connected to a valid FE DAI. While the are checks in soc-core.c and soc-pcm.c that verify this, a component driver may attempt to workaround this by loading an invalid graph through the topology file. Be strict and fail topology loading when invalid graph is encountered. Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com> --- sound/soc/soc-topology.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)