Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
New issues: - [Medium] Missing `phy_exit()` cleanup for the PHY initialized in `rk3568_lvds_probe()`. Pre-existing issues: - [High] The driver illegally overwrites the `funcs`, `ops`, and `driver_private` fields of a `drm_bridge` object obtained via `drm_of_find_panel_or_bridge()`. - [High] Unbalanced `of_node_put()` causes a use-after-free/double-free of a device node reference. -- --- Patch [3]: [PATCH v2 3/4] drm/rockchip: lvds: add RK3568 support --- commit ff72089a77c6360b2f3e3636251c56b8d17b88e7 Author: Rok Markovic <[email protected]> drm/rockchip: lvds: add RK3568 support The RK3568 LVDS transmitter has no register block of its own. It is driven entirely through the GRF and re-uses the MIPI DSI0 D-PHY in PHY_MODE_LVDS. Link: https://lore.kernel.org/all/[email protected]/ > diff --git a/drivers/gpu/drm/rockchip/rockchip_lvds.c > b/drivers/gpu/drm/rockchip/rockchip_lvds.c > index 150299c12298a..fcf599ae6c6dd 100644 > --- a/drivers/gpu/drm/rockchip/rockchip_lvds.c > +++ b/drivers/gpu/drm/rockchip/rockchip_lvds.c [ ... ] > @@ -512,6 +614,17 @@ static int px30_lvds_probe(struct platform_device *pdev, > return phy_power_on(lvds->dphy); > } > > +static int rk3568_lvds_probe(struct platform_device *pdev, > + struct rockchip_lvds *lvds) > +{ > + /* The phy is powered on from the encoder enable path, not here. */ > + lvds->dphy = devm_phy_get(&pdev->dev, "dphy"); > + if (IS_ERR(lvds->dphy)) > + return PTR_ERR(lvds->dphy); > + > + return phy_init(lvds->dphy); > +} > + [Severity: Medium] Since devm_phy_get() does not automatically call phy_exit(), does this code leak the PHY's initialization state and resources when the driver is unbound or fails to probe after initialization? rk3568_lvds_probe() calls phy_init(lvds->dphy) directly. If phy_init() succeeds but the probe later fails, or when the device is removed, it appears phy_exit() is never called to clean up. [ ... ] > @@ -601,6 +723,8 @@ static int rockchip_lvds_bind(struct device *dev, struct > device *master, > encoder = &lvds->encoder.encoder; > encoder->possible_crtcs = drm_of_find_possible_crtcs(drm_dev, > dev->of_node); > + rockchip_drm_encoder_set_crtc_endpoint_id(&lvds->encoder, > + dev->of_node, 0, 0); > > ret = drm_simple_encoder_init(drm_dev, encoder, DRM_MODE_ENCODER_LVDS); > if (ret < 0) { [Severity: High] This is a pre-existing issue, but I noticed a potential reference counting issue in rockchip_lvds_bind(): drivers/gpu/drm/rockchip/rockchip_lvds.c:rockchip_lvds_bind() { ... if (lvds->panel) remote = lvds->panel->dev->of_node; else remote = lvds->bridge->of_node; ... pm_runtime_enable(dev); of_node_put(remote); of_node_put(port); } The remote node pointer is borrowed directly without taking a new reference (e.g. via of_node_get()), but is then unconditionally passed to of_node_put(). Can this cause a double-free or use-after-free of the device node when the reference count is decremented incorrectly? [Severity: High] This isn't a bug introduced by this patch, but I also noticed that rockchip_lvds_bind() overwrites the bridge fields: drivers/gpu/drm/rockchip/rockchip_lvds.c:rockchip_lvds_bind() { ... if (lvds->bridge) { lvds->bridge->driver_private = lvds; lvds->bridge->ops = DRM_BRIDGE_OP_MODES; lvds->bridge->funcs = &rockchip_lvds_bridge_funcs; ... } If drm_of_find_panel_or_bridge() returns an external bridge (when lvds->panel is NULL), does this overwrite the external bridge's own function pointers and private data? This could erase the original operations like atomic_enable and atomic_disable, preventing the external bridge from functioning, and could cause memory corruption or crashes when the DRM core invokes the overwritten function pointers. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=3
