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: Define user readable error codes for atomic ioctl Date: Tue, 24 Feb 2026 10:26:59 +1000 Message-ID: In-Reply-To: <20260223-atomic-v10-1-f59c8def2e70@intel.com> References: <20260223-atomic-v10-0-f59c8def2e70@intel.com> <20260223-atomic-v10-1-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 This patch defines the UAPI structures and enums for error reporting. > +enum drm_mode_atomic_failure_codes { > + DRM_MODE_ATOMIC_INVALID_API_USAGE, > + DRM_MODE_ATOMIC_CRTC_NEED_FULL_MODESET, > + DRM_MODE_ATOMIC_NEED_FULL_MODESET, > + DRM_MODE_ATOMIC_ASYNC_PROP_CHANGED, > +}; `DRM_MODE_ATOMIC_INVALID_API_USAGE` has the value 0. Since the error struct is zero-initialized, userspace cannot distinguish "no error code was set" from "the error was invalid API usage." The enum should start at 1, or a sentinel/none value should be added at 0. `DRM_MODE_ATOMIC_NEED_FULL_MODESET` is defined here but never used in any patch in this series. Is it intended for future use? If so, it should not be part of the initial UAPI -- adding unused UAPI values creates a commitment without any implementation to validate the semantics. > + * @DRM_MODE_ATOMIC_INVALID_API_USAGE: invallid API usage(DRM_ATOMIC not > + * enabled, invalid falg, page_flip event Typos: "invallid" should be "invalid", "falg" should be "flag." > + * @failure_code: error codes defined in enum drm_moide_atomic_failure_code Typo: "drm_moide_atomic_failure_code" should be "drm_mode_atomic_failure_codes." > +struct drm_mode_atomic_err_code { > + __u64 failure_code; > + __u64 failure_objs_ptr; > + __u64 reserved; > + __u32 count_objs; > + char failure_string[DRM_MODE_ATOMIC_FAILURE_STRING_LEN]; > +}; `failure_objs_ptr`, `count_objs`, and `reserved` are defined here but never populated by any patch in this series. For UAPI, it's better to add fields when they are actually implemented. Shipping dead fields creates an API contract without any way to validate it. The `reserved` field in a new UAPI struct must be validated to be zero by the kernel if it's intended for future extension. No such validation exists. The struct has a `__u32 count_objs` followed by a `char` array, which means the total struct size is 156 bytes (not a multiple of 8). While this works, adding an explicit `__u32 pad` after `count_objs` would be more consistent with DRM UAPI conventions. --- Generated by Claude Code Patch Reviewer