Re: [PATCH 10/11] ASoC: fsl: mpc5200_dma: Use guard() for spin locks

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Concurrent ALSA triggers for playback and capture substreams can race when updating the shared `regs->isr_imr.imr` hardware register, potentially dropping interrupt enable bits.
--

commit 41f1f50873252390b25ba84ac77438b563c3e7ea
Author: bui duc phuc <phucduc.bui@gmail.com>

ASoC: fsl: mpc5200_dma: Use guard() for spin locks

This commit cleans up the mpc5200_dma code by using scoped_guard() for spin
locks. This is intended to be purely a code refactoring with no behavior
change.

> diff --git a/sound/soc/fsl/mpc5200_dma.c b/sound/soc/fsl/mpc5200_dma.c
> index 56e2cf2f727ba..bfedb2dea0b32 100644
> --- a/sound/soc/fsl/mpc5200_dma.c
> +++ b/sound/soc/fsl/mpc5200_dma.c

[ ... ]

> @@ -158,13 +158,13 @@ static int psc_dma_trigger(struct snd_soc_component *component,
>  			substream->pstr->stream, s->period_count);
>  		s->active = 0;
>  
> -		spin_lock_irqsave(&psc_dma->lock, flags);
> -		bcom_disable(s->bcom_task);
> -		if (substream->pstr->stream == SNDRV_PCM_STREAM_CAPTURE)
> -			bcom_gen_bd_rx_reset(s->bcom_task);
> -		else
> -			bcom_gen_bd_tx_reset(s->bcom_task);
> -		spin_unlock_irqrestore(&psc_dma->lock, flags);
> +		scoped_guard(spinlock_irqsave, &psc_dma->lock) {
> +			bcom_disable(s->bcom_task);
> +			if (substream->pstr->stream == SNDRV_PCM_STREAM_CAPTURE)
> +				bcom_gen_bd_rx_reset(s->bcom_task);
> +			else
> +				bcom_gen_bd_tx_reset(s->bcom_task);
> +		}
>  
>  		break;
>  

[Severity: High]
This is a pre-existing issue, but can concurrent ALSA triggers for playback
and capture substreams race when updating the shared regs->isr_imr.imr hardware
register?

Looking down at the end of psc_dma_trigger(), the interrupt mask is updated
entirely outside of the psc_dma->lock:

    /* Update interrupt enable settings */
    imr = 0;
    if (psc_dma->playback.active)
        imr |= MPC52xx_PSC_IMR_TXEMP;
    if (psc_dma->capture.active)
        imr |= MPC52xx_PSC_IMR_ORERR;
    out_be16(&regs->isr_imr.imr, psc_dma->imr | imr);

If CPU1 triggers playback and CPU2 triggers capture simultaneously, could
they both read stale values for the other stream's active flag?

If so, one of the concurrent writes to regs->isr_imr.imr would overwrite
the other, potentially dropping either the MPC52xx_PSC_IMR_TXEMP or
MPC52xx_PSC_IMR_ORERR bit. This could leave the corresponding stream without
interrupts and stall the audio stream indefinitely.

Since this patch is refactoring the locking in this function, would it make
sense to expand the scope of the new scoped_guard to cover the s->active
updates and this hardware register write?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260612132639.78086-1-phucduc.bui@gmail.com?part=10