Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [High] Double-free of OF node reference resulting from borrowed node assignment - [Medium] Improper Error Handling: Unconditional execution of bridge registration shadows PHY creation errors --
--- Patch [4]: [PATCH v5 4/6] phy: rockchip: phy-rockchip-typec: Add DRM AUX bridge --- commit c26cb3c5d012dc638272484e672dcad955d4a455 Author: Chaoyi Chen <[email protected]> phy: rockchip: phy-rockchip-typec: Add DRM AUX bridge Using the DRM_AUX_BRIDGE helper to create the transparent DRM bridge device. > diff --git a/drivers/phy/rockchip/phy-rockchip-typec.c > b/drivers/phy/rockchip/phy-rockchip-typec.c > index d9701b6106d55..48070b50416e5 100644 > --- 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: Medium] If devm_phy_create() fails and returns an error pointer, the code still unconditionally calls drm_aux_bridge_register_from_node(). If the bridge registration fails, its error code will be stored in ret and returned below, which completely shadows the original error from devm_phy_create() (such as -EPROBE_DEFER). Should devm_phy_create()'s return value be checked before attempting to register the bridge? [Severity: High] Does this cause a double-free of the child_np node reference? The for_each_available_child_of_node() loop automatically holds a reference to child_np. When child_np is passed to drm_aux_bridge_register_from_node(), the bridge takes ownership of the node reference without the caller acquiring an additional reference via of_node_get(). If registration succeeds, the bridge will eventually call of_node_put() in its drm_aux_bridge_release() handler. Meanwhile, the iterator loop also automatically calls of_node_put() when advancing to the next node, leading to a double-free on the success path. > + } 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: High] If drm_aux_bridge_register_from_node() fails, it calls of_node_put() internally. Because this error path explicitly calls of_node_put(child_np) again, would this result in an immediate double-free? Would it be better to call of_node_get(child_np) before passing it to drm_aux_bridge_register_from_node() to ensure the bridge has its own reference? > 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=4
