diff mbox series

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

Message ID 20260430110652.558622-15-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 
In a somewhat similar situation as the Tegra USB host controller driver,
the Tegra XUDC driver for USB gadget mode needs to get to a struct
usb_phy that sits behind the same OF node as the Generic PHY. It does
that directly, which will no longer be possible. The PHY provider is
also the xusb padctl driver.

The rework here is also to implement a parallel OF node lookup path
based on the "phys" phandle and the #phy-cells of the padctl provider.

Some further notes:
- create a local "usbphy" variable to hold the devm_usb_get_phy_by_node()
  output. This makes the error checks more obvious (avoids keeping an
  error-encoded pointer in xudc->usbphy[i] even temporarily).
- the "if (IS_ERR(utmi_phy)) .. else if (utmi_phy) .. else if (!utmi_phy)"
  pattern can be simplified, considering that neither the IS_ERR() nor
  the NULL case continue execution in the current block. Therefore, we
  can move the case where the "utmi_phy" is a valid pointer outside the
  "if" checks, and this reduces the code indentation level.

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/gadget/udc/tegra-xudc.c | 35 ++++++++++++++++++++---------
 1 file changed, 24 insertions(+), 11 deletions(-)

Comments

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

Patch

diff --git a/drivers/usb/gadget/udc/tegra-xudc.c b/drivers/usb/gadget/udc/tegra-xudc.c
index e9d33be02866..cf4e6c87e44d 100644
--- a/drivers/usb/gadget/udc/tegra-xudc.c
+++ b/drivers/usb/gadget/udc/tegra-xudc.c
@@ -3494,6 +3494,7 @@  static void tegra_xudc_device_params_init(struct tegra_xudc *xudc)
 
 static int tegra_xudc_phy_get(struct tegra_xudc *xudc)
 {
+	struct device_node *np = dev_of_node(xudc->dev);
 	int err = 0, usb3_companion_port;
 	unsigned int i, j;
 
@@ -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;
 
 		/* 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]) {
-			/* Get usb-phy, if utmi phy is available */
-			xudc->usbphy[i] = devm_usb_get_phy_by_node(xudc->dev,
-						xudc->utmi_phy[i]->dev.of_node,
-						NULL);
-			if (IS_ERR(xudc->usbphy[i])) {
-				err = PTR_ERR(xudc->usbphy[i]);
-				dev_err_probe(xudc->dev, err,
-					      "failed to get usbphy-%d\n", i);
-				goto clean_up;
-			}
 		} 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;
+
+		/* 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);
+		if (IS_ERR(usbphy)) {
+			err = PTR_ERR(usbphy);
+			dev_err_probe(xudc->dev, err,
+				      "failed to get usbphy-%d\n", i);
+			goto clean_up;
+		}
+		xudc->usbphy[i] = usbphy;
+
 		/* Get USB3 phy */
 		usb3_companion_port = tegra_xusb_padctl_get_usb3_companion(xudc->padctl, i);
 		if (usb3_companion_port < 0)