Re: [PATCH v10 2/4] 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:
- [Medium] The patch introduces a new UAPI recovery method `DRM_WEDGE_RECOVERY_COLD_RESET` (`cold-reset`) but fails to document it in the DRM UAPI documentation.

Pre-existing issues:
- [High] The `event_string` buffer used for the `WEDGED=` uevent is fixed at 32 bytes (`WEDGE_STR_LEN`), which is too small to hold multiple recovery methods, leading to silent string truncation and malformed UAPI strings.
--

--- Patch [2]: [PATCH v10 2/4] drm: Add DRM_WEDGE_RECOVERY_COLD_RESET recovery method ---
commit 683268e37f79f71da69dfdeafdea583f1453ed24
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 does adding a new recovery method increase
the risk of truncating the UAPI event string?

Looking at drm_dev_wedged_event(), the event_string buffer is fixed to 32
bytes (WEDGE_STR_LEN). If a driver sets multiple recovery methods (like rebind,
bus-reset, and cold-reset), their concatenated length plus "WEDGED=" could
easily exceed this limit:

drivers/gpu/drm/drm_drv.c:drm_dev_wedged_event() {
    ...
		len += scnprintf(event_string + len, sizeof(event_string) - len, "%s,", recovery);
	}

	if (recovery)
		/* Get rid of trailing comma */
		event_string[len - 1] = '\0';
    ...
}

When the buffer truncates, won't the trailing comma removal overwrite the last
valid character of the truncated string and result in malformed UAPI strings
being sent to userspace (e.g., "WEDGED=rebind,bus-reset,cold-r")?

[Severity: Medium]
Should this new recovery method also be documented in the UAPI documentation?

The current Recovery section table in Documentation/gpu/drm-uapi.rst lists all
other UAPI recovery methods and their expectations:

    =============== ========================================
    Recovery method Consumer 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
    =============== ========================================

Could "cold-reset" be added here so userspace consumers have official
documentation for implementing their recovery rules?

>  	default:
>  		return NULL;
>  	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260713083939.251872-6-mallesh.koujalagi@intel.com?part=2