Re: [PATCH v11 1/3] drm: Add DRM_WEDGE_RECOVERY_COLD_RESET recovery method

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [Low] The newly introduced `cold-reset` uAPI string is missing from the official device wedging documentation.

Pre-existing issues:
- [High] The `event_string` buffer used for the uevent is too small to hold multiple wedge recovery methods, leading to silent truncation and malformed uevent strings being sent to userspace.
--

--- Patch [1]: [PATCH v11 1/3] drm: Add DRM_WEDGE_RECOVERY_COLD_RESET recovery method ---
commit 187c2c07431486fea095780d51c31e9abe6dc614
Author: Mallesh Koujalagi <mallesh.koujalagi@intel.com>

drm: Add DRM_WEDGE_RECOVERY_COLD_RESET recovery method

Introduce DRM_WEDGE_RECOVERY_COLD_RESET (BIT(4)) recovery method to handle
scenarios requiring device power cycle.

> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> index 1ff0bf7cba6aa..94abe7a811a52 100644
> --- a/drivers/gpu/drm/drm_drv.c
> +++ b/drivers/gpu/drm/drm_drv.c
> @@ -537,6 +537,8 @@ static const char *drm_get_wedge_recovery(unsigned int opt)
>  		return "bus-reset";
>  	case DRM_WEDGE_RECOVERY_VENDOR:
>  		return "vendor-specific";
> +	case DRM_WEDGE_RECOVERY_COLD_RESET:
> +		return "cold-reset";

[Severity: High]
This is a pre-existing issue, but is the WEDGE_STR_LEN buffer large enough
to hold all combinations now that a new recovery method is added?

In drivers/gpu/drm/drm_drv.c:drm_dev_wedged_event():

#define WEDGE_STR_LEN 32
...
	len = scnprintf(event_string, sizeof(event_string), "%s", "WEDGED=");
...
		len += scnprintf(event_string + len, sizeof(event_string) - len,
				 "%s,", recovery);
...
	if (recovery)
		/* Get rid of trailing comma */
		event_string[len - 1] = '\0';

If a device enters an unrecoverable state with multiple flags set
(like DRM_WEDGE_RECOVERY_REBIND | DRM_WEDGE_RECOVERY_BUS_RESET |
DRM_WEDGE_RECOVERY_COLD_RESET), the string representation exceeds the 32 byte
limit of event_string.

Because scnprintf caps the write, the trailing comma removal logic will
inadvertently overwrite the last valid character, leading to a malformed
uevent string being sent to userspace (e.g., WEDGED=rebind,bus-reset,cold-r).
Could this prevent userspace parsers from recognizing the method?

>  	default:
>  		return NULL;
>  	}
> diff --git a/include/drm/drm_device.h b/include/drm/drm_device.h
> index 768a8dae83c52..75f030d027ee4 100644
> --- a/include/drm/drm_device.h
> +++ b/include/drm/drm_device.h
> @@ -37,6 +37,7 @@ struct pci_controller;
>  #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 */
> +#define DRM_WEDGE_RECOVERY_COLD_RESET	BIT(4)	/* remove device + slot power cycle + rescan */

[Severity: Low]
Should this newly introduced cold-reset method also be added to the uAPI
documentation?

The device wedging documentation in Documentation/gpu/drm-uapi.rst lists
consumer expectations for other methods (none, rebind, bus-reset,
vendor-specific, unknown), but cold-reset is currently missing.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260720101815.526813-5-mallesh.koujalagi@intel.com?part=1