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

[Vladimir Oltean <vladimir.oltean@nxp.com>]

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);
> [ ... ]