Re: [PATCH 3/5] phy: rockchip: phy-rockchip-typec: Add DRM AUX bridge

[sashiko-bot@kernel.org]

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 <chaoyi.chen@rock-chips.com>
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/20260608070805.88-1-kernel@airkyi.com?part=3

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy