* [PATCH] ASoC: acpi: fix: continue searching when machine is ignored @ 2018-11-01 2:36 Keyon Jie 2018-11-01 2:51 ` Pierre-Louis Bossart 0 siblings, 1 reply; 5+ messages in thread From: Keyon Jie @ 2018-11-01 2:36 UTC (permalink / raw) To: alsa-devel; +Cc: Keyon Jie, pierre-louis.bossart 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. Signed-off-by: Keyon Jie <yang.jie@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; -- 2.17.1 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] ASoC: acpi: fix: continue searching when machine is ignored 2018-11-01 2:36 [PATCH] ASoC: acpi: fix: continue searching when machine is ignored Keyon Jie @ 2018-11-01 2:51 ` Pierre-Louis Bossart 2018-11-15 23:01 ` Pierre-Louis Bossart 0 siblings, 1 reply; 5+ messages in thread From: Pierre-Louis Bossart @ 2018-11-01 2:51 UTC (permalink / raw) To: Keyon Jie, alsa-devel On 10/31/18 9:36 PM, Keyon Jie wrote: > 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@linux.intel.com> > > Signed-off-by: Keyon Jie <yang.jie@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; ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] ASoC: acpi: fix: continue searching when machine is ignored 2018-11-01 2:51 ` Pierre-Louis Bossart @ 2018-11-15 23:01 ` Pierre-Louis Bossart 2018-11-16 3:03 ` Keyon Jie 0 siblings, 1 reply; 5+ messages in thread From: Pierre-Louis Bossart @ 2018-11-15 23:01 UTC (permalink / raw) To: Keyon Jie, alsa-devel, Mark Brown; +Cc: Patel, Chintan M 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@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). 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. Thanks -Pierre > >> >> Signed-off-by: Keyon Jie <yang.jie@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@alsa-project.org > http://mailman.alsa-project.org/mailman/listinfo/alsa-devel _______________________________________________ Alsa-devel mailing list Alsa-devel@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/alsa-devel ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] ASoC: acpi: fix: continue searching when machine is ignored 2018-11-15 23:01 ` Pierre-Louis Bossart @ 2018-11-16 3:03 ` Keyon Jie 2018-11-16 13:52 ` Pierre-Louis Bossart 0 siblings, 1 reply; 5+ messages in thread From: Keyon Jie @ 2018-11-16 3:03 UTC (permalink / raw) To: Pierre-Louis Bossart, alsa-devel, Mark Brown; +Cc: Patel, Chintan M 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@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 */ 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@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@alsa-project.org >> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel > _______________________________________________ > Alsa-devel mailing list > Alsa-devel@alsa-project.org > http://mailman.alsa-project.org/mailman/listinfo/alsa-devel _______________________________________________ Alsa-devel mailing list Alsa-devel@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/alsa-devel ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] ASoC: acpi: fix: continue searching when machine is ignored 2018-11-16 3:03 ` Keyon Jie @ 2018-11-16 13:52 ` Pierre-Louis Bossart 0 siblings, 0 replies; 5+ messages in thread From: Pierre-Louis Bossart @ 2018-11-16 13:52 UTC (permalink / raw) To: Keyon Jie, alsa-devel, Mark Brown; +Cc: Patel, Chintan M 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@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@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@alsa-project.org >>> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel >> _______________________________________________ >> Alsa-devel mailing list >> Alsa-devel@alsa-project.org >> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel _______________________________________________ Alsa-devel mailing list Alsa-devel@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/alsa-devel ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2018-11-16 13:52 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-11-01 2:36 [PATCH] ASoC: acpi: fix: continue searching when machine is ignored Keyon Jie 2018-11-01 2:51 ` Pierre-Louis Bossart 2018-11-15 23:01 ` Pierre-Louis Bossart 2018-11-16 3:03 ` Keyon Jie 2018-11-16 13:52 ` Pierre-Louis Bossart
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.