diff mbox series

[6/9] ALSA: hda: Fix page fault in snd_hda_codec_shutdown()

Message ID 20220706120230.427296-7-cezary.rojewski@intel.com
State Superseded
Headers show
Series [1/9] ALSA: hda: Do not unset preset when cleaning up codec | expand

Commit Message

Cezary Rojewski July 6, 2022, 12:02 p.m. UTC
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(-)

Comments

Pierre-Louis Bossart July 15, 2022, 6:16 p.m. UTC | #1
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;
Takashi Iwai July 15, 2022, 6:23 p.m. UTC | #2
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
Cezary Rojewski July 17, 2022, 10:05 a.m. UTC | #3
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 mbox series

Patch

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;