Re: [PATCH v2 2/3] drm/xe/pcode: Improve PCODE logging using xe_log helper

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

New issues:
- [Medium] Incomplete migration to xe_log_err for PCODE errors

Pre-existing issues:
- [High] Unbounded exponential backoff in PCODE polling loop causes massive sleeps and dangerous udelay() calls
--

--- Patch [2]: [PATCH v2 2/3] drm/xe/pcode: Improve PCODE logging using xe_log helper ---
commit be49e77e4b5b367f065e60c69fb19b1b95cdf273
Author: Mallesh Koujalagi <[email protected]>

drm/xe/pcode: Improve PCODE logging using xe_log helper

This commit updates the driver to use the xe_log_err() helper for PCODE
errors instead of drm_err(). This aims to add the PCODE component tag to the
log messages to make issues easier to track.

> Use xe_log helper for PCODE errors instead of drm_err(). This
> adds PCODE component tag, making issues easier to detect and
> track with automated tools.

[Severity: Medium]
Did we miss updating the drm_err() call in pcode_mailbox_status()?

drivers/gpu/drm/xe/xe_pcode.c:pcode_mailbox_status() {
	if (err) {
		drm_err(&tile_to_xe(tile)->drm, "PCODE Mailbox failed: %d %s",
			err_decode, err_str);

		return err_decode;
	}
...
}

It seems this error logging statement was left unconverted. Leaving it as
drm_err() means automated tools tracking the PCODE component tag might miss
these critical hardware mailbox failures.

> diff --git a/drivers/gpu/drm/xe/xe_pcode.c b/drivers/gpu/drm/xe/xe_pcode.c
> index ccc3bdeed6bb2..c69440ea477b4 100644
> --- a/drivers/gpu/drm/xe/xe_pcode.c
> +++ b/drivers/gpu/drm/xe/xe_pcode.c

[ ... ]

> @@ -219,8 +220,8 @@ int xe_pcode_request(struct xe_tile *tile, u32 mbox, u32 request,
>  	 * requests, and for any quirks of the PCODE firmware that delays
>  	 * the request completion.
>  	 */
> -	drm_err(&tile_to_xe(tile)->drm,
> -		"PCODE timeout, retrying with preemption disabled\n");
> +	xe_log_err(tile, PCODE, -ETIMEDOUT,
> +		   "Timeout, retrying with preemption disabled\n");
>  	preempt_disable();
>  	ret = pcode_try_request(tile, mbox, request, reply_mask, reply, &status,
>  				true, 50 * 1000, true);

[Severity: High]
This isn't a bug introduced by this patch, but does the polling loop in
pcode_try_request() have an unbounded exponential backoff?

Looking at the pcode_try_request() function called here:

	for (slept = 0; slept < timeout_us; slept += wait) {
		...
		if (atomic)
			udelay(wait);
		else
			usleep_range(wait, wait << 1);
		wait <<= 1;
	}

Because wait is doubled on every iteration without an upper limit, it
exponentially grows. When called from xe_pcode_request() during a timeout
retry with atomic = true and a 50,000us timeout, wait reaches 32,768us,
resulting in a udelay(32768). Busy-waiting for over 32 milliseconds with
preemption disabled can trigger CPU soft lockups.

Similarly, when pcode_try_request() is called for operations with a 3-minute
timeout (timeout_us = 180000000) during initialization in xe_pcode_ready(),
the wait value grows to over 90 seconds. This causes a massive
usleep_range(90000000, 180000000) that stalls the thread for minutes at a
time without checking the hardware status, which could severely degrade
driver probe times.

[ ... ]

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