[PATCH 1/1] drm/xe/pcode: Fix pcode_try_request polling delays

[PATCH 1/1] drm/xe/pcode: Fix pcode_try_request polling delays

[Alan Previn]

Honor OOB max for udelay. Also add a backoff for 'wait'
as it exponentially increases so that doesn't overflow
and also caps to a sane limit so we dont suffer overtly
extended wait times as we progress.

Fixes: 7dc9b92dcfef ("drm/xe: Remove i915_utils dependency from xe_pcode.")
Signed-off-by: Alan Previn <alan.previn.teres.alexis@intel.com>
---
 drivers/gpu/drm/xe/xe_pcode.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/xe/xe_pcode.c b/drivers/gpu/drm/xe/xe_pcode.c
index 266deecbb100..25d1a354ee36 100644
--- a/drivers/gpu/drm/xe/xe_pcode.c
+++ b/drivers/gpu/drm/xe/xe_pcode.c
@@ -146,6 +146,7 @@ int xe_pcode_read(struct xe_tile *tile, u32 mbox, u32 *val0, u32 *val1)
 	return err;
 }
 
+#define __XE_MMIO_WAIT_MAX_BACKOFF_100MS (100 * USEC_PER_MSEC)
 static int pcode_try_request(struct xe_tile *tile, u32 mbox,
 			     u32 request, u32 reply_mask, u32 reply,
 			     int *status, bool atomic, int timeout_us, bool locked)
@@ -165,10 +166,11 @@ static int pcode_try_request(struct xe_tile *tile, u32 mbox,
 			return 0;
 
 		if (atomic)
-			udelay(wait);
+			udelay(min_t(int, wait, (MAX_UDELAY_MS * 1000)));
 		else
 			usleep_range(wait, wait << 1);
 		wait <<= 1;
+		wait = min_t(int, wait << 1, __XE_MMIO_WAIT_MAX_BACKOFF_100MS);
 	}
 
 	return -ETIMEDOUT;

base-commit: d8a353e238652bec29fd452babdaae0044d30b9b
-- 
2.43.0

Re: [PATCH 1/1] drm/xe/pcode: Fix pcode_try_request polling delays

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] drm/xe/pcode: duplicate left shift of wait in pcode_try_request()
- [High] drm/xe/pcode: uncapped wait added to slept in pcode_try_request()
--

commit b0181a87e207629275e050ae1f5d781e540c4b82
Author: Alan Previn <alan.previn.teres.alexis@intel.com>

drm/xe/pcode: Fix pcode_try_request polling delays

This commit clamps the atomic udelay() argument in pcode_try_request() to
MAX_UDELAY_MS * 1000 and adds a 100 ms upper bound to the exponential
backoff wait interval.

> diff --git a/drivers/gpu/drm/xe/xe_pcode.c b/drivers/gpu/drm/xe/xe_pcode.c
> index 266deecbb100a..25d1a354ee369 100644
> --- a/drivers/gpu/drm/xe/xe_pcode.c
> +++ b/drivers/gpu/drm/xe/xe_pcode.c
> @@ -146,6 +146,7 @@ int xe_pcode_read(struct xe_tile *tile, u32 mbox, u32 *val0, u32 *val1)
>  	return err;
>  }
>  
> +#define __XE_MMIO_WAIT_MAX_BACKOFF_100MS (100 * USEC_PER_MSEC)
>  static int pcode_try_request(struct xe_tile *tile, u32 mbox,
>  			     u32 request, u32 reply_mask, u32 reply,
>  			     int *status, bool atomic, int timeout_us, bool locked)
> @@ -165,10 +166,11 @@ static int pcode_try_request(struct xe_tile *tile, u32 mbox,
>  			return 0;
>  
>  		if (atomic)
> -			udelay(wait);
> +			udelay(min_t(int, wait, (MAX_UDELAY_MS * 1000)));
>  		else
>  			usleep_range(wait, wait << 1);
>  		wait <<= 1;
> +		wait = min_t(int, wait << 1, __XE_MMIO_WAIT_MAX_BACKOFF_100MS);

[Severity: High]
Should the existing wait <<= 1 statement above be removed? Leaving both
shifts in pcode_try_request() shifts wait left twice on every iteration,
quadrupling wait at each step (10 -> 40 -> 160 -> 640 -> 2560 us) instead
of doubling it.

Because the loop header accumulates elapsed time via slept += wait after
the loop body finishes:

drivers/gpu/drm/xe/xe_pcode.c:pcode_try_request() {
	for (slept = 0; slept < timeout_us; slept += wait) {

slept also advances by 4x the delay just performed, causing
xe_pcode_ready() and xe_pcode_request() to time out with -ETIMEDOUT before
timeout_us has actually elapsed.

[Severity: High]
When atomic is true, udelay() is clamped to MAX_UDELAY_MS * 1000 (for
example, 5000 us on x86 or 2000 us on arm64), but wait continues to grow
up to __XE_MMIO_WAIT_MAX_BACKOFF_100MS (100000 us) and is added directly
to slept in the loop header:

drivers/gpu/drm/xe/xe_pcode.c:pcode_try_request() {
	for (slept = 0; slept < timeout_us; slept += wait) {

When xe_pcode_request() retries with preemption disabled:

drivers/gpu/drm/xe/xe_pcode.c:xe_pcode_request() {
	preempt_disable();
	ret = pcode_try_request(tile, mbox, request, reply_mask, reply, &status,
				true, 50 * 1000, true);
	preempt_enable();

won't slept accumulate the unclamped wait value once wait exceeds
MAX_UDELAY_MS * 1000, causing the 50 ms atomic retry to return -ETIMEDOUT
after only a fraction of the intended 50 ms delay?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261030173038.681250-2-alan.previn.teres.alexis@intel.com?part=1

Re: [PATCH 1/1] drm/xe/pcode: Fix pcode_try_request polling delays

[Teres Alexis, Alan Previn]

Both of shashiko's comments are valid - will fix it now.
Will also rename the backoff definition from __XE_MMIO_WAIT_MAX_BACKOFF_100MS 
to __XE_PCODE_WAIT_MAX_BACKOFF_100MS (copy and paste mistake)

...alan

[PATCH 1/1] drm/xe/pcode: Fix pcode_try_request polling delays

[Alan Previn]

Honor OOB max for udelay. Also add a backoff for 'wait'
as it exponentially increases so that doesn't overflow
and also caps to a sane limit so we dont suffer overtly
extended wait times as we progress.

v2: Fix accumnulation of 'wait' so 'slept' is accurate and
    accounts for atomic-udelay clamping. (Sashiko)

Fixes: 7dc9b92dcfef ("drm/xe: Remove i915_utils dependency from xe_pcode.")
Signed-off-by: Alan Previn <alan.previn.teres.alexis@intel.com>
---
 drivers/gpu/drm/xe/xe_pcode.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/xe/xe_pcode.c b/drivers/gpu/drm/xe/xe_pcode.c
index 266deecbb100..d2ed5edf003e 100644
--- a/drivers/gpu/drm/xe/xe_pcode.c
+++ b/drivers/gpu/drm/xe/xe_pcode.c
@@ -146,6 +146,7 @@ int xe_pcode_read(struct xe_tile *tile, u32 mbox, u32 *val0, u32 *val1)
 	return err;
 }
 
+#define __XE_PCODE_WAIT_MAX_BACKOFF_100MS (100 * USEC_PER_MSEC)
 static int pcode_try_request(struct xe_tile *tile, u32 mbox,
 			     u32 request, u32 reply_mask, u32 reply,
 			     int *status, bool atomic, int timeout_us, bool locked)
@@ -164,11 +165,14 @@ static int pcode_try_request(struct xe_tile *tile, u32 mbox,
 		if ((*status == 0) && ((request & reply_mask) == reply))
 			return 0;
 
-		if (atomic)
-			udelay(wait);
-		else
+		wait = min_t(int, wait << 1, __XE_PCODE_WAIT_MAX_BACKOFF_100MS);
+		if (atomic) {
+			/* don't overflow udelay */
+			wait = min_t(int, wait, (MAX_UDELAY_MS * 1000);
+			udelay(wait));
+		} else {
 			usleep_range(wait, wait << 1);
-		wait <<= 1;
+		}
 	}
 
 	return -ETIMEDOUT;

base-commit: 2cd78713d917a02987f21b437561a5e1e13ddda2
-- 
2.43.0

Re: [PATCH 1/1] drm/xe/pcode: Fix pcode_try_request polling delays

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] drm/xe/pcode: syntax error in pcode_try_request() due to misplaced parenthesis
--

commit 3c7a166635d4e14258beaf50fb0fab91cf40aa3f
Author: Alan Previn <alan.previn.teres.alexis@intel.com>

drm/xe/pcode: Fix pcode_try_request polling delays

This commit caps the exponential backoff delay in pcode_try_request() to 100ms
and clamps the wait duration to the maximum allowed udelay value when polling
in atomic context.

> diff --git a/drivers/gpu/drm/xe/xe_pcode.c b/drivers/gpu/drm/xe/xe_pcode.c
> index 266deecbb100a..d2ed5edf003e3 100644
> --- a/drivers/gpu/drm/xe/xe_pcode.c
> +++ b/drivers/gpu/drm/xe/xe_pcode.c
> @@ -146,6 +146,7 @@ int xe_pcode_read(struct xe_tile *tile, u32 mbox, u32 *val0, u32 *val1)
>  	return err;
>  }
>  
> +#define __XE_PCODE_WAIT_MAX_BACKOFF_100MS (100 * USEC_PER_MSEC)
>  static int pcode_try_request(struct xe_tile *tile, u32 mbox,
>  			     u32 request, u32 reply_mask, u32 reply,
>  			     int *status, bool atomic, int timeout_us, bool locked)
> @@ -164,11 +165,14 @@ static int pcode_try_request(struct xe_tile *tile, u32 mbox,
>  		if ((*status == 0) && ((request & reply_mask) == reply))
>  			return 0;
>  
> -		if (atomic)
> -			udelay(wait);
> -		else
> +		wait = min_t(int, wait << 1, __XE_PCODE_WAIT_MAX_BACKOFF_100MS);
> +		if (atomic) {
> +			/* don't overflow udelay */
> +			wait = min_t(int, wait, (MAX_UDELAY_MS * 1000);
> +			udelay(wait));

[Severity: High]
Does this break compilation of drivers/gpu/drm/xe/xe_pcode.c? In the atomic
branch of pcode_try_request(), the closing parenthesis for min_t() is missing
before the semicolon and appears after udelay(wait) instead:

			wait = min_t(int, wait, (MAX_UDELAY_MS * 1000));
			udelay(wait);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261102174608.1279278-2-alan.previn.teres.alexis@intel.com?part=1