From mboxrd@z Thu Jan  1 00:00:00 1970
From: Claude Code Review Bot <claude-review@example.com>
To: dri-devel-reviews@example.com
Subject: Claude review: drm/atomic: Return user readable error in atomic_ioctl
Date: Tue, 24 Feb 2026 10:27:00 +1000
Message-ID: <review-patch5-20260223-atomic-v10-5-f59c8def2e70@intel.com>
In-Reply-To: <20260223-atomic-v10-5-f59c8def2e70@intel.com>
References: <20260223-atomic-v10-0-f59c8def2e70@intel.com> <20260223-atomic-v10-5-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 is the main functional patch and has several issues.

> +	if (!arg->reserved)
> +		drm_dbg_atomic(dev,
> +			       "memory not allocated for drm_atomic error reporting\n");
> +	else
> +		/* Update the error code if any error to allow user handling it */
> +		error_code_ptr = (struct drm_mode_atomic_err_code __user *)
> +				 (unsigned long)arg->reserved;

Emitting a debug message every time userspace does NOT opt into error reporting seems backwards. Most userspace (including all existing userspace) will not set `reserved`, so this will spam the debug log on every successful atomic ioctl call. This debug message should be removed.

> +	memset(&state->error_code, 0, sizeof(*error_code_ptr));

When `arg->reserved` is 0, `error_code_ptr` is uninitialized. Using `sizeof(*error_code_ptr)` works because `sizeof` is evaluated at compile time, but this is confusing. It should be `sizeof(state->error_code)` for clarity.

> +	if (arg->flags & ~DRM_MODE_ATOMIC_FLAGS) {
> +		drm_mode_atomic_add_error_msg(&state->error_code,
> +					      DRM_MODE_ATOMIC_INVALID_API_USAGE,
> +					      "invalid flag");
>  		ret = -EINVAL;
>  		goto out;
>  	}

The patch removes the `drm_dbg_atomic` call for the invalid flags case, replacing it solely with the error_msg function. But `drm_mode_atomic_add_error_msg` calls `drm_dbg_atomic` internally, so the debug output is preserved. This is fine, though the other error paths (e.g., "atomic cap not enabled") keep both the explicit `drm_dbg_atomic` AND the error_msg call, which results in double debug output for those paths. The approach should be consistent.

> -	if (arg->reserved) {
> -		drm_dbg_atomic(dev, "commit failed: reserved field set\n");
> -		ret = -EINVAL;
> -		goto out;
> -	}

The `reserved` field validation is removed because the field is now repurposed. This relies on old userspace always passing 0 in `reserved`. Since the old kernel rejected non-zero values, well-behaved userspace should indeed pass 0. However, the safer pattern used elsewhere in DRM is to add a new flag to opt into the new behavior, rather than silently reinterpreting a previously-validated field.

> +out:
> +	/* Update the error code if any error to allow user handling it */
> +	if (ret < 0 && arg->reserved) {
> +		if (copy_to_user(error_code_ptr, &state->error_code,
> +				 sizeof(state->error_code)))
> +			ret = -EFAULT;
> +	}

This runs on the `-EDEADLK` path before the deadlock recovery check. If `copy_to_user` fails (e.g., the user pointer is invalid), `ret` is changed to `-EFAULT`, and the `ret == -EDEADLK` check below will be false. This means a bad error pointer will **prevent deadlock recovery**, turning a transient retry into a permanent failure. The `copy_to_user` should be skipped for `-EDEADLK`, or the EDEADLK check should be moved before the copy_to_user.

Also, on the `-EDEADLK` retry path, this writes potentially-stale error data to userspace (from a failed attempt that will be retried). The retry might succeed, but userspace has already received error data.

> +			if (ret) {
> +				drm_mode_atomic_add_error_msg(&state->error_code,
> +							      DRM_MODE_ATOMIC_ASYNC_PROP_CHANGED,
> +							      "property change not allowed in async flip");
> +			}

In `drm_atomic_set_property`, the error message is added for connector and crtc async property changes. But looking at the code flow, the `drm_atomic_check_prop_changes` call is made only inside the `if (async_flip)` block. The error message should include which property or object triggered it, similar to how the plane case includes `obj->id` and `plane->name`.

---
Generated by Claude Code Patch Reviewer