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 <[email protected]> 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/[email protected]?part=6
