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

Thank you for your contribution! Sashiko AI review found 11 potential issue=
(s) to consider:
- [High] DSP initialization fails unconditionally during resume due to regm=
ap `cache_only` mode.
- [High] Concurrent asynchronous firmware loading requests cause a Use-Afte=
r-Free during device unbind.
- [High] Unkillable task deadlock on module unload (remove) if the device i=
s removed after a suspend/resume cycle.
- [High] Ignored `regmap_read()` return values result in uninitialized stac=
k variables driving hardware logic.
- [Medium] Silent failure when rejecting DAC mux path changes during playba=
ck.
- [Medium] Unprotected access to `nau8360->anc_enable` and hardware registe=
rs creates a data race in `nau8360_dsp_switch`.
- [Medium] Short fragments from the DSP can cause uninitialized stack memor=
y to be used in `nau8360_dsp_kcs_setup`.
- [Medium] Out-of-bounds `tdm_tx_func_slot` configurations cause undefined =
behavior via bit shifts.
- [Medium] Data race on the `dsp_created` flag between firmware callback an=
d ALSA startup.
- [Medium] Manual DAPM pin toggling of the 'Sense' widget breaks I/V captur=
e streams when the DSP is bypassed.
- [Low] Incompatible pointer type warning in `nau8360_validate_tdm_slots`.
--

--- Patch [2]: [PATCH v7 2/2] ASoC: codecs: nau8360: Add support for NAU83G=
60 amplifier ---
commit 498f738336b985a294d2d2e4f2b2064ec352e710
Author: Neo Chang <[email protected]>

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/nau8360-dsp.c b/sound/soc/codecs/nau8360-ds=
p.c
> index 14ff6c5705d14.....
> --- /dev/null
> +++ b/sound/soc/codecs/nau8360-dsp.c
[ ... ]
> +static int nau8360_read_data_payload(struct snd_soc_component *cp,
> +	unsigned short dsp_addr, int frag_len, bool msg_param,
> +	void *data, int data_size, int *data_count)
> +{
> +	struct nau8360 *nau8360 =3D snd_soc_component_get_drvdata(cp);
> +	struct device *dev =3D cp->dev;
> +	unsigned int payload;
> +	u32 *data_buf =3D data;
> +	int i, j, ret;
> +
> +	*data_count =3D (msg_param) ? data_size : 0;
> +	for (i =3D 0; i < frag_len - 1; i++) {

[Severity: Medium]
What happens if the DSP hardware returns a trailing fragment with a frag_len
of 1?=20

The loop extracting payload bytes wouldn't execute, leaving the data buffer=
=20
untouched without returning an error.

[ ... ]
> +static int nau8360_dsp_kcs_setup(struct snd_soc_component *cp, int offse=
t, int size,
> +	const void *data, unsigned short dsp_addr)
> +{
[ ... ]
> +		/* checking KCS result */
> +		ret =3D nau8360_dsp_exec_command(cp, NAU8360_DSP_CMD_GET_KCS_RSLTS,
> +			0, NAU8360_DSP_DATA_BYTE, NULL,
> +			NAU8360_DSP_DATA_BYTE, &kcs_rst, dsp_addr);
> +		if (ret)
> +			return ret;
> +		if (kcs_rst !=3D NAU8360_DSP_KCS_RSLTS_SUCCESS)
> +			return -EINVAL;

[Severity: Medium]
Could this evaluate uninitialized stack memory?

If nau8360_read_data_payload() encounters a frag_len of 1, kcs_rst is left=
=20
uninitialized but the reply validation still returns success. Evaluating=20
kcs_rst here could then randomly fail or pass based on whatever was on the=
=20
stack.

[ ... ]
> +static int nau8360_dsp_chan_kcs_setup(struct snd_soc_component *cp,
> +	const char *fw_name, int dsp_addr)
> +{
[ ... ]
> +	ctx =3D kmalloc(sizeof(*ctx), GFP_KERNEL);
> +	if (!ctx)
> +		return -ENOMEM;
> +
> +	ctx->cp =3D cp;
> +	ctx->dsp_addr =3D dsp_addr;
> +
> +	reinit_completion(&nau8360->fw_done[core_id]);
> +
> +	ret =3D request_firmware_nowait(THIS_MODULE, true,
> +		fw_name, dev, GFP_KERNEL, ctx, nau8360_dsp_fw_cb);

[Severity: High]
Does this create a race condition if called multiple times before the previ=
ous=20
firmware request finishes?

Reinitializing the completion and launching a duplicate request could leave=
=20
multiple asynchronous callbacks pending, which might bypass later unbind
synchronizations.

> diff --git a/sound/soc/codecs/nau8360.c b/sound/soc/codecs/nau8360.c
> index 4a5fe315850fd.....
> --- /dev/null
> +++ b/sound/soc/codecs/nau8360.c
[ ... ]
> +static int nau8360_get_tdm_chan_len(struct nau8360 *nau8360)
> +{
> +	int val;
> +
> +	regmap_read(nau8360->regmap, NAU8360_R0C_I2S_PCM_CTRL2, &val);
> +	val =3D (val & NAU8360_TDM_CLEN_MASK) >> NAU8360_TDM_CLEN_SFT;
> +
> +	return (val << 3) + 16;
> +}

[Severity: High]
Can regmap_read() fail here?

If an I2C error occurs, the return value is ignored and the uninitialized=20
stack variable val is used to compute the channel length. This garbage=20
value then propagates into the mathematical bounds for TDM slots.

[ ... ]
> +static void nau8360_dsp_switch(struct snd_soc_component *component, bool=
 enable)
> +{
> +	struct nau8360 *nau8360 =3D snd_soc_component_get_drvdata(component);
> +	struct regmap *regmap =3D nau8360->regmap;
> +	int value =3D NAU8360_PEQ_BAND_8;
> +
> +	/* If DSP is enabled, unstall HW3 engine and DSP, loading DSP firmware,
> +	 * and configure PEQ after dsp reset.
> +	 */
> +	if (enable) {
> +		value =3D nau8360->anc_enable ? NAU8360_PEQ_BAND_15 : NAU8360_PEQ_BAND=
_12;
> +		nau8360_dsp_enable(regmap, true);
> +	} else {
> +		dev_dbg(nau8360->dev, "Bypass DSP path");
> +		nau8360_dsp_enable(regmap, false);
> +	}
> +	regmap_update_bits(regmap, NAU8360_R9D_PEQ_CTL, NAU8360_PEQ_BAND_MASK,
> +		value << NAU8360_PEQ_BAND_SFT);
> +}

[Severity: Medium]
Is it safe to read nau8360->anc_enable and update the PEQ register without
holding nau8360->lock?=20

Looking at nau8360_anc_put(), it protects these variables with a mutex.
Concurrent execution from ALSA control put handlers might result in a stale
hardware state here.

[ ... ]
> +static int nau8360_dac_mux_put_enum(struct snd_kcontrol *kcontrol,
> +	struct snd_ctl_elem_value *ucontrol)
> +{
> +	struct snd_soc_dapm_context *dapm =3D snd_soc_dapm_kcontrol_to_dapm(kco=
ntrol);
> +	struct snd_soc_component *component =3D snd_soc_dapm_to_component(dapm);
> +	struct nau8360 *nau8360 =3D snd_soc_component_get_drvdata(component);
> +	struct soc_enum *e =3D (struct soc_enum *)kcontrol->private_value;
> +	unsigned int *item =3D ucontrol->value.enumerated.item;
> +	int ret =3D 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;
> +	}

[Severity: Medium]
Should this return an error code like -EBUSY?=20

Returning 0 when rejecting the change silently fails and tricks userspace=20
into thinking the hardware path was updated.

[ ... ]
> +static int nau8360_startup(struct snd_pcm_substream *substream, struct s=
nd_soc_dai *dai)
> +{
> +	struct snd_soc_component *component =3D dai->component;
> +	struct nau8360 *nau8360 =3D snd_soc_component_get_drvdata(component);
> +	unsigned int i2s_mask =3D NAU8360_FRAME_START_MASK | NAU8360_RX_OFFSET_=
MASK;
> +	unsigned int i2s_fmt =3D NAU8360_FRAME_START_H2L | NAU8360_RX_OFFSET_I2=
S;
> +	int val;
> +
> +	if (nau8360->dsp_created !=3D NAU8360_DSP_FW_ALL_DONE) {
> +		dev_warn(nau8360->dev, "DSP firmware is not ready yet!");
> +		return -EBUSY;
> +	}

[Severity: Medium]
Is a lock needed here when checking the dsp_created flag?=20

The firmware callback nau8360_dsp_fw_cb() modifies this flag under=20
nau8360->lock. Reading it unlocked might cause a torn read and spuriously=20
reject playback streams.

> +
> +	if (substream->stream =3D=3D SNDRV_PCM_STREAM_PLAYBACK) {
> +		regmap_read(nau8360->regmap, NAU8360_R0B_I2S_PCM_CTRL1, &val);
> +		if ((val & i2s_mask) =3D=3D i2s_fmt)
> +			regmap_update_bits(nau8360->regmap, NAU8360_R0B_I2S_PCM_CTRL1,
> +				NAU8360_EN_TDM_RX, NAU8360_EN_TDM_RX);

[Severity: High]
Does regmap_read() need its return value checked here?=20

Similar to nau8360_get_tdm_chan_len(), if this read fails due to bus issues=
,=20
an uninitialized stack variable will be evaluated to determine if the recei=
ver=20
should be enabled.

> +
> +		if (nau8360_dsp_active(component))
> +			snd_soc_dapm_enable_pin(nau8360->dapm, "Sense");
> +	}

[Severity: Medium]
Will this break I/V capture streams when the DSP is bypassed?=20

The Sense widget is the DAPM source for ADC_I and ADC_V. If it's never enab=
led=20
when nau8360_dsp_active() evaluates to false, DAPM won't power up the ADCs,=
=20
leaving the host recording silence over TDM.

[ ... ]
> +static int nau8360_validate_tdm_slots(struct device *dev, unsigned int m=
ask,
> +	const int *func_slots, const char * const *func_names,
> +	int num_funcs, const char *dir,
> +	unsigned int *slot_used)

[Severity: Low]
Does the func_slots parameter need its type updated?=20

The struct fields tdm_tx_func_slot and tdm_rx_func_slot were updated to u32=
,=20
which results in passing an unsigned int array to a const int pointer here.

> +{
> +	int i;
> +	unsigned int func_slot;
> +	*slot_used =3D 0;
> +
> +	if (!mask)
> +		return 0;
> +
> +	for (i =3D 0; i < num_funcs; i++) {
> +		func_slot =3D func_slots[i];
[ ... ]
> +		if (func_slot >=3D NAU8360_TDM_MAX_CHAN) {
> +			dev_warn(dev, "Invalid %s %s slot index: %d",
> +				dir, func_names[i], func_slot);
> +			continue;
> +		}

[Severity: Medium]
Should this clear the invalid slot from the array?=20

It logs a warning and continues, but leaves the out-of-bounds value intact =
in
the original array for later consumption.

[ ... ]
> +static void nau8360_enable_tdm_channels(struct snd_soc_component *cp,
> +	int rx_slot_used, int tx_slot_used)
> +{
> +	struct nau8360 *nau8360 =3D snd_soc_component_get_drvdata(cp);
> +	int i, slot;
> +	unsigned int val =3D 0;
> +	bool enable;
> +
> +	for (i =3D 0; i < NAU8360_TDM_TXN; i++) {
> +		slot =3D nau8360->tdm_tx_func_slot[i];
> +		enable =3D (slot !=3D TDM_SLOT_NONE) && (tx_slot_used & BIT(slot));

[Severity: Medium]
Can this trigger undefined behavior?=20

If the invalid slot value from earlier (e.g. >=3D 32) remains in the array,=
=20
evaluating BIT(slot) will bit-shift out of bounds.

[ ... ]
> +static int __maybe_unused nau8360_suspend(struct snd_soc_component *comp=
onent)
> +{
> +	struct nau8360 *nau8360 =3D snd_soc_component_get_drvdata(component);
> +	int i;
> +
> +	for (i =3D 0; i < NAU8360_DSP_CORE_NUM; i++)
> +		wait_for_completion(&nau8360->fw_done[i]);

[Severity: High]
Does this consume the completion token without replacing it if the resume
fails?=20

If nau8360_resume() doesn't properly replenish the token, a subsequent modu=
le=20
unload will deadlock forever when it waits on this completion.

[ ... ]
> +static int __maybe_unused nau8360_resume(struct snd_soc_component *compo=
nent)
> +{
> +	struct nau8360 *nau8360 =3D snd_soc_component_get_drvdata(component);
> +	struct regmap *regmap =3D nau8360->regmap;
> +	int ret;
> +
> +	/* disable Sense at standby */
> +	snd_soc_dapm_disable_pin(nau8360->dapm, "Sense");
> +	snd_soc_dapm_sync(nau8360->dapm);
> +
> +	ret =3D nau8360_dsp_setup(component);
> +
> +	regcache_cache_only(regmap, false);

[Severity: High]
Can the DSP initialization succeed here?=20

nau8360_dsp_setup() is called before clearing the cache_only flag. Since=20
nau8360_dsp_setup() relies on polling hardware registers over the bus, it=20
will silently fail every time during resume, skipping firmware loads and=20
leaving the hardware unprotected.

[ ... ]
> +static void nau8360_i2c_remove(struct i2c_client *client)
> +{
> +	struct nau8360 *nau8360 =3D i2c_get_clientdata(client);
> +	int i;
> +
> +	for (i =3D 0; i < NAU8360_DSP_CORE_NUM; i++)
> +		wait_for_completion(&nau8360->fw_done[i]);
> +}

[Severity: High]
If nau8360_suspend() consumed the token and nau8360_resume() failed to repl=
ace
it, won't this cause an unkillable deadlock?

Additionally, if duplicate firmware requests were launched by=20
nau8360_dsp_chan_kcs_setup(), this wait_for_completion() will only synchron=
ize=20
against the first one. The second pending callback could execute after the=
=20
device unbinds, triggering a use-after-free on the nau8360 struct.

--=20
Sashiko AI review =C2=B7 https://sashiko.dev/#/patchset/20260804032951.1069=
[email protected]?part=3D2