Re: [PATCH v8 6/6] drm/xe/ras: Add debugfs entry to inject punit error

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Unconditional registration of the `inject_punit_error` debugfs file exposes PF-only hardware operations to Virtual Functions (VFs) and ignores sysctrl capability checks.
- [Low] Missing forward declaration of `struct dentry` in `xe_ras.h` when `CONFIG_DRM_XE_DEBUG` is disabled.
--

commit 185b28acb4e5add829b3fd14f6a6416e4d2a9fe3
Author: Mallesh Koujalagi <mallesh.koujalagi@intel.com>

drm/xe/ras: Add debugfs entry to inject punit error
    
Add a write-only debugfs file 'inject_punit_error' under the DRI device
root that allows triggering punit_error_handler() manually for testing.
Writing 1 to the file invokes the handler, which suppresses spurious
Surprise Link Down AER events on slots and declares the
device wedged for a cold reset recovery.

> diff --git a/drivers/gpu/drm/xe/xe_ras.c b/drivers/gpu/drm/xe/xe_ras.c
> index 15c2fa0d323a6..281ef4dce2188 100644
> --- a/drivers/gpu/drm/xe/xe_ras.c
> +++ b/drivers/gpu/drm/xe/xe_ras.c
[ ... ]
> +void xe_ras_debugfs_register(struct xe_device *xe, struct dentry *root)
> +{
> +	debugfs_create_file("inject_punit_error", 0200, root, xe,
> +			    &inject_punit_error_fops);

[Severity: Medium]
Could this expose PF-only hardware operations to Virtual Functions (VFs)?

Writing to this debugfs file executes punit_error_handler(), which
manipulates the physical AER configuration on the Upstream Switch Port (USP)
via PCI config space and triggers a PCIe cold reset. According to SR-IOV
constraints, VFs must not access PF-only hardware resources like physical
PCIe bridge configurations.

Additionally, this doesn't check if the device actually supports sysctrl
before allowing the error injection. Should this function include an early
return guard like if (IS_SRIOV_VF(xe) || !xe->info.has_sysctrl) before
creating the file?

> +}
> +#endif /* CONFIG_DRM_XE_DEBUG */
[ ... ]
> diff --git a/drivers/gpu/drm/xe/xe_ras.h b/drivers/gpu/drm/xe/xe_ras.h
> index 8d106c708ff19..c9f84ef238c42 100644
> --- a/drivers/gpu/drm/xe/xe_ras.h
> +++ b/drivers/gpu/drm/xe/xe_ras.h
> @@ -16,4 +16,11 @@ void xe_ras_counter_threshold_crossed(struct xe_device *xe,
>  void xe_ras_init(struct xe_device *xe);
>  enum xe_ras_recovery_action xe_ras_process_errors(struct xe_device *xe);
>  
> +#ifdef CONFIG_DRM_XE_DEBUG
> +struct dentry;
> +void xe_ras_debugfs_register(struct xe_device *xe, struct dentry *root);
> +#else
> +static inline void xe_ras_debugfs_register(struct xe_device *xe, struct dentry *root) {}

[Severity: Low]
Does this cause a compiler warning when CONFIG_DRM_XE_DEBUG is disabled?

The forward declaration of struct dentry is inside the #ifdef block, but the
#else block uses struct dentry * in the parameter list of the static inline
function. In C, this scopes the struct declaration to the function prototype
itself if it wasn't previously declared.

Should the struct dentry; forward declaration be moved outside the #ifdef
block to ensure it is visible to the inline stub?

> +#endif
> +
>  #endif

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260612080722.26726-8-mallesh.koujalagi@intel.com?part=6