Re: [PATCH 2/4] drm/xe/pcode: Use int instead of u32 for mailbox status

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

New issues:
- [High] The patch causes an incompatible pointer type build warning/error because it fails to update the caller `xe_pcode_ready()`.

Pre-existing issues:
- [High] The polling loop in `pcode_try_request()` corrupts the original request payload by overwriting it with the hardware reply.
- [High] Uncapped exponential backoff causes massively oversized `usleep_range` and illegal `udelay` values.
--

--- Patch [2]: [PATCH 2/4] drm/xe/pcode: Use int instead of u32 for mailbox status ---
commit 4ff550781bd5498e3e5ce5f24fbf2a211b0938c1
Author: Umesh Nerlige Ramappa <[email protected]>

drm/xe/pcode: Use int instead of u32 for mailbox status

This commit changes the status parameter in pcode_try_request() and
xe_pcode_request() from u32 to int. Mailbox status is of type int,
and using u32 was likely a typo.

> diff --git a/drivers/gpu/drm/xe/xe_pcode.c b/drivers/gpu/drm/xe/xe_pcode.c
> index ccc3bdeed6bb2..e9125341481b7 100644
> --- a/drivers/gpu/drm/xe/xe_pcode.c
> +++ b/drivers/gpu/drm/xe/xe_pcode.c
> @@ -149,7 +149,7 @@ int xe_pcode_read(struct xe_tile *tile, u32 mbox, u32 *val, u32 *val1)
>  
>  static int pcode_try_request(struct xe_tile *tile, u32 mbox,
>  			     u32 request, u32 reply_mask, u32 reply,
> -			     u32 *status, bool atomic, int timeout_us, bool locked)
> +			     int *status, bool atomic, int timeout_us, bool locked)

[Severity: High]
Does this change cause a compile failure? In xe_pcode_ready(), status is
still defined as a u32 and passed as an argument to pcode_try_request():

xe_pcode_ready() {
	u32 status, request = DGFX_GET_INIT_STATUS;
	...
	ret = pcode_try_request(tile, DGFX_PCODE_STATUS, request,
				DGFX_INIT_STATUS_COMPLETE,
				DGFX_INIT_STATUS_COMPLETE,
				&status, false, timeout_us, locked);
	...
}

This results in passing a u32 pointer where an int pointer is expected.

[Severity: High]
This is a pre-existing issue, but does the polling loop in
pcode_try_request() corrupt the original request payload by overwriting it
with the hardware reply?

In pcode_try_request(), the request parameter is passed by reference to
__pcode_mailbox_rw() (and pcode_mailbox_rw()):

	*status = __pcode_mailbox_rw(tile, mbox, &request, NULL, 1, true,
				     atomic);

If the initial hardware poll fails, since return_data=true, the function
overwrites the request variable with the data read from PCODE_DATA0. On
subsequent loop iterations, corrupted data will be sent to the PCODE
hardware instead of the original request.

[Severity: High]
This is also a pre-existing issue, but does the uncapped exponential backoff
in pcode_try_request() cause oversized usleep_range() and illegal udelay()
values?

In the polling loop:

	if (atomic)
		udelay(wait);
	else
		usleep_range(wait, wait << 1);
	wait <<= 1;

The wait variable unconditionally doubles without an upper bound. When
called from xe_pcode_ready() with a 3-minute timeout, wait grows to over
83 million, causing usleep_range() to sleep for up to 167 seconds at a
time. When called from xe_pcode_request() with atomic=true and a 50ms
timeout, wait reaches 20,480, resulting in udelay(20480), which violates
kernel latency guidelines for atomic contexts.

>  {
>  	int slept, wait = 10;
>  

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2