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@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.
Sure, in the next revision I'll mimic the behaviour found in soc-core.c.
