Re: [PATCH] dmaengine: pl330: Bound UNTIL loop

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] dmaengine: pl330: second MMIO read in UNTIL() can miss transient state
--

commit ec02cf0a6d8895f26c57f9c3aaba3b3bd470e676
Author: Eugen Hristev <ehristev@kernel.org>

dmaengine: pl330: Bound UNTIL loop

This commit bounds the UNTIL macro loop with a timeout budget and checks its
return value in pl330_start_thread() to prevent the driver from hanging if a
bus fault leaves a DMA thread stuck in a transitional state.

> diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
> index c2d0b473b8f48..9ced60fef9ffb 100644
> --- a/drivers/dma/pl330.c
> +++ b/drivers/dma/pl330.c
> @@ -248,8 +248,15 @@ enum pl330_byteswap {
>   */
>  #define MCODE_BUFF_PER_REQ	256
>  
> -/* Use this _only_ to wait on transient states */
> -#define UNTIL(t, s)	while (!(_state(t) & (s))) cpu_relax();
> +/* returns true if state reached within loop budget */
> +#define UNTIL(t, s, loops) \
> +	({ \
> +		unsigned long _l = (loops); \
> +		while (!(_state(t) & (s)) && _l) { \
> +			cpu_relax(); _l--; \
> +		 } \
> +		!!(_state(t) & (s)); \
> +	 })

[ ... ]

> @@ -1059,12 +1067,24 @@ static bool _trigger(struct pl330_thread *thrd)
>  
>  static bool pl330_start_thread(struct pl330_thread *thrd)
>  {
> +	/* msecs_to_loops(5) gives the same budget used by _until_dmac_idle */
> +	const unsigned long tout = msecs_to_loops(5);
> +
>  	switch (_state(thrd)) {
>  	case PL330_STATE_FAULT_COMPLETING:
> -		UNTIL(thrd, PL330_STATE_FAULTING | PL330_STATE_KILLING);
> +		if (!UNTIL(thrd, PL330_STATE_FAULTING | PL330_STATE_KILLING,
> +			   tout)) {
> +			dev_err(thrd->dmac->ddma.dev,
> +				"pl330: thread stuck in FAULT_COMPLETING\n");
> +			return false;
> +		}
>  
>  		if (_state(thrd) == PL330_STATE_KILLING)
> -			UNTIL(thrd, PL330_STATE_STOPPED)
> +			if (!UNTIL(thrd, PL330_STATE_STOPPED, tout)) {
> +				dev_err(thrd->dmac->ddma.dev,
> +					"pl330: thread stuck KILLING->STOPPED\n");
> +				return false;
> +			}

[Severity: Medium]
Each invocation of _state(thrd) performs an MMIO readl() of the hardware
status register (DS or CS(thrd->id)).

In UNTIL(), after the while loop exits because (_state(t) & (s)) matched, the
macro performs a second MMIO read via !!(_state(t) & (s)) to compute the
return value.

In pl330_start_thread() under PL330_STATE_FAULT_COMPLETING, UNTIL() waits for
PL330_STATE_FAULTING | PL330_STATE_KILLING. Because PL330_STATE_KILLING is a
transient state that automatically transitions to PL330_STATE_STOPPED, what
happens if the while condition observes PL330_STATE_KILLING and exits the
loop, and the hardware finishes transitioning to PL330_STATE_STOPPED before
the trailing !!(_state(t) & (s)) read executes?

Wouldn't the second _state(t) read return PL330_STATE_STOPPED, causing UNTIL()
to evaluate to false and making pl330_start_thread() report a false timeout
and fail to start the thread?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261104-b4-pl330-until-v1-1-b541929da163@kernel.org?part=1