Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [High] backlight_notify_brightness() is called inside backlight_update_status() while bd->ops_lock is held, creating an AB-BA deadlock risk with DRM listeners. - [Medium] Hardware/hotkey-driven brightness changes reported via backlight_force_update() bypass backlight_notify_brightness(), causing in-kernel listeners to lose sync with the actual state. --
commit 56d45c464432e9b84ad73dff6ec5f6ddda9af544 Author: Mario Limonciello <[email protected]> backlight: add kernel-internal backlight API This commit adds internal backlight helpers allowing DRM drivers to provide an interface for user-space to control the backlight via DRM connector properties. It introduces a brightness-changed notification and a per-device takeover count to inhibit legacy sysfs writes while a luminance-aware DRM client is in control. > diff --git a/drivers/video/backlight/backlight.c > b/drivers/video/backlight/backlight.c > index 4401f6294ccc8..1c700c1e77c3f 100644 > --- a/drivers/video/backlight/backlight.c > +++ b/drivers/video/backlight/backlight.c [ ... ] > @@ -548,6 +600,20 @@ int backlight_unregister_notifier(struct notifier_block > *nb) > } > EXPORT_SYMBOL(backlight_unregister_notifier); > > +/** > + * backlight_notify_brightness - notify brightness change to listeners > + * @bd: backlight device that changed > + * > + * Notify registered listeners that the backlight brightness has changed. > + * This is called automatically after successful brightness updates. > + */ > +void backlight_notify_brightness(struct backlight_device *bd) > +{ > + blocking_notifier_call_chain(&backlight_notifier, > + BACKLIGHT_BRIGHTNESS_CHANGED, bd); > +} > +EXPORT_SYMBOL(backlight_notify_brightness); [Severity: Medium] Will in-kernel listeners lose sync with the actual hardware backlight state when brightness is changed via a hardware hotkey or ACPI event? When physical hotkey presses trigger platform drivers to invoke backlight_force_update(): drivers/video/backlight/backlight.c:backlight_force_update() { ... mutex_unlock(&bd->ops_lock); backlight_generate_event(bd, reason); } It emits a user-space uevent via backlight_generate_event(), but it doesn't call the newly added backlight_notify_brightness() helper. Should it also notify in-kernel DRM connector luminance properties here? > diff --git a/include/linux/backlight.h b/include/linux/backlight.h > index 015183d129f96..fac1acbc698a1 100644 > --- a/include/linux/backlight.h > +++ b/include/linux/backlight.h [ ... ] > @@ -325,6 +351,10 @@ static inline int backlight_update_status(struct > backlight_device *bd) > ret = bd->ops->update_status(bd); > mutex_unlock(&bd->update_lock); > > + /* Notify DRM and other listeners that brightness changed */ > + if (ret == 0) > + backlight_notify_brightness(bd); [Severity: High] Does calling backlight_notify_brightness() here create an AB-BA deadlock risk with DRM listeners? The caller of backlight_update_status() often holds bd->ops_lock (for example, in backlight_set_brightness() or legacy sysfs writes). This means the blocking notifier chain is fired synchronously under ops_lock. If a DRM listener acquires a DRM lock during the notification callback, it establishes an ops_lock -> drm_lock dependency. Conversely, if a DRM driver calls backlight_set_brightness() while holding DRM locks, it will attempt to acquire ops_lock, resulting in a drm_lock -> ops_lock dependency. Should the notification be deferred until after ops_lock is released, just as backlight_generate_event() is handled? > + > return ret; > } > -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=2
