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: determine the hotspots attribute first Date: Sat, 16 May 2026 15:55:01 +1000 Message-ID: In-Reply-To: <20260511021926.67673-1-yaolu@kylinos.cn> References: <20260511021926.67673-1-yaolu@kylinos.cn> <20260511021926.67673-1-yaolu@kylinos.cn> X-Mailer: Claude Code Patch Reviewer Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Patch Review **Correctness: Good.** The reordering is correct in both `drm_atomic_plane_= set_property()` and `drm_atomic_plane_get_property()`. The `hotspot_x_prope= rty` / `hotspot_y_property` checks are moved above the `plane->funcs->atomi= c_set/get_property` fallback, which is the right position =E2=80=94 core pr= operties should always be matched before falling through to driver callback= s. In the set path (line 633-635 after patch): ```c } else if (plane->funcs->atomic_set_property) { return plane->funcs->atomic_set_property(plane, state, property, val); ``` Now correctly sits after all core property checks and just before the final= `else` error branch. Same pattern applies to the get path (lines 700-701). **Issues to address:** 1. **Commit message is vague and unclear.** "determine the hotspots attribu= te first" doesn't accurately describe the change. A better subject would be= something like: > `drm/atomic: check hotspot properties before driver callback fallback` The body text is also unclear: > "If drm driver has both 'DRIVER_CURSOR_HOTSPOT' and 'atomic_set/get_pr= operty' will cause hotspots property set/get error." This should explain *why* the error occurs: the driver's `atomic_set_pro= perty` callback matches the hotspot property before the core code can handl= e it, and the driver callback doesn't know how to handle it, so it returns = an error. A suggested rewrite: > The driver's `atomic_set/get_property` callback is a catch-all for > driver-specific properties. It was checked before the hotspot > properties in the if-else chain, so for any driver that implements > both `DRIVER_CURSOR_HOTSPOT` and `plane->funcs->atomic_set_property`, > the hotspot properties would be dispatched to the driver callback > instead of being handled by the core code, resulting in errors. > > Move the hotspot property checks before the driver callback fallback, > consistent with all other core property checks in the chain. 2. **Missing `Fixes:` tag.** This fixes a bug introduced when hotspot suppo= rt was added (commit `4f49a260d4e5` "drm/atomic: Add support for mouse hots= pots" or similar =E2=80=94 the commit that added the hotspot handling to `d= rm_atomic_uapi.c`). A Fixes tag is needed for stable backport consideration. 3. **No indication of testing.** The commit message should mention how this= was tested, or at least which driver exhibited the problem (if any). If th= is was found by code inspection, say so. **Verdict:** The code change itself is correct and clean =E2=80=94 no funct= ional objection. The commit message and metadata need rework before this is= merge-ready. Request the author to v2 with improved commit message, a Fixe= s tag, and a note about how this was found/tested. --- Generated by Claude Code Patch Reviewer