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 <[email protected]> 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/[email protected]?part=12
