Re: [PATCH 2/5] ASoC: topology: Do not ignore route checks when parsing graphs

[Cezary Rojewski <cezary.rojewski@xxxxxxxxx>]

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.