Re: [PATCH v6 17/18] ASoC: renesas: rz-ssi: Use generic PCM dmaengine APIs

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:

New issues:
- [High] Removal of the `.remove` callback leaves the hardware un-reset and active if the driver is unbound.

Pre-existing issues:
- [Critical] In PIO mode, if both streams share `irq_rt`, concurrent playback and capture causes an infinite interrupt storm.
- [High] TOCTOU race on `strm->substream` leading to a NULL pointer dereference in the interrupt handler.
- [High] Hardware error recovery blindly forces paused streams to start, causing interrupt storms.
--

commit 7b32dcd4a1c5c9a1cf65850718c1a5545f3b08a5
Author: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

ASoC: renesas: rz-ssi: Use generic PCM dmaengine APIs

This commit reworks the Renesas RZ-SSI driver to use the generic PCM
dmaengine APIs instead of managing DMA directly. It replaces explicit DMA
resource cleanup with devres helpers, removes the driver remove callback,
and increases buffer sizes to prevent software underruns.

> diff --git a/sound/soc/renesas/rz-ssi.c b/sound/soc/renesas/rz-ssi.c
> index d4e1dded3a9c1..9fe8a639c47c1 100644
> --- a/sound/soc/renesas/rz-ssi.c
> +++ b/sound/soc/renesas/rz-ssi.c

[ ... ]

> @@ -606,12 +591,6 @@ static irqreturn_t rz_ssi_interrupt(int irq, void *data)
>  	if (irq == ssi->irq_int) { /* error or idle */
>  		bool is_stopped = !!(ssisr & (SSISR_RUIRQ | SSISR_ROIRQ |
>  					      SSISR_TUIRQ | SSISR_TOIRQ));

[Severity: High]
This is a pre-existing issue, but does this error recovery logic blindly
force paused streams to start?

The is_stopped flag evaluates to true if ANY stream encounters an error.

If one stream is in a paused state (where its substream pointer is still
non-NULL), an error on the opposite stream will spontaneously restart the
paused stream here, which could lead to immediate underruns/overruns and an
interrupt storm.

> -		int i, count;
> -
> -		if (rz_ssi_is_dma_enabled(ssi))
> -			count = 4;
> -		else
> -			count = 1;
>  
>  		if (ssi->capture.substream && is_stopped) {
>  			if (ssisr & SSISR_RUIRQ)

[ ... ]

> @@ -631,19 +610,41 @@ static irqreturn_t rz_ssi_interrupt(int irq, void *data)

[ ... ]

>  		/* Add/remove more data */
>  		if (ssi->capture.substream && is_stopped) {
> -			for (i = 0; i < count; i++)
> +			if (rz_ssi_is_dma_enabled(ssi)) {
> +				if (ssi->dmas[SNDRV_PCM_STREAM_CAPTURE])
> +					dmaengine_resume(ssi->dmas[SNDRV_PCM_STREAM_CAPTURE]);
> +			} else {
>  				strm_capture->transfer(ssi, strm_capture);
> +			}
>  		}
>  
>  		if (ssi->playback.substream && is_stopped) {
> -			for (i = 0; i < count; i++)
> +			if (rz_ssi_is_dma_enabled(ssi)) {
> +				if (ssi->dmas[SNDRV_PCM_STREAM_PLAYBACK])
> +					dmaengine_resume(ssi->dmas[SNDRV_PCM_STREAM_PLAYBACK]);
> +			} else {
>  				strm_playback->transfer(ssi, strm_playback);
> +			}
>  		}
>  
>  		/* Resume */

[Severity: High]
This is a pre-existing issue, but is there a TOCTOU race here that could
cause a NULL pointer dereference?

When stopping a stream, rz_ssi_dai_trigger() calls rz_ssi_stop() (which masks
interrupts at the device level but lacks synchronize_irq()) and then calls
rz_ssi_stream_quit(), which sets strm->substream = NULL.

If the interrupt handler is already running concurrently on another CPU, it
could pass the lockless check on the stream's substream pointer, but then
rz_ssi_start() may fetch strm->substream again from memory after it was
nulled by trigger(STOP).

[ ... ]

> @@ -679,153 +680,11 @@ static irqreturn_t rz_ssi_interrupt(int irq, void *data)
>  	return IRQ_HANDLED;
>  }

[Severity: Critical]
This is a pre-existing issue, but does the irq_rt block in rz_ssi_interrupt()
cause an infinite interrupt storm in PIO mode when both streams are active
and share irq_rt?

It checks if the playback substream is valid and services ONLY the playback
stream:

	if (irq == ssi->irq_rt) {
		if (ssi->playback.substream) {
			strm_playback->transfer(ssi, &ssi->playback);
		} else {
			strm_capture->transfer(ssi, &ssi->capture);
			rz_ssi_reg_mask_setl(ssi, SSIFSR, SSIFSR_RDF, 0);
		}
	}

If both playback and capture are open concurrently, any capture interrupt
(RX FIFO full) will be ignored.

The RX FIFO is never drained, the interrupt condition remains asserted,
and returning IRQ_HANDLED causes an immediate re-entry.

[ ... ]

> @@ -1236,43 +1137,19 @@ static int rz_ssi_probe(struct platform_device *pdev)

[ ... ]

> -static void rz_ssi_remove(struct platform_device *pdev)
> -{
> -	struct rz_ssi_priv *ssi = dev_get_drvdata(&pdev->dev);
> -
> -	rz_ssi_release_dma_channels(ssi);
>  
> -	reset_control_assert(ssi->rstc);
> +	return devm_snd_soc_register_component(dev, component_driver,
> +					       rz_ssi_soc_dai,
> +					       ARRAY_SIZE(rz_ssi_soc_dai));
>  }

[Severity: High]
Does removing the remove callback leave the hardware un-reset and active
if the driver is unbound?

The driver uses devm_reset_control_get_exclusive(), which only registers a
cleanup action to drop the reference (via reset_control_put()), but does not
assert the hardware reset line.

Without the explicit reset_control_assert() call that was removed here,
unloading the driver while the device is active will skip resetting the
hardware, potentially causing a resource leak or instability.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260526084710.3491480-1-claudiu.beznea@kernel.org?part=17