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 <[email protected]> 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/[email protected]?part=1
