| Message ID | 20220706120230.427296-7-cezary.rojewski@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ALSA: hda: Codec-reload bug fixes and cleanups | expand |
On 7/6/22 07:02, Cezary Rojewski wrote: > If early probe of HDAudio bus driver fails e.g.: due to missing > firmware file, snd_hda_codec_shutdown() ends in manipulating > uninitialized codec->pcm_list_head causing page fault. > > Iinitialization of HDAudio codec in ASoC is split in two: > - snd_hda_codec_device_init() > - snd_hda_codec_device_new() > > snd_hda_codec_device_init() is called during probe_codecs() by HDAudio > bus driver while snd_hda_codec_device_new() is called by > codec-component's ->probe(). The second call will not happen until all > components required by related sound card are present within the ASoC > framework. With firmware failing to load during the PCI's deferred > initialization i.e.: probe_work(), no platform components are ever > registered. HDAudio codec enumeration is done at that point though, so > the codec components became registered to ASoC framework, calling > snd_hda_codec_device_init() in the process. > > Now, during platform reboot snd_hda_codec_shutdown() is called for every > codec found on the HDAudio bus causing oops if any of them has not > completed both of their initialization steps. Relocating field > initialization fixes the issue. > > Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com> This patch causes an across-the-board regression on all SOF platforms using an HDaudio or iDISP codec. Only 'nocodec' platforms are unaffected, see results at https://sof-ci.01.org/linuxpr/PR3763/build394/devicetest/ Reverting this commit seems to fix the issue. Upstream merge: https://github.com/thesofproject/linux/pull/3763 Issue and bisect results https://github.com/thesofproject/linux/issues/3764 I don't know what this was supposed to fix on the shutdown path but it's clearly having side effects on the probe/init path. > --- > sound/pci/hda/hda_codec.c | 41 +++++++++++++++++++-------------------- > 1 file changed, 20 insertions(+), 21 deletions(-) > > diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c > index 75e85bf58681..677d0a78f19c 100644 > --- a/sound/pci/hda/hda_codec.c > +++ b/sound/pci/hda/hda_codec.c > @@ -930,8 +930,28 @@ snd_hda_codec_device_init(struct hda_bus *bus, unsigned int codec_addr, > } > > codec->bus = bus; > + codec->depop_delay = -1; > + codec->fixup_id = HDA_FIXUP_ID_NOT_SET; > + codec->core.dev.release = snd_hda_codec_dev_release; > + codec->core.exec_verb = codec_exec_verb; > codec->core.type = HDA_DEV_LEGACY; > > + mutex_init(&codec->spdif_mutex); > + mutex_init(&codec->control_mutex); > + snd_array_init(&codec->mixers, sizeof(struct hda_nid_item), 32); > + snd_array_init(&codec->nids, sizeof(struct hda_nid_item), 32); > + snd_array_init(&codec->init_pins, sizeof(struct hda_pincfg), 16); > + snd_array_init(&codec->driver_pins, sizeof(struct hda_pincfg), 16); > + snd_array_init(&codec->cvt_setups, sizeof(struct hda_cvt_setup), 8); > + snd_array_init(&codec->spdif_out, sizeof(struct hda_spdif_out), 16); > + snd_array_init(&codec->jacktbl, sizeof(struct hda_jack_tbl), 16); > + snd_array_init(&codec->verbs, sizeof(struct hda_verb *), 8); > + INIT_LIST_HEAD(&codec->conn_list); > + INIT_LIST_HEAD(&codec->pcm_list_head); > + INIT_DELAYED_WORK(&codec->jackpoll_work, hda_jackpoll_work); > + refcount_set(&codec->pcm_ref, 1); > + init_waitqueue_head(&codec->remove_sleep); > + > return codec; > } > EXPORT_SYMBOL_GPL(snd_hda_codec_device_init); > @@ -984,29 +1004,8 @@ int snd_hda_codec_device_new(struct hda_bus *bus, struct snd_card *card, > if (snd_BUG_ON(codec_addr > HDA_MAX_CODEC_ADDRESS)) > return -EINVAL; > > - codec->core.dev.release = snd_hda_codec_dev_release; > - codec->core.exec_verb = codec_exec_verb; > - > codec->card = card; > codec->addr = codec_addr; > - mutex_init(&codec->spdif_mutex); > - mutex_init(&codec->control_mutex); > - snd_array_init(&codec->mixers, sizeof(struct hda_nid_item), 32); > - snd_array_init(&codec->nids, sizeof(struct hda_nid_item), 32); > - snd_array_init(&codec->init_pins, sizeof(struct hda_pincfg), 16); > - snd_array_init(&codec->driver_pins, sizeof(struct hda_pincfg), 16); > - snd_array_init(&codec->cvt_setups, sizeof(struct hda_cvt_setup), 8); > - snd_array_init(&codec->spdif_out, sizeof(struct hda_spdif_out), 16); > - snd_array_init(&codec->jacktbl, sizeof(struct hda_jack_tbl), 16); > - snd_array_init(&codec->verbs, sizeof(struct hda_verb *), 8); > - INIT_LIST_HEAD(&codec->conn_list); > - INIT_LIST_HEAD(&codec->pcm_list_head); > - refcount_set(&codec->pcm_ref, 1); > - init_waitqueue_head(&codec->remove_sleep); > - > - INIT_DELAYED_WORK(&codec->jackpoll_work, hda_jackpoll_work); > - codec->depop_delay = -1; > - codec->fixup_id = HDA_FIXUP_ID_NOT_SET; > > #ifdef CONFIG_PM > codec->power_jiffies = jiffies;
On Fri, 15 Jul 2022 20:16:14 +0200, Pierre-Louis Bossart wrote: > > > > On 7/6/22 07:02, Cezary Rojewski wrote: > > If early probe of HDAudio bus driver fails e.g.: due to missing > > firmware file, snd_hda_codec_shutdown() ends in manipulating > > uninitialized codec->pcm_list_head causing page fault. > > > > Iinitialization of HDAudio codec in ASoC is split in two: > > - snd_hda_codec_device_init() > > - snd_hda_codec_device_new() > > > > snd_hda_codec_device_init() is called during probe_codecs() by HDAudio > > bus driver while snd_hda_codec_device_new() is called by > > codec-component's ->probe(). The second call will not happen until all > > components required by related sound card are present within the ASoC > > framework. With firmware failing to load during the PCI's deferred > > initialization i.e.: probe_work(), no platform components are ever > > registered. HDAudio codec enumeration is done at that point though, so > > the codec components became registered to ASoC framework, calling > > snd_hda_codec_device_init() in the process. > > > > Now, during platform reboot snd_hda_codec_shutdown() is called for every > > codec found on the HDAudio bus causing oops if any of them has not > > completed both of their initialization steps. Relocating field > > initialization fixes the issue. > > > > Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com> > > This patch causes an across-the-board regression on all SOF platforms > using an HDaudio or iDISP codec. Only 'nocodec' platforms are > unaffected, see results at > https://sof-ci.01.org/linuxpr/PR3763/build394/devicetest/ > > Reverting this commit seems to fix the issue. > > Upstream merge: > https://github.com/thesofproject/linux/pull/3763 > > Issue and bisect results > https://github.com/thesofproject/linux/issues/3764 > > I don't know what this was supposed to fix on the shutdown path but it's > clearly having side effects on the probe/init path. Yeah, obviously the patch ignores the fact that hdac_hda does initialize the HD-audio codec without snd_hda_codec_device_init(). I'm going to revert the commit. thanks, Takashi
On 2022-07-15 8:23 PM, Takashi Iwai wrote: > On Fri, 15 Jul 2022 20:16:14 +0200, > Pierre-Louis Bossart wrote: ... >> This patch causes an across-the-board regression on all SOF platforms >> using an HDaudio or iDISP codec. Only 'nocodec' platforms are >> unaffected, see results at >> https://sof-ci.01.org/linuxpr/PR3763/build394/devicetest/ >> >> Reverting this commit seems to fix the issue. >> >> Upstream merge: >> https://github.com/thesofproject/linux/pull/3763 >> >> Issue and bisect results >> https://github.com/thesofproject/linux/issues/3764 >> >> I don't know what this was supposed to fix on the shutdown path but it's >> clearly having side effects on the probe/init path. > > Yeah, obviously the patch ignores the fact that hdac_hda does > initialize the HD-audio codec without snd_hda_codec_device_init(). > > I'm going to revert the commit. Thanks for the report and sorry the trouble. This patch is still valid - will re-check hdac_hda.c and update it so that both the regression and the page fault are gone. Regards, Czarek
diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c index 75e85bf58681..677d0a78f19c 100644 --- a/sound/pci/hda/hda_codec.c +++ b/sound/pci/hda/hda_codec.c @@ -930,8 +930,28 @@ snd_hda_codec_device_init(struct hda_bus *bus, unsigned int codec_addr, } codec->bus = bus; + codec->depop_delay = -1; + codec->fixup_id = HDA_FIXUP_ID_NOT_SET; + codec->core.dev.release = snd_hda_codec_dev_release; + codec->core.exec_verb = codec_exec_verb; codec->core.type = HDA_DEV_LEGACY; + mutex_init(&codec->spdif_mutex); + mutex_init(&codec->control_mutex); + snd_array_init(&codec->mixers, sizeof(struct hda_nid_item), 32); + snd_array_init(&codec->nids, sizeof(struct hda_nid_item), 32); + snd_array_init(&codec->init_pins, sizeof(struct hda_pincfg), 16); + snd_array_init(&codec->driver_pins, sizeof(struct hda_pincfg), 16); + snd_array_init(&codec->cvt_setups, sizeof(struct hda_cvt_setup), 8); + snd_array_init(&codec->spdif_out, sizeof(struct hda_spdif_out), 16); + snd_array_init(&codec->jacktbl, sizeof(struct hda_jack_tbl), 16); + snd_array_init(&codec->verbs, sizeof(struct hda_verb *), 8); + INIT_LIST_HEAD(&codec->conn_list); + INIT_LIST_HEAD(&codec->pcm_list_head); + INIT_DELAYED_WORK(&codec->jackpoll_work, hda_jackpoll_work); + refcount_set(&codec->pcm_ref, 1); + init_waitqueue_head(&codec->remove_sleep); + return codec; } EXPORT_SYMBOL_GPL(snd_hda_codec_device_init); @@ -984,29 +1004,8 @@ int snd_hda_codec_device_new(struct hda_bus *bus, struct snd_card *card, if (snd_BUG_ON(codec_addr > HDA_MAX_CODEC_ADDRESS)) return -EINVAL; - codec->core.dev.release = snd_hda_codec_dev_release; - codec->core.exec_verb = codec_exec_verb; - codec->card = card; codec->addr = codec_addr; - mutex_init(&codec->spdif_mutex); - mutex_init(&codec->control_mutex); - snd_array_init(&codec->mixers, sizeof(struct hda_nid_item), 32); - snd_array_init(&codec->nids, sizeof(struct hda_nid_item), 32); - snd_array_init(&codec->init_pins, sizeof(struct hda_pincfg), 16); - snd_array_init(&codec->driver_pins, sizeof(struct hda_pincfg), 16); - snd_array_init(&codec->cvt_setups, sizeof(struct hda_cvt_setup), 8); - snd_array_init(&codec->spdif_out, sizeof(struct hda_spdif_out), 16); - snd_array_init(&codec->jacktbl, sizeof(struct hda_jack_tbl), 16); - snd_array_init(&codec->verbs, sizeof(struct hda_verb *), 8); - INIT_LIST_HEAD(&codec->conn_list); - INIT_LIST_HEAD(&codec->pcm_list_head); - refcount_set(&codec->pcm_ref, 1); - init_waitqueue_head(&codec->remove_sleep); - - INIT_DELAYED_WORK(&codec->jackpoll_work, hda_jackpoll_work); - codec->depop_delay = -1; - codec->fixup_id = HDA_FIXUP_ID_NOT_SET; #ifdef CONFIG_PM codec->power_jiffies = jiffies;
If early probe of HDAudio bus driver fails e.g.: due to missing firmware file, snd_hda_codec_shutdown() ends in manipulating uninitialized codec->pcm_list_head causing page fault. Iinitialization of HDAudio codec in ASoC is split in two: - snd_hda_codec_device_init() - snd_hda_codec_device_new() snd_hda_codec_device_init() is called during probe_codecs() by HDAudio bus driver while snd_hda_codec_device_new() is called by codec-component's ->probe(). The second call will not happen until all components required by related sound card are present within the ASoC framework. With firmware failing to load during the PCI's deferred initialization i.e.: probe_work(), no platform components are ever registered. HDAudio codec enumeration is done at that point though, so the codec components became registered to ASoC framework, calling snd_hda_codec_device_init() in the process. Now, during platform reboot snd_hda_codec_shutdown() is called for every codec found on the HDAudio bus causing oops if any of them has not completed both of their initialization steps. Relocating field initialization fixes the issue. Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com> --- sound/pci/hda/hda_codec.c | 41 +++++++++++++++++++-------------------- 1 file changed, 20 insertions(+), 21 deletions(-)