Re: [PATCH v8 04/14] drm: add connector backlight (LUMINANCE) infrastructure

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 13 potential issue(s) to consider:
- [Critical] Concurrent backlight unlinking and connector teardown causes a Use-After-Free due to missing references and early return bypassing `cancel_work_sync`.
- [High] Turning off the backlight asynchronously during pipeline disable breaks hardware shutdown sequencing, risking screen corruption.
- [High] Backlight hardware state is mutated asynchronously before atomic commit validation, failing to roll back on commit failure.
- [High] Lockless, in-place modification of `connector->state->luminance` from asynchronous notifier context causes Write-After-Free.
- [High] Lockless read of `crtc->state` in `drm_atomic_helper_connector_apply_luminance` causes a Use-After-Free.
- [High] Lockless read of `connector->state` of unrelated connectors in `drm_atomic_crtc_set_backlight` causes a Use-After-Free.
- [Medium] Concurrency between `drm_backlight_inhibit_legacy_all` and `drm_backlight_link` leads to double-counted or overwritten `luminance_clients`.
- [Medium] Triggering a `WARN_ON` for routine hardware communication failures allows trivial kernel log spam and potential system panics.
- [Medium] Legacy DPMS changes incorrectly dim all connectors on shared CRTCs, and conflict with the atomic commit tail logic.
- [Medium] Backlight is enabled on disconnected/un-driven panels during DPMS ON.
- [Low] The fallback brightness assignment in `drm_backlight_bd_set_luminance` is mathematically guaranteed to fail due to a hardcoded `U16_MAX`.
- [Low] Property bounds are not updated when dynamically linking a backend with a different brightness range.
- [Low] Broken teardown symmetry leaves the backlight sysfs symlink unremoved, resulting in dead code.
--

commit 4014f9d380d34194182229a03855db38075d5806
Author: Mario Limonciello <mario.limonciello@amd.com>
Subject: drm: add connector backlight (LUMINANCE) infrastructure

This commit introduces a per-connector LUMINANCE range property to control
backlight brightness through the DRM atomic modeset path. It embeds a
backlight state in each connector and synchronizes brightness changes
asynchronously via a workqueue.

> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index 285aac3554dfd..110cd81e5d6db 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
[ ... ]
> @@ -1230,6 +1231,10 @@ drm_atomic_helper_commit_encoder_bridge_disable(struct drm_device *dev,
>  		 * it away), so we won't call disable hooks twice.
>  		 */
>  		bridge = drm_bridge_chain_get_first_bridge(encoder);
> +
> +		/* Turn the backlight off before disabling the pipeline. */
> +		drm_backlight_set_luminance(connector, 0);
> +
>  		drm_atomic_bridge_chain_disable(bridge, state);
>  		drm_bridge_put(bridge);

[Severity: High]
Does this sequence break the display hardware shutdown requirements?

By queuing the backlight off operation asynchronously via
drm_backlight_set_luminance() and immediately disabling the bridge chain,
the pipeline might be shut down before the backlight actually turns off.
Could this lead to screen corruption or panel damage?

[ ... ]
> @@ -1775,6 +1780,24 @@ void drm_atomic_helper_commit_modeset_enables(struct drm_device *dev,
>  }
>  EXPORT_SYMBOL(drm_atomic_helper_commit_modeset_enables);
>  
> +/**
> + * drm_atomic_helper_connector_apply_luminance - apply connector luminance from atomic state
> + * @conn_state: atomic connector state to apply luminance for
> + *
> + * Updates the backlight luminance from the atomic connector state. If the
> + * connector has a linked backlight and is associated with an active CRTC,
> + * push the luminance value to hardware.
> + */
> +void drm_atomic_helper_connector_apply_luminance(const struct drm_connector_state *conn_state)
> +{
> +	struct drm_connector *connector = conn_state->connector;
> +
> +	if (conn_state->crtc && conn_state->crtc->state &&
> +	    conn_state->crtc->state->active)

[Severity: High]
Can a concurrent CRTC modeset free the CRTC state while it is being accessed
here?

Since the CRTC lock might not be held during pure connector property
commits, reading conn_state->crtc->state->active could result in a
use-after-free if the CRTC state is swapped and freed simultaneously.

> +		drm_backlight_set_luminance(connector, conn_state->luminance);
> +}
> +EXPORT_SYMBOL(drm_atomic_helper_connector_apply_luminance);
> +
[ ... ]
> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
> index 5ea593b3a98ec..07d4bcb16cd23 100644
> --- a/drivers/gpu/drm/drm_atomic_uapi.c
> +++ b/drivers/gpu/drm/drm_atomic_uapi.c
[ ... ]
> @@ -1133,6 +1144,22 @@ static struct drm_pending_vblank_event *create_vblank_event(
>  	return e;
>  }
>  
> +static void drm_atomic_crtc_set_backlight(struct drm_crtc *crtc, bool active)
> +{
> +	struct drm_connector_list_iter conn_iter;
> +	struct drm_connector *connector;
> +
> +	drm_connector_list_iter_begin(crtc->dev, &conn_iter);
> +	drm_for_each_connector_iter(connector, &conn_iter) {
> +		if (!connector->state || connector->state->crtc != crtc)
> +			continue;

[Severity: High]
Could this lockless read of connector->state trigger a use-after-free?

This function iterates all connectors and reads their states while holding
only the connection_mutex. If another connector receives a concurrent
property update, its state could be swapped and freed while being read here.

> +
> +		drm_backlight_set_luminance(connector,
> +					    active ? connector->state->luminance : 0);
> +	}
> +	drm_connector_list_iter_end(&conn_iter);
> +}
> +
>  int drm_atomic_connector_commit_dpms(struct drm_atomic_commit *state,
>  				     struct drm_connector *connector,
>  				     int mode)
> @@ -1155,9 +1182,29 @@ int drm_atomic_connector_commit_dpms(struct drm_atomic_commit *state,
>  	if (connector->dpms == mode)
>  		goto out;
>  
> +	crtc = connector->state ? connector->state->crtc : NULL;
> +
> +	/* Handle backlight brightness coordination with DPMS state changes */
> +	if (old_mode != DRM_MODE_DPMS_OFF && mode == DRM_MODE_DPMS_OFF) {
> +		/* DPMS ON -> OFF: dim all connectors driven by this CRTC. */
> +		if (crtc)
> +			drm_atomic_crtc_set_backlight(crtc, false);

[Severity: Medium]
Will this cause unintended dimming or flickering on cloned displays?

When one connector on a shared CRTC transitions to DPMS OFF, calling
drm_atomic_crtc_set_backlight() queues an OFF request for all connectors
driven by that CRTC, even if the other connectors are still supposed to
remain active.

> +		else
> +			drm_backlight_set_luminance(connector, 0);
> +	}
> +
>  	connector->dpms = mode;
>  
> -	crtc = connector->state->crtc;
> +	/* DPMS OFF -> ON: restore brightness to property value */
> +	if (old_mode == DRM_MODE_DPMS_OFF && mode == DRM_MODE_DPMS_ON &&
> +	    connector->state) {
> +		if (crtc)
> +			drm_atomic_crtc_set_backlight(crtc, true);
> +		else
> +			drm_backlight_set_luminance(connector,
> +						    connector->state->luminance);

[Severity: Medium]
Is it safe to turn on the backlight for a disconnected panel?

If a connector is turned on (DPMS ON) but has no active CRTC (crtc is NULL),
this branch restores the committed luminance. Illuminating a panel that isn't
receiving a video signal could violate hardware sequencing constraints and
stress the panel.

[Severity: High]
Does this rollback path properly revert the physical backlight changes?

The physical backlight state is mutated asynchronously before calling
drm_atomic_commit(state). If the commit fails (for example, due to 
validation failure), the software DPMS state is rolled back at the out
label:

out:
	if (ret != 0)
		connector->dpms = old_mode;

However, the physical backlight might remain turned off since the
asynchronous work was already dispatched and cannot be rolled back here.

> +	}
> +
>  	if (!crtc)
>  		goto out;
>  	ret = drm_atomic_add_affected_connectors(state, crtc);
[ ... ]
> diff --git a/drivers/gpu/drm/drm_backlight.c b/drivers/gpu/drm/drm_backlight.c
> new file mode 100644
> index 0000000000000..dd02d1878727e
> --- /dev/null
> +++ b/drivers/gpu/drm/drm_backlight.c
[ ... ]
> +static void __drm_backlight_worker(struct work_struct *w)
> +{
> +	struct drm_backlight *b = container_of(w, struct drm_backlight, work);
[ ... ]
> +	/*
> +	 * The backend stays valid here: drm_backlight_do_unlink() clears
> +	 * @backend and drops its reference only after cancel_work_sync(), so an
> +	 * in-flight worker always sees a live backend.
> +	 */
> +	if (funcs && funcs->set_luminance)
> +		WARN_ON(funcs->set_luminance(b, v));

[Severity: Medium]
Could this WARN_ON() be triggered by routine hardware communication failures?

If the backend communicates over I2C or similar buses, transient timeouts 
or disconnections might routinely return error codes. Using WARN_ON() could 
allow local userspace to spam the kernel log or cause a system panic if 
panic_on_warn is enabled.

[ ... ]
> +/* caller must hold @drm_backlight_lock */
> +static void __drm_backlight_readback(struct drm_backlight *b, unsigned int v)
> +{
[ ... ]
> +	/*
> +	 * Reflect a hardware-side brightness change (firmware hotkeys, or a
> +	 * legacy sysfs write while not inhibited) back into the connector's
> +	 * committed luminance so a read-back returns the real value.
> +	 */
> +	if (connector->state)
> +		connector->state->luminance = min(v, max);

[Severity: High]
Can this in-place modification of connector->state cause a write-after-free?

Since this function is called asynchronously from a sysfs handler holding 
only drm_backlight_lock, a concurrent atomic commit could duplicate, commit, 
and free the connector->state object, causing this write to corrupt memory.

> +}
[ ... ]
> +static void drm_backlight_do_unlink(struct drm_backlight *b)
> +{
> +	struct backlight_device *bd;
> +	unsigned int clients;
> +
> +	/*
> +	 * Stop new work first, but leave @backend in place so an in-flight
> +	 * worker keeps a valid backend to operate on. Capture the linked
> +	 * backlight_device (if any) while @funcs is still set.
> +	 */
> +	scoped_guard(spinlock, &drm_backlight_lock) {
> +		if (!b->funcs)
> +			return;

[Severity: Critical]
Does this early return leak a pending or running worker?

If b->funcs is NULL, the function returns immediately without calling
cancel_work_sync(&b->work). This could leave the worker operating on
memory that might be subsequently freed.

> +		bd = drm_backlight_bd(b);
> +		clients = b->luminance_clients;
> +		b->funcs = NULL;
> +		list_del_init(&b->list);
> +	}
[ ... ]
> +int drm_backlight_link(struct drm_connector *connector,
> +		       struct backlight_device *bd)
> +{
[ ... ]
> +	max = bd->props.max_brightness;
> +	if (max && !connector->luminance_property) {
> +		ret = drm_backlight_create_property(connector, max, false);

[Severity: Low]
Will the LUMINANCE property bounds remain stale if a new backend is linked?

If a backend is unlinked and a new one is linked with a different 
max_brightness range, the property is not recreated because 
connector->luminance_property already exists. This might artificially 
clamp the new backend to the old backend's maximum brightness.

> +		if (ret)
> +			return ret;
> +	}
> +
> +	scoped_guard(spinlock, &drm_backlight_lock) {
> +		b->funcs = &drm_backlight_bd_funcs;
> +		b->backend = bd;
> +		backlight_device_ref(bd);
> +		list_add(&b->list, &drm_backlight_list);
> +		/*
> +		 * Inherit the device-wide inhibit level so a connector linked
> +		 * (or hotplugged) while luminance-aware clients are active also
> +		 * has its legacy sysfs control inhibited.
> +		 */
> +		b->luminance_clients = connector->dev->mode_config.luminance_clients;

[Severity: Medium]
Is there a race condition here that could double-count luminance_clients?

If a new connector is linked concurrently with drm_backlight_inhibit_legacy_all(),
it might inherit the already-incremented global dev->mode_config.luminance_clients
count here.

> +		if (b->luminance_clients)
> +			atomic_add(b->luminance_clients, &bd->drm_takeover);
[ ... ]
> +void drm_backlight_inhibit_legacy_all(struct drm_device *dev)
> +{
> +	struct drm_connector_list_iter iter;
> +	struct drm_connector *connector;
> +
> +	scoped_guard(spinlock, &drm_backlight_lock)
> +		dev->mode_config.luminance_clients++;
> +
> +	drm_connector_list_iter_begin(dev, &iter);
> +	drm_for_each_connector_iter(connector, &iter)
> +		drm_backlight_inhibit_legacy(connector);

[Severity: Medium]
Since the lock is dropped between incrementing the global luminance_clients
count and iterating the connectors, the iterator could hit the newly linked
connector and increment its count a second time. This double-count would
prevent the legacy sysfs interface from ever being uninhibited.

> +	drm_connector_list_iter_end(&iter);
> +}
[ ... ]
> +static int drm_backlight_bd_set_luminance(struct drm_backlight *b,
> +					  unsigned int value)
> +{
> +	struct backlight_device *bd = b->backend;
> +	int rc;
> +
> +	rc = backlight_set_brightness(bd, value, BACKLIGHT_UPDATE_DRM);
> +	if (rc)
> +		backlight_set_brightness(bd, U16_MAX, BACKLIGHT_UPDATE_DRM);

[Severity: Low]
Is this fallback brightness assignment guaranteed to fail?

Because backlight_set_brightness() strictly bounds-checks the requested
value against bd->props.max_brightness (which is typically around 255),
passing U16_MAX (65535) will mathematically guarantee an -EINVAL failure,
making this error recovery path ineffective.

> +
> +	return rc;
> +}
[ ... ]
> +static int drm_backlight_notify(struct notifier_block *self,
> +				unsigned long event, void *data)
> +{
[ ... ]
> +		for (;;) {
> +			struct drm_backlight *found = NULL;
> +
> +			scoped_guard(spinlock, &drm_backlight_lock) {
> +				list_for_each_entry(b, &drm_backlight_list, list) {
> +					if (drm_backlight_bd(b) == bd) {
> +						found = b;
> +						break;
> +					}
> +				}
> +			}
> +			if (!found)
> +				break;
> +			drm_backlight_do_unlink(found);

[Severity: Critical]
Is there a risk of the connector being freed concurrently here?

The notifier iterates the list under the lock, finds a match, drops the 
lock, and then calls drm_backlight_do_unlink(found). Without holding a 
reference to the connector, a concurrent teardown (like a hardware hotplug)
could free the connector memory, resulting in a use-after-free when 
drm_backlight_do_unlink accesses found->funcs.

> +		}
> +		break;
[ ... ]
> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> index 8b4baed060f3a..92ed0c1879d53 100644
> --- a/drivers/gpu/drm/drm_connector.c
> +++ b/drivers/gpu/drm/drm_connector.c
[ ... ]
> @@ -944,6 +948,8 @@ EXPORT_SYMBOL(drm_connector_dynamic_register);
>  void drm_connector_unregister(struct drm_connector *connector)
>  {
>  	mutex_lock(&connector->mutex);
> +	drm_backlight_unregister(connector);
> +
>  	if (connector->registration_state != DRM_CONNECTOR_REGISTERED) {

[Severity: Low]
Does unregistering the backlight here break the sysfs teardown symmetry?

Because this clears the backend link before drm_sysfs_connector_remove_early()
is called, the latter will fail to fetch the backlight device and miss
calling sysfs_remove_link(&connector->kdev->kobj, "backlight").

[ ... ]
> diff --git a/drivers/gpu/drm/drm_sysfs.c b/drivers/gpu/drm/drm_sysfs.c
> index ef4e923a87284..2f4ce7d040f9d 100644
> --- a/drivers/gpu/drm/drm_sysfs.c
> +++ b/drivers/gpu/drm/drm_sysfs.c
[ ... ]
>  void drm_sysfs_connector_remove_early(struct drm_connector *connector)
>  {
> +	struct backlight_device *bd = drm_backlight_get_device(connector);
> +
> +	if (bd) {
> +		sysfs_remove_link(&connector->kdev->kobj, "backlight");

[Severity: Low]
Because drm_backlight_unregister() has already been called in 
drm_connector_unregister(), drm_backlight_get_device() will return NULL, 
leaving this cleanup path unreachable.

> +		backlight_device_unref(bd);
> +	}
> +
>  	if (connector->ddc)
>  		sysfs_remove_link(&connector->kdev->kobj, "ddc");
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260908044035.62093-1-mario.limonciello@amd.com?part=4