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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [Critical] Concurrency data race in rz_ssi_interrupt() leads to NULL pointer dereference.
- [High] Error recovery logic leaves DMA channels permanently paused if a stream is stopped.
- [High] Incomplete state cleanup in full-duplex mode causes permanent hardware stall of active streams.
- [Medium] Deletion of the remove callback causes hardware state and reset leak on driver unbind.
- [Medium] dmaengine_resume() is called on unpaused DMA channels during full-duplex error recovery, violating DMA API contracts.
--

commit 3df78c9407acc2ae86671be88fbd3bf33a9d7d4f
Author: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

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

This commit migrates the Renesas RZ/G2L and RZ/G3S SSI driver to use generic
PCM dmaengine APIs instead of custom DMA handling. It also removes the
explicit remove callback, increases buffer sizes to reduce underruns, and
simplifies driver logic.

> diff --git a/sound/soc/renesas/rz-ssi.c b/sound/soc/renesas/rz-ssi.c
> index d4e1dded3a9c1..5406f9a3547f6 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));
>  
>  		if (ssi->capture.substream && is_stopped) {
>  			if (ssisr & SSISR_RUIRQ)
>  				strm_capture->uerr_num++;
>  			if (ssisr & SSISR_ROIRQ)
>  				strm_capture->oerr_num++;
>  
>  			rz_ssi_stop(ssi, strm_capture);
>  		}
>  
>  		if (ssi->playback.substream && is_stopped) {
>  			if (ssisr & SSISR_TUIRQ)
>  				strm_playback->uerr_num++;
>  			if (ssisr & SSISR_TOIRQ)
>  				strm_playback->oerr_num++;
>  
>  			rz_ssi_stop(ssi, strm_playback);
>  		}
>  
> +		if (!rz_ssi_is_stream_running(&ssi->playback) &&
> +		    !rz_ssi_is_stream_running(&ssi->capture) &&
> +		    rz_ssi_is_dma_enabled(ssi)) {
> +			if (ssi->dmas[SNDRV_PCM_STREAM_PLAYBACK])
> +				dmaengine_pause(ssi->dmas[SNDRV_PCM_STREAM_PLAYBACK]);
> +			if (ssi->dmas[SNDRV_PCM_STREAM_CAPTURE] &&
> +			    /* Avoid calling pause twice in case of half duplex. */
> +			    ssi->dmas[SNDRV_PCM_STREAM_PLAYBACK] !=
> +			    ssi->dmas[SNDRV_PCM_STREAM_CAPTURE])
> +				dmaengine_pause(ssi->dmas[SNDRV_PCM_STREAM_CAPTURE]);
> +		}
> +
>  		/* Clear all flags */
>  		rz_ssi_reg_mask_setl(ssi, SSISR, SSISR_TOIRQ | SSISR_TUIRQ |
>  				     SSISR_ROIRQ | SSISR_RUIRQ, 0);
>  
>  		/* Add/remove more data */
>  		if (ssi->capture.substream && is_stopped) {
> -			for (i = 0; i < count; i++)
> +			if (rz_ssi_is_dma_enabled(ssi))
> +				dmaengine_resume(ssi->dmas[SNDRV_PCM_STREAM_CAPTURE]);

Can this sequence lead to a NULL pointer dereference?

When a stream is stopped and closed, ALSA calls rz_ssi_dai_trigger(STOP)
(which clears the substream pointer) followed by rz_ssi_shutdown(), which
clears ssi->dmas[stream]:

sound/soc/renesas/rz-ssi.c:rz_ssi_shutdown() {
    ...
    ssi->dmas[substream->stream] = NULL;
}

If an in-flight interrupt handler is executing concurrently, the driver
disables device-level IRQs but never calls synchronize_irq(). The handler
could observe a non-NULL substream, but by the time it calls
dmaengine_resume(ssi->dmas[stream]), the teardown code on another CPU
could have nullified the pointer.

Can this error recovery logic leave the DMA channel permanently paused?

During an under/overrun error, the code above calls dmaengine_pause() on
both channels if neither stream is running. However, dmaengine_resume()
is only conditionally called if substream is non-NULL.

If one stream was intentionally stopped by the user (meaning its substream
is NULL), its DMA channel is left indefinitely paused. Since the ALSA
generic dmaengine PCM framework caches and reuses DMA channels, the channel
will remain paused on the next trigger(START), resulting in a silent audio
failure.

Does calling dmaengine_resume() here violate the DMA API contract?

In full-duplex mode, if an underrun/overrun occurs on one stream, the check
for !rz_ssi_is_stream_running() for both streams evaluates to false, so
dmaengine_pause() is completely skipped.

However, dmaengine_resume() is unconditionally called for the failing
stream. Calling resume on a channel that is currently active and was never
paused could cause state machine corruption in the DMA controller driver.

>  		}
>  
>  		if (ssi->playback.substream && is_stopped) {
> -			for (i = 0; i < count; i++)
> +			if (rz_ssi_is_dma_enabled(ssi))
> +				dmaengine_resume(ssi->dmas[SNDRV_PCM_STREAM_PLAYBACK]);
>  		}
>  
>  		/* Resume */
>  		if (ssi->playback.substream && is_stopped)
>  			rz_ssi_start(ssi, &ssi->playback);
>  		if (ssi->capture.substream && is_stopped)
>  			rz_ssi_start(ssi, &ssi->capture);
>  	}

This isn't a bug introduced by this patch, but does the full-duplex error
recovery code cause a permanent hardware stall?

When operating in full-duplex mode, stopping one stream triggers an
underrun/overrun interrupt. The interrupt handler calls rz_ssi_stop() for
the remaining active stream and then rz_ssi_start() to recover it.

Because tx_active and rx_active remain true, rz_ssi_start() enters the
one_stream_triggered branch and sets it to true, without re-enabling the
SSICR_TEN and SSICR_REN hardware bits. The surviving stream is left
permanently stalled in hardware until both streams are closed.

[ ... ]
> -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);
> -}

Does dropping the remove callback leak the hardware state and reset line on
driver unbind?

The driver relies on devm_pm_runtime_enable() to handle PM cleanup.
However, when the driver unbinds, pm_runtime_disable() cancels any pending
asynchronous suspends but does not force the device into a suspended state
if it was currently RPM_ACTIVE.

Consequently, rz_ssi_runtime_suspend() is bypassed, the reset line is never
asserted, and the PM domain clocks may remain enabled.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260512121219.216159-1-claudiu.beznea.uj@bp.renesas.com?part=16