[PATCH] dmaengine: pl330: Bound UNTIL loop

[PATCH] dmaengine: pl330: Bound UNTIL loop

[Eugen Hristev]

Bound UNTIL loop with a timeout.
It may happen that by accident pl330 will read/write a wrong address
and generate a bus fault. In such cases it will not complete the
condition that UNTIL waits for. To have the driver continue and not
hang, use a timeout.

Assisted-by: Github Copilot
Signed-off-by: Eugen Hristev <ehristev@kernel.org>
---
 drivers/dma/pl330.c | 36 ++++++++++++++++++++++++++++++------
 1 file changed, 30 insertions(+), 6 deletions(-)

diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
index c2d0b473b8f4..9ced60fef9ff 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)); \
+	 })
 
 #ifdef PL330_DEBUG_MCGEN
 static unsigned cmd_line;
@@ -979,7 +986,8 @@ static void _stop(struct pl330_thread *thrd)
 	u32 inten = readl(regs + INTEN);
 
 	if (_state(thrd) == PL330_STATE_FAULT_COMPLETING)
-		UNTIL(thrd, PL330_STATE_FAULTING | PL330_STATE_KILLING);
+		UNTIL(thrd, PL330_STATE_FAULTING | PL330_STATE_KILLING,
+		      msecs_to_loops(5));
 
 	/* Return if nothing needs to be done */
 	if (_state(thrd) == PL330_STATE_COMPLETING
@@ -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;
+			}
 		fallthrough;
 
 	case PL330_STATE_FAULTING:
@@ -1073,7 +1093,11 @@ static bool pl330_start_thread(struct pl330_thread *thrd)
 
 	case PL330_STATE_KILLING:
 	case PL330_STATE_COMPLETING:
-		UNTIL(thrd, PL330_STATE_STOPPED)
+		if (!UNTIL(thrd, PL330_STATE_STOPPED, tout)) {
+			dev_err(thrd->dmac->ddma.dev,
+				"pl330: thread stuck, not reaching STOPPED\n");
+			return false;
+		}
 		fallthrough;
 
 	case PL330_STATE_STOPPED:

---
base-commit: 6d0933ae9d46674a6ea63a87e9e9ab4e662670c9
change-id: 20261104-b4-pl330-until-8a287981ed5d

Best regards,
--  
Eugen Hristev <ehristev@kernel.org>

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

[sashiko-bot]

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