From mboxrd@z Thu Jan 1 00:00:00 1970 From: Claude Code Review Bot To: dri-devel-reviews@example.com Subject: Claude review: backlight: add kernel-internal backlight API Date: Thu, 04 Jun 2026 14:54:30 +1000 Message-ID: In-Reply-To: <20260531114908.1693426-3-superm1@kernel.org> References: <20260531114908.1693426-1-superm1@kernel.org> <20260531114908.1693426-3-superm1@kernel.org> X-Mailer: Claude Code Patch Reviewer Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Patch Review **Concern: Double notification in backlight_set_brightness** `backlight_set_brightness` calls `backlight_update_status(bd)` and then `ba= cklight_generate_event()`. But in patch 3, `backlight_update_status` is mod= ified to also call `backlight_notify_brightness()`. This means a single `ba= cklight_set_brightness` call will: 1. Call `backlight_update_status` =E2=86=92 which calls `backlight_notify_b= rightness` (notifier chain) 2. Then call `backlight_generate_event` (uevent) 3. The notifier from step 1 fires `__drm_backlight_real_changed` which is s= upposed to update the DRM property The `backlight_generate_event` call here is arguably correct (it sends a ue= vent), but the interaction with the notifier added in patch 3 creates a loo= p risk: DRM sets brightness =E2=86=92 `backlight_set_brightness` =E2=86=92 = `backlight_update_status` =E2=86=92 `backlight_notify_brightness` =E2=86=92= DRM notifier =E2=86=92 `__drm_backlight_real_changed`. The `__drm_backligh= t_real_changed` function updates `connector->state->luminance` under spinlo= ck, which should be safe, but it's worth verifying there's no re-entrant pa= th. **Concern: ops_lock held across backlight_generate_event** ```c + guard(mutex)(&bd->ops_lock); + if (bd->ops) { + if (value > bd->props.max_brightness) + return -EINVAL; + bd->props.brightness =3D value; + rc =3D backlight_update_status(bd); + } + if (rc =3D=3D 0) + backlight_generate_event(bd, reason); ``` `backlight_generate_event` calls `kobject_uevent_env` which can sleep and a= llocate. It's called with `ops_lock` held (via `guard`). While `ops_lock` i= s a mutex so sleeping is OK, holding it longer than necessary across a ueve= nt broadcast isn't ideal. But it's not a correctness bug. **Minor**: `backlight_set_brightness` sends a uevent even when `bd->ops` is= NULL (rc stays 0 in that case). Should it return early if ops is NULL? --- Generated by Claude Code Patch Reviewer