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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 10 potential issue(s) to consider:
- [High] Unsynchronized asynchronous firmware loading during repeated suspend/resume cycles or component unbinding leads to a Use-After-Free of the device struct.
- [High] TDM TX channels are incorrectly enabled using function indices instead of slot indices, breaking TDM routing.
- [High] `nau8360_read_data_payload()` reads an extra fragment, consuming the trailing fragment and causing protocol desynchronization.
- [High] Failure to check the return value of `regmap_read` leads to the use of uninitialized stack variables.
- [High] 32-bit payload writes to DSP registers are silently rejected by the regmap core because the driver configures `val_bits = 16`.
- [High] The custom `nau8360_reg_read` function uses a single 6-byte `i2c_read_buf` for both the TX and RX phases of an I2C transfer, causing an unsafe DMA cacheline sharing violation.
- [Medium] Missing bounds check on TDM slot indices read from Device Tree leads to undefined behavior via out-of-bounds shift.
- [Medium] `nau8360_set_tdm_slot` lacks mutex protection on the fast path, causing race conditions in concurrent TDM configurations.
- [Medium] TOCTOU race in `nau8360_dac_mux_put_enum` allows changing the DAC path during active playback.
- [Low] Passing `int *` to `device_property_read_u32_array` causes an incompatible pointer type warning.
--

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

ASoC: codecs: nau8360: Add support for NAU83G60 amplifier

This commit adds support for the Nuvoton NAU83G60 audio codec. It features 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
> --- a/sound/soc/codecs/nau8360-dsp.c
> +++ b/sound/soc/codecs/nau8360-dsp.c

[ ... ]

> @@ -198,6 +198,8 @@ static int nau8360_message_to_dsp(struct snd_soc_component *cp,
>  		return ret;
>  
>  	/* sending preamble fragment */
> +	payload = nau8360_pack_preamble(cmd_info->cmd_id, frag_len);
> +	snd_soc_component_write(cp, dsp_addr, payload);

[Severity: High]
The regmap configuration sets val_bits = 16, which caps the maximum allowed
write value to 0xFFFF. In nau8360_message_to_dsp(), 32-bit payloads are
written without checking the return value. Will the regmap framework silently
reject these 32-bit writes, preventing any DSP communication?

[ ... ]

> @@ -308,6 +308,18 @@ static int nau8360_read_data_payload(struct snd_soc_component *cp,
>  		if (*data_count >= NAU8360_DSP_DATA_BYTE) {
>  			*data_buf++ = payload;
>  			*data_count -= NAU8360_DSP_DATA_BYTE;
> +		} else if (*data_count > 0) {
> +			for (j = 0; j < *data_count; j++)
> +				((u8 *)data_buf)[j] = (payload >> (j * 8)) & 0xff;
> +
> +			*data_count = 0;
> +			break;
> +		}
> +	}

[Severity: High]
The loop executes frag_len - 1 times. When msg_param is true and data_size is
fully consumed, *data_count becomes 0 via the *data_count -=
NAU8360_DSP_DATA_BYTE path, but the loop does not break. Will this execute
the remaining iterations and mistakenly consume the trailing fragment,
causing a protocol error when nau8360_validate_trailing() subsequently
attempts to read it?

[ ... ]

> @@ -628,6 +628,15 @@ static int nau8360_dsp_chan_kcs_setup(struct snd_soc_component *cp,
>  
>  	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]
The function request_firmware_nowait() executes asynchronously and lacks a
component remove callback to cancel it. In nau8360_i2c_remove(), we wait for
the completion only once per core. If multiple callbacks were queued during
repeated suspend and resume cycles, could the later callbacks execute after
devres frees the device context, causing a use-after-free on the nau8360
struct?

> diff --git a/sound/soc/codecs/nau8360.c b/sound/soc/codecs/nau8360.c
> --- a/sound/soc/codecs/nau8360.c
> +++ b/sound/soc/codecs/nau8360.c

[ ... ]

> @@ -321,6 +321,11 @@ 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;

[Severity: High]
The return value of regmap_read() is not checked before val is used. If the
I2C transfer fails, could this result in operating on uninitialized stack
memory and configuring corrupted channel lengths?

[ ... ]

> @@ -557,6 +557,17 @@ static int nau8360_validate_tdm_slots(struct device *dev, unsigned int mask,
>  			continue;
>  		}
>  
> +		if (!(mask & BIT(func_slot))) {
> +			dev_warn(dev, "%s %s mapped to slot %d, but disabled by mask!",
> +				dir, func_names[i], func_slot);
> +			continue;
> +		}
> +
> +		if (*slot_used & BIT(func_slot)) {
> +			dev_err(dev, "%s %s slot %d collision!",
> +				dir, func_names[i], func_slot);
> +			return -EINVAL;
> +		}

[Severity: Medium]
The func_slot values are read from the device tree and directly passed to the
BIT(func_slot) macro. If a device tree specifies a slot index of 32 or
greater, does this lead to an out-of-bounds shift and undefined behavior?

[ ... ]

> @@ -587,6 +587,14 @@ static void nau8360_enable_tdm_channels(struct snd_soc_component *cp,
>  	int i;
>  	unsigned int val = 0;
>  
> +	for (i = 0; i < NAU8360_TDM_TXN; i++)
> +		nau8360_set_tdm_tx_func(cp, i, tx_slot_used & BIT(i));

[Severity: High]
The tx_slot_used variable represents a bitmask of active slot IDs. Here, the
loop iterates over the function index i. Is it intentional to perform a
bitwise AND against the function index instead of the slot ID? This appears
to enable a function only if its assigned slot ID happens to match its
function ID.

[ ... ]

> @@ -614,6 +614,10 @@ static int nau8360_set_tdm_slot(struct snd_soc_dai *dai, unsigned int tx_mask,
>  	unsigned int tx_slot_used = 0, rx_slot_used = 0;
>  	int ret = 0;
>  
> +	if (!slots || !slot_width) {
> +		nau8360_enable_tdm_channels(cp, 0, 0);
> +		return 0;
> +	}

[Severity: Medium]
The fast path for empty slots or slot_width bypasses the internal lock. If a
concurrent thread configures TDM from another audio stream, could this
lockless path interleave hardware updates and corrupt the shared hardware
state?

[ ... ]

> @@ -865,6 +865,11 @@ static int nau8360_dac_mux_put_enum(struct snd_kcontrol *kcontrol,
>  	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;
> +	}
> +
> +	ret = snd_soc_dapm_put_enum_double(kcontrol, ucontrol);

[Severity: Medium]
The snd_soc_dapm_get_bias_level() check is performed before calling
snd_soc_dapm_put_enum_double(), which acquires the DAPM mutex. Could a
concurrent playback stream acquire the lock and start playback in between
these calls, resulting in the hardware path being changed during active
playback?

[ ... ]

> @@ -1659,6 +1659,10 @@ static int nau8360_read_device_properties(struct nau8360 *nau8360)
>  	const char *firmware_names[NAU8360_DSP_FW_NUM];
>  	int i, ret;
>  
> +	ret = device_property_read_u32_array(dev, "nuvoton,dsp-tx-slot-mapping",
> +		nau8360->tdm_tx_func_slot, NAU8360_TDM_TXN);

[Severity: Low]
The arrays nau8360->tdm_tx_func_slot and tdm_rx_func_slot are declared as
arrays of int. Does passing them to device_property_read_u32_array(), which
expects a u32 pointer, cause a compiler warning about incompatible pointer
types?

[ ... ]

> @@ -1987,6 +1987,14 @@ static int nau8360_reg_read(void *context, unsigned int reg, unsigned int *value
>  	struct i2c_msg xfer[2];
>  	int ret;
>  
> +	nau8360->i2c_read_buf[0] = (reg >> 8) & 0xff;
> +	nau8360->i2c_read_buf[1] = reg & 0xff;
> +	xfer[0].addr = client->addr;
> +	xfer[0].len = 2;
> +	xfer[0].buf = nau8360->i2c_read_buf;
> +	xfer[0].flags = 0;
> +
> +	xfer[1].addr = client->addr;
> +	xfer[1].len = (NAU8360_IS_DSP_REG(reg)) ? 4 : 2;
> +	xfer[1].buf = nau8360->i2c_read_buf + 2;
> +	xfer[1].flags = I2C_M_RD;
> +
> +	ret = i2c_transfer(client->adapter, xfer, ARRAY_SIZE(xfer));

[Severity: High]
The buffer i2c_read_buf is used for both the transmit and receive phases of
the I2C transfer within the same i2c_transfer() call. Since the buffer is
only 6 bytes, both point into the exact same cache line. On architectures
with non-coherent DMA, could concurrent cache invalidations for the receive
buffer destroy the transmit data?

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