Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider: - [High] DRM takeover protection is missing from bl_power_store(), allowing userspace to bypass DRM exclusivity and toggle the backlight power state. - [Medium] backlight_set_brightness() incorrectly returns success (0) and generates a spurious event when the backlight device is unregistered or unbound (bd->ops is NULL). - [Medium] guard(mutex)(&bd->ops_lock) unintentionally extends the lock scope over backlight_generate_event(), violating intended lock lifetime and risking lock contention. - [Medium] backlight_notify_brightness() is called from inconsistent locking contexts, exposing registered notifier chains to AB-BA deadlocks or lockdep splats. --
commit bb82de7554b837e30f99cb722d8f1a608b68529b Author: Mario Limonciello <[email protected]> Subject: backlight: add kernel-internal backlight API This commit adds kernel-internal backlight helpers to allow modifying backlights from within DRM. It introduces a brightness-changed notification and a per-device takeover count to inhibit legacy sysfs writes while a DRM client is in control. > diff --git a/drivers/video/backlight/backlight.c > b/drivers/video/backlight/backlight.c > index 51d8c3501768b..eec2aeadcc4b9 100644 > --- a/drivers/video/backlight/backlight.c > +++ b/drivers/video/backlight/backlight.c [ ... ] > @@ -214,6 +217,13 @@ static ssize_t brightness_store(struct device *dev, > struct backlight_device *bd = to_backlight_device(dev); > unsigned long brightness; > > + /* A luminance-aware DRM client has taken over this backlight; the > + * legacy sysfs interface is disabled until the last such client > + * goes away. > + */ > + if (atomic_read(&bd->drm_takeover) > 0) > + return -EBUSY; > + [Severity: High] Since drm_takeover prevents legacy sysfs writes here, does bl_power_store() also need this check? Without it, could userspace write to the legacy bl_power sysfs attribute to toggle the backlight power state, completely bypassing the DRM exclusivity constraint? [ ... ] > @@ -514,6 +524,39 @@ static int devm_backlight_device_match(struct device > *dev, void *res, > return *r == data; > } > > +/** > + * backlight_set_brightness - set brightness on a backlight device > + * @bd: backlight device to operate on > + * @value: brightness value to set on the device > + * @reason: backlight-change reason to use for notifications > + * > + * This is the in-kernel API equivalent of writing into the 'brightness' > sysfs > + * file. It calls into the underlying backlight driver to change the > brightness > + * value. > + * A uevent notification is sent with the reason set to @reason. > + * Return: 0 if successfully notified, -EINVAL for invalid values > + */ > +int backlight_set_brightness(struct backlight_device *bd, unsigned int value, > + enum backlight_update_reason reason) > +{ > + int rc = 0; > + > + guard(mutex)(&bd->ops_lock); > + if (bd->ops) { [Severity: Medium] If the backlight device is unregistered or unbound and bd->ops is NULL, this skips the if block, but rc remains 0. Could this falsely signal success to the caller and generate a spurious uevent even though no brightness change actually occurred? > + if (value > bd->props.max_brightness) > + return -EINVAL; > + > + dev_dbg(&bd->dev, "set brightness to %u\n", value); > + bd->props.brightness = value; > + rc = backlight_update_status(bd); > + } > + if (rc == 0) > + backlight_generate_event(bd, reason); [Severity: Medium] The function-scoped guard causes ops_lock to remain held during the call to backlight_generate_event(), which in turn calls sysfs_notify() and kobject_uevent_env(). Does this unintentionally extend the lock scope over the event generation, departing from the convention of releasing the lock before emitting events and potentially increasing lock contention? > + > + return rc; > +} > +EXPORT_SYMBOL_GPL(backlight_set_brightness); [ ... ] > diff --git a/include/linux/backlight.h b/include/linux/backlight.h > index 015183d129f96..18d9145198b5f 100644 > --- a/include/linux/backlight.h > +++ b/include/linux/backlight.h [ ... ] > @@ -325,6 +347,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: Medium] This emits the notifier call without guaranteeing a consistent state of ops_lock across all call paths. When backlight_update_status() is called from the sysfs path via backlight_device_set_brightness(), bd->ops_lock is held. However, when triggered by driver hotkeys, ops_lock might not be held. Does this expose registered notifier chains to inconsistent locking contexts, risking lockdep splats or AB-BA deadlocks if listeners acquire locks that nest with ops_lock? > + > return ret; > } > -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=2
