Re: [PATCH v7 02/12] backlight: add kernel-internal backlight API

[sashiko-bot@kernel.org]

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 <mario.limonciello@amd.com>
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/20260831210531.2334221-1-mario.limonciello@amd.com?part=2