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

Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
- [High] Firmware memory leak on DSP setup error path.
- [High] Out-of-bounds array read in dev_dbg due to unvalidated `source` parameter.
- [High] State loss and audio breakage due to permanent disabling of TDM bits on shutdown.
- [High] API Contract Violation: Abuse of SND_SOC_CLOCK_OUT breaks generic machine drivers.
- [High] Uninitialized stack memory used due to ignored regmap_read return value.
- [Medium] TOCTOU race in ALSA control handlers allowing unsafe hardware access during playback.
- [Medium] Lockless access to `load_fw_done` flag without memory barriers.
--

--- Patch [2]: [PATCH v8 2/2] ASoC: codecs: nau8360: Add support for NAU83G60 amplifier ---
commit ff5d05e48c27ea57bf7a154300926246f16f3e8f
Author: Neo Chang <[email protected]>
Date:   Thu Aug 13 06:43:27 2026 +0000

ASoC: codecs: nau8360: Add support for NAU83G60 amplifier

This commit adds support for the Nuvoton NAU83G60 audio codec. The driver
includes I2C initialization, ALSA DAPM configuration, TDM routing, and an
asynchronous workqueue mechanism for DSP firmware loading.

> 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
[ ... ]
> @@ -467,6 +467,46 @@ static int nau8360_dsp_chan_kcs_setup(struct snd_soc_component *cp,
[ ... ]
> +	ret = request_firmware(&fw, fw_name, cp->dev);
> +	if (ret) {
> +		dev_err(cp->dev, "failed to load firmware (%d)", ret);
> +		return ret;
> +	}
> +
> +	buf_off = 0;
> +	buf_len = nau8360->kcs_setup_size = fw->size;
> +	mutex_lock(&nau8360->lock);
> +	ret = nau8360_dsp_kcs_setup(cp, buf_off, buf_len, fw->data, dsp_addr);
> +	mutex_unlock(&nau8360->lock);
> +	if (ret) {
> +		dev_err(cp->dev, "send DSP command %s fail (%d)",
> +			dsp_cmd_table[NAU8360_DSP_CMD_SET_KCS_SETUP], ret);
> +		return ret;

[Severity: High]
Does this error path leak the firmware memory? The function returns early
here without calling release_firmware() on fw, which could lead to a leak if
the DSP initialization fails.

> +	}
> +	release_firmware(fw);
> +
> +	return 0;
> +}

[ ... ]
> diff --git a/sound/soc/codecs/nau8360.c b/sound/soc/codecs/nau8360.c
> --- /dev/null
> +++ b/sound/soc/codecs/nau8360.c
[ ... ]
> @@ -156,6 +156,15 @@ static bool nau8360_volatile_reg(struct device *dev, unsigned int reg)
[ ... ]
> +static int nau8360_get_tdm_chan_len(struct nau8360 *nau8360)
> +{
> +	int val;
> +
> +	regmap_read(nau8360->regmap, NAU8360_R0C_I2S_PCM_CTRL2, &val);

[Severity: High]
Can this result in uninitialized stack memory usage? The return value of
regmap_read() is ignored here. If the underlying I2C transfer fails, val will
remain uninitialized.

> +	val = (val & NAU8360_TDM_CLEN_MASK) >> NAU8360_TDM_CLEN_SFT;
> +
> +	return (val << 3) + 16;
> +}

[ ... ]
> @@ -234,6 +234,31 @@ static int nau8360_peq_coeff_put(struct snd_kcontrol *kcontrol,
[ ... ]
> +static int nau8360_peq_coeff_put(struct snd_kcontrol *kcontrol,
> +	struct snd_ctl_elem_value *ucontrol)
> +{
[ ... ]
> +	if (snd_soc_dapm_get_bias_level(dapm) > SND_SOC_BIAS_STANDBY) {
> +		dev_dbg(nau8360->dev, "PEQ access is not allowed during playback");
> +		return 0;
> +	}
> +
> +	reg = nau8360_peq_regaddr(kcontrol->id.name);

[Severity: Medium]
Is there a race condition here between the bias level check and the
subsequent hardware access? The bias level is checked locklessly without
holding the DAPM mutex. If playback starts immediately after this check, the
hardware registers might be modified while actively streaming, bypassing the
intended safety checks.

[ ... ]
> @@ -374,6 +374,27 @@ static int nau8360_startup(struct snd_pcm_substream *substream, struct snd_soc_dai *dai)
[ ... ]
> +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->load_fw_done) {

[Severity: Medium]
Could this read of load_fw_done miss the completed state? It is updated by a
background workqueue and read here without memory barriers. The thread
running this function might not observe the completed firmware load due to
weak architectural ordering guarantees, leading to spurious EBUSY failures.

> +		dev_warn(nau8360->dev, "DSP firmware is not ready yet!");
> +		return -EBUSY;
> +	}
> +
> +	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
> +		regmap_read(nau8360->regmap, NAU8360_R0B_I2S_PCM_CTRL1, &val);

[Severity: High]
Can this also use uninitialized stack memory if the regmap_read() fails? The
return value is not checked before using val in the condition below.

> +		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 this sequence permanently break TDM routing for capture streams and
non-I2S formats?

The nau8360_shutdown() function unconditionally clears NAU8360_EN_TDM_TX
or NAU8360_EN_TDM_RX. Here in startup(), NAU8360_EN_TDM_TX is never
re-enabled for capture streams. Additionally, NAU8360_EN_TDM_RX is only
restored if the format is strictly I2S. Since generic machine drivers typically
call set_tdm_slot() only once during initialization, the TDM configuration
might be lost after the first stream stops.

> +
> +		if (nau8360_dsp_active(component))
> +			snd_soc_dapm_enable_pin(nau8360->dapm, "Sense");
> +	}
> +
> +	return 0;
> +}
> +
> +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);

[ ... ]
> @@ -630,6 +630,42 @@ static int nau8360_set_sysclk(struct snd_soc_component *cp,
[ ... ]
> +static int nau8360_set_sysclk(struct snd_soc_component *cp,
> +	int clk_id, int source, unsigned int freq, int dir)
> +{
> +	struct nau8360 *nau8360 = snd_soc_component_get_drvdata(cp);
> +	struct regmap *regmap = nau8360->regmap;
> +	struct device *dev = nau8360->dev;
> +	static const char * const idtab[] = { "DIG", "ANA", "Internal" };
> +	static const char * const srctab[] = { "MCLK", "PLL", "HIRC48M", "BCLK" };
> +	int ret;
> +
> +	if (dir == SND_SOC_CLOCK_OUT) {
> +		dev_dbg(dev, "sysclk: freq %d (out)", freq);
> +		return nau8360_set_sysclk_output(nau8360, freq);

[Severity: High]
Does this break compatibility with generic ALSA machine drivers? This code
requires the machine driver to pass SND_SOC_CLOCK_OUT to configure the
internal system clock target rate. Generic machine drivers (like
simple-audio-card) only call set_sysclk() with SND_SOC_CLOCK_IN, which would
leave the internal sys_clk as 0 and cause subsequent divider calculations in
nau8360_dig_sys_clk() to fail.

> +	}
> +
> +	switch (clk_id) {
[ ... ]
> +	case NAU8360_CLK_ID_DIG:
> +		dev_dbg(dev, "sysclk: id %d (%s), src %d (%s), freq %d (in)",
> +			clk_id, idtab[clk_id], source, srctab[source], freq);

[Severity: High]
Could this result in an out-of-bounds array read? The srctab array has 4
elements, but the source parameter is provided directly by the caller and is
used as an index here without prior validation.

> +
> +		if (source == NAU8360_CLK_SRC_BCLK)
> +			return -EINVAL;

[ ... ]
> @@ -1056,6 +1056,12 @@ static void nau8360_load_fw_work(struct work_struct *work)
> +static void nau8360_load_fw_work(struct work_struct *work)
> +{
> +	struct nau8360 *nau8360 = container_of(work, struct nau8360, load_fw_work);
> +	struct snd_soc_component *cp = snd_soc_dapm_to_component(nau8360->dapm);
> +	int ret;
> +
> +	ret = nau8360_dsp_init(cp);
> +	if (ret) {
> +		dev_err(nau8360->dev, "Failed to initialize DSP: %d\n", ret);
> +		nau8360_dsp_enable(nau8360->regmap, false);
> +		return;
> +	}
> +	nau8360->load_fw_done = true;

[Severity: Medium]
Is a memory barrier needed here? This flag is written from a workqueue and
read in nau8360_startup(), so the update might not be properly synchronized
without smp_store_release() and smp_load_acquire().

> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2