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

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 <[email protected]>
>> ---
>>   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

>> +	 * 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
>>