Re: [PATCH v7 phy-next 13/27] usb: host: tegra: avoid direct dereference of phy->dev.of_node

Vladimir Oltean
2026-05-05T09:39:37Z

Sashiko says:
> commit 037a329bd1e70f26b8d861fb06f3bbc539fb2c49
> Author: Vladimir Oltean <vladimir.oltean@nxp.com>
> usb: host: tegra: avoid direct dereference of phy->dev.of_node
>
> This commit updates the Tegra USB host driver to retrieve the PHY OF node
> using the "phys" phandle rather than directly accessing phy->dev.of_node.
> This addresses an upcoming change where struct phy will become an opaque
> pointer. It also simplifies a debug message to log the array index.
>
> > diff --git a/drivers/usb/host/xhci-tegra.c b/drivers/usb/host/xhci-tegra.c
> > index d2214d309e96..99b0ac9763f9 100644
> > --- a/drivers/usb/host/xhci-tegra.c
> > +++ b/drivers/usb/host/xhci-tegra.c
> > @@ -1491,6 +1491,7 @@ static int tegra_xhci_id_notify(struct notifier_block *nb,
> >
> > static int tegra_xusb_init_usb_phy(struct tegra_xusb *tegra)
> > {
> > + struct device_node *np = dev_of_node(tegra->dev);
> > unsigned int i;
> >
> > tegra->usbphy = devm_kcalloc(tegra->dev, tegra->num_usb_phys,
> > @@ -1504,23 +1505,33 @@ static int tegra_xusb_init_usb_phy(struct tegra_xusb *tegra)
> > tegra->otg_usb3_port = -EINVAL;
> >
> > for (i = 0; i < tegra->num_usb_phys; i++) {
> > - struct phy *phy = tegra_xusb_get_phy(tegra, "usb2", i);
> > + struct of_phandle_args args;
> > + struct usb_phy *usbphy;
> > + int index, err;
> > + char prop[8];
> >
> > - if (!phy)
> > + snprintf(prop, sizeof(prop), "usb2-%d", i);
> > +
> > + /*
> > + * usb-phy is optional, continue if it's not available.
> > + */
> > + index = of_property_match_string(np, "phy-names", prop);
> > + if (index < 0)
> > continue;
> >
> > - tegra->usbphy[i] = devm_usb_get_phy_by_node(tegra->dev,
> > - phy->dev.of_node,
> > - &tegra->id_nb);
> > - if (!IS_ERR(tegra->usbphy[i])) {
> > - dev_dbg(tegra->dev, "usbphy-%d registered", i);
> > - otg_set_host(tegra->usbphy[i]->otg, &tegra->hcd->self);
> > - } else {
> > - /*
> > - * usb-phy is optional, continue if its not available.
> > - */
> > - tegra->usbphy[i] = NULL;
> > - }
> > + err = of_parse_phandle_with_args(np, "phys", "#phy-cells",
> > + index, &args);
> > + if (err)
> > + continue;
>
> The call to of_parse_phandle_with_args() increments the reference count
> of the device tree node and populates it in args.np. The new code passes
> args.np to devm_usb_get_phy_by_node(), but does not appear to call
> of_node_put() to drop the reference when it is done. Does this leak the
> device node reference on both the success and error paths of the loop?

I've added an of_node_put(args.np) after devm_usb_get_phy_by_node() for v8.

> > +
> > + usbphy = devm_usb_get_phy_by_node(tegra->dev, args.np,
> > + &tegra->id_nb);
>
> Also in tegra_xusb_init_usb_phy(), the single notifier block instance
> tegra->id_nb is passed to devm_usb_get_phy_by_node() for every USB PHY
> being processed. Because the struct notifier_block API uses an embedded next
> pointer for its singly-linked list, does registering the exact same object
> to multiple independent PHY notifier chains overwrite its next pointer and
> corrupt the lists?

If this is a problem, it is a pre-existing and completely unrelated
problem which I don't have knowledge how to handle, so I will leave it
alone.

> > + if (IS_ERR(usbphy))
> > + continue;
> > +
> > + tegra->usbphy[i] = usbphy;
> > + dev_dbg(tegra->dev, "usbphy-%d registered", i);
> > + otg_set_host(tegra->usbphy[i]->otg, &tegra->hcd->self);
> > }
> >
> > return 0;
> [...]
> > drivers/usb/host/xhci-tegra.c | 42 ++++++++++++++++++++++-------------
> > 1 file changed, 26 insertions(+), 16 deletions(-)
> >
> > @@ -2168,8 +2179,7 @@ static void tegra_xhci_disable_phy_wake(struct tegra_xusb *tegra)
> > continue;
> >
> > if (tegra_xusb_padctl_remote_wake_detected(padctl, tegra->phys[i]))
> > - dev_dbg(tegra->dev, "%pOF remote wake detected\n",
> > - tegra->phys[i]->dev.of_node);
> > + dev_dbg(tegra->dev, "PHY %d remote wake detected\n", i);
> >
> > tegra_xusb_padctl_disable_phy_wake(padctl, tegra->phys[i]);
> > }
>
> In tegra_xhci_disable_phy_wake(), the loop counter i is declared as an
> unsigned int. The updated dev_dbg() message formats this variable using
> the %d specifier. This isn't a bug since the values are bounded by
> tegra->num_phys, but should it use %u to match the unsigned integer type?

I've changed to %u printing for v8.