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