Re: [PATCH 09/11] drm/mcde: dsi: don't create a panel_bridge

[sashiko-bot@kernel.org]

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

New issues:
- [High] The reference to the remote bridge (next_bridge) acquired during bind is permanently leaked on unbind.

Pre-existing issues:
- [High] Component unbind is missing drm_bridge_remove(), leading to list corruption and memory leaks upon rebind or module unload.
- [High] The driver returns -EINVAL instead of -EPROBE_DEFER when the downstream bridge is not found, breaking probe deferral.
- [High] Calling devm_clk_get() inside a repeatable component bind function causes a devres leak.
--

--- Patch [9]: [PATCH 09/11] drm/mcde: dsi: don't create a panel_bridge ---
commit 62f9247edbd74ae414416ce89cd9ec90e6811731
Author: Luca Ceresoli <luca.ceresoli@bootlin.com>

drm/mcde: dsi: don't create a panel_bridge

Now a panel_bridge is automatically created for every drm_panel, so this
code (similar to an open-coded variant of drm_of_find_panel_or_bridge())
would lead to creating a second panel_bridge for the same panel.

> diff --git a/drivers/gpu/drm/mcde/mcde_dsi.c b/drivers/gpu/drm/mcde/mcde_dsi.c
> index ce078aa3910f2..ec45440d20d70 100644
> --- a/drivers/gpu/drm/mcde/mcde_dsi.c
> +++ b/drivers/gpu/drm/mcde/mcde_dsi.c
[ ... ]
> @@ -1076,7 +1075,6 @@ static int mcde_dsi_bind(struct device *dev, struct device *master,
>  	struct drm_device *drm = data;
>  	struct mcde *mcde = to_mcde(drm);
>  	struct mcde_dsi *d = dev_get_drvdata(dev);
> -	struct drm_panel *panel = NULL;

[Severity: High]
This is a pre-existing issue, but does calling devm_clk_get() inside
mcde_dsi_bind() cause a devres memory leak?

Since component drivers can be unbound and rebound multiple times (such as
via sysfs or master probe deferral), devres allocations in the bind
path will accumulate every time the component binds, because devres chunks
are bound to the platform device's lifetime, not the component bind/unbind
lifecycle.

[ ... ]
> @@ -1104,40 +1102,21 @@ static int mcde_dsi_bind(struct device *dev, struct device *master,
>  
>  	/* Look for a panel as a child to this node */
>  	for_each_available_child_of_node_scoped(dev->of_node, child) {
> -		panel = of_drm_find_panel(child);
> -		if (IS_ERR(panel)) {
> -			dev_err(dev, "failed to find panel try bridge (%ld)\n",
> -				PTR_ERR(panel));
> -			panel = NULL;
> -
> -			bridge = of_drm_find_and_get_bridge(child);
> -			if (!bridge) {
> -				dev_err(dev, "failed to find bridge\n");
> -				return -EINVAL;
> -			}
> +		bridge = of_drm_find_and_get_bridge(child);
> +		if (!bridge) {
> +			dev_err(dev, "failed to find bridge\n");
> +			return -EINVAL;

[Severity: High]
This isn't a bug introduced by this patch, but should this return
-EPROBE_DEFER instead of -EINVAL when the downstream bridge is not found?

Returning a hard -EINVAL when of_drm_find_and_get_bridge() returns NULL
aborts the display probe irrevocably if the downstream panel/bridge probes
after the MCDE DSI host, rather than deferring the probe until it is ready.

[ ... ]
> @@ -1159,8 +1138,6 @@ static void mcde_dsi_unbind(struct device *dev, struct device *master,
>  {
>  	struct mcde_dsi *d = dev_get_drvdata(dev);
>  
> -	if (d->panel)
> -		drm_panel_bridge_remove(d->bridge.next_bridge);

[Severity: High]
Since this patch removes drm_panel_bridge_remove(), is a call to
drm_bridge_put(d->bridge.next_bridge) needed here to balance the reference
acquired in mcde_dsi_bind()?

In mcde_dsi_bind(), a reference to the downstream bridge is acquired and
stored via:

    d->bridge.next_bridge = drm_bridge_get(bridge);

Without a corresponding drm_bridge_put() here, the reference is stranded.
Upon the next rebind, a new reference is acquired and overwrites the old
pointer, permanently leaking the downstream bridge reference.

[Severity: High]
This is a pre-existing issue, but is a call to drm_bridge_remove(&d->bridge)
missing from mcde_dsi_unbind()?

mcde_dsi_bind() registers the bridge globally via
drm_bridge_add(&d->bridge). Without drm_bridge_remove() in the unbind path,
a rebind will call drm_bridge_add() on the same bridge object.

This invokes list_del_init(&bridge->list) without holding the bridge_lock,
introducing a data race that can corrupt the global bridge_list if traversed
concurrently. Additionally, it prevents the bridge's refcount from ever
reaching zero.

>  	regmap_update_bits(d->prcmu, PRCM_DSI_SW_RESET,
>  			   PRCM_DSI_SW_RESET_DSI0_SW_RESETN, 0);
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260814-drm-bridge-every-panel-v1-0-19cd5277cc8d@bootlin.com?part=9