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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 13 potential issue(s) to consider:
- [Critical] The regmap configuration forces 16-bit truncation on 32-bit DSP registers.
- [High] Dynamic creation of DAPM widgets and routes within an ALSA kcontrol `put` handler without instantiation.
- [High] Using atomic polling with a 1000us delay on a sleepable I2C regmap.
- [High] TDM TX is permanently disabled after the first capture stream stops.
- [High] Concurrent ALSA kcontrol callbacks race on shared hardware state, causing corrupted PEQ memory accesses.
- [High] Synchronous `request_firmware` in a kcontrol `put` handler and `resume` path causes deadlocks.
- [High] PEQ coefficients are silently lost during suspend/resume due to missing hardware memory access bit.
- [High] Stack-allocated memory buffers are passed directly to I2C transfer functions.
- [Medium] `nau8360_vbat_level()` uses uninitialized stack memory if `regmap_read()` fails.
- [Medium] Unhandled failure in `nau8360_dsp_init` causes duplicated DAPM widgets to be leaked.
- [Medium] Uninitialized stack variable `status` is used to verify DSP state if the DSP returns a 0-length or trailing-only reply.
- [Medium] TOCTOU race when checking DAPM bias level and unprotected state modification of `dsp_enable`.
- [Medium] Unprotected concurrent access to shared TDM slot caching arrays.
--

commit ecf4fed8f174ef2256d85d18a781234a1012265b
Author: Neo Chang <YLCHANG2@nuvoton.com>
Subject: 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.

> 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

[ ... ]

> +/* checking for DSP IDLE pattern */
> +static int nau8360_dsp_idle(struct snd_soc_component *component, unsigned short dsp_addr)
> +{
> +	struct nau8360 *nau8360 = snd_soc_component_get_drvdata(component);
> +	unsigned int idle_pattern, timeout = NAU8360_DSP_IDLE_RETRY * USEC_PER_MSEC;
> +	int ret;
> +
> +	ret = regmap_read_poll_timeout_atomic(nau8360->regmap, dsp_addr, idle_pattern,
> +		idle_pattern == NAU8360_DSP_COMM_IDLE_WORD, USEC_PER_MSEC, timeout);

[Severity: High]
Does using the atomic variant of regmap poll timeout with a 1000 microsecond
delay cause soft lockups due to prolonged busy-waiting? Since the underlying
I2C transfer sleeps anyway, should this use the non-atomic
regmap_read_poll_timeout instead?

[ ... ]

> +static int nau8360_dsp_chan_kcs_setup(struct snd_soc_component *cp,
> +	const char *fw_name, int dsp_addr)
> +{
> +	struct nau8360 *nau8360 = snd_soc_component_get_drvdata(cp);
> +	const struct firmware *fw;
> +	int ret, status, buf_off, buf_len;
> +
> +	ret = nau8360_dsp_get_cmd_put(cp, dsp_addr,
> +			NAU8360_DSP_CMD_GET_FRAME_STATUS, &status);
> +	if (ret || !(status & NAU8360_DSP_ALGO_OK)) {

[Severity: Medium]
If the DSP returns a reply containing only a trailing fragment, does the
payload read loop in nau8360_reply_from_dsp get skipped, leaving the status
variable unmodified? Can this cause this uninitialized stack variable to be
evaluated here, leading to unpredictable behavior?

> +		dev_err(cp->dev, "DSP %x is not ready", dsp_addr);
> +		ret = -EIO;
> +		goto err;
> +	}
> +
> +	dev_info(cp->dev, "DSP %x is ready to load firmware %s, status %x",
> +		dsp_addr, fw_name, status);
> +
> +	ret = request_firmware(&fw, fw_name, cp->dev);

[Severity: High]
Can calling request_firmware synchronously from a kcontrol put handler
(via nau8360_dac_mux_put_enum) or during the resume path risk blocking the
ALSA controls rwsem or deadlocking the resume thread?

[ ... ]

> +int nau8360_dsp_init(struct snd_soc_component *component)
> +{
> +	struct nau8360 *nau8360 = snd_soc_component_get_drvdata(component);
> +	struct snd_soc_dapm_context *dapm = nau8360->dapm;
> +	int ret;
> +
> +	dev_info(component->dev, "DSP initializing...");
> +	ret = nau8360_dsp_set_kcs_setup(component);
> +	if (ret)
> +		goto err;
> +
> +	ret = snd_soc_dapm_new_controls(dapm, nau8360_dsp_dapm_widgets,
> +			ARRAY_SIZE(nau8360_dsp_dapm_widgets));
> +	if (ret) {
> +		dev_err(component->dev, "add DSP widget fail (%d)", ret);
> +		goto err;
> +	}
> +	ret = snd_soc_dapm_add_routes(dapm, nau8360_dsp_dapm_routes,
> +			ARRAY_SIZE(nau8360_dsp_dapm_routes));

[Severity: High]
Does adding DAPM widgets and routes dynamically from a kcontrol put handler
leave the new widgets uninstantiated without a subsequent call to
snd_soc_dapm_new_widgets()? Could this cause the DSP Clock supply to fail to
power up?

[Severity: Medium]
If snd_soc_dapm_add_routes fails here, does this return an error without
freeing the widgets created by snd_soc_dapm_new_controls? Since dsp_created
remains false, will subsequent attempts to enable the DSP allocate duplicate
widgets and leak memory?

> 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

[ ... ]

> +static int nau8360_tdm_rx_slot_put(struct snd_kcontrol *kcontrol,
> +	struct snd_ctl_elem_value *ucontrol)
> +{
> +	struct snd_soc_component *cp = snd_kcontrol_chip(kcontrol);
> +	struct nau8360 *nau8360 = snd_soc_component_get_drvdata(cp);
> +	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, func_idx, val_to_cache;
> +	bool changed = false;
> +	bool is_none = (item[0] == NAU8360_TDM_RXN);
> +
> +	func_idx = nau8360_get_tdm_rx_func_idx(e);
> +	if (item[0] >= e->items || func_idx < 0)
> +		return -EINVAL;
> +
> +	val_to_cache = is_none ? TDM_SLOT_NONE : item[0];
> +	if (nau8360->tdm_rx_func_slot[func_idx] == val_to_cache) {
> +		dev_dbg(cp->dev, "RX func %s: Slot no change (%d)",
> +			nau8360_rx_func_names[func_idx], val_to_cache);
> +		goto done;
> +	}
> +
> +	nau8360->tdm_rx_func_slot[func_idx] = val_to_cache;

[Severity: Medium]
Are the arrays tdm_rx_func_slot and tdm_tx_func_slot updated without any
locking? Can concurrent ALSA put callbacks or overlapping hw_params calls
result in data races and inconsistent slot validation logic?

[ ... ]

> +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;
> +	bool changed = false;
> +
> +	if (reg < 0) {
> +		ret = -EIO;
> +		goto err;
> +	}
> +	data = kmemdup(ucontrol->value.bytes.data, params->max, GFP_KERNEL);
> +	if (!data) {
> +		ret = -ENOMEM;
> +		goto err;
> +	}
> +
> +	snd_soc_component_update_bits(cp, NAU8360_R9D_PEQ_CTL, NAU8360_HW1_MEM_TEST,
> +		NAU8360_HW1_MEM_TEST);

[Severity: High]
Can concurrent execution of the PEQ get and put kcontrol callbacks race on
the NAU8360_HW1_MEM_TEST bit modification? Could one thread clear the bit
while another is reading or writing, leading to corrupted PEQ memory accesses?

[ ... ]

> +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 explicitly clearing the NAU8360_EN_TDM_TX bit during shutdown without
re-enabling it during startup or hw_params cause subsequent capture streams
to fail and yield silence?

[ ... ]

> +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;
> +
> +	if (snd_soc_dapm_get_bias_level(dapm) > SND_SOC_BIAS_STANDBY) {

[Severity: Medium]
Is this check for the DAPM bias level vulnerable to a time-of-check to
time-of-use race since it happens outside the DAPM card mutex? Could a
concurrent stream start immediately after the check, leading to DSP path
switching while playback is active? Also, is nau8360->dsp_enable being
accessed and modified without locking here?

[ ... ]

> +static inline int nau8360_vbat_level(struct regmap *regmap)
> +{
> +	int value;
> +
> +	regmap_read(regmap, NAU8360_R21_VBAT_READOUT, &value);
> +	/* multiple 100 on value scale */
> +	return (value * 100 + NAU8360_VBAT_BASE) / NAU8360_VBAT_STEP;
> +}

[Severity: Medium]
If the I2C read fails, does this function use an uninitialized stack
variable to calculate the VBAT level? Could this lead to configuring
critical sawtooth parameters based on corrupted data if the garbage value
happens to fall within the valid range?

[ ... ]

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

[Severity: High]
Since regcache_sync restores registers in numerical order, will it write the
PEQ coefficients starting at 0x100 before the HW1_MEM_TEST bit is set at
0x9D, causing the hardware to silently reject the writes and lose custom
equalizer settings across suspend and resume cycles?

[ ... ]

> +static int nau8360_reg_write(void *context, unsigned int reg, unsigned int value)
> +{
> +	struct i2c_client *client = context;
> +	int ret, count = 0;
> +	u8 buf[6];
> +
> +	buf[count++] = reg >> 8;
> +	buf[count++] = reg & 0xff;
> +	if (reg != NAU8360_RF000_DSP_COMM && reg != NAU8360_RF002_DSP_COMM) {
> +		/* format for Codec, 2 bytes value and endian big */
> +		buf[count++] = value >> 8;
> +		buf[count++] = value & 0xff;
> +		dev_dbg(&client->dev, " %x <= %x", reg, value);
> +	} else {
> +		/* format for DSP, 4 bytes value and native */
> +		buf[count++] = value & 0xff;
> +		buf[count++] = (value >> 8) & 0xff;
> +		buf[count++] = (value >> 16) & 0xff;
> +		buf[count++] = (value >> 24) & 0xff;
> +	}
> +
> +	ret = i2c_master_send(client, buf, count);

[Severity: High]
Are stack-allocated arrays being passed directly to i2c_master_send and
i2c_transfer? If the I2C adapter utilizes DMA, does mapping stack memory
violate architecture rules and risk memory corruption?

[ ... ]

> +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,
> +	.writeable_reg = nau8360_writeable_reg,
> +	.volatile_reg = nau8360_volatile_reg,
> +	.reg_read = nau8360_reg_read,
> +	.reg_write = nau8360_reg_write,

[Severity: Critical]
Since nau8360_regmap_config sets val_bits to 16, does the regmap core silently
apply a 0xFFFF mask to all read values? When reading 32-bit DSP registers
via the custom nau8360_reg_read, does this cause the upper 16 bits to be
discarded, breaking DSP firmware loading and communication?

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