From mboxrd@z Thu Jan 1 00:00:00 1970 From: Claude Code Review Bot To: dri-devel-reviews@example.com Subject: Claude review: drm/atomic: Add error_code element in atomic_state Date: Tue, 24 Feb 2026 10:26:59 +1000 Message-ID: In-Reply-To: <20260223-atomic-v10-2-f59c8def2e70@intel.com> References: <20260223-atomic-v10-0-f59c8def2e70@intel.com> <20260223-atomic-v10-2-f59c8def2e70@intel.com> X-Mailer: Claude Code Patch Reviewer Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit MIME-Version: 1.0 Patch Review > + failure_string = kvasprintf(GFP_ATOMIC, format, varg); > + > + drm_dbg_atomic(state->dev, "Commit failed: %s\n", failure_string); > + strscpy_pad(err_code->failure_string, failure_string, > + sizeof(err_code->failure_string)); > + va_end(varg); Two bugs here. First, `kvasprintf()` allocates memory that is **never freed with `kfree()`**. Every call to this function leaks memory. Second, if `kvasprintf()` returns NULL (allocation failure), both `drm_dbg_atomic` and `strscpy_pad` will dereference a NULL pointer. The fix is to add `kfree(failure_string)` after `strscpy_pad`, and to handle the NULL case. Alternatively, since the goal is just to format into a fixed-size buffer, `vsnprintf` directly into `err_code->failure_string` would avoid the allocation entirely and be simpler. > + failure_string = kvasprintf(GFP_ATOMIC, format, varg); `GFP_ATOMIC` is for interrupt/spinlock context. This function is called from ioctl handlers (process context), so `GFP_KERNEL` would be appropriate. But as noted above, the allocation can be avoided entirely. > + /* @error_code: pointer to struct holding failure reason and string > + * > + * struct to convey user readable error to the user. > + * Error codes defined in enum drm_mode_atomic_failure_flags > + */ > + struct drm_mode_atomic_err_code error_code; The kernel-doc says "pointer to struct" but this is an embedded struct, not a pointer. Also references `drm_mode_atomic_failure_flags` which doesn't exist -- it should be `drm_mode_atomic_failure_codes`. The comment is missing the `/**` kernel-doc opening marker (uses `/*` instead). Embedding a UAPI struct (`struct drm_mode_atomic_err_code` from `drm_mode.h`) directly in a kernel-internal struct (`struct drm_atomic_state`) is unusual. The UAPI types (`__u64`, `char[]`) are unnecessary for kernel-internal storage. A more conventional approach would be to use kernel types internally and convert when copying to userspace. --- Generated by Claude Code Patch Reviewer