Re: [PATCH RFC v2 24/24] drm/mxsfb/lcdif: enable bridge hotplug

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] drm: API violation via double execution of bridge .attach()
- [High] drm: race condition allows concurrent bridge attach execution
- [Medium] drm: spurious warning on teardown of incomplete pipeline
--

commit 58600c33214c844d55e169ec965ac93160d06726
Author: Luca Ceresoli <luca.ceresoli@bootlin.com>

drm/mxsfb/lcdif: enable bridge hotplug
    
Switch to the new drm_hotplug_helper() to handle bridge hotplugging.

> diff --git a/drivers/gpu/drm/mxsfb/lcdif_drv.c b/drivers/gpu/drm/mxsfb/lcdif_drv.c
> index e2173c4d6fc28..3d8904a48d9a9 100644
> --- a/drivers/gpu/drm/mxsfb/lcdif_drv.c
> +++ b/drivers/gpu/drm/mxsfb/lcdif_drv.c
[ ... ]
> @@ -94,10 +94,10 @@ static int lcdif_attach_bridge(struct lcdif_drm_private *lcdif)
>  					     "Failed to attach bridge for endpoint%u\n",
>  					     of_ep.id);
>  
> -		connector = drm_bridge_connector_init(lcdif->drm, encoder);
> -		if (IS_ERR(connector))
> -			return dev_err_probe(dev, PTR_ERR(connector),
> -					     "Failed to init bridge_connector for endpoint%u\n",
> +		hotplug_helper = drmm_hotplug_helper_init(lcdif->drm, encoder);

[Severity: Medium]
Will this cause a spurious warning during standard module unload or unbind if
the bridge pipeline is incomplete?

When the device is torn down, the drmm cleanup action
drm_hotplug_helper_fini() unconditionally calls
drm_hotplug_helper_connector_remove(). If the bridge pipeline never completed
(e.g., a downstream bridge was never plugged or probed),
hotplug_helper->connector remains NULL.

In drivers/gpu/drm/display/drm_hotplug_helper.c:
drm_hotplug_helper_connector_remove(), this evaluates to true and triggers a
warning:

	if (drm_WARN_ON(hotplug_helper->drm, !hotplug_helper->connector))
		return;

[Severity: High]
Does using this helper violate the DRM API contract by executing the bridge's
.attach() callback twice?

When a hotplug event occurs, the helper retrieves the last bridge currently in
the chain. Since this bridge is already in the chain, its .attach() callback
was already executed by the drm_bridge_attach() call made earlier in
lcdif_attach_bridge().

However, in drivers/gpu/drm/display/drm_hotplug_helper.c:
drm_hotplug_helper_try_complete(), the helper directly invokes attach again:

	err = last_bridge->funcs->attach(last_bridge, hotplug_helper->encoder,
					 DRM_BRIDGE_ATTACH_NO_CONNECTOR);

Can this lead to memory leaks, double list additions, or crashes in bridge
drivers that do not expect .attach() to be called multiple times?

[Severity: High]
Is there a race condition when processing concurrent hotplug events in the
helper?

In drivers/gpu/drm/display/drm_hotplug_helper.c:
drm_hotplug_helper_try_complete(), there is a state check without holding the
connector_mutex:

	if (hotplug_helper->connector)
		return;

Since hotplug notifier events can be dispatched concurrently via the blocking
notifier chain (which uses a read-write semaphore), two simultaneous events
can both see connector == NULL.

Both threads will then proceed to concurrently execute
last_bridge->funcs->attach(), causing data races. Furthermore, both threads
may proceed to drm_hotplug_helper_connector_add(), where one will hit the
drm_WARN_ON after acquiring the mutex.

> +		if (IS_ERR(hotplug_helper))
> +			return dev_err_probe(dev, PTR_ERR(hotplug_helper),
> +					     "Failed to init hotplug_helper for endpoint%u\n",
>  					     of_ep.id);
>  	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261001-drm-bridge-hotplug-v2-0-8e34986dcb68@bootlin.com?part=24