Re: [PATCH v4 1/9] drm: Add a vendor-specific recovery method to device wedged uevent

[Riana Tauro <riana.tauro@intel.com>]

On 7/11/2025 2:29 PM, Simona Vetter wrote:
> On Thu, Jul 10, 2025 at 11:37:14AM +0200, Christian König wrote:
>> On 10.07.25 11:01, Simona Vetter wrote:
>>> On Wed, Jul 09, 2025 at 12:52:05PM -0400, Rodrigo Vivi wrote:
>>>> On Wed, Jul 09, 2025 at 05:18:54PM +0300, Raag Jadav wrote:
>>>>> On Wed, Jul 09, 2025 at 04:09:20PM +0200, Christian König wrote:
>>>>>> On 09.07.25 15:41, Simona Vetter wrote:
>>>>>>> On Wed, Jul 09, 2025 at 04:50:13PM +0530, Riana Tauro wrote:
>>>>>>>> Certain errors can cause the device to be wedged and may
>>>>>>>> require a vendor specific recovery method to restore normal
>>>>>>>> operation.
>>>>>>>>
>>>>>>>> Add a recovery method 'WEDGED=vendor-specific' for such errors. Vendors
>>>>>>>> must provide additional recovery documentation if this method
>>>>>>>> is used.
>>>>>>>>
>>>>>>>> v2: fix documentation (Raag)
>>>>>>>>
>>>>>>>> Cc: André Almeida <andrealmeid@igalia.com>
>>>>>>>> Cc: Christian König <christian.koenig@amd.com>
>>>>>>>> Cc: David Airlie <airlied@gmail.com>
>>>>>>>> Cc: <dri-devel@lists.freedesktop.org>
>>>>>>>> Suggested-by: Raag Jadav <raag.jadav@intel.com>
>>>>>>>> Signed-off-by: Riana Tauro <riana.tauro@intel.com>
>>>>>>>
>>>>>>> I'm not really understanding what this is useful for, maybe concrete
>>>>>>> example in the form of driver code that uses this, and some tool or
>>>>>>> documentation steps that should be taken for recovery?
>>>>
>>>> The case here is when FW underneath identified something badly corrupted on
>>>> FW land and decided that only a firmware-flashing could solve the day and
>>>> raise interrupt to the driver. At that point we want to wedge, but immediately
>>>> hint the admin the recommended action.
>>>>
>>>>>>
>>>>>> The recovery method for this particular case is to flash in a new firmware.
>>>>>>
>>>>>>> The issues I'm seeing here is that eventually we'll get different
>>>>>>> vendor-specific recovery steps, and maybe even on the same device, and
>>>>>>> that leads us to an enumeration issue. Since it's just a string and an
>>>>>>> enum I think it'd be better to just allocate a new one every time there's
>>>>>>> a new strange recovery method instead of this opaque approach.
>>>>>>
>>>>>> That is exactly the opposite of what we discussed so far.
>>>
>>> Sorry, I missed that context.
>>>
>>>>>> The original idea was to add a firmware-flush recovery method which
>>>>>> looked a bit wage since it didn't give any information on what to do
>>>>>> exactly.
>>>>>>
>>>>>> That's why I suggested to add a more generic vendor-specific event
>>>>>> with refers to the documentation and system log to see what actually
>>>>>> needs to be done.
>>>>>>
>>>>>> Otherwise we would end up with events like firmware-flash, update FW
>>>>>> image A, update FW image B, FW version mismatch etc....
>>>
>>> Yeah, that's kinda what I expect to happen, and we have enough numbers for
>>> this all to not be an issue.
>>>
>>>>> Agree. Any newly allocated method that is specific to a vendor is going to
>>>>> be opaque anyway, since it can't be generic for all drivers. This just helps
>>>>> reduce the noise in DRM core.
>>>>>
>>>>> And yes, there could be different vendor-specific cases for the same driver
>>>>> and the driver should be able to provide the means to distinguish between
>>>>> them.
>>>>
>>>> Sim, what's your take on this then?
>>>>
>>>> Should we get back to the original idea of firmware-flash?
>>>
>>> Maybe intel-firmware-flash or something, meaning prefix with the vendor?
>>>
>>> The reason I think it should be specific is because I'm assuming you want
>>> to script this. And if you have a big fleet with different vendors, then
>>> "vendor-specific" doesn't tell you enough. But if it's something like
>>> $vendor-$magic_step then it does become scriptable, and we do have have a
>>> place to put some documentation on what you should do instead.
>>>
>>> If the point of this interface isn't that it's scriptable, then I'm not
>>> sure why it needs to be an uevent?
>>
>> You should probably read up on the previous discussion, cause that is
>> exactly what I asked as well :)
>>
>> And no, it should *not* be scripted. That would be a bit brave for a
>> firmware update where you should absolutely not power down the system
>> for example.
> 
> I guess if we clearly state that this is for manual recovery only, or for
> cases where you exactly know what you're doing (fleet-specific scripts
> instead of generic distros), I guess this very opaque code makes sense.
> 
> But we should clearly document then that doing anything scripted here is
> very much "you get to keep the pieces", and definitely don't try to do
> something fancy generic.


The documentation is part of the series but was sent only to intel-xe 
mailing list. Will re-send the entire series to dri-devel

https://lore.kernel.org/intel-xe/aHH2XGuOvz8bSlax@black.fi.intel.com/T/#m883269cf0b1f6891ecc9c24d3d45325f46d56572


> 
> Which without documentation is just really confusing when some of the
> other error codes clearly look like they're meant to facilitate scripted
> recovery.
> 

To get consensus on the patch, is 'vendor-specific' okay or is it better 
to have 'firmware-flash' with additional event parameter 'vendor' if 
number of macros is not a concern?

Thanks
Riana
>> In my understanding the new value "vendor-specific" basically means it
>> is a known issue with a documented solution, while "unknown" means the
>> driver has no idea how to solve it.
> 
> I think that's another detail which should be documented clearly.
> -Sima
>>
>> Regards,
>> Christian.
>>
>>> I guess if you all want to stick with vendor-specific then I think that's
>>> ok with me too, but the docs should at least explain how to figure out
>>> from the uevent which vendor you're on with a small example. What I'm
>>> worried is that if we have this on multiple drivers userspace will
>>> otherwise make a complete mess and might want to run the wrong recovery
>>> steps.
>>>
>>> I think ideally, no matter what, we'd have a concrete driver patch which
>>> then also comes with the documentation for what exactly you're supposed to
>>> do as something you can script. And not just this stand-alone patch here.
>>>
>>> Cheers, Sima
>>>>
>>>>>
>>>>> Raag
>>>>>
>>>>>>>> ---
>>>>>>>>   Documentation/gpu/drm-uapi.rst | 9 +++++----
>>>>>>>>   drivers/gpu/drm/drm_drv.c      | 2 ++
>>>>>>>>   include/drm/drm_device.h       | 4 ++++
>>>>>>>>   3 files changed, 11 insertions(+), 4 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/Documentation/gpu/drm-uapi.rst b/Documentation/gpu/drm-uapi.rst
>>>>>>>> index 263e5a97c080..c33070bdb347 100644
>>>>>>>> --- a/Documentation/gpu/drm-uapi.rst
>>>>>>>> +++ b/Documentation/gpu/drm-uapi.rst
>>>>>>>> @@ -421,10 +421,10 @@ Recovery
>>>>>>>>   Current implementation defines three recovery methods, out of which, drivers
>>>>>>>>   can use any one, multiple or none. Method(s) of choice will be sent in the
>>>>>>>>   uevent environment as ``WEDGED=<method1>[,..,<methodN>]`` in order of less to
>>>>>>>> -more side-effects. If driver is unsure about recovery or method is unknown
>>>>>>>> -(like soft/hard system reboot, firmware flashing, physical device replacement
>>>>>>>> -or any other procedure which can't be attempted on the fly), ``WEDGED=unknown``
>>>>>>>> -will be sent instead.
>>>>>>>> +more side-effects. If recovery method is specific to vendor
>>>>>>>> +``WEDGED=vendor-specific`` will be sent and userspace should refer to vendor
>>>>>>>> +specific documentation for further recovery steps. If driver is unsure about
>>>>>>>> +recovery or method is unknown, ``WEDGED=unknown`` will be sent instead
>>>>>>>>   
>>>>>>>>   Userspace consumers can parse this event and attempt recovery as per the
>>>>>>>>   following expectations.
>>>>>>>> @@ -435,6 +435,7 @@ following expectations.
>>>>>>>>       none            optional telemetry collection
>>>>>>>>       rebind          unbind + bind driver
>>>>>>>>       bus-reset       unbind + bus reset/re-enumeration + bind
>>>>>>>> +    vendor-specific vendor specific recovery method
>>>>>>>>       unknown         consumer policy
>>>>>>>>       =============== ========================================
>>>>>>>>   
>>>>>>>> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
>>>>>>>> index cdd591b11488..0ac723a46a91 100644
>>>>>>>> --- a/drivers/gpu/drm/drm_drv.c
>>>>>>>> +++ b/drivers/gpu/drm/drm_drv.c
>>>>>>>> @@ -532,6 +532,8 @@ static const char *drm_get_wedge_recovery(unsigned int opt)
>>>>>>>>   		return "rebind";
>>>>>>>>   	case DRM_WEDGE_RECOVERY_BUS_RESET:
>>>>>>>>   		return "bus-reset";
>>>>>>>> +	case DRM_WEDGE_RECOVERY_VENDOR:
>>>>>>>> +		return "vendor-specific";
>>>>>>>>   	default:
>>>>>>>>   		return NULL;
>>>>>>>>   	}
>>>>>>>> diff --git a/include/drm/drm_device.h b/include/drm/drm_device.h
>>>>>>>> index 08b3b2467c4c..08a087f149ff 100644
>>>>>>>> --- a/include/drm/drm_device.h
>>>>>>>> +++ b/include/drm/drm_device.h
>>>>>>>> @@ -26,10 +26,14 @@ struct pci_controller;
>>>>>>>>    * Recovery methods for wedged device in order of less to more side-effects.
>>>>>>>>    * To be used with drm_dev_wedged_event() as recovery @method. Callers can
>>>>>>>>    * use any one, multiple (or'd) or none depending on their needs.
>>>>>>>> + *
>>>>>>>> + * Refer to "Device Wedging" chapter in Documentation/gpu/drm-uapi.rst for more
>>>>>>>> + * details.
>>>>>>>>    */
>>>>>>>>   #define DRM_WEDGE_RECOVERY_NONE		BIT(0)	/* optional telemetry collection */
>>>>>>>>   #define DRM_WEDGE_RECOVERY_REBIND	BIT(1)	/* unbind + bind driver */
>>>>>>>>   #define DRM_WEDGE_RECOVERY_BUS_RESET	BIT(2)	/* unbind + reset bus device + bind */
>>>>>>>> +#define DRM_WEDGE_RECOVERY_VENDOR	BIT(3)	/* vendor specific recovery method */
>>>>>>>>   
>>>>>>>>   /**
>>>>>>>>    * struct drm_wedge_task_info - information about the guilty task of a wedge dev
>>>>>>>> -- 
>>>>>>>> 2.47.1
>>>>>>>>
>>>>>>>
>>>>>>
>>>
>>
>