Re: [PATCH v2 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] Regmap configuration truncates 32-bit DSP register reads to 16 bits, permanently breaking DSP communication.
- [High] Error handler in `nau8360_dac_mux_put_enum` unconditionally reverses the DSP state, causing severe playback disruption.
- [High] TOCTOU race condition in `nau8360_dac_mux_put_enum` leads to duplicate DAPM widget creation and list corruption.
- [High] Stack buffer overflow in `nau8360_reply_from_dsp` when parsing trailing DSP bytes.
- [High] `nau8360_set_tdm_slot()` fails unconditionally for typical TDM configurations due to zero-initialized slot arrays.
- [High] Synchronous firmware loading during system resume creates a system-wide deadlock hazard.
- [High] PEQ coefficients are ignored by the hardware during `regcache_sync()`, leading to lost EQ settings after resume.
- [High] Data race on `NAU8360_R9D_PEQ_CTL` corrupts hardware register access during concurrent PEQ operations.
- [High] Uninitialized variable `data_count` evaluated conditionally in `nau8360_reply_from_dsp`.
- [Medium] Inappropriate use of `GFP_DMA` exhausts restricted physical memory zones unnecessarily.
--

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

ASoC: codecs: nau8360: Add support for NAU83G60 amplifier

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

> --- /dev/null
> +++ b/sound/soc/codecs/nau8360-dsp.c
[ ... ]
> +static int nau8360_reply_from_dsp(struct snd_soc_component *component,
> +	const struct nau8360_cmd_info *cmd_info, int data_size,
> +	void *data, unsigned short dsp_addr)
> +{
[ ... ]
> +	frag_payload_len = frag_len - 1;
> +	if (cmd_info->msg_param)
> +		data_count = data_size;
> +
> +	for (i = 0; i < frag_payload_len; i++) {
> +		ret = regmap_read(nau8360->regmap, dsp_addr, &payload);
> +		if (ret) {
> +			dev_err(dev, "failed to read payload of dsp");
> +			goto err;
> +		}
> +
> +		if (cmd_info->msg_param) {
> +			if (data_count >= NAU8360_DSP_DATA_BYTE) {
> +				*data_buf++ = payload;
> +				data_count -= NAU8360_DSP_DATA_BYTE;
> +				payload_read(dev, payload);
> +			} else {
> +				for (j = 0; j < NAU8360_DSP_DATA_BYTE; j++) {
> +					((u8 *)data_buf)[j] = (payload >> (j * 8)) & 0xff;
> +					data_count--;
> +					if (data_count <= 0)
> +						break;
> +				}

Is it possible for this loop to write past the end of the destination buffer?
If the DSP hardware sends more payload words than requested, data_count could
drop to 0 or below before entering the else block. The loop writes 1 byte out
of bounds before checking if data_count <= 0, potentially causing a stack
buffer overflow.

> +				payload_read(dev, payload);
> +				break;
> +			}
> +		} else {
> +			*data_buf = payload;
> +			payload_read(dev, payload);
> +		}
> +	}
> +
> +	/* check the reply length same as request */
> +	if (data_count && (cmd_info->cmd_id == NAU8360_DSP_CMD_GET_KCS_RSLTS ||
> +			cmd_info->cmd_id == NAU8360_DSP_CMD_GET_KCS_SETUP)) {

Could data_count be uninitialized here? If cmd_info->msg_param is false,
data_count is never assigned, yet it is evaluated in this condition. This
could result in undefined behavior and trigger panics from dynamic analysis
tools.

> --- /dev/null
> +++ b/sound/soc/codecs/nau8360.c
[ ... ]
> +static int nau8360_peq_coeff_get(struct snd_kcontrol *kcontrol,
> +	struct snd_ctl_elem_value *ucontrol)
> +{
> +	struct snd_soc_component *cp = snd_kcontrol_chip(kcontrol);
> +	struct soc_bytes_ext *params = (void *)kcontrol->private_value;
> +	int i, value, reg = nau8360_peq_regaddr(kcontrol->id.name);
> +	u16 *val = (u16 *)ucontrol->value.bytes.data;
> +
> +	if (reg < 0)
> +		return -EIO;
> +
> +	snd_soc_component_update_bits(cp, NAU8360_R9D_PEQ_CTL, NAU8360_HW1_MEM_TEST,
> +		NAU8360_HW1_MEM_TEST);
> +	for (i = 0; i < params->max / sizeof(u16); i++) {
> +		value = snd_soc_component_read(cp, reg + i);
> +		*(val + i) = cpu_to_be16(value);
> +	}
> +	snd_soc_component_update_bits(cp, NAU8360_R9D_PEQ_CTL, NAU8360_HW1_MEM_TEST, 0);

Can concurrent access to the PEQ registers corrupt hardware state? ALSA .get
and .put callbacks for SND_SOC_BYTES_EXT controls can be executed concurrently
by multiple userspace threads. Toggling NAU8360_HW1_MEM_TEST without an
internal driver lock might allow one thread to clear the bit while another is
still reading or writing, leading to silent data corruption.

> +
> +	return 0;
> +}
> +
> +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 soc_bytes_ext *params = (void *)kcontrol->private_value;
> +	int i, ret, reg = nau8360_peq_regaddr(kcontrol->id.name);
> +	__be16 *data;
> +
> +	if (reg < 0) {
> +		ret = -EIO;
> +		goto err;
> +	}
> +	data = kmemdup(ucontrol->value.bytes.data, params->max, GFP_KERNEL | GFP_DMA);

Is the use of GFP_DMA necessary here? The allocated memory appears to be used
only sequentially by the CPU to byte-swap words before passing them to
snd_soc_component_write, and is never accessed by a hardware DMA controller.
Using GFP_DMA unnecessarily taxes the highly constrained DMA memory zone,
which can lead to spurious allocation failures.

> +	if (!data) {
> +		ret = -ENOMEM;
> +		goto err;
> +	}
> +
> +	snd_soc_component_update_bits(cp, NAU8360_R9D_PEQ_CTL, NAU8360_HW1_MEM_TEST,
> +		NAU8360_HW1_MEM_TEST);
> +	for (i = 0; i < params->max / sizeof(u16); i++)
> +		snd_soc_component_write(cp, reg + i, be16_to_cpu(*(data + i)));
> +	snd_soc_component_update_bits(cp, NAU8360_R9D_PEQ_CTL, NAU8360_HW1_MEM_TEST, 0);
[ ... ]
> +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;
> +	bool dsp_en = snd_soc_enum_item_to_val(e, item[0]);
> +	int ret = 0;
> +
> +	if (nau8360->dsp_enable == dsp_en)
> +		goto done;

Does this check allow a time-of-check to time-of-use race condition? This
check occurs outside of any lock before initiating the DSP switch. If two
threads write the same value concurrently, both could bypass the check and
execute nau8360_dsp_switch. This could result in duplicate execution of
snd_soc_dapm_new_controls and snd_soc_dapm_add_routes, potentially corrupting
the ALSA DAPM widget lists.

> +
> +	if (snd_soc_dapm_get_bias_level(dapm) > SND_SOC_BIAS_STANDBY) {
> +		dev_err(nau8360->dev, "changing path is not allowed during playback");
> +		ret = -EINVAL;
> +		goto err;
> +	}
> +
> +	ret = nau8360_dsp_switch(component, dsp_en);
> +	if (ret)
> +		goto err;
> +
> +	ret = snd_soc_dapm_put_enum_double(kcontrol, ucontrol);
> +	if (ret < 0)
> +		goto err;
> +
> +	nau8360->dsp_enable = dsp_en;
> +
> +done:
> +	return ret;
> +err:
> +	nau8360_dsp_switch(component, !dsp_en);

Could this error handler cause severe playback disruption? If the component is
actively playing audio, the code aborts the mux change and jumps to err, but
then unconditionally forces the DSP into the state opposite to what the user
requested, ignoring the actual current state.

> +	return ret;
> +}
[ ... ]
> +static int nau8360_set_tdm_slot(struct snd_soc_dai *dai, unsigned int tx_mask,
> +	unsigned int rx_mask, int slots, int slot_width)
> +{
[ ... ]
> +	if (tx_mask) {
> +		for (i = 0; i < NAU8360_TDM_TXN; i++) {
> +			func_slot = nau8360->tdm_tx_func_slot[i];
> +			if (!(tx_mask & BIT(func_slot))) {
> +				dev_warn(cp->dev, "TX %s mapped to slot %d, but disabled by tx_mask!",
> +					nau8360_tx_func_names[i], func_slot);
> +				continue;
> +			}
> +
> +			if (tx_slot_used & BIT(func_slot)) {
> +				dev_err(cp->dev, "TX %s slot %d collision!",
> +					nau8360_tx_func_names[i], func_slot);
> +				return -EINVAL;
> +			}

Will this logic always fail for typical TDM configurations? The
tdm_tx_func_slot array is allocated with devm_kzalloc, so all 8 TX functions
default to slot 0. When a machine driver calls snd_soc_dai_set_tdm_slot to
enable slot 0, the first function marks slot 0 as used, and the second
function immediately triggers this collision error.

> +
> +			tx_slot_used |= BIT(func_slot);
> +		}
[ ... ]
> +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;
> +
> +	regcache_cache_only(regmap, false);
> +	regcache_sync(regmap);

Are the PEQ coefficients ignored by the hardware during this regcache_sync?
The hardware requires the NAU8360_HW1_MEM_TEST bit in NAU8360_R9D_PEQ_CTL to
be set before writing to the PEQ coefficient registers. Because that control
register is marked as volatile, it is excluded from the cache, so the test bit
remains inactive (0) while regcache_sync blindly writes the cached PEQ values,
leaving the PEQ RAM uninitialized after resume.

> +	/* disable Sense at standby */
> +	snd_soc_dapm_disable_pin(nau8360->dapm, "Sense");
> +	snd_soc_dapm_sync(nau8360->dapm);
> +
> +	regmap_update_bits(regmap, NAU8360_R90_HW2_CTL0, NAU8360_HW2_STALL, 0);
> +	nau8360_dsp_software_reset(component);
> +	if (nau8360->dsp_enable) {
> +		nau8360_dsp_enable(regmap, true);
> +		/* loading DSP firmware */
> +		ret = nau8360_dsp_reinit(component);

Could this create a system-wide deadlock hazard during resume? The
nau8360_dsp_reinit function synchronously calls request_firmware. If the
filesystem (or block device) is not fully resumed yet, the firmware loader
will block indefinitely, while the filesystem cannot resume because the
suspend/resume sequence is blocked by the driver.

> +		if (ret) {
> +			nau8360_dsp_enable(regmap, false);
> +			dev_err(nau8360->dev, "can't enable DSP (%d)", ret);
> +			goto err;
> +		}
> +	}
> +
> +	return 0;
> +err:
> +	return ret;
> +}
[ ... ]
> +static const struct regmap_config nau8360_regmap_config = {
> +	.reg_bits = NAU8360_REG_ADDR_LEN,
> +	.val_bits = NAU8360_REG_DATA_LEN,

Does this configuration truncate 32-bit DSP register reads to 16 bits?
NAU8360_REG_DATA_LEN is 16. Even though the custom nau8360_reg_read callback
successfully reads 32-bit data for DSP addresses, the regmap core will mask
the returned value using map->format.val_mask (which is 0xFFFF). This
truncates the upper 16 bits of responses like the idle pattern 0xf4f3f2f1,
causing initialization loops to always time out and fail.

> +
> +	.max_register = NAU8360_REG_MAX,
> +	.readable_reg = nau8360_readable_reg,

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