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

[Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>]

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;