Re: [PATCH V17 09/12] drm/xe/configfs: Add vram bad page reservation policy

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] Dynamic configfs lookup at runtime causes state loss if the directory is removed.
- [Medium] Silent hardware corruption offlining under default policy (skips logging).
- [Low] Redundant `#include "xe_configfs.h"` directive.
--

--- Patch [9]: [PATCH V17 09/12] drm/xe/configfs: Add vram bad page reservation policy ---
commit ce0caab1275a7dd6bc0fdd6afde67d346ea6c808
Author: Tejas Upadhyay <[email protected]>

drm/xe/configfs: Add vram bad page reservation policy

The interface enables setting the policy for how bad pages are
handled in VRAM. This is crucial for maintaining system
stability in scenarios where VRAM degradation occurs.

> diff --git a/drivers/gpu/drm/xe/xe_ttm_vram_mgr.c b/drivers/gpu/drm/xe/xe_ttm_vram_mgr.c
> index 9311aa3728f84..51b98460481d9 100644
> --- a/drivers/gpu/drm/xe/xe_ttm_vram_mgr.c
> +++ b/drivers/gpu/drm/xe/xe_ttm_vram_mgr.c
> @@ -15,6 +15,8 @@
>  #include "regs/xe_regs.h"
>  #include "xe_bo.h"
>  #include "xe_configfs.h"
> +#include "xe_configfs.h"

[Severity: Low]
This isn't a bug, but is it intentional to include xe_configfs.h twice
on consecutive lines here?

>  #include "xe_device.h"
>  #include "xe_exec_queue.h"

[ ... ]

> @@ -860,6 +862,14 @@ int xe_ttm_vram_handle_addr_fault(struct xe_device *xe, u64 addr)
>  	vram_mgr = &vr->ttm;
>  	mm = &vram_mgr->mm;
>  
> +	policy = xe_configfs_get_bad_page_reservation(to_pci_dev(xe->drm.dev));

[Severity: Medium]
Is it safe to dynamically query the configfs tree during the hardware fault
handler?

If userspace removes the configfs directory via rmdir,
xe_configfs_get_bad_page_reservation() will fail to find the group and
silently revert to the default policy.

Since the attribute can only be set before binding, would it be more
robust to cache this value in struct xe_device at probe time rather than
evaluating it dynamically at runtime?

> +	if (!policy) {
> +		drm_err(&xe->drm, "0x%llx is reported as corrupted address by HW\n",
> +			addr);
> +		/* Let RAS report to FW to drop addr from SRAM queue */
> +		return -EOPNOTSUPP;
> +	}

[Severity: Medium]
Does this skip logging when the policy is set to offlining?

The commit message states that policy 1 means "Logging and offlining"
and 0 means "Logging only". However, the error logging is placed inside
the if (!policy) block. If policy is 1 (the default), the handler bypasses
this block and proceeds to reserve the page without logging the
degradation in dmesg.

> +
>  	/* Reserve page at address */
>  	return xe_ttm_vram_reserve_page_at_addr(xe, addr - vr->dpa_base, vram_mgr, mm);
>  }

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