Re: [PATCH v12 4/4] drm/xe/ras: Use fault-inject to trigger punit error handler

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

On Tue, Aug 04, 2026 at 01:33:18PM +0530, Mallesh, Koujalagi wrote:
> 
> On 30-07-2026 12:56 pm, Mallesh, Koujalagi wrote:
> > 
> > On 29-07-2026 12:55 am, Rodrigo Vivi wrote:
> > > On Fri, Jul 24, 2026 at 03:33:07PM +0530, Mallesh Koujalagi wrote:
> > > > Use fault-inject framework to trigger punit_error_handler()
> > > > for testing.
> > > > 
> > > > Usage:
> > > >    echo 100 > .../inject_punit_error/probability
> > > >    echo 1   > .../inject_punit_error/times
> > > > 
> > > > Signed-off-by: Mallesh Koujalagi <mallesh.koujalagi@intel.com>
> > > > ---
> > > >   drivers/gpu/drm/xe/xe_debugfs.c |  4 ++++
> > > >   drivers/gpu/drm/xe/xe_debugfs.h |  2 ++
> > > >   drivers/gpu/drm/xe/xe_ras.c     | 17 +++++++++++++++++
> > > >   3 files changed, 23 insertions(+)
> > > > 
> > > > diff --git a/drivers/gpu/drm/xe/xe_debugfs.c
> > > > b/drivers/gpu/drm/xe/xe_debugfs.c
> > > > index 5a3877fcb0f0..5599d493feca 100644
> > > > --- a/drivers/gpu/drm/xe/xe_debugfs.c
> > > > +++ b/drivers/gpu/drm/xe/xe_debugfs.c
> > > > @@ -42,6 +42,7 @@
> > > >     DECLARE_FAULT_ATTR(gt_reset_failure);
> > > >   DECLARE_FAULT_ATTR(inject_csc_hw_error);
> > > > +DECLARE_FAULT_ATTR(inject_punit_error);
> > > >     static bool csc_hw_error_available(struct xe_device *xe)
> > > >   {
> > > > @@ -62,6 +63,8 @@ static struct {
> > > >       { .name = "inject_csc_hw_error",
> > > >         .attr = &inject_csc_hw_error,
> > > >         .is_visible = csc_hw_error_available },
> > > > +    { .name = "inject_punit_error",
> > > > +      .attr = &inject_punit_error },
> > > >   };
> > > >     /*
> > > > @@ -76,6 +79,7 @@ bool xe_fault_##name(void)                \
> > > >     FAULT_ACTION(gt_reset, gt_reset_failure)
> > > >   FAULT_ACTION(csc_hw_error, inject_csc_hw_error)
> > > > +FAULT_ACTION(punit_error, inject_punit_error)
> > > >     static void xe_fault_inject_debugfs_register(struct xe_device *xe,
> > > >                            struct dentry *root)
> > > > diff --git a/drivers/gpu/drm/xe/xe_debugfs.h
> > > > b/drivers/gpu/drm/xe/xe_debugfs.h
> > > > index cd56f7442b99..dd57914dd4f2 100644
> > > > --- a/drivers/gpu/drm/xe/xe_debugfs.h
> > > > +++ b/drivers/gpu/drm/xe/xe_debugfs.h
> > > > @@ -13,10 +13,12 @@ struct xe_device;
> > > >   #ifdef CONFIG_DEBUG_FS
> > > >   bool xe_fault_gt_reset(void);
> > > >   bool xe_fault_csc_hw_error(void);
> > > > +bool xe_fault_punit_error(void);
> > > >   void xe_debugfs_register(struct xe_device *xe);
> > > >   #else
> > > >   static inline bool xe_fault_gt_reset(void) { return false; }
> > > >   static inline bool xe_fault_csc_hw_error(void) { return false; }
> > > > +static inline bool xe_fault_punit_error(void) { return false; }
> > > >   static inline void xe_debugfs_register(struct xe_device *xe) { }
> > > >   #endif
> > > >   diff --git a/drivers/gpu/drm/xe/xe_ras.c
> > > > b/drivers/gpu/drm/xe/xe_ras.c
> > > > index 92b4181026cb..da06c16cf7ed 100644
> > > > --- a/drivers/gpu/drm/xe/xe_ras.c
> > > > +++ b/drivers/gpu/drm/xe/xe_ras.c
> > > > @@ -3,6 +3,7 @@
> > > >    * Copyright © 2026 Intel Corporation
> > > >    */
> > > >   +#include "xe_debugfs.h"
> > > >   #include "xe_device.h"
> > > >   #include "xe_drm_ras.h"
> > > >   #include "xe_pm.h"
> > > > @@ -391,6 +392,22 @@ enum xe_ras_recovery_action
> > > > xe_ras_process_errors(struct xe_device *xe)
> > > >       size_t rlen;
> > > >       int ret;
> > > >   +    /*
> > > > +     * Only allow the injected PUNIT error once the DRM device
> > > > is registered.
> > > That entirely defeats the purpose! :)
> > > 
> > > Sashiko identified this corner case so I asked you to ensure that
> > > this is covered
> > > and to prove that you should provide a fault-inject with test case.
> > > 
> > > But then you are skipping exactly the corner case. That makes
> > > absolutely no sense.
> > 
> > You are absolutely right!! Guarding the injection with
> > xe->drm.registered was the wrong approach.
> > 
> > The correct fix is structural, we are splitting xe_ras_init function
> > into xe_ras_init_early() (keep all bookkeeping) and
> > 
> > xe_ras_init() (called after  drm_dev_register in which
> > xe_ras_process_errors now lives).
> > 
> > 
> > Thanks,
> > 
> > -/Mallesh
> > 
> Sashiko corner case issue is addressed with patch: https://gitlab.freedesktop.org/drm/tip/-/commit/20bc4883c7c0e28c3ba6c76ccc279486c349dd3e
> 
> Do we still need this patch? Please let me know anything else I need to
> address here.

At least resend the series so Sashiko can check with the new base.

But I would still prefer if you send that in a way that that case
can actually be validated, not behind some strange checks but
pure fault-inject.

> 
> Thanks,
> 
> -/Mallesh
> 
> > > > +     * xe_ras_process_errors() also runs during probe (via
> > > > xe_ras_init()), before
> > > > +     * drm_dev_register() calls device_add() on the DRM minor's
> > > > kdev. At that
> > > > +     * point kdev->kobj.parent is not yet linked into the sysfs
> > > > hierarchy, so
> > > > +     * kobject_get_path() returns "/card0" instead of the real
> > > > sysfs path.
> > > > +     * drm_dev_wedged_event() would then emit a KOBJ_CHANGE
> > > > uevent with a wrong
> > > > +     * DEVPATH that udev cannot resolve, silently dropping the
> > > > event and leaving
> > > > +     * the cold-reset recovery broken.
> > > > +     */
> > > > +    if (xe->drm.registered && xe_fault_punit_error()) {
> > > > +        xe_err(xe, "[RAS]: PUNIT error injected\n");
> > > > +        punit_error_handler(xe);
> > > > +        return XE_RAS_RECOVERY_ACTION_DISCONNECT;
> > > > +    }
> > > > +
> > > >       if (!xe->info.has_sysctrl)
> > > >           return XE_RAS_RECOVERY_ACTION_RESET;
> > > >   --
> > > > 2.48.1
> > > >