[alsa-devel] [PATCH v2 09/25] ASoC: soc-core: tidyup for snd_soc_dapm_add_routes()

[Pierre-Louis Bossart]

On 8/20/19 8:38 AM, Cezary Rojewski wrote:
> On 2019-08-20 14:36, Pierre-Louis Bossart wrote:
>>
>>
>> On 8/20/19 6:18 AM, Cezary Rojewski wrote:
>>> On 2019-08-07 03:31, Kuninori Morimoto wrote:
>>>>
>>>> From: Kuninori Morimoto <kuninori.morimoto.gx at renesas.com>
>>>>
>>>> snd_soc_dapm_add_routes() registers routes by using
>>>> for(... i < num; ...). If routes was NULL, num should be zero.
>>>> Thus, we don't need to check about route pointer.
>>>> This patch also cares missing return value.
>>>>
>>>> Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx at renesas.com>
>>>> ---
>>>> v1 -> v2
>>>>
>>>>     - check return value
>>>>     - change Subject
>>>>
>>>>   sound/soc/soc-core.c | 23 +++++++++++++----------
>>>>   1 file changed, 13 insertions(+), 10 deletions(-)
>>>>
>>>> diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c
>>>> index 21cdd3c..ca1b04c 100644
>>>> --- a/sound/soc/soc-core.c
>>>> +++ b/sound/soc/soc-core.c
>>>> @@ -1310,10 +1310,11 @@ static int soc_probe_component(struct 
>>>> snd_soc_card *card,
>>>>       if (ret < 0)
>>>>           goto err_probe;
>>>> -    if (component->driver->dapm_routes)
>>>> -        snd_soc_dapm_add_routes(dapm,
>>>> -                    component->driver->dapm_routes,
>>>> -                    component->driver->num_dapm_routes);
>>>> +    ret = snd_soc_dapm_add_routes(dapm,
>>>> +                      component->driver->dapm_routes,
>>>> +                      component->driver->num_dapm_routes);
>>>> +    if (ret < 0)
>>>> +        goto err_probe;
>>>>       list_add(&dapm->list, &card->dapm_list);
>>>>       /* see for_each_card_components */
>>>> @@ -2060,13 +2061,15 @@ static int snd_soc_instantiate_card(struct 
>>>> snd_soc_card *card)
>>>>           snd_soc_add_card_controls(card, card->controls,
>>>>                         card->num_controls);
>>>> -    if (card->dapm_routes)
>>>> -        snd_soc_dapm_add_routes(&card->dapm, card->dapm_routes,
>>>> -                    card->num_dapm_routes);
>>>> +    ret = snd_soc_dapm_add_routes(&card->dapm, card->dapm_routes,
>>>> +                      card->num_dapm_routes);
>>>> +    if (ret < 0)
>>>> +        goto probe_end;
>>>> -    if (card->of_dapm_routes)
>>>> -        snd_soc_dapm_add_routes(&card->dapm, card->of_dapm_routes,
>>>> -                    card->num_of_dapm_routes);
>>>> +    ret = snd_soc_dapm_add_routes(&card->dapm, card->of_dapm_routes,
>>>> +                      card->num_of_dapm_routes);
>>>> +    if (ret < 0)
>>>> +        goto probe_end;
>>>>       /* try to set some sane longname if DMI is available */
>>>>       snd_soc_set_dmi_name(card, NULL);
>>>>
>>>
>>> Hello there,
>>>
>>> I've run a validation cycle on recent broonie/for-next and this 
>>> commit caused regression. However, it may be simply an error on board 
>>> side instead.
>>>
>>> Previously, ret from snd_soc_dapm_add_routes has been ignored thus it 
>>> was permissive for addition of several routes to fail. As long as 
>>> some routes succeeded, card was working just fine. Now it's no longer 
>>> the case - behavior of the card initialization has changed: it is 
>>> required for ALL routes to succeed before card can be fully 
>>> instantiated.
>>>
>>> Must say collapsing snd_soc_instantiate_card is a wonderful way to 
>>> test your card's removal flow (soc__cleanup_card_resources and 
>>> friends)..
>>>
>>> Question is simple: are we staying with all-for-one/ one-for-all 
>>> approach or we reverting to permissive behavior?
>>
>> Can you elaborate in which test case this patch creates a problem? 
>> Just curious why the route addition fails in the first place.
> 
> If snd_soc_instantiate_card fails so does any test, really. Red wall was 
> easy to spot even for a hungry developer : )
> 
> Our cnl_rt274 board declares several routes, yet our topology does not 
> provide necessary info for all of them. And thus, addition of some 
> routes fails. This was fine till now. That's also why I'd mentioned in 
> the very first sentence: it might be simply a board issue. Maybe we 
> should have never abused permissive behavior in the first place.

Yep, and that driver is not upstream as well so Intel can't complain here...