Re: [PATCH 3/5] drm/xe/xe_ras: Add support to query page offline queue and list

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Comparing cumulatively increasing `count` against a decreasing queue size (`total_pages`) causes spurious failures and prevents fully draining the offline queue.
- [Medium] Un-clamped increment of pagination counter `count` causes silent skipping of offlined pages if firmware returns a large `pages_returned` value.
--

--- Patch [3]: [PATCH 3/5] drm/xe/xe_ras: Add support to query page offline queue and list ---
commit e8cc2bc6a8f50a7f0975b37b113c85a2c4043858
Author: Riana Tauro <riana.tauro@intel.com>

drm/xe/xe_ras: Add support to query page offline queue and list

This commit adds support to query the page offline list and queue from the
firmware during module load. The page offline list retrieves pages already
offlined, and the queue retrieves pages pending to be offlined.

> diff --git a/drivers/gpu/drm/xe/xe_ras.c b/drivers/gpu/drm/xe/xe_ras.c
> index a6fa755ffebf7..b320ba6d1d735 100644
> --- a/drivers/gpu/drm/xe/xe_ras.c
> +++ b/drivers/gpu/drm/xe/xe_ras.c
> @@ -328,6 +328,102 @@ static bool ras_counter_is_valid(struct xe_device *xe, struct xe_ras_error_class
>  	return true;
>  }
>  
> +static void get_queued_pages(struct xe_device *xe)
> +{

[ ... ]

> +		for (i = 0; i < response.pages_returned && i < XE_RAS_NUM_PAGES; i++)
> +			handle_page_offline(xe, response.page_addresses[i], true);
> +
> +		count += response.pages_returned;

[Severity: Medium]
If the firmware returns a response.pages_returned value greater than
XE_RAS_NUM_PAGES (25), we cap the processing loop at 25 but still add the full
un-clamped response.pages_returned to count. Does this mean we might silently
skip processing some pages?

> +		if (!response.pages_returned)
> +			break;
> +
> +		if (count > response.total_pages) {

[Severity: High]
When handle_page_offline() processes a queued page (with send_cmd=true), it
sends a PAGE_OFFLINE command to the firmware to consume the page. Does this
cause the firmware's remaining queue size (response.total_pages) to decrease
on subsequent loop iterations? 

If count accumulates monotonically while response.total_pages shrinks, will
this check spuriously fail and abort midway through processing a queue that
requires multiple fetches?

> +			xe_log_err(xe, SYSCTRL, -EINVAL,
> +				   "Pages returned from queue exceed total pages %u, returned %u\n",
> +				   response.total_pages, count);
> +			return;
> +		}
> +	} while (response.additional_data);
> +}
> +
> +static void get_offlined_list(struct xe_device *xe)
> +{

[ ... ]

> +		for (i = 0; i < response.pages_returned && i < XE_RAS_NUM_PAGES; i++)
> +			handle_page_offline(xe, response.page_addresses[i], false);
> +
> +		count += response.pages_returned;

[Severity: Medium]
Similar to the issue in get_queued_pages(), if response.pages_returned exceeds
XE_RAS_NUM_PAGES, count increments by the full un-clamped amount.

Since count is subsequently used to set request.index for the next sequence,
will this cause unprocessed entries to be permanently skipped?

> +		if (!response.pages_returned)
> +			break;
> +
> +		if (count > response.total_pages) {

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260825063615.3697317-7-riana.tauro@intel.com?part=3