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

Vladimir Oltean
2026-05-05T09:35:11Z

Sashiko says:
> commit 1ca88ec2e954b3a331eb5afb8081406d9200c8f6
> Author: Vladimir Oltean <vladimir.oltean@nxp.com>
> usb: gadget: tegra-xudc: avoid direct dereference of phy->dev.of_node
>
> This commit replaces direct dereferences of the device tree node with a
> parallel lookup using the phys phandle and the padctl provider. It also
> introduces a local usbphy variable to hold the output and simplify error
> checks.
>
> > diff --git a/drivers/usb/gadget/udc/tegra-xudc.c b/drivers/usb/gadget/udc/tegra-xudc.c
> > index e9d33be028664..cf4e6c87e44da 100644
> > --- a/drivers/usb/gadget/udc/tegra-xudc.c
> > +++ b/drivers/usb/gadget/udc/tegra-xudc.c
> [ ... ]
> > @@ -3515,7 +3516,10 @@ static int tegra_xudc_phy_get(struct tegra_xudc *xudc)
> > xudc->vbus_nb.notifier_call = tegra_xudc_vbus_notify;
> >
> > for (i = 0; i < xudc->soc->num_phys; i++) {
> > + struct of_phandle_args args;
> > char phy_name[] = "usb.-.";
> > + struct usb_phy *usbphy;
> > + int index, err;
>
> This declares a local err variable inside the loop, shadowing the outer
> err variable from the function scope.
> Later in this loop, if devm_phy_optional_get() or devm_usb_get_phy_by_node()
> fails, the code assigns the error code to this inner err and uses goto
> clean_up.
> Since the inner err is destroyed when leaving the loop block, will the
> function incorrectly return the outer err which is still 0?

This is correct, I've removed the duplicated "err" variable declaration
for v8.

> >
> > /* Get USB2 phy */
> > snprintf(phy_name, sizeof(phy_name), "usb2-%d", i);
> [ ... ]
> > @@ -3525,22 +3529,31 @@ static int tegra_xudc_phy_get(struct tegra_xudc *xudc)
> > dev_err_probe(xudc->dev, err,
> > "failed to get PHY for phy-name usb2-%d\n", i);
> > goto clean_up;
> > - } else if (xudc->utmi_phy[i]) {
> [ ... ]
> > } else if (!xudc->utmi_phy[i]) {
> > /* if utmi phy is not available, ignore USB3 phy get */
> > continue;
> > }
> >
> > + index = of_property_match_string(np, "phy-names", phy_name);
> > + if (index < 0)
> > + continue;
> > +
> > + err = of_parse_phandle_with_args(np, "phys", "#phy-cells",
> > + index, &args);
> > + if (err)
> > + continue;
>
> If either of these device tree parsing steps fail, the code uses continue
> to skip to the next iteration.
> Does this unintentionally skip the entire USB3 PHY initialization block
> that follows later in this loop for the current port?

This is correct, although theoretically of_property_match_string() and
of_parse_phandle_with_args() should not fail as long as devm_phy_optional_get()
succeeded for that very same phy_name starting from the very same OF node.
But nonetheless, if this happens due to bugs, I've instead treated the
condition as fatal and printed error messages for v8.

> > +
> > + /* Get usb-phy, if utmi phy is available */
> > + usbphy = devm_usb_get_phy_by_node(xudc->dev, args.np, NULL);
> > + of_node_put(args.np);
> [ ... ]