[alsa-devel] [PATCH] ASoC: acpi: fix: continue searching when machine is ignored

[Pierre-Louis Bossart]

On 11/15/18 9:03 PM, Keyon Jie wrote:
>
>
> On 2018/11/16 上午7:01, Pierre-Louis Bossart wrote:
>> Hi Keyon,
>>
>>>> The machine_quirk may return NULL which means the acpi entries 
>>>> should be
>>>> skipped and search for next matched entry is needed, here add return
>>>> check here and continue for NULL case.
>>>
>>> Nice catch, this was missed in the other patchset since the ACPI ID 
>>> to ignore was the last in the table...
>>>
>>> Acked-by: Pierre-Louis Bossart <pierre-louis.bossart at linux.intel.com>
>>
>> I was wondering if this patch was merged, but actually it's good Mark 
>> wasn't copied on this one: the code doesn't quite work as intended 
>> and shouldn't be merged as is.
>>
>> Chintan and I see an oops and reboot on KBL Chromebooks, when the 
>> quirk returns NULL, the mach variable becomes NULL and cannot be 
>> modified with pointer arithmetic (which doesn't show with the diff 
>> format).
>
> Ah, yeap, I omitted this, so we may need introduce a new pointer to 
> fix that, something like this:
>
> diff --git a/sound/soc/soc-acpi.c b/sound/soc/soc-acpi.c
> index b8e72b52db30..0d967d0ca222 100644
> --- a/sound/soc/soc-acpi.c
> +++ b/sound/soc/soc-acpi.c
> @@ -10,11 +10,22 @@ struct snd_soc_acpi_mach *
>  snd_soc_acpi_find_machine(struct snd_soc_acpi_mach *machines)
>  {
>         struct snd_soc_acpi_mach *mach;
> +       struct snd_soc_acpi_mach *quirk_mach = NULL;
>
>         for (mach = machines; mach->id[0]; mach++) {
>                 if (acpi_dev_present(mach->id, NULL, -1)) {
> -                       if (mach->machine_quirk)
> -                               mach = mach->machine_quirk(mach);
> +                       if (mach->machine_quirk) {
> +                               quirk_mach = mach->machine_quirk(mach);
> +
> +                               if (!quirk_mach)
> +                                       /* skip if no quirk matched */
> +                                       continue;
> +                               else
> +                                       /* return quirk matched 
> machine */
> +                                       return quirk_mach;
> +                       }
> +
> +                       /* return acpi matched machine directly */

Yes, we tried something similar, see 
https://github.com/plbossart/sound/commit/9acfd674141bfc0a0bdadda1c3f7b16cc61047de


> return mach;
>                 }
>         }
>
>
>>
>> It's very likely btw that anyone trying a recent kernel on these 
>> devices would not see the audio driver probe at all, so we do want to 
>> try other entries - but of course without this nasty side effect. I 
>> will submit a v2 when I've sorted out other problems on Chrome.
>
> That's fine.
>
> Thanks,
> ~Keyon
>
>>
>> Thanks
>>
>> -Pierre
>>
>>>
>>>>
>>>> Signed-off-by: Keyon Jie <yang.jie at linux.intel.com>
>>>> ---
>>>>   sound/soc/soc-acpi.c | 5 ++++-
>>>>   1 file changed, 4 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/sound/soc/soc-acpi.c b/sound/soc/soc-acpi.c
>>>> index b8e72b52db30..faf8941363e4 100644
>>>> --- a/sound/soc/soc-acpi.c
>>>> +++ b/sound/soc/soc-acpi.c
>>>> @@ -15,7 +15,10 @@ snd_soc_acpi_find_machine(struct 
>>>> snd_soc_acpi_mach *machines)
>>>>           if (acpi_dev_present(mach->id, NULL, -1)) {
>>>>               if (mach->machine_quirk)
>>>>                   mach = mach->machine_quirk(mach);
>>>> -            return mach;
>>>> +
>>>> +            /* return matched machine, continue otherwise */
>>>> +            if (mach)
>>>> +                return mach;
>>>>           }
>>>>       }
>>>>       return NULL;
>>> _______________________________________________
>>> Alsa-devel mailing list
>>> Alsa-devel at alsa-project.org
>>> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
>> _______________________________________________
>> Alsa-devel mailing list
>> Alsa-devel at alsa-project.org
>> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel