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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 11 potential issue(s) to consider:
- [High] DSP initialization fails unconditionally during resume due to regmap `cache_only` mode.
- [High] Concurrent asynchronous firmware loading requests cause a Use-After-Free during device unbind.
- [High] Unkillable task deadlock on module unload (remove) if the device is removed after a suspend/resume cycle.
- [High] Ignored `regmap_read()` return values result in uninitialized stack variables driving hardware logic.
- [Medium] Silent failure when rejecting DAC mux path changes during playback.
- [Medium] Unprotected access to `nau8360->anc_enable` and hardware registers creates a data race in `nau8360_dsp_switch`.
- [Medium] Short fragments from the DSP can cause uninitialized stack memory to be used in `nau8360_dsp_kcs_setup`.
- [Medium] Out-of-bounds `tdm_tx_func_slot` configurations cause undefined behavior via bit shifts.
- [Medium] Data race on the `dsp_created` flag between firmware callback and ALSA startup.
- [Medium] Manual DAPM pin toggling of the 'Sense' widget breaks I/V capture streams when the DSP is bypassed.
- [Low] Incompatible pointer type warning in `nau8360_validate_tdm_slots`.
--

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

ASoC: codecs: nau8360: Add support for NAU83G60 amplifier

Add support for the Nuvoton NAU83G60 audio codec. The NAU83G60 is a
stereo 30W+30W smart amplifier with an integrated low-latency
Advanced Audio DSP.

> diff --git a/sound/soc/codecs/nau8360-dsp.c b/sound/soc/codecs/nau8360-dsp.c
> index 14ff6c5705d14.....
> --- /dev/null
> +++ b/sound/soc/codecs/nau8360-dsp.c
[ ... ]
> +static int nau8360_read_data_payload(struct snd_soc_component *cp,
> +	unsigned short dsp_addr, int frag_len, bool msg_param,
> +	void *data, int data_size, int *data_count)
> +{
> +	struct nau8360 *nau8360 = snd_soc_component_get_drvdata(cp);
> +	struct device *dev = cp->dev;
> +	unsigned int payload;
> +	u32 *data_buf = data;
> +	int i, j, ret;
> +
> +	*data_count = (msg_param) ? data_size : 0;
> +	for (i = 0; i < frag_len - 1; i++) {

[Severity: Medium]
What happens if the DSP hardware returns a trailing fragment with a frag_len
of 1? 

The loop extracting payload bytes wouldn't execute, leaving the data buffer 
untouched without returning an error.

[ ... ]
> +static int nau8360_dsp_kcs_setup(struct snd_soc_component *cp, int offset, int size,
> +	const void *data, unsigned short dsp_addr)
> +{
[ ... ]
> +		/* checking KCS result */
> +		ret = nau8360_dsp_exec_command(cp, NAU8360_DSP_CMD_GET_KCS_RSLTS,
> +			0, NAU8360_DSP_DATA_BYTE, NULL,
> +			NAU8360_DSP_DATA_BYTE, &kcs_rst, dsp_addr);
> +		if (ret)
> +			return ret;
> +		if (kcs_rst != NAU8360_DSP_KCS_RSLTS_SUCCESS)
> +			return -EINVAL;

[Severity: Medium]
Could this evaluate uninitialized stack memory?

If nau8360_read_data_payload() encounters a frag_len of 1, kcs_rst is left 
uninitialized but the reply validation still returns success. Evaluating 
kcs_rst here could then randomly fail or pass based on whatever was on the 
stack.

[ ... ]
> +static int nau8360_dsp_chan_kcs_setup(struct snd_soc_component *cp,
> +	const char *fw_name, int dsp_addr)
> +{
[ ... ]
> +	ctx = kmalloc(sizeof(*ctx), GFP_KERNEL);
> +	if (!ctx)
> +		return -ENOMEM;
> +
> +	ctx->cp = cp;
> +	ctx->dsp_addr = dsp_addr;
> +
> +	reinit_completion(&nau8360->fw_done[core_id]);
> +
> +	ret = request_firmware_nowait(THIS_MODULE, true,
> +		fw_name, dev, GFP_KERNEL, ctx, nau8360_dsp_fw_cb);

[Severity: High]
Does this create a race condition if called multiple times before the previous 
firmware request finishes?

Reinitializing the completion and launching a duplicate request could leave 
multiple asynchronous callbacks pending, which might bypass later unbind
synchronizations.

> diff --git a/sound/soc/codecs/nau8360.c b/sound/soc/codecs/nau8360.c
> index 4a5fe315850fd.....
> --- /dev/null
> +++ b/sound/soc/codecs/nau8360.c
[ ... ]
> +static int nau8360_get_tdm_chan_len(struct nau8360 *nau8360)
> +{
> +	int val;
> +
> +	regmap_read(nau8360->regmap, NAU8360_R0C_I2S_PCM_CTRL2, &val);
> +	val = (val & NAU8360_TDM_CLEN_MASK) >> NAU8360_TDM_CLEN_SFT;
> +
> +	return (val << 3) + 16;
> +}

[Severity: High]
Can regmap_read() fail here?

If an I2C error occurs, the return value is ignored and the uninitialized 
stack variable val is used to compute the channel length. This garbage 
value then propagates into the mathematical bounds for TDM slots.

[ ... ]
> +static void nau8360_dsp_switch(struct snd_soc_component *component, bool enable)
> +{
> +	struct nau8360 *nau8360 = snd_soc_component_get_drvdata(component);
> +	struct regmap *regmap = nau8360->regmap;
> +	int value = NAU8360_PEQ_BAND_8;
> +
> +	/* If DSP is enabled, unstall HW3 engine and DSP, loading DSP firmware,
> +	 * and configure PEQ after dsp reset.
> +	 */
> +	if (enable) {
> +		value = nau8360->anc_enable ? NAU8360_PEQ_BAND_15 : NAU8360_PEQ_BAND_12;
> +		nau8360_dsp_enable(regmap, true);
> +	} else {
> +		dev_dbg(nau8360->dev, "Bypass DSP path");
> +		nau8360_dsp_enable(regmap, false);
> +	}
> +	regmap_update_bits(regmap, NAU8360_R9D_PEQ_CTL, NAU8360_PEQ_BAND_MASK,
> +		value << NAU8360_PEQ_BAND_SFT);
> +}

[Severity: Medium]
Is it safe to read nau8360->anc_enable and update the PEQ register without
holding nau8360->lock? 

Looking at nau8360_anc_put(), it protects these variables with a mutex.
Concurrent execution from ALSA control put handlers might result in a stale
hardware state here.

[ ... ]
> +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;
> +	}

[Severity: Medium]
Should this return an error code like -EBUSY? 

Returning 0 when rejecting the change silently fails and tricks userspace 
into thinking the hardware path was updated.

[ ... ]
> +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;
> +
> +	if (nau8360->dsp_created != NAU8360_DSP_FW_ALL_DONE) {
> +		dev_warn(nau8360->dev, "DSP firmware is not ready yet!");
> +		return -EBUSY;
> +	}

[Severity: Medium]
Is a lock needed here when checking the dsp_created flag? 

The firmware callback nau8360_dsp_fw_cb() modifies this flag under 
nau8360->lock. Reading it unlocked might cause a torn read and spuriously 
reject playback streams.

> +
> +	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);

[Severity: High]
Does regmap_read() need its return value checked here? 

Similar to nau8360_get_tdm_chan_len(), if this read fails due to bus issues, 
an uninitialized stack variable will be evaluated to determine if the receiver 
should be enabled.

> +
> +		if (nau8360_dsp_active(component))
> +			snd_soc_dapm_enable_pin(nau8360->dapm, "Sense");
> +	}

[Severity: Medium]
Will this break I/V capture streams when the DSP is bypassed? 

The Sense widget is the DAPM source for ADC_I and ADC_V. If it's never enabled 
when nau8360_dsp_active() evaluates to false, DAPM won't power up the ADCs, 
leaving the host recording silence over TDM.

[ ... ]
> +static int nau8360_validate_tdm_slots(struct device *dev, unsigned int mask,
> +	const int *func_slots, const char * const *func_names,
> +	int num_funcs, const char *dir,
> +	unsigned int *slot_used)

[Severity: Low]
Does the func_slots parameter need its type updated? 

The struct fields tdm_tx_func_slot and tdm_rx_func_slot were updated to u32, 
which results in passing an unsigned int array to a const int pointer here.

> +{
> +	int i;
> +	unsigned int func_slot;
> +	*slot_used = 0;
> +
> +	if (!mask)
> +		return 0;
> +
> +	for (i = 0; i < num_funcs; i++) {
> +		func_slot = func_slots[i];
[ ... ]
> +		if (func_slot >= NAU8360_TDM_MAX_CHAN) {
> +			dev_warn(dev, "Invalid %s %s slot index: %d",
> +				dir, func_names[i], func_slot);
> +			continue;
> +		}

[Severity: Medium]
Should this clear the invalid slot from the array? 

It logs a warning and continues, but leaves the out-of-bounds value intact in
the original array for later consumption.

[ ... ]
> +static void nau8360_enable_tdm_channels(struct snd_soc_component *cp,
> +	int rx_slot_used, int tx_slot_used)
> +{
> +	struct nau8360 *nau8360 = snd_soc_component_get_drvdata(cp);
> +	int i, slot;
> +	unsigned int val = 0;
> +	bool enable;
> +
> +	for (i = 0; i < NAU8360_TDM_TXN; i++) {
> +		slot = nau8360->tdm_tx_func_slot[i];
> +		enable = (slot != TDM_SLOT_NONE) && (tx_slot_used & BIT(slot));

[Severity: Medium]
Can this trigger undefined behavior? 

If the invalid slot value from earlier (e.g. >= 32) remains in the array, 
evaluating BIT(slot) will bit-shift out of bounds.

[ ... ]
> +static int __maybe_unused nau8360_suspend(struct snd_soc_component *component)
> +{
> +	struct nau8360 *nau8360 = snd_soc_component_get_drvdata(component);
> +	int i;
> +
> +	for (i = 0; i < NAU8360_DSP_CORE_NUM; i++)
> +		wait_for_completion(&nau8360->fw_done[i]);

[Severity: High]
Does this consume the completion token without replacing it if the resume
fails? 

If nau8360_resume() doesn't properly replenish the token, a subsequent module 
unload will deadlock forever when it waits on this completion.

[ ... ]
> +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;
> +	int ret;
> +
> +	/* disable Sense at standby */
> +	snd_soc_dapm_disable_pin(nau8360->dapm, "Sense");
> +	snd_soc_dapm_sync(nau8360->dapm);
> +
> +	ret = nau8360_dsp_setup(component);
> +
> +	regcache_cache_only(regmap, false);

[Severity: High]
Can the DSP initialization succeed here? 

nau8360_dsp_setup() is called before clearing the cache_only flag. Since 
nau8360_dsp_setup() relies on polling hardware registers over the bus, it 
will silently fail every time during resume, skipping firmware loads and 
leaving the hardware unprotected.

[ ... ]
> +static void nau8360_i2c_remove(struct i2c_client *client)
> +{
> +	struct nau8360 *nau8360 = i2c_get_clientdata(client);
> +	int i;
> +
> +	for (i = 0; i < NAU8360_DSP_CORE_NUM; i++)
> +		wait_for_completion(&nau8360->fw_done[i]);
> +}

[Severity: High]
If nau8360_suspend() consumed the token and nau8360_resume() failed to replace
it, won't this cause an unkillable deadlock?

Additionally, if duplicate firmware requests were launched by 
nau8360_dsp_chan_kcs_setup(), this wait_for_completion() will only synchronize 
against the first one. The second pending callback could execute after the 
device unbinds, triggering a use-after-free on the nau8360 struct.

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