Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [High] Double-free of device node `child_np` due to missing `of_node_get()` - [Medium] Logic error and potential error masking due to incorrect check ordering --
commit 35100033d8cc196dcdf5880de55b8c3780fffd11 Author: Chaoyi Chen <[email protected]> Subject: phy: rockchip: phy-rockchip-typec: Add DRM AUX bridge This commit updates the Rockchip Type-C PHY driver to use the DRM_AUX_BRIDGE helper. It registers a transparent DRM bridge device for the DP port defined in the device tree. > diff --git a/drivers/phy/rockchip/phy-rockchip-typec.c > b/drivers/phy/rockchip/phy-rockchip-typec.c > --- a/drivers/phy/rockchip/phy-rockchip-typec.c > +++ b/drivers/phy/rockchip/phy-rockchip-typec.c [ ... ] > @@ -1162,16 +1163,24 @@ static int rockchip_typec_phy_probe(struct > platform_device *pdev) > > for_each_available_child_of_node(np, child_np) { > struct phy *phy; > + ret = 0; > > - if (of_node_name_eq(child_np, "dp-port")) > + if (of_node_name_eq(child_np, "dp-port")) { > phy = devm_phy_create(dev, child_np, > &rockchip_dp_phy_ops); > - else if (of_node_name_eq(child_np, "usb3-port")) > + ret = drm_aux_bridge_register_from_node(dev, child_np); [Severity: High] Does this cause a double-free of the child_np device node? The loop macro for_each_available_child_of_node() provides a temporary reference to child_np that is automatically released when the loop advances. However, drm_aux_bridge_register_from_node() passes this node to device_set_node() which assigns the node without incrementing its reference count. When the auxiliary bridge is later unregistered, drm_aux_bridge_release() unconditionally calls of_node_put() on this node: drivers/gpu/drm/bridge/aux-bridge.c:drm_aux_bridge_release() { ... of_node_put(dev->of_node); ... } Because an additional reference was never acquired, this can cause a reference count underflow and a use-after-free of the device node structure. Should we call of_node_get(child_np) before passing it to drm_aux_bridge_register_from_node()? > + } else if (of_node_name_eq(child_np, "usb3-port")) > phy = devm_phy_create(dev, child_np, > &rockchip_usb3_phy_ops); > else > continue; > > + if (ret) { > + pm_runtime_disable(dev); > + of_node_put(child_np); > + return ret; > + } > + [Severity: Medium] If devm_phy_create() fails, will this error be masked if drm_aux_bridge_register_from_node() also fails? If devm_phy_create() returns an ERR_PTR, but drm_aux_bridge_register_from_node() fails and returns a non-zero value, the error check here captures the bridge error instead of the PHY error. This would return an incorrect error code and skip the "failed to create phy" debug log entirely. Also, if the PHY fails but the bridge succeeds, the code registers an unnecessary auxiliary device only for devres to immediately tear it down. Would it be better to check IS_ERR(phy) immediately after devm_phy_create() and before registering the bridge? > if (IS_ERR(phy)) { > dev_err(dev, "failed to create phy: %pOFn\n", > child_np); -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=3
