[alsa-devel] [PATCH 3/7] ASoC: SOF: core: release resources on errors in probe_continue

[Takashi Iwai]

On Fri, 24 Jan 2020 22:36:21 +0100,
Pierre-Louis Bossart wrote:
> 
> The initial intent of releasing resources in the .remove does not work
> well with HDaudio codecs. If the probe_continue() fails in a work
> queue, e.g. due to missing firmware or authentication issues, we don't
> release any resources, and as a result the kernel oopses during
> suspend operations.
> 
> The suggested fix is to release all resources during errors in
> probe_continue(), and use fw_state to track resource allocation
> state, so that .remove does not attempt to release the same
> hardware resources twice. PM operations are also modified so that
> no action is done if DSP resources have been freed due to
> an error at probe.
> 
> Reported-by: Takashi Iwai <tiwai at suse.de>
> Co-developed-by: Kai Vehmanen <kai.vehmanen at linux.intel.com>
> Signed-off-by: Kai Vehmanen <kai.vehmanen at linux.intel.com>
> Bugzilla:  http://bugzilla.suse.com/show_bug.cgi?id=1161246
> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart at linux.intel.com>

This deserves for Cc to stable, as the bug already hits on both 5.4
and 5.5 kernels.

Reviewed-by: Takashi Iwai <tiwai at suse.de>


thanks,

Takashi

> ---
>  sound/soc/sof/core.c | 33 ++++++++++++---------------------
>  sound/soc/sof/pm.c   |  4 ++++
>  2 files changed, 16 insertions(+), 21 deletions(-)
> 
> diff --git a/sound/soc/sof/core.c b/sound/soc/sof/core.c
> index f517ab448a1d..34cefbaf2d2a 100644
> --- a/sound/soc/sof/core.c
> +++ b/sound/soc/sof/core.c
> @@ -244,7 +244,6 @@ static int sof_probe_continue(struct snd_sof_dev *sdev)
>  
>  	return 0;
>  
> -#if !IS_ENABLED(CONFIG_SND_SOC_SOF_PROBE_WORK_QUEUE)
>  fw_trace_err:
>  	snd_sof_free_trace(sdev);
>  fw_run_err:
> @@ -255,22 +254,10 @@ static int sof_probe_continue(struct snd_sof_dev *sdev)
>  	snd_sof_free_debug(sdev);
>  dbg_err:
>  	snd_sof_remove(sdev);
> -#else
>  
> -	/*
> -	 * when the probe_continue is handled in a work queue, the
> -	 * probe does not fail so we don't release resources here.
> -	 * They will be released with an explicit call to
> -	 * snd_sof_device_remove() when the PCI/ACPI device is removed
> -	 */
> -
> -fw_trace_err:
> -fw_run_err:
> -fw_load_err:
> -ipc_err:
> -dbg_err:
> -
> -#endif
> +	/* all resources freed, update state to match */
> +	sdev->fw_state = SOF_FW_BOOT_NOT_STARTED;
> +	sdev->first_boot = true;
>  
>  	return ret;
>  }
> @@ -353,10 +340,12 @@ int snd_sof_device_remove(struct device *dev)
>  	if (IS_ENABLED(CONFIG_SND_SOC_SOF_PROBE_WORK_QUEUE))
>  		cancel_work_sync(&sdev->probe_work);
>  
> -	snd_sof_fw_unload(sdev);
> -	snd_sof_ipc_free(sdev);
> -	snd_sof_free_debug(sdev);
> -	snd_sof_free_trace(sdev);
> +	if (sdev->fw_state > SOF_FW_BOOT_NOT_STARTED) {
> +		snd_sof_fw_unload(sdev);
> +		snd_sof_ipc_free(sdev);
> +		snd_sof_free_debug(sdev);
> +		snd_sof_free_trace(sdev);
> +	}
>  
>  	/*
>  	 * Unregister machine driver. This will unbind the snd_card which
> @@ -364,13 +353,15 @@ int snd_sof_device_remove(struct device *dev)
>  	 * before freeing the snd_card.
>  	 */
>  	snd_sof_machine_unregister(sdev, pdata);
> +
>  	/*
>  	 * Unregistering the machine driver results in unloading the topology.
>  	 * Some widgets, ex: scheduler, attempt to power down the core they are
>  	 * scheduled on, when they are unloaded. Therefore, the DSP must be
>  	 * removed only after the topology has been unloaded.
>  	 */
> -	snd_sof_remove(sdev);
> +	if (sdev->fw_state > SOF_FW_BOOT_NOT_STARTED)
> +		snd_sof_remove(sdev);
>  
>  	/* release firmware */
>  	release_firmware(pdata->fw);
> diff --git a/sound/soc/sof/pm.c b/sound/soc/sof/pm.c
> index 84290bbeebdd..a0cde053b61a 100644
> --- a/sound/soc/sof/pm.c
> +++ b/sound/soc/sof/pm.c
> @@ -56,6 +56,10 @@ static int sof_resume(struct device *dev, bool runtime_resume)
>  	if (!sof_ops(sdev)->resume || !sof_ops(sdev)->runtime_resume)
>  		return 0;
>  
> +	/* DSP was never successfully started, nothing to resume */
> +	if (sdev->first_boot)
> +		return 0;
> +
>  	/*
>  	 * if the runtime_resume flag is set, call the runtime_resume routine
>  	 * or else call the system resume routine
> -- 
> 2.20.1
>