Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [High] Passing `child_np` to `drm_aux_bridge_register_from_node()` without acquiring an additional reference leads to a refcount underflow and potential double-free. - [Medium] The result of `devm_phy_create()` is not checked immediately, leading to potential error shadowing and broken probe deferral. --
--- Patch [5]: [PATCH v6 5/7] phy: rockchip: phy-rockchip-typec: Add DRM AUX bridge --- commit ed030870fca01a8999b3193c38becaaeb4276a91 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] Should we check if devm_phy_create() failed before registering the bridge? If devm_phy_create() fails with an error like -EPROBE_DEFER, but drm_aux_bridge_register_from_node() also fails, the code below checks 'ret' first and returns that error. This could shadow the -EPROBE_DEFER from the PHY creation and prevent the driver from properly probing again. [Severity: High] Does this code need to call of_node_get() on child_np before passing it here? The for_each_available_child_of_node() loop only holds a reference to child_np during the current iteration, dropping it when advancing to the next node. However, drm_aux_bridge_register_from_node() assigns the node to the bridge device without incrementing the reference count, and later calls of_node_put() in its release callback. Will this lead to a refcount underflow and use-after-free of the node memory once the loop advances? > + } 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); [Severity: High] If drm_aux_bridge_register_from_node() fails, does this cause a double-free of the OF node? Because the loop reference was passed directly to the bridge registration without incrementing the reference count, and the bridge already calls of_node_put() on its own internal error path, calling of_node_put() here again will drop the reference count a second time. > + return ret; > + } > + > 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=5
