Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [Medium] `backlight_set_brightness()` incorrectly returns success (0) and generates an erroneous uevent when called on an unregistered or unbound backlight device (`bd->ops == NULL`). - [Medium] The function-scoped `guard(mutex)(&bd->ops_lock)` holds the lock during `backlight_generate_event()`, violating established subsystem locking patterns and risking deadlocks. --
commit ef4454a90178562afe2f7457649f35d82b14a03f Author: Mario Limonciello (AMD) <[email protected]> backlight: add kernel-internal backlight API This patch adds kernel-internal backlight helpers to allow modifying backlights from within DRM. This provides user-space with a way to control backlight via DRM connector properties rather than relying solely on sysfs. > diff --git a/drivers/video/backlight/backlight.c > b/drivers/video/backlight/backlight.c > index ff2c2084c73a4..cd1a161ae7bc6 100644 > --- a/drivers/video/backlight/backlight.c > +++ b/drivers/video/backlight/backlight.c [ ... ] > +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) { > + if (value > bd->props.max_brightness) > + return -EINVAL; > + bd->props.brightness = value; > + rc = backlight_update_status(bd); > + } [Severity: Medium] Does this incorrectly return success and generate a spurious event if the backlight device is unregistered or unbound? If bd->ops is NULL, the update block is skipped and rc remains 0. Could this be initialized to a value like -ENXIO to correctly indicate failure instead? > + if (rc == 0) > + backlight_generate_event(bd, reason); [Severity: Medium] Should the ops_lock be released before generating the uevent? Because guard() is used at function scope, the lock is held across backlight_generate_event(). This means the lock is held during kobject_uevent_env() and sysfs_notify(), which might lead to stalls or lock inversions. Would it be safer to use a scoped_guard() restricted exclusively to the bd->ops operations block? > + > + return rc; > +} -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=2
