Re: [PATCH v8 4/7] drm/bridge: aux: Add drm_aux_bridge_register_from_node()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] drm/bridge: aux: missing of_node_get() in drm_aux_bridge_register_from_node()
- [High] drm/bridge: aux: improper OF node setup in drm_aux_bridge_register_from_node()
--

commit 36532c7bf6a02e3a0e3f70873a369995bccbc3c6
Author: Chaoyi Chen <chaoyi.chen@rock-chips.com>

drm/bridge: aux: Add drm_aux_bridge_register_from_node()

This commit adds drm_aux_bridge_register_from_node() so callers can specify an
explicit device_node for the auxiliary bridge instead of always using the
parent device's of_node.

> diff --git a/drivers/gpu/drm/bridge/aux-bridge.c b/drivers/gpu/drm/bridge/aux-bridge.c
> index 90f76bb529cc2..f1adcdece7b95 100644
> --- a/drivers/gpu/drm/bridge/aux-bridge.c
> +++ b/drivers/gpu/drm/bridge/aux-bridge.c
> @@ -35,8 +35,9 @@ static void drm_aux_bridge_unregister_adev(void *_adev)
>  }
>  
>  /**
> - * drm_aux_bridge_register - Create a simple bridge device to link the chain
> + * drm_aux_bridge_register_from_node - Create a simple bridge device to link the chain
>   * @parent: device instance providing this bridge
> + * @np: device node pointer corresponding to this bridge instance
>   *
>   * Creates a simple DRM bridge that doesn't implement any drm_bridge
>   * operations. Such bridges merely fill a place in the bridge chain linking
> @@ -44,7 +45,7 @@ static void drm_aux_bridge_unregister_adev(void *_adev)
>   *
>   * Return: zero on success, negative error code on failure
>   */
> -int drm_aux_bridge_register(struct device *parent)
> +int drm_aux_bridge_register_from_node(struct device *parent, struct device_node *np)
>  {
>  	struct auxiliary_device *adev;
>  	int ret;
> @@ -64,7 +65,10 @@ int drm_aux_bridge_register(struct device *parent)
>  	adev->dev.parent = parent;
>  	adev->dev.release = drm_aux_bridge_release;
>  
> -	device_set_of_node_from_dev(&adev->dev, parent);
> +	if (np)
> +		device_set_node(&adev->dev, of_fwnode_handle(np));
> +	else
> +		device_set_of_node_from_dev(&adev->dev, parent);

[Severity: High]
Does calling device_set_node() when np is non-NULL underflow the OF node
reference count on cleanup?

In drivers/base/core.c, device_set_of_node_from_dev() increments the reference
count via of_node_get() and marks the node as reused, whereas
device_set_node() only assigns the pointers:

drivers/base/core.c:
void device_set_of_node_from_dev(struct device *dev, const struct device *dev2)
{
	of_node_put(dev->of_node);
	dev->of_node = of_node_get(dev2->of_node);
	dev_set_of_node_reused(dev);
}

void device_set_node(struct device *dev, struct fwnode_handle *fwnode)
{
	dev->fwnode = fwnode;
	dev->of_node = to_of_node(fwnode);
}

However, both the auxiliary_device_init() failure path in
drm_aux_bridge_register_from_node() and the device release callback
drm_aux_bridge_release() (reached if auxiliary_device_add() fails via
auxiliary_device_uninit() -> put_device(), or when the parent unbinds via
drm_aux_bridge_unregister_adev()) unconditionally drop a reference on
dev->of_node:

drivers/gpu/drm/bridge/aux-bridge.c:drm_aux_bridge_release() {
	struct auxiliary_device *adev = to_auxiliary_dev(dev);

	of_node_put(dev->of_node);
	ida_free(&drm_aux_bridge_ida, adev->id);

	kfree(adev);
}

When a caller such as rockchip_typec_phy_probe() passes a child node via
for_each_available_child_of_node(np, child_np) without taking an extra
reference for the bridge, won't this drop a reference that
drm_aux_bridge_register_from_node() never acquired?

[Severity: High]
Also, is device_set_node() appropriate here when np is shared with a parent or
subnode device (such as the dp-port PHY subnode in phy-rockchip-typec)?

Unlike device_set_of_node_from_dev(), device_set_node() sets adev->dev.fwnode
and leaves DEV_FLAG_OF_NODE_REUSED unset on &adev->dev.

Because adev->dev.fwnode is set and the node is not marked reused:
1. device_add() assigns np->fwnode.dev = &adev->dev, so fw_devlink treats the
   aux_bridge auxiliary device as the supplier for np. If np is a PHY subnode
   without an OF graph port (for example, tcphy0_dp / tcphy1_dp referenced via
   phys = <&tcphy0_dp>), drm_aux_bridge_probe() returns -ENODEV and leaves the
   auxiliary device unbound, which can cause consumers like cdn_dp to defer
   probe indefinitely with -EPROBE_DEFER.
2. really_probe() -> pinctrl_bind_pins() only skips pin binding when
   of_node_is_reused() is true, which can conflict if np carries pinctrl
   properties.

Should the non-NULL np path instead set adev->dev.of_node = of_node_get(np)
and call dev_set_of_node_reused(&adev->dev) to match
device_set_of_node_from_dev()?

[ ... ]

>  	ret = auxiliary_device_init(adev);
>  	if (ret) {

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261020072816.180-1-kernel@airkyi.com?part=4

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