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 <[email protected]> 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/[email protected]?part=4
