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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 12 potential issue(s) to consider:
- [Critical] 32-bit DSP payloads are silently discarded by the 16-bit regmap configuration.
- [High] ACPI probing fails due to Device Tree-exclusive APIs.
- [High] Use-After-Free due to uncancelled asynchronous firmware loading.
- [High] DMA-unsafe stack buffers used for I2C transfers.
- [High] Uninitialized stack variables used when regmap_read fails.
- [High] Uninitialized `ret` variable returned in TDM kcontrols.
- [High] DSP protocol corruption due to missing locking in the DAPM clock event handler.
- [Medium] Setting TDM slots to 'Slot None' fails to update hardware registers.
- [Medium] TDM TX slot offsets become mathematically incorrect when channel length changes.
- [Medium] Missing `FW_LOADER` dependency causes silent probe failure.
- [Medium] Data race on `tdm_chan_len` state during concurrent `hw_params` operations.
- [Medium] TDM streams are not disabled when slots or slot_width evaluates to zero.
--

commit 68a685818cfb23df9395e09002059cbcf768941a
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/Kconfig b/sound/soc/codecs/Kconfig
> --- a/sound/soc/codecs/Kconfig
> +++ b/sound/soc/codecs/Kconfig

[ ... ]

> @@ -2837,6 +2838,15 @@ config SND_SOC_NAU8825
>  	tristate
>  	depends on I2C
>  
> +config SND_SOC_NAU8360
> +	tristate "Nuvoton Technology Corporation NAU83G60 Stereo Smart Amplifier"
> +	depends on I2C

[Severity: Medium]
Could the omission of a select FW_LOADER dependency cause silent probe
failures on minimal configurations when request_firmware_nowait() is called?

> +	help
> +	  The NAU83G60 is a stereo 30W+30W smart amplifier with integrated

[ ... ]

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

[ ... ]

> @@ -149,6 +149,42 @@ static int nau8360_message_to_dsp(struct snd_soc_component *component,
> ...
> +	/* sending payload + padding */
> +	payload = nau8360_pack_param(param_offset, param_size);
> +	snd_soc_component_write(component, dsp_addr, payload);

[Severity: Critical]
Will these 32-bit DSP payloads be silently discarded by the regmap validation
layer since val_bits is set to 16, and should we check the return value of
snd_soc_component_write()?

> +	frag_cnt++;

[ ... ]

> @@ -308,6 +308,24 @@ static void nau8360_dsp_fw_cb(const struct firmware *fw, void *context)
> ...
> +	mutex_lock(&nau8360->lock);

[Severity: High]
Can this result in a use-after-free if the device is unbound before the
asynchronous firmware callback finishes, considering the devm-managed nau8360
struct might already be freed?

[ ... ]

> @@ -345,6 +345,26 @@ static int nau8360_dsp_chan_kcs_setup(struct snd_soc_component *cp,
> ...
> +	ret = request_firmware_nowait(THIS_MODULE, true,
> +		fw_name, cp->dev, GFP_KERNEL, ctx, nau8360_dsp_fw_cb);

[Severity: High]
Is there a risk of a use-after-free here since the asynchronous request is
launched without an associated cancellation handle in a remove hook?

[ ... ]

> @@ -404,6 +404,22 @@ static int nau8360_dsp_clock_event(struct snd_soc_dapm_widget *w,
> ...
> +	dsp_dbg(component->dev, "send DSP command %s", dsp_cmd_table[cmd]);
> +	ret = nau8360_send_dsp_broadcast(component, cmd);

[Severity: High]
Does this transmit multi-fragment I2C payloads without acquiring the
protective nau8360->lock, potentially corrupting the DSP protocol if
asynchronous firmware loading is executing concurrently?

[ ... ]

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

[ ... ]

> @@ -493,6 +493,34 @@ static int nau8360_tdm_tx_slot_put(struct snd_kcontrol *kcontrol,
> +	struct soc_enum *e = (struct soc_enum *)kcontrol->private_value;
> +	unsigned int *item = ucontrol->value.enumerated.item;
> +	unsigned int mask = e->mask << e->shift_l;
> +	int ret, scale, slot, func_idx, val_to_cache;
> +	bool changed = false;
> +	bool is_none = (item[0] == NAU8360_TDM_TXN);
> +

[ ... ]

> +	if (is_none) {
> +		dev_dbg(cp->dev, "Slot is None, skipping register update");
> +		goto done;
> +	}

[Severity: Medium]
Does bypassing the register update here leave the hardware state stale when
Slot None is selected?

> +
> +	compute_slotx_scale(cp, &scale);
> +	slot = item[0] * scale;
> +	ret = snd_soc_component_update_bits(cp, e->reg, mask, slot << e->shift_l);

[Severity: Medium]
Do the TX slot offsets become mathematically incorrect if
nau8360_set_tdm_slot() is later called to change the channel length without
recalculating the existing active TX offsets?

> +
> +done:
> +	mutex_unlock(&nau8360->lock);
> +	return ret < 0 ? ret : (int)changed;

[Severity: High]
Could this return an uninitialized ret variable to user-space if the
requested slot matches the current cached slot or if Slot None is chosen,
jumping directly to the done label?

> +}

[ ... ]

> @@ -998,6 +998,22 @@ static inline int nau8360_vbat_level(struct regmap *regmap, int *vbat)
> +{
> +	struct device *dev = regmap_get_device(regmap);
> +	int value;
> +
> +	regmap_read(regmap, NAU8360_R21_VBAT_READOUT, &value);
> +
> +	/* multiple 100 on value scale */
> +	*vbat = (value * 100 + NAU8360_VBAT_BASE) / NAU8360_VBAT_STEP;

[Severity: High]
Can this evaluate an uninitialized stack variable if the underlying I2C read
fails, causing unpredictable hardware behavior?

[ ... ]

> @@ -1111,6 +1111,54 @@ static int nau8360_set_tdm_slot(struct snd_soc_dai *dai, unsigned int tx_mask,
> ...
> +	if (!slots || !slot_width)
> +		return 0;

[Severity: Medium]
Will returning early here prevent the hardware from explicitly disabling TDM
streams when slots or slot_width evaluates to zero?

[ ... ]

> +	mutex_unlock(&nau8360->lock);
> +
> +	nau8360->tdm_chan_len = slot_width;

[Severity: Medium]
Does modifying this shared state variable outside the bounds of the
synchronization mutex introduce a data race during concurrent hardware
parameter operations?

> +	regmap_update_bits(nau8360->regmap, NAU8360_R0C_I2S_PCM_CTRL2,
> +		NAU8360_TDM_CLEN_MASK,
> +		((nau8360->tdm_chan_len - 16) >> 3) << NAU8360_TDM_CLEN_SFT);

[ ... ]

> @@ -2313,6 +2313,22 @@ static int nau8360_reg_write(void *context, unsigned int reg, unsigned int value)
> ...
> +	u8 buf[6];
> ...
> +	ret = i2c_master_send(client, buf, count);

[Severity: High]
Is it safe to pass stack-allocated buffers to the I2C transfer layer,
considering stack memory might not be DMA-mappable with CONFIG_VMAP_STACK
enabled?

[ ... ]

> @@ -2335,6 +2335,26 @@ static int nau8360_reg_read(void *context, unsigned int reg, unsigned int *value)
> ...
> +	u8 buf[4];
> +	u16 reg_buf;
> ...
> +	ret = i2c_transfer(client->adapter, xfer, ARRAY_SIZE(xfer));

[Severity: High]
Can passing these stack-allocated buffers to i2c_transfer() cause issues
if the I2C controller utilizes a DMA engine?

[ ... ]

> @@ -2358,6 +2358,16 @@ static const struct regmap_config nau8360_regmap_config = {
> +	.reg_bits = NAU8360_REG_ADDR_LEN,
> +	.val_bits = NAU8360_REG_DATA_LEN,

[Severity: Critical]
Does setting val_bits to 16 restrict the regmap core to a maximum value of
0xFFFF, which would silently discard the 32-bit DSP payloads used in
nau8360_message_to_dsp()?

[ ... ]

> @@ -2379,6 +2379,30 @@ static int nau8360_read_device_properties(struct nau8360 *nau8360)
> ...
> +	ret = of_property_count_strings(np, "firmware-name");

[Severity: High]
Could this cause ACPI probing to fail since of_property_count_strings()
expects a non-NULL Device Tree node? Should we use device_property_* array
reading functions instead to ensure ACPI compatibility?

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