Re: [PATCH v3 2/6] 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 5 potential issue(s) to consider:
- [Medium] xe_ras: missing bounds check on pages_returned causes silent page skipping
- [Medium] xe_ras: unhandled errors in handle_page_offline abort queue fetching
- [Medium] xe_ras: integer overflow in loop termination check causes hang
- [Medium] xe_ras: missing endianness conversions in sysctrl payload structures
- [Low] build: pointer sign mismatch in xe_sysctrl_send_command call
--

commit c79936a34d5a6eb6a5f10a97e90fa4f355928529
Author: Riana Tauro <riana.tauro@intel.com>

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

This patch adds support to query the list of pages offlined by firmware
and the queue of pages pending to be offlined. It implements mailbox
commands to fetch the data from the firmware during module load.

> diff --git a/drivers/gpu/drm/xe/xe_ras.c b/drivers/gpu/drm/xe/xe_ras.c
> index 96ad473388a95..f1da7f983637a 100644
> --- a/drivers/gpu/drm/xe/xe_ras.c
> +++ b/drivers/gpu/drm/xe/xe_ras.c
> @@ -335,6 +335,77 @@ static bool ras_counter_is_valid(struct xe_device *xe, struct xe_ras_error_class
>  	return true;
>  }
>  
> +static void get_offline_pages(struct xe_device *xe, u32 cmd, void *req, size_t req_size,
> +			      void *resp, size_t resp_size,
> +			      struct xe_ras_offline_common *common, bool offline)
> +{
> +	struct xe_sysctrl_mailbox_command command = {0};
> +	struct xe_ras_offline_list_request *list_req;
> +	u32 total_pages = 0, count = 0;
> +	ssize_t rlen;
> +	int ret, i;
> +
> +	list_req = req ? req : NULL;
> +
> +	xe_sysctrl_create_command(&command, XE_SYSCTRL_GROUP_GFSP, cmd, req, req_size, resp,
> +				  resp_size);
> +
> +	do {
> +		memset(resp, 0, resp_size);
> +
> +		if (list_req)
> +			list_req->index = count;

[Severity: Medium]
Since the sysctrl mailbox header explicitly uses little-endian
data conversions, does the payload data also need to be little-endian? 
Defining the index as a native u32 and assigning it directly without
cpu_to_le32() might send corrupted values to the firmware on big-endian
architectures.

> +
> +		ret = xe_sysctrl_send_command(&xe->sc, &command, &rlen);

[Severity: Low]
Does passing a pointer to the signed ssize_t rlen variable here cause a
pointer sign mismatch compiler warning? It looks like
xe_sysctrl_send_command() expects a size_t * for its third argument.

> +		if (ret) {
> +			xe_log_err(xe, SYSCTRL, ret, "failed to get page offline data, cmd=%#x\n",
> +				   cmd);
> +			return;
> +		}
> +
> +		if (rlen != resp_size) {
> +			xe_log_err(xe, SYSCTRL, -EINVAL,
> +				   "unexpected page offline response length %zu (expected %zu), cmd=%#x\n",
> +				   rlen, resp_size, cmd);
> +			return;
> +		}
> +
> +		for (i = 0; i < common->pages_returned && i < XE_RAS_NUM_PAGES; i++)
> +			handle_page_offline(xe, common->page_addresses[i], offline);

[Severity: Medium]
Can handle_page_offline() fail (for instance with -EIO for a critical BO) and
return early without sending the offline command to the firmware? 

Because the return value is ignored here, the page might be left in the
firmware's pending queue. Would the firmware then repeatedly return the same
unhandled pages on the next fetch, eventually causing the loop below to
abruptly abort the entire queue processing once count exceeds total_pages?

> +
> +		count += common->pages_returned;

[Severity: Medium]
If the firmware returns a pages_returned value greater than XE_RAS_NUM_PAGES
(25), the loop above only processes up to 25 pages. Does incrementing count by
the full unvalidated pages_returned value incorrectly shift the list_req->index
for the next request, causing the driver to silently skip the remaining
poisoned pages?

> +		if (!common->pages_returned)
> +			break;
> +
> +		if (!total_pages)
> +			total_pages = common->total_pages;
> +
> +		if (count > total_pages) {

[Severity: Medium]
Both count and total_pages are 32-bit integers. If the firmware provides
total_pages as the maximum u32 value and additional_data remains set, could
count wrap around to 0? If so, this termination check would never be true,
resulting in an infinite loop.

> +			xe_log_err(xe, SYSCTRL, -EINVAL,
> +				   "Pages returned exceed total pages %u, returned %u, cmd=%#x\n",
> +				   total_pages, count, cmd);
> +			return;
> +		}
> +	} while (common->additional_data);
> +}

[ ... ]
> diff --git a/drivers/gpu/drm/xe/xe_ras_types.h b/drivers/gpu/drm/xe/xe_ras_types.h
> index f119489bcdf21..021ffbd6d4e2c 100644
> --- a/drivers/gpu/drm/xe/xe_ras_types.h
> +++ b/drivers/gpu/drm/xe/xe_ras_types.h
[ ... ]
> @@ -330,6 +331,40 @@ struct xe_ras_page_offline_response {
>  	u32 reserved;
>  } __packed;
>  
> +/**
> + * struct xe_ras_offline_common - Common structure for offline list and queue
> + */
> +struct xe_ras_offline_common {
> +	/** @total_pages: Total number of queued pages */
> +	u32 total_pages;
> +	/** @pages_returned: Number of pages returned in this response */
> +	u32 pages_returned;
> +	/** @page_addresses: Array of page addresses (4KB aligned) */
> +	u64 page_addresses[XE_RAS_NUM_PAGES];
> +	/** @additional_data: Indicates if more data is available */
> +	u8 additional_data;
> +	/** @reserved: Reserved for future use */
> +	u8 reserved[3];
> +} __packed;
> +
> +/**
> + * struct xe_ras_offline_list_request - Request for get offline list command
> + */
> +struct xe_ras_offline_list_request {
> +	/** @index: Zero-based index into the offline page list */
> +	u32 index;
> +} __packed;

[Severity: Medium]
Should the index field (and fields in the common structure above) be defined
using __le32 or __le64 types instead of native types to ensure correct
endianness conversion when communicating with the sysctrl firmware?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260928061830.417395-8-riana.tauro@intel.com?part=2