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

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

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.