Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [High] Missing drm_bridge_remove() in error paths and driver unbind leads to memory leaks and dangling pointers in the global bridge_list. --
--- Patch [1]: [PATCH v5] drm/ingenic: fix bridge allocation --- commit 25461fe1d8c0e90d5647e8b8309dd0a052459895 Author: H. Nikolaus Schaller <[email protected]> drm/ingenic: fix bridge allocation This commit fixes an issue with the bridge allocation API by replacing direct struct drm_bridge allocation with devm_drm_bridge_alloc(). It also updates the bridge lifecycle by calling drm_bridge_add() prior to drm_bridge_attach(). > diff --git a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c > b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c > index 738a80b2550ff..47f02d655b3c6 100644 > --- a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c > +++ b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c [ ... ] > @@ -1316,13 +1321,27 @@ static int ingenic_drm_bind(struct device *dev, bool > has_components) > > drm_encoder_helper_add(encoder, > &ingenic_drm_encoder_helper_funcs); > > - ib->bridge.funcs = &ingenic_drm_bridge_funcs; > + bridge_alloc = devm_drm_bridge_alloc(priv->dev, > + struct > ingenic_drm_bridge_alloc, > + bridge, > + &ingenic_drm_bridge_funcs); > + if (IS_ERR(bridge_alloc)) { > + ret = PTR_ERR(bridge_alloc); > + goto err_drvdata; > + } > + > + ib->bridge = &bridge_alloc->bridge; > + ib->bridge->of_node = priv->dev->of_node; > + > ib->next_bridge = bridge; > > - ret = drm_bridge_attach(encoder, &ib->bridge, NULL, > + drm_bridge_add(ib->bridge); > + > + ret = drm_bridge_attach(encoder, ib->bridge, NULL, > DRM_BRIDGE_ATTACH_NO_CONNECTOR); > if (ret) { > dev_err(dev, "Unable to attach bridge\n"); > + drm_bridge_remove(ib->bridge); > goto err_drvdata; > } [Severity: High] Does this code leak the bridge and leave a dangling pointer if a subsequent initialization step fails? Right after this drm_bridge_attach() block, ingenic_drm_bind() calls drm_bridge_connector_init(). If that initialization fails (or if any later loop iteration fails), the code jumps to err_drvdata: connector = drm_bridge_connector_init(drm, encoder); if (IS_ERR(connector)) { dev_err(dev, "Unable to init connector\n"); ret = PTR_ERR(connector); goto err_drvdata; } Because drm_bridge_remove() is only called when drm_bridge_attach() fails, the bridge added via drm_bridge_add() is never removed from the global bridge_list on these later error paths. Additionally, does the driver leave the bridge in the global list during device unbind? Looking at ingenic_drm_unbind(), there appears to be no matching call to drm_bridge_remove() for the bridges registered in the bind loop. This could lead to a use-after-free oops if the module is unloaded, leaving bridge->funcs pointing to unmapped memory. Could devm_drm_bridge_add() be used here instead to ensure the bridge is automatically removed from the global list on error paths and during unbind? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/f77ed4e1547e452668a6549e3966471a6b638a6b.1787593585.git....@goldelico.com?part=1
