Re: [PATCH v4 2/2] ASoC: airoha: Add support for AN7581 PCM driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 12 potential issue(s) to consider:
- [High] ASoC: airoha: an7581_pcm_fe_hw_params logic error permanently rejects first stream
- [High] ASoC: airoha: probe unconditionally fails due to IRQF_ONESHOT without thread_fn
- [High] ASoC: airoha: unbounded tx_pos/rx_pos accumulation breaks ALSA pointer tracking
- [High] ASoC: airoha: missing return on constraint failure in startup corrupts state
- [High] ASoC: airoha: Use-After-Free of DMA descriptors on driver unbind due to devres ordering
- [High] ASoC: airoha: Use-After-Free of ALSA runtime due to missing IRQ synchronization in shutdown
- [High] sound: airoha: missing interrupt status acknowledgment causes IRQ storm
- [High] sound: soc: airoha: inverted users check in hw_params breaks all streams
- [High] sound: soc: airoha: early exit in IRQ handler stalls stream
- [Medium] ASoC: airoha: premature return in IRQ handler stalls DMA on underrun
- [Medium] ALSA: airoha: missing endianness conversion for hardware DMA descriptors
- [Medium] ASoC: airoha: incorrect PCM interleaved format advertised for non-interleaved hardware
--

commit d46acbcb8f477a3db86ab3f295bc95f3a191a323
Author: Christian Marangi <ansuelsmth@gmail.com>

ASoC: airoha: Add support for AN7581 PCM driver
    
The Airoha AN7581 SoC provides a PCM interface for VoIP applications,
supporting playback and capture at sample rates of 8 kHz and 16 kHz.
Add an ASoC platform driver for the AN7581 PCM controller configured for
ZSI attached voice chip.

diff --git a/sound/soc/airoha/an7581-pcm.c b/sound/soc/airoha/an7581-pcm.c
new file mode 100644
index 0000000000000..568c11dc7e6b9
--- /dev/null
+++ b/sound/soc/airoha/an7581-pcm.c
@@ -0,0 +1,904 @@
+[ ... ]
> +struct an7581_pcm_desc {
> +	u32 status;
> +	u32 channel_mask;
> +	u32 dma_addr;
> +} __packed;

[Severity: Medium]
Will this work correctly on big-endian architectures? Since the descriptors are
shared with the hardware DMA, using native u32 instead of explicit __le32 means
the data will be written and read backwards if the kernel is compiled for a
Big Endian target. Should we use __le32 and explicit cpu_to_le32() conversions?

> +
> +struct an7581_pcm_priv {
[ ... ]
> +static const struct snd_pcm_hardware an7581_pcm_hardware = {
> +	.info			= SNDRV_PCM_INFO_MMAP |
> +				  SNDRV_PCM_INFO_MMAP_VALID |
> +				  SNDRV_PCM_INFO_INTERLEAVED,
> +	.formats		= SNDRV_PCM_FMTBIT_U8 | SNDRV_PCM_FMTBIT_U16_LE,

[Severity: Medium]
Is it correct to advertise SNDRV_PCM_INFO_INTERLEAVED here?
Later in an7581_pcm_fe_hw_params(), the hardware is configured with a
per-channel memory offset, indicating that the hardware expects a
non-interleaved layout:

	regmap_update_bits(priv->map, AN7581_PCM_CHBFOSR,
			   AN7581_PCM_DESC_CHBF_OFFSET,
			   FIELD_PREP(AN7581_PCM_DESC_CHBF_OFFSET,
				      AN7581_PCM_FRAMES_PER_DESC * bit_width / 8));

If userspace writes interleaved samples as advertised, might the hardware
misinterpret the audio data?

[ ... ]
> +static int an7581_pcm_fe_startup(struct snd_pcm_substream *substream,
> +				 struct snd_soc_dai *dai)
> +{
[ ... ]
> +	ret = snd_pcm_hw_constraint_integer(runtime, SNDRV_PCM_HW_PARAM_PERIODS);
> +	if (ret < 0)
> +		dev_err(rtd->card->dev, "snd_pcm_hw_constraint_integer failed\n");
> +
> +	/* Cache stream substream for IRQ handling */
> +	priv->stream[substream->stream] = substream;

[Severity: High]
Does this code inadvertently continue execution and modify driver state when
snd_pcm_hw_constraint_integer() fails? By missing a return statement, the
function proceeds to cache the substream pointer and increment priv->users
before returning the error code. Because the startup failed, ALSA will abort
and never call shutdown, permanently leaking the users count and leaving a
dangling substream pointer in priv->stream that could cause a Use-After-Free
during the next interrupt.

[ ... ]
> +static void an7581_pcm_fe_shutdown(struct snd_pcm_substream *substream,
> +				   struct snd_soc_dai *dai)
> +{
[ ... ]
> +	priv->stream[substream->stream] = NULL;
> +
> +	usleep_range(10000, 20000);

[Severity: High]
Is it possible for a concurrent interrupt handler to execute here?
By clearing the stream pointer without calling synchronize_irq(), an already
executing handler on another CPU might have already passed the NULL check. Once
this function returns, ALSA will free the substream runtime. If the delayed IRQ
handler then resumes, could it dereference the freed runtime pointer?

[ ... ]
> +static int an7581_pcm_fe_hw_params(struct snd_pcm_substream *substream,
> +				   struct snd_pcm_hw_params *params,
> +				   struct snd_soc_dai *dai)
> +{
[ ... ]
> +	mutex_lock(&priv->user_lock);
> +	if (priv->users <= 1 &&
> +	    (priv->current_rate != rate ||
> +	     priv->current_channels != channels)) {
> +		dev_err(rtd->card->dev, "playback/capture rates/channels must match in full-duplex mode\n");
> +		mutex_unlock(&priv->user_lock);
> +		return -EBUSY;
> +	}

[Severity: High]
Does this inverted logic unconditionally reject the first stream?
Since an7581_pcm_fe_startup() increments priv->users, it will be exactly 1 for
the first stream. The condition priv->users <= 1 evaluates to true, and since
the uninitialized priv->current_rate is 0, it will mismatch the requested rate.
This causes the driver to return -EBUSY immediately, preventing hardware setup.
Should this check be for priv->users > 1 instead?

[ ... ]
> +static snd_pcm_uframes_t an7581_pcm_pointer(struct snd_soc_component *component,
> +					    struct snd_pcm_substream *substream)
> +{
[ ... ]
> +	spin_lock_irqsave(&priv->desc_lock, flags);
> +	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
> +		pos = bytes_to_frames(runtime, priv->tx_pos);
> +	else
> +		pos = bytes_to_frames(runtime, priv->rx_pos);
> +	spin_unlock_irqrestore(&priv->desc_lock, flags);
> +
> +	return pos % runtime->buffer_size;
> +}

[Severity: High]
Could the continuously accumulating tx_pos and rx_pos cause a discontinuous jump
when the 32-bit integers wrap? After accumulating 4GB (which wraps to 0), the
value is almost certainly not an exact multiple of the ALSA buffer_size.
Applying modulo here will cause the ALSA frame pointer to jump backwards
unexpectedly, triggering an XRUN and halting the audio stream. Should
the byte counters be bounded or wrapped explicitly at the buffer boundary
instead?

[ ... ]
> +static irqreturn_t an7581_pcm_irq_handler(int irq, void *data)
> +{
> +	struct snd_pcm_substream *substream;
> +	struct an7581_pcm_priv *priv = data;
> +	unsigned long flags;
> +	u32 status, isr = 0;
> +
> +	regmap_read(priv->map, AN7581_PCM_ISR, &isr);

[Severity: High]
Does this handler leave the interrupt asserted? After reading the
AN7581_PCM_ISR register, there is no corresponding regmap_write() to clear the
asserted status bits. Without a write-1-to-clear acknowledgment, the interrupt
line may remain asserted, potentially leading to an unrecoverable interrupt
storm.

> +
> +	/* Scan descriptor on receiving RX desc update interrupt */
> +	if (isr & AN7581_PCM_IMR_RDESC_UPDATE_INT) {
[ ... ]
> +			desc->status |= AN7581_PCM_DESC_STATUS_OWNERSHIP_DMA;
> +
> +			if (snd_pcm_capture_hw_avail(runtime) < runtime->period_size)
> +				goto handle_tx;
> +
> +			rx_poll = true;
> +		}
> +
> +		if (rx_poll)
> +			regmap_write(priv->map, AN7581_PCM_RPDR, AN7581_PCM_RX_POLLING);
> +	}
> +
> +handle_tx:
> +	/* Scan descriptor on receiving TX desc update interrupt */
> +	if (isr & AN7581_PCM_IMR_TDESC_UPDATE_INT) {
[ ... ]
> +			desc->status |= AN7581_PCM_DESC_STATUS_OWNERSHIP_DMA;
> +
> +			if (snd_pcm_playback_hw_avail(runtime) < runtime->period_size)
> +				return IRQ_HANDLED;
> +
> +			tx_poll = true;
> +		}

[Severity: High]
Could these early exits abandon completed descriptors and stall the stream?
The loop iterates over descriptors that the hardware has already completed.
If the driver returns early here (or jumps to handle_tx) due to an ALSA
underrun, it breaks out of the loop and skips processing the rest of the
completed descriptors. This leaves them permanently owned by the CPU, stalls
the DMA, and misses the wakeup write to the polling register. Because ALSA
core already handles overruns and underruns safely, shouldn't the driver
always process all completed descriptors without aborting the loop?

[ ... ]
> +static int an7581_pcm_probe(struct platform_device *pdev)
> +{
[ ... ]
> +	ret = devm_request_threaded_irq(dev, irq, an7581_pcm_irq_handler,
> +					NULL, IRQF_ONESHOT, dev_name(dev), priv);
> +	if (ret)
> +		return ret;

[Severity: High]
Is it valid to pass IRQF_ONESHOT with a NULL thread_fn? The core IRQ subsystem
rejects this combination because it doesn't make sense to keep the interrupt
masked for a thread that doesn't exist. This will cause
devm_request_threaded_irq() to unconditionally return -EINVAL, causing probe
to fail every time.

> +
> +	spin_lock_init(&priv->desc_lock);
> +	mutex_init(&priv->user_lock);
> +
[ ... ]
> +	priv->tx_descs = dmam_alloc_coherent(dev, sizeof(struct an7581_pcm_desc) *
> +					     AN7581_PCM_TX_DESCRIPTORS, &priv->tx_dma,
> +					     GFP_KERNEL);

[Severity: High]
Could this allocation order lead to a Use-After-Free during driver unbind?
Since devres frees resources in reverse order (LIFO), priv->tx_descs and
priv->rx_descs will be freed before the IRQ is released. Because
an7581_pcm_remove() doesn't call synchronize_irq(), an executing interrupt
handler might access the DMA descriptors after devres has already freed them.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261012115408.87066-1-ansuelsmth@gmail.com?part=2