Re: [PATCH v7 12/12] drm/i915/display: use drm backlight

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [Low] Unused include `<drm/drm_backlight.h>` in `intel_dp.c`

Pre-existing issues:
- [High] TOCTOU race in sysfs backlight update leading to NULL pointer dereference during non-blocking display disable
--

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

drm/i915/display: use drm backlight

This commit converts the Intel display drivers (i915 and xe) to use the DRM
backlight infrastructure, allowing brightness control via the connector
LUMINANCE property. It links the backlight device to the eDP connector on
registration and unlinks it on unregistration.

> diff --git a/drivers/gpu/drm/i915/display/intel_backlight.c b/drivers/gpu/drm/i915/display/intel_backlight.c
> index b128896cb1c2d..b0cd4dc3adc00 100644
> --- a/drivers/gpu/drm/i915/display/intel_backlight.c
> +++ b/drivers/gpu/drm/i915/display/intel_backlight.c

[ ... ]

> @@ -1015,6 +1018,7 @@ void intel_backlight_device_unregister(struct intel_connector *connector)
>  	struct intel_panel *panel = &connector->panel;
>  
>  	if (panel->backlight.device) {
> +		drm_backlight_link(&connector->base, NULL);
>  		backlight_device_unregister(panel->backlight.device);
>  		panel->backlight.device = NULL;
>  	}

[Severity: High]
This is a pre-existing issue, but could this cause a NULL pointer dereference
during a non-blocking display disable?

If there is a concurrent userspace write to the legacy sysfs brightness file
during a non-blocking display disable on VLV/CHV hardware, we might hit a race.
During the non-blocking commit, connection_mutex is dropped before the hardware
is disabled, allowing intel_backlight_device_update_status() to execute.

It uses the newly swapped connector->base.state (where crtc = NULL), but
panel->backlight.enabled remains true until the asynchronous commit tail
disables it.

This seems to lead to intel_panel_set_backlight() passing the state directly
to the backend without checking if conn_state->crtc is NULL:

drivers/gpu/drm/i915/display/intel_backlight.c:intel_panel_set_backlight() {
    ...
    if (panel->backlight.enabled)
        intel_panel_actually_set_backlight(conn_state, hw_level);
    ...
}

Which then calls into vlv_set_backlight() where the NULL crtc is dereferenced:

drivers/gpu/drm/i915/display/intel_backlight.c:vlv_set_backlight() {
    ...
    enum pipe pipe = to_intel_crtc(conn_state->crtc)->pipe;
    u32 tmp;

    tmp = intel_de_read(display, VLV_BLC_PWM_CTL(pipe)) & ~BACKLIGHT_DUTY_CYCLE_MASK;
    intel_de_write(display, VLV_BLC_PWM_CTL(pipe), tmp | level);
    ...
}

Does this require a check for conn_state->crtc being NULL before passing the
state, similar to how other backlight functions handle it?

[ ... ]

> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
> index 113d767e62e9c..2381a80a4e671 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> @@ -43,6 +43,7 @@
>  #include <drm/display/drm_dsc_helper.h>
>  #include <drm/display/drm_hdmi_helper.h>
>  #include <drm/drm_atomic_helper.h>
> +#include <drm/drm_backlight.h>
>  #include <drm/drm_crtc.h>
>  #include <drm/drm_edid.h>
>  #include <drm/drm_fixed.h>

[Severity: Low]
Is this include used anywhere in this file?

It looks like <drm/drm_backlight.h> is added to intel_dp.c but no symbols from
it are utilized in this file, resulting in a dead include.

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