Re: [PATCH v3] ASoC: codecs: nau8360: Block DSP path selection when firmware load fails

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [Medium] Returning 0 instead of a negative error code when rejecting a control write falsely signals success to userspace.
- [Medium] Un-ratelimited dev_warn in a userspace-triggerable put callback allows kernel log spam.

Pre-existing issues:
- [High] AB-BA deadlock between nau8360->lock and DAPM mutex during concurrent stream startup and mux update.
--

commit 54e06a9679644c8f5b17d3ad1a405ed38c16ac9a
Author: Neo Chang <YLCHANG2@nuvoton.com>

ASoC: codecs: nau8360: Block DSP path selection when firmware load fails

This commit blocks the DSP path selection in nau8360_dac_mux_put_enum()
if the firmware is not ready. It adds a check to reject the userspace
request and print a warning to prevent invalid routing states.

> diff --git a/sound/soc/codecs/nau8360.c b/sound/soc/codecs/nau8360.c
> index 89b8ee80d6c86..57a5023fe7b7c 100644
> --- a/sound/soc/codecs/nau8360.c
> +++ b/sound/soc/codecs/nau8360.c
> @@ -719,6 +719,11 @@ static int nau8360_dac_mux_put_enum(struct snd_kcontrol *kcontrol,
>  		return ret;
>  	}
>  
> +	if (item[0] == NAU8360_DAC_SRC_DSP && !nau8360->load_fw_done) {
> +		dev_warn(nau8360->dev, "Cannot enable DSP: Firmware not ready or disabled\n");

[Severity: Medium]
Can this un-ratelimited warning lead to kernel log spam? If a userspace
application continuously attempts to set the DSP path while the firmware
is not loaded, it might flood dmesg and hide other system messages.

> +		return ret;

[Severity: Medium]
Since ret is initialized to 0, does returning 0 here falsely signal success
to userspace?

The commit message mentions that returning 0 rejects the invalid request, but
it looks like when userspace attempts to set the DAC path to DSP before the
firmware is loaded, the application might cache the control state as DSP
while the hardware remains in HW1.

Should this return a negative error code like -EBUSY or -EINVAL instead to
properly reject the request?

> +	}
> +
>  	mutex_lock(&nau8360->lock);
>  
>  	ret = snd_soc_dapm_put_enum_double(kcontrol, ucontrol);

[Severity: High]
This is a pre-existing issue, but does this locking sequence create an AB-BA
deadlock? 

In nau8360_dac_mux_put_enum(), nau8360->lock is acquired, and then
snd_soc_dapm_put_enum_double() takes the DAPM mutex:

nau8360_dac_mux_put_enum()
  mutex_lock(&nau8360->lock);
  snd_soc_dapm_put_enum_double()
    snd_soc_dapm_mutex_lock(card);

However, if a stream startup occurs concurrently, dapm_power_widgets() will
hold the DAPM mutex and can trigger nau8360_dsp_clock_event() which acquires
nau8360->lock:

dapm_power_widgets()
  snd_soc_dapm_mutex_lock(card);
  ...
  nau8360_dsp_clock_event()
    mutex_lock(&nau8360->lock);

Could an unprivileged user trigger both actions concurrently and deadlock
the audio subsystem?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260908030342.222655-1-YLCHANG2@nuvoton.com?part=1