Re: [PATCH v10 2/2] ASoC: codecs: nau8360: Add support for NAU83G60 amplifier

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] Regmap `val_bits = 16` causes all 32-bit DSP commands to be rejected and replies to be truncated, completely breaking DSP initialization.
- [High] Asymmetric clearing of TDM TX and RX enable flags breaks subsequent audio capture streams.
- [High] AB-BA lock inversion deadlock between `nau8360->lock` and the ASoC DAPM mutex.
- [High] Audio playback can resume before asynchronous DSP firmware loading completes, potentially causing DSP crashes or invalid audio output.
- [High] The background work `load_fw_work` is scheduled in the ASoC component `.probe` callback, but the driver lacks an ASoC component `.remove` callback to cancel it.
- [High] nau8360_resume() corrupts the cached clock and DSP state by calling nau8360_dsp_bootup() without bypassing the regcache.
--

--- Patch [2]: [PATCH v10 2/2] ASoC: codecs: nau8360: Add support for NAU83G60 amplifier ---
commit a968f346c53b7f4b17a1854ba82cf86c1d38cc5f
Author: Neo Chang <YLCHANG2@nuvoton.com>
Subject: ASoC: codecs: nau8360: Add support for NAU83G60 amplifier

This commit introduces support for the Nuvoton NAU83G60 stereo smart
amplifier. The driver implements I2C control, DSP firmware loading for
the KCS algorithm, and DAPM routing for DAC, ADC, and internal engines.

> --- /dev/null
> +++ b/sound/soc/codecs/nau8360.c
[ ... ]
> +static int nau8360_peq_coeff_put(struct snd_kcontrol *kcontrol,
> +	struct snd_ctl_elem_value *ucontrol)
> +{
> +	struct snd_soc_component *cp = snd_kcontrol_chip(kcontrol);
> +	struct snd_soc_dapm_context *dapm = snd_soc_component_to_dapm(cp);
> +	struct nau8360 *nau8360 = snd_soc_component_get_drvdata(cp);
[ ... ]
> +	snd_soc_dapm_mutex_lock(dapm);
> +	if (snd_soc_component_active(cp)) {
> +		dev_dbg(nau8360->dev,
> +			"PEQ coefficient access is ignored during audio is active");
> +		ret = -EBUSY;
> +		goto unlock_dapm;
> +	}
> +
> +	reg = nau8360_peq_regaddr(kcontrol->id.name);
> +	if (reg < 0) {
> +		ret = reg;
> +		goto unlock_dapm;
> +	}
> +
> +	data = kmemdup(ucontrol->value.bytes.data, params->max, GFP_KERNEL);
> +	if (!data) {
> +		ret = -ENOMEM;
> +		goto unlock_dapm;
> +	}
> +
> +	mutex_lock(&nau8360->lock);

[Severity: High]
Could this lock ordering cause an AB-BA deadlock with the ALSA mixer?

In nau8360_peq_coeff_put(), the code acquires snd_soc_dapm_mutex_lock() before
mutex_lock(&nau8360->lock). However, in nau8360_dac_mux_put_enum() below, the
driver acquires mutex_lock(&nau8360->lock) and then calls
snd_soc_dapm_put_enum_double(), which blocks acquiring the DAPM mutex.
Since the ALSA core permits concurrent put callbacks on different kcontrols,
this inversion might hang the system.

> +static int nau8360_dac_mux_put_enum(struct snd_kcontrol *kcontrol,
> +	struct snd_ctl_elem_value *ucontrol)
> +{
> +	struct snd_soc_dapm_context *dapm = snd_soc_dapm_kcontrol_to_dapm(kcontrol);
> +	struct snd_soc_component *component = snd_soc_dapm_to_component(dapm);
> +	struct nau8360 *nau8360 = snd_soc_component_get_drvdata(component);
> +	struct soc_enum *e = (struct soc_enum *)kcontrol->private_value;
> +	unsigned int *item = ucontrol->value.enumerated.item;
> +	int ret = 0;
> +
> +	if (snd_soc_dapm_get_bias_level(dapm) > SND_SOC_BIAS_STANDBY) {
> +		dev_warn(nau8360->dev, "changing path is not allowed during playback");
> +		return ret;
> +	}
> +
> +	mutex_lock(&nau8360->lock);
> +
> +	ret = snd_soc_dapm_put_enum_double(kcontrol, ucontrol);

[ ... ]
> +static int nau8360_startup(struct snd_pcm_substream *substream, struct snd_soc_dai *dai)
> +{
> +	struct snd_soc_component *component = dai->component;
> +	struct nau8360 *nau8360 = snd_soc_component_get_drvdata(component);
> +	unsigned int i2s_mask = NAU8360_FRAME_START_MASK | NAU8360_RX_OFFSET_MASK;
> +	unsigned int i2s_fmt = NAU8360_FRAME_START_H2L | NAU8360_RX_OFFSET_I2S;
> +	int val = 0;
> +
> +	flush_work(&nau8360->load_fw_work);
> +
> +	if (nau8360_dsp_active(component) && !nau8360->load_fw_done) {
> +		dev_warn(nau8360->dev, "DSP firmware is not ready yet!");
> +		return -EBUSY;
> +	}
> +
> +	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
> +		regmap_read(nau8360->regmap, NAU8360_R0B_I2S_PCM_CTRL1, &val);
> +		if ((val & i2s_mask) == i2s_fmt)
> +			regmap_update_bits(nau8360->regmap, NAU8360_R0B_I2S_PCM_CTRL1,
> +				NAU8360_EN_TDM_RX, NAU8360_EN_TDM_RX);
> +
> +		if (nau8360_dsp_active(component))
> +			snd_soc_dapm_enable_pin(nau8360->dapm, "Sense");
> +	}
> +
> +	return 0;
> +}
> +
> +static void nau8360_shutdown(struct snd_pcm_substream *substream,
> +	struct snd_soc_dai *dai)
> +{
> +	struct snd_soc_component *component = dai->component;
> +	struct nau8360 *nau8360 = snd_soc_component_get_drvdata(component);
> +	unsigned int tdm_mask;
> +
> +	tdm_mask = (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) ?
> +		NAU8360_EN_TDM_RX : NAU8360_EN_TDM_TX;
> +	regmap_update_bits(nau8360->regmap, NAU8360_R0B_I2S_PCM_CTRL1,
> +		tdm_mask, 0);

[Severity: High]
Does this asymmetric clearing of TDM flags break subsequent capture streams?

In nau8360_shutdown(), NAU8360_EN_TDM_TX is permanently cleared when a capture
stream is closed. However, nau8360_startup() lacks any logic to restore
NAU8360_EN_TDM_TX for capture streams. This seems to result in the TDM capture
interface being permanently disabled after the first use. Additionally,
startup() restricts RX restoration strictly to the I2S format, which might
break playback for Left/Right-Justified formats.

[ ... ]
> +static inline void nau8360_dsp_software_reset(struct snd_soc_component *component)
> +{
> +	/* Enable PLL for successful DSP reset. After DSP is alive,
> +	 * system clock switches to internal clock and disable PLL.
> +	 */
> +	snd_soc_component_update_bits(component, NAU8360_R72_PLL_CFG0,
> +		NAU8360_PD_PLL_MASK, NAU8360_PD_PLL_EN);
> +	msleep(50);
> +	snd_soc_component_write(component, NAU8360_R01_DSP_SOFTWARE_RST, 0x5a5a);
> +	snd_soc_component_write(component, NAU8360_R01_DSP_SOFTWARE_RST, 0xa5a5);
> +	snd_soc_component_set_sysclk(component, NAU8360_CLK_ID_INT, 0, 0,
> +		SND_SOC_CLOCK_IN);
> +}
[ ... ]
> +static int __maybe_unused nau8360_resume(struct snd_soc_component *component)
> +{
> +	struct nau8360 *nau8360 = snd_soc_component_get_drvdata(component);
> +	struct regmap *regmap = nau8360->regmap;
> +
> +	regcache_cache_only(regmap, false);
> +
> +	nau8360_dsp_bootup(component);
> +
> +	nau8360_peq_mem_enable(regmap, true);
> +	regcache_sync(regmap);

[Severity: High]
Might this sequence corrupt the cached clock and DSP state during resume?

By calling regcache_cache_only(regmap, false) before nau8360_dsp_bootup(), the
cache is active when nau8360_dsp_software_reset() sets the sysclk to
NAU8360_CLK_ID_INT. These writes will update the regcache and clear its dirty
flags. When regcache_sync() runs immediately afterward, it sees the cache
matches the hardware for these clock registers, leaving the clobbered internal
clock settings in place instead of restoring the pre-suspend values.

> +	nau8360_peq_mem_enable(regmap, false);
> +
> +	/* disable Sense at standby */
> +	snd_soc_dapm_disable_pin(nau8360->dapm, "Sense");
> +	snd_soc_dapm_sync(nau8360->dapm);
> +
> +	nau8360_dsp_fw_load(nau8360);
> +
> +	return 0;
> +}

[Severity: High]
Can audio playback resume before the asynchronous DSP firmware loading
completes?

In nau8360_resume(), nau8360_dsp_fw_load() schedules background work to load
firmware over I2C and returns immediately. During an ALSA PCM stream resume,
the core framework issues prepare() and trigger(RESUME) to restart the stream
without calling startup(). Since there is no .prepare callback in
nau8360_dai_ops below to flush the background work, an active audio stream
will forcefully restart while the DSP is still being configured.

> +static const struct snd_soc_component_driver soc_comp_dev_nau8360 = {
> +	.probe			= nau8360_codec_probe,
> +	.set_sysclk		= nau8360_set_sysclk,
> +	.set_pll		= nau8360_set_pll,
> +	.suspend		= nau8360_suspend,
> +	.resume			= nau8360_resume,
> +	.controls		= nau8360_snd_controls,
> +	.num_controls		= ARRAY_SIZE(nau8360_snd_controls),
> +	.dapm_widgets		= nau8360_dapm_widgets,
> +	.num_dapm_widgets	= ARRAY_SIZE(nau8360_dapm_widgets),
> +	.dapm_routes		= nau8360_dapm_routes,
> +	.num_dapm_routes	= ARRAY_SIZE(nau8360_dapm_routes),
> +	.suspend_bias_off	= 1,
> +	.idle_bias_on		= 1,
> +	.use_pmdown_time	= 1,
> +	.endianness		= 1,
> +};

[Severity: High]
Could the missing .remove callback in the ASoC component driver lead to a UAF?

The background task load_fw_work is scheduled during the ASoC component
.probe() and dereferences nau8360->dapm when it runs. If the ASoC component
unbinds, the core frees the DAPM context. Because soc_comp_dev_nau8360 omits
a .remove callback to flush or cancel the workqueue, the background task can
continue executing and access freed memory.

> +static const struct snd_soc_dai_ops nau8360_dai_ops = {
> +	.startup	= nau8360_startup,
> +	.shutdown	= nau8360_shutdown,
> +	.hw_params	= nau8360_hw_params,
> +	.set_fmt	= nau8360_set_fmt,
> +	.set_tdm_slot	= nau8360_set_tdm_slot,
> +};
[ ... ]
> +static const struct regmap_config nau8360_regmap_config = {
> +	.reg_bits = NAU8360_REG_ADDR_LEN,
> +	.val_bits = NAU8360_REG_DATA_LEN,
> +
> +	.max_register = NAU8360_REG_MAX,
> +	.readable_reg = nau8360_readable_reg,

[Severity: High]
Will setting .val_bits to 16 truncate or reject 32-bit DSP messages?

The custom nau8360_reg_write and nau8360_reg_read functions perform 32-bit I2C
transactions for DSP registers. However, by setting NAU8360_REG_DATA_LEN
(which appears to be 16) for .val_bits, the core regmap framework will set
map->max_val to 0xFFFF. When nau8360_message_to_dsp() writes 32-bit command
payloads, the regmap core's value check will immediately reject the write with
-EINVAL. Additionally, 32-bit reads from the DSP will have their upper 16 bits
silently truncated.

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