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

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The PCODE polling loop in `pcode_try_request` overwrites its own `request` parameter with the hardware response, causing subsequent polling iterations to send corrupted/invalid commands.
- [High] The polling loop in `pcode_try_request` doubles its wait time indefinitely without an upper bound, leading to severe latency spikes and misuse of the `usleep_range` and `udelay` APIs.
--

--- Patch [1]: [PATCH] drm/xe/pcode: Use int instead of u32 for mailbox status ---
commit d73e65e5c069900a41101231b929154ebfc4ed92
Author: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>

drm/xe/pcode: Use int instead of u32 for mailbox status
    
Mailbox status is returned as int, but some callers read it as u32
(likely a typo). Use int instead.

> diff --git a/drivers/gpu/drm/xe/xe_pcode.c b/drivers/gpu/drm/xe/xe_pcode.c
> index e1b8062541a91..5f7fc61c9b97c 100644
> --- a/drivers/gpu/drm/xe/xe_pcode.c
> +++ b/drivers/gpu/drm/xe/xe_pcode.c
> @@ -148,7 +148,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)
>  {
>  	int slept, wait = 10;
>  

[Severity: High]
This is a pre-existing issue, but does the polling loop in this function
overwrite its own request parameter with the hardware response?

Looking at pcode_try_request(), the request parameter is passed to
pcode_mailbox_rw():

	for (slept = 0; slept < timeout_us; slept += wait) {
		if (locked)
			*status = pcode_mailbox_rw(tile, mbox, &request, NULL, 1, true,
						   atomic);
		else
			*status = __pcode_mailbox_rw(tile, mbox, &request, NULL, 1, true,
						     atomic);
		if ((*status == 0) && ((request & reply_mask) == reply))
			return 0;

Since pcode_mailbox_rw() unconditionally overwrites the memory pointed to by
&request with the hardware response when return_data is true, if the expected
condition is false and the loop continues, won't the next iteration send the
hardware's previous reply back as the new request command instead of the
original command?

[Severity: High]
This isn't a bug introduced by this patch, but does the wait time double
indefinitely without an upper bound?

Further down in the loop:

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

When called from xe_pcode_ready(), timeout_us is 3 minutes (180,000,000 us).
The wait variable will grow geometrically up to ~134 million microseconds,
causing the loop to sleep for over 134 seconds in a single contiguous chunk.

Additionally, when called from xe_pcode_request() with preemption disabled
(atomic = true), timeout_us is 50,000. The wait variable doubles up to 40,960.
Calling udelay(40960) stalls the CPU for ~41ms with preemption disabled, which
could trigger NMI watchdogs and cause system instability. Should there be a cap
on the wait time?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260818231140.729887-2-umesh.nerlige.ramappa@intel.com?part=1