Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
Pre-existing issues: - [High] Calling kobject_uevent_env() (which can sleep) while holding the drm_backlight_lock spinlock in drm_backlight_link(). - [High] Error paths in drm_connector_register() fail to unregister the backlight device, leading to a freed pointer on the global backlight list. -- commit 91fa46bfbc0b179001176c9331c615c49eadeb08 Author: Mario Limonciello (AMD) <[email protected]> drm/i915/display: use drm backlight Convert Intel display drivers (i915 and xe) to use the new DRM backlight infrastructure. This allows brightness control via DRM connector properties. > diff --git a/drivers/gpu/drm/i915/display/intel_backlight.c > b/drivers/gpu/drm/i915/display/intel_backlight.c > index b128896cb1c2d..4543c2a8f3568 100644 > --- a/drivers/gpu/drm/i915/display/intel_backlight.c > +++ b/drivers/gpu/drm/i915/display/intel_backlight.c [ ... ] > @@ -1004,6 +1005,8 @@ int intel_backlight_device_register(struct > intel_connector *connector) > "[CONNECTOR:%d:%s] backlight device %s registered\n", > connector->base.base.id, connector->base.name, name); > > + drm_backlight_link(connector->base.backlight, bd); > + [Severity: High] This is a pre-existing issue, but does calling drm_backlight_link() here introduce a sleep-in-atomic bug? Looking at drm_backlight_link() in drm_backlight.c, it uses a scope-based spinlock: guard(spinlock)(&drm_backlight_lock); send_uevent = __drm_backlight_link(b, bd); if (send_uevent) kobject_uevent_env(&b->connector->kdev->kobj, KOBJ_CHANGE, (char **)ep); kobject_uevent_env() allocates memory with GFP_KERNEL and can sleep. Since the spinlock guard is still active when kobject_uevent_env() is called, this could trigger lockdep warnings or system crashes. > out: > kfree(name); [ ... ] > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c > b/drivers/gpu/drm/i915/display/intel_dp.c > index 0ce0c09835f6d..cd35cce329a55 100644 > --- a/drivers/gpu/drm/i915/display/intel_dp.c > +++ b/drivers/gpu/drm/i915/display/intel_dp.c [ ... ] > @@ -7421,6 +7422,13 @@ intel_dp_init_connector(struct intel_digital_port > *dig_port, > type, &intel_dp->aux.ddc); > drm_connector_helper_add(&connector->base, > &intel_dp_connector_helper_funcs); > > + if (type == DRM_MODE_CONNECTOR_eDP) { > + int r = drm_backlight_alloc(&connector->base); > + [Severity: High] This is a pre-existing issue, but does allocating the backlight here expose a list corruption issue during connector registration failures? If a failure occurs during drm_connector_register() (for example, in drm_sysfs_connector_add()), the error path does not unregister the backlight device. Since it was never fully registered, drm_connector_cleanup() will eventually skip calling drm_connector_unregister() but will directly call drm_backlight_free(), which does: kfree(b); connector->backlight = NULL; Because the backlight was never removed from the global drm_backlight_list, freeing it here leaves a dangling pointer on the global list, which could cause a use-after-free panic during later list traversals. > + if (r) > + drm_err(display->drm, "Failed to allocate backlight: > %d\n", r); > + } > + > if (!HAS_GMCH(display) && DISPLAY_VER(display) < 12) > connector->base.interlace_allowed = true; > -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=10
