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@xxxxxxxxx> >>> --- >>> 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;
