Re: [PATCH v7 05/12] drm: add DRM_CLIENT_CAP_LUMINANCE

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Connector hotplugging or unplugging while DRM_CLIENT_CAP_LUMINANCE is active causes state desynchronization and triggers kernel warnings.
- [High] Concurrent calls to DRM_IOCTL_SET_CLIENT_CAP can leak `luminance_clients` reference counts, permanently inhibiting legacy sysfs backlight control (DoS).
--

commit d72da18676683e0c9579165708469f0cf974aa5b
Author: Mario Limonciello <mario.limonciello@amd.com>

drm: add DRM_CLIENT_CAP_LUMINANCE

This commit adds a DRIVER_CONNECTOR_LUMINANCE feature and a
DRM_CLIENT_CAP_LUMINANCE client capability. When a client enables this
capability, the kernel inhibits legacy sysfs backlight control by taking
over the backlight devices.

> diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c
> --- a/drivers/gpu/drm/drm_file.c
> +++ b/drivers/gpu/drm/drm_file.c

[ ... ]

> @@ -252,6 +253,10 @@ void drm_file_free(struct drm_file *file)
>  	if (drm_core_check_feature(dev, DRIVER_MODESET)) {
>  		drm_fb_release(file);
>  		drm_property_destroy_user_blobs(dev, file);
> +		if (file->supports_luminance_control) {
> +			drm_backlight_uninhibit_legacy_all(dev);
> +			file->supports_luminance_control = false;
> +		}
>  	}

[Severity: High]
Will this correctly handle connectors that are hotplugged or unplugged
after the compositor sets DRM_CLIENT_CAP_LUMINANCE?

drm_backlight_uninhibit_legacy_all() iterates over the list of currently
present connectors. If a compositor enables DRM_CLIENT_CAP_LUMINANCE and a
monitor is later hotplugged via DP MST, the new connector will not have
its legacy sysfs backlight control inhibited.

When the compositor exits and drm_file_free() runs:

For newly hotplugged connectors, it will try to uninhibit them even though
they were never inhibited, triggering WARN_ON(b->luminance_clients == 0)
in drm_backlight_uninhibit_legacy().

Conversely, for connectors unplugged before the compositor exits,
drm_backlight_connector_cleanup() will fire WARN_ON(b->luminance_clients)
because the connector is destroyed while its luminance_clients count is
still 1.

> diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
> --- a/drivers/gpu/drm/drm_ioctl.c
> +++ b/drivers/gpu/drm/drm_ioctl.c

[ ... ]

> @@ -392,6 +394,21 @@ drm_setclientcap(struct drm_device *dev, void *data, struct drm_file *file_priv)
>  		file_priv->plane_color_pipeline = req->value;
>  		break;
>  	}
> +	case DRM_CLIENT_CAP_LUMINANCE:
> +		if (!drm_core_check_feature(dev, DRIVER_CONNECTOR_LUMINANCE))
> +			return -EOPNOTSUPP;
> +		if (!file_priv->atomic)
> +			return -EINVAL;
> +		if (req->value > 1)
> +			return -EINVAL;
> +		if (req->value == file_priv->supports_luminance_control)
> +			break;
> +		if (req->value)
> +			drm_backlight_inhibit_legacy_all(dev);
> +		else
> +			drm_backlight_uninhibit_legacy_all(dev);
> +		file_priv->supports_luminance_control = req->value;
> +		break;

[Severity: High]
Does this check-then-act sequence lack synchronization?

If two threads share a single DRM file descriptor and concurrently invoke
DRM_IOCTL_SET_CLIENT_CAP with DRM_CLIENT_CAP_LUMINANCE=1, both threads
could evaluate the supports_luminance_control condition as false before
either updates it.

Both threads would then call drm_backlight_inhibit_legacy_all(dev), causing
a double-increment of the luminance_clients reference count. When the file
descriptor is closed, drm_file_free() will only call
drm_backlight_uninhibit_legacy_all() once, permanently leaving the legacy
sysfs backlight interface inhibited and returning -EBUSY.

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