Re: [PATCH v3 3/6] drm/xe: Separate drm-ras netlink data from device and firmware RAS state

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] drm/xe/ras: accidental deletion of platform check in xe_ras_init
--

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

drm/xe: Separate drm-ras netlink data from device and firmware RAS state

This commit separates the DRM RAS data used for netlink reporting from the
device and firmware RAS state in xe_device. It introduces a new xe_ras_state
structure to encapsulate configuration such as the disable_page_offline flag.

> diff --git a/drivers/gpu/drm/xe/xe_ras.c b/drivers/gpu/drm/xe/xe_ras.c
> index f1da7f983637a..f7232429080a8 100644
> --- a/drivers/gpu/drm/xe/xe_ras.c
> +++ b/drivers/gpu/drm/xe/xe_ras.c
[ ... ]
> @@ -1104,19 +1115,18 @@ void xe_ras_init(struct xe_device *xe)
>  {
>  	int ret;
>  
> -	/*
> -	 * TODO: Replace platform check with xe->info.has_disable_vram_page_offline
> -	 * once the feature flag is plumbed through device info.
> -	 */
> -	if (xe->info.platform == XE_CRESCENTISLAND)
> -		xe->ras.disable_vram_page_offline =
> -			xe_configfs_get_disable_vram_page_offline(to_pci_dev(xe->drm.dev));
> -
>  	xe_drm_ras_init(xe);
>  
>  	if (!xe->info.has_sysctrl)
>  		return;
>  
> +	/*
> +	 * TODO: Replace platform check with xe->info.has_disable_vram_page_offline
> +	 * once the feature flag is plumbed through device info.
> +	 */
> +	xe->ras.state.disable_page_offline =
> +		xe_configfs_get_disable_vram_page_offline(to_pci_dev(xe->drm.dev));

[Severity: Low]
Was the removal of the platform check intentional here?

The TODO comment mentions replacing the platform check, but it looks like the
if (xe->info.platform == XE_CRESCENTISLAND) condition was simply deleted
rather than replaced. 

This changes the evaluation scope of the configfs policy in xe_ras_init(). It
will now be evaluated for all platforms that have has_sysctrl set to true,
rather than being restricted to XE_CRESCENTISLAND.

Additionally, because VFs always have has_sysctrl set to 0:

drivers/gpu/drm/xe/xe_pci.c:xe_info_init() {
    ...
	if (IS_SRIOV_VF(xe)) {
		xe->info.has_sysctrl = 0;
    ...
}

The policy will no longer be initialized on VFs due to the early return.
While this shouldn't have a functional impact on VFs since they don't handle
physical VRAM faults, was it intended to evaluate this policy on all other
sysctrl-capable platforms without plumbing the feature flag first?

> +
>  	if (IS_ENABLED(CONFIG_PCIEAER))
>  		ras_usp_aer_init(xe);
>

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