| Message ID | 20240304190536.1783332-3-cezary.rojewski@intel.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | ASoC: Harden DAPM route checks and Intel fixes | expand |
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. > 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; > } > > return ret;
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. 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. Kind regards, Czarek >> 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; >> } >> >> return ret;
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. >>> } >>> return ret;
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(-)