Re: [PATCH v6 09/10] drm/bridge: auto-link panel backlight in bridge connector

[sashiko-bot@kernel.org]

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

New issues:
- [High] NULL pointer dereference when calling drm_backlight_link() on an unregistered connector
- [Medium] Unconditional overwrite of return value 'ret' ignores drm_backlight_alloc() failures
- [Medium] Reference leak and WARN_ON if drm_connector_attach_encoder() fails
- [Low] Build failure due to missing static inline fallback for drm_panel_bridge_to_panel()

Pre-existing issues:
- [High] Sleeping in atomic context when drm_backlight_link() sends a uevent while holding a spinlock
--

commit 97d3e5fef0b8faac0a3e7bb0d3351e3d883fc3e8
Author: Mario Limonciello (AMD) <superm1@kernel.org>

drm/bridge: auto-link panel backlight in bridge connector

The link should allow clients to change luminance property.

Tested-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com> # SM8150-HDK
Signed-off-by: Mario Limonciello (AMD) <superm1@kernel.org>
Tested-by: Simon Ser <contact@emersion.fr>

> diff --git a/drivers/gpu/drm/display/drm_bridge_connector.c b/drivers/gpu/drm/display/drm_bridge_connector.c
> index 649969fca1413..23a3802294625 100644
> --- a/drivers/gpu/drm/display/drm_bridge_connector.c
> +++ b/drivers/gpu/drm/display/drm_bridge_connector.c
[ ... ]
> @@ -1051,9 +1053,20 @@ struct drm_connector *drm_bridge_connector_init(struct drm_device *drm,
>  		connector->polled = DRM_CONNECTOR_POLL_CONNECT
>  				  | DRM_CONNECTOR_POLL_DISCONNECT;
>  
> -	if (panel_bridge)
> +	if (panel_bridge) {
> +		struct drm_panel *panel;
> +
>  		drm_panel_bridge_set_orientation(connector, panel_bridge);
>  
> +		panel = drm_panel_bridge_to_panel(panel_bridge);
> +		if (panel && panel->backlight) {
> +			ret = drm_backlight_alloc(connector);
> +			if (!ret)
> +				drm_backlight_link(connector->backlight,
> +						   panel->backlight);

[Severity: Medium]
If drm_backlight_alloc() returns an error code like -ENOMEM, is it safe to
ignore it?

The error code stored in 'ret' appears to be unconditionally overwritten by the
subsequent call to drm_connector_attach_encoder(), silently ignoring the
allocation failure.

[Severity: High]
Can calling drm_backlight_link() here cause a kernel panic?

This links the backlight during connector initialization, before the connector
is registered to sysfs.

In drivers/gpu/drm/drm_backlight.c:drm_backlight_link():

	if (send_uevent)
		kobject_uevent_env(&b->connector->kdev->kobj, KOBJ_CHANGE,
				   (char **)ep);

If connector->kdev is NULL at this point, sending a uevent could dereference
an invalid pointer.

[Severity: High]
This is a pre-existing issue, but does drm_backlight_link() sleep while
holding a spinlock?

In drivers/gpu/drm/drm_backlight.c:drm_backlight_link():

	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);

The lock is held across the entire function scope, including the
kobject_uevent_env() call, which can allocate memory with GFP_KERNEL and
sleep. Could this trigger a 'scheduling while atomic' BUG?

> +		}
> +	}
> +
>  	if (support_hdcp && IS_REACHABLE(CONFIG_DRM_DISPLAY_HELPER) &&
>  	    IS_ENABLED(CONFIG_DRM_DISPLAY_HDCP_HELPER))
>  		drm_connector_attach_content_protection_property(connector, true);

[Severity: Medium]
If drm_connector_attach_encoder() fails just after this block, does this leak
a reference to the backlight_device and trigger a WARN_ON?

This error path bails out without unlinking the backlight that was successfully
linked above:

	ret = drm_connector_attach_encoder(connector, encoder);
	if (ret)
		return ERR_PTR(ret);

Because the connector was never registered, the link is never released. When
drm_connector_cleanup() runs, it might hit WARN_ON(b->link) and free the
structure without dropping the reference.

> diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
> index 4ba3a5deef9a6..6311ca726b45f 100644
> --- a/include/drm/drm_bridge.h
> +++ b/include/drm/drm_bridge.h
[ ... ]
> @@ -1586,6 +1586,7 @@ void drm_bridge_hpd_notify(struct drm_bridge *bridge,
>  
>  #ifdef CONFIG_DRM_PANEL_BRIDGE
>  bool drm_bridge_is_panel(const struct drm_bridge *bridge);
> +struct drm_panel *drm_panel_bridge_to_panel(struct drm_bridge *bridge);

[Severity: Low]
Will this cause a build failure when CONFIG_DRM_PANEL_BRIDGE is disabled?

The function declaration lacks a static inline fallback when the config is
disabled, which might cause an implicit function declaration error when called
unconditionally in drm_bridge_connector_init().

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