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