Re: [PATCH v4 3/4] ASoC: es9039q2m: add ESS Technology ES9039Q2M codec driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] ASoC: es9039q2m: FIR coefficient upload corrupts data with TLV header
- [Low] i2c: positional initializer used for i2c_device_id
--

commit 3aee2b23f8220349aa3bc4bfbb0d2fa6dfeb5b60
Author: Karl Asseily <karl@asseily.com>

ASoC: es9039q2m: add ESS Technology ES9039Q2M codec driver

This commit introduces a new ALSA SoC codec driver for the ESS Technology
ES9039Q2M 32-bit audio DAC. It supports software mode over I2C, implementing
programmable FIR filters, custom routing, and DSD/DoP stream detection.

> diff --git a/sound/soc/codecs/es9039q2m.c b/sound/soc/codecs/es9039q2m.c
> new file mode 100644
> index 0000000000000..4e9f86cb78606
> --- /dev/null
> +++ b/sound/soc/codecs/es9039q2m.c

[ ... ]

> +static int es9039_fir_put(struct snd_kcontrol *kcontrol,
> +			  const unsigned int __user *bytes, unsigned int size)
> +{
> +	struct snd_soc_component *c = snd_kcontrol_chip(kcontrol);
> +	struct es9039q2m_priv *priv = snd_soc_component_get_drvdata(c);
> +	struct soc_bytes_ext *be = (void *)kcontrol->private_value;
> +	struct es9039_fir_ctl *p = container_of(be, struct es9039_fir_ctl, be);
> +	unsigned int idx = p->stage_4x ? 1 : 0;
> +	u8 *buf;
> +	int ret;
> +
> +	if (size != p->taps * ES9039_COEFF_BYTES)
> +		return -EINVAL;
> +
> +	buf = memdup_user(bytes, size);

[Severity: High]
Does this copy include the ALSA TLV header along with the coefficient payload?

When userspace writes coefficients via the ALSA TLV control, snd_ctl_tlv_ioctl
passes the TLV array down to snd_soc_bytes_tlv_callback. This array starts
with a 2-word header containing the type and length.

Because memdup_user starts from the very beginning of the bytes array without
skipping this 8-byte header, the hardware's coefficient RAM might receive the
header data instead of the actual coefficients, and the last 8 bytes of the
intended payload would be truncated.

> +	if (IS_ERR(buf))
> +		return PTR_ERR(buf);

[ ... ]

> +static const struct i2c_device_id es9039q2m_i2c_id[] = {
> +	{ "es9039q2m" },

[Severity: Low]
This isn't a functional bug, but should this use a named initializer to
conform to the I2C subsystem guidelines for i2c_device_id arrays? For
example, using { .name = "es9039q2m" }.

> +	{ }
> +};
> +MODULE_DEVICE_TABLE(i2c, es9039q2m_i2c_id);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260918182317.143223-1-karl@asseily.com?part=3