diff mbox series

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

Message ID 20260430110652.558622-14-vladimir.oltean@nxp.com (mailing list archive)
State New
Headers
Series Split Generic PHY consumer and provider |

Commit Message

Vladimir Oltean April 30, 2026, 11:06 a.m. UTC 
A piece of context which seems relevant here is that the USB subsystem
is transitioning from struct usb_phy to struct phy (belonging to the
Generic PHY subsystem). Commit 1a229d8690a0 ("Revert "usb: phy: add usb
phy notify port status API"") seems to confirm that this is the case.

In the transition process, some PHY provider drivers register themselves
as both Generic PHY and USB PHY in an attempt to bridge the API gap.
Such is the case with drivers/phy/tegra/xusb.c, accessed here by the
Tegra USB host driver. This USB host expects the PHY device behind the
Generic PHY to also be a USB PHY, and calls
devm_usb_get_phy_by_node(phy->dev.of_node).

The Generic PHY exposes no API to get the OF node from a PHY device, so
the Tegra USB host driver gets it directly. However, "struct phy" will
be made an opaque pointer, to avoid misuse, so this will no longer be
possible.

Considering the fact that the Generic PHY/USB PHY duality is a
transitional state, I am deliberately not planning to make the life of
this driver any easier by providing a helper to get to the OF node
somehow. Instead, implement a parallel lookup path through which the
Tegra USB host driver can continue to get to the OF node provided by the
padctl component, using the 'phys' phandle.

Secondly (minor issue) the driver uses the phy->dev.of_node again to
print using dev_dbg() that a "remote wake" was detected. Just print the
index at which the PHY appears inside the driver's tegra->phys[] array
instead.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: JC Kuo <jckuo@nvidia.com>
Cc: Johan Hovold <johan+linaro@kernel.org>
Cc: Jonathan Hunter <jonathanh@nvidia.com>
Cc: Mathias Nyman <mathias.nyman@intel.com>
Cc: Thierry Reding <thierry.reding@gmail.com>

v5->v7: none
v4->v5: patch is new
---
 drivers/usb/host/xhci-tegra.c | 42 ++++++++++++++++++++++-------------
 1 file changed, 26 insertions(+), 16 deletions(-)

Comments

Vladimir Oltean May 5, 2026, 9:39 a.m. UTC | #1 
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.
diff mbox series

Patch

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;
+
+		usbphy = devm_usb_get_phy_by_node(tegra->dev, args.np,
+						  &tegra->id_nb);
+		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;
@@ -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]);
 	}