USB: xhci-plat: fix legacy PHY double init

[PATCH] USB: xhci-plat: fix legacy PHY double init

Posted by Johan Hovold 2 years, 1 month ago

Commits 7b8ef22ea547 ("usb: xhci: plat: Add USB phy support") and
9134c1fd0503 ("usb: xhci: plat: Add USB 3.0 phy support") added support
for looking up legacy PHYs from the sysdev devicetree node and
initialising them.

This broke drivers such as dwc3 which manages PHYs themself as the PHYs
would now be initialised twice, something which specifically can lead to
resources being left enabled during suspend (e.g. with the
usb_phy_generic PHY driver).

As the dwc3 driver uses driver-name matching for the xhci platform
device, fix this by only looking up and initialising PHYs for devices
that have been matched using OF.

Note that checking that the platform device has a devicetree node would
currently be sufficient, but that could lead to subtle breakages in case
anyone ever tries to reuse an ancestor's node.

Fixes: 7b8ef22ea547 ("usb: xhci: plat: Add USB phy support")
Fixes: 9134c1fd0503 ("usb: xhci: plat: Add USB 3.0 phy support")
Cc: stable@vger.kernel.org      # 4.1
Cc: Maxime Ripard <mripard@kernel.org>
Cc: Stanley Chang <stanley_chang@realtek.com>
Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 drivers/usb/host/xhci-plat.c | 50 +++++++++++++++++++++---------------
 1 file changed, 30 insertions(+), 20 deletions(-)

diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
index 28218c8f1837..01d19d17153b 100644
--- a/drivers/usb/host/xhci-plat.c
+++ b/drivers/usb/host/xhci-plat.c
@@ -13,6 +13,7 @@
 #include <linux/module.h>
 #include <linux/pci.h>
 #include <linux/of.h>
+#include <linux/of_device.h>
 #include <linux/platform_device.h>
 #include <linux/usb/phy.h>
 #include <linux/slab.h>
@@ -148,7 +149,7 @@ int xhci_plat_probe(struct platform_device *pdev, struct device *sysdev, const s
 	int			ret;
 	int			irq;
 	struct xhci_plat_priv	*priv = NULL;
-
+	bool			of_match;
 
 	if (usb_disabled())
 		return -ENODEV;
@@ -253,16 +254,23 @@ int xhci_plat_probe(struct platform_device *pdev, struct device *sysdev, const s
 					 &xhci->imod_interval);
 	}
 
-	hcd->usb_phy = devm_usb_get_phy_by_phandle(sysdev, "usb-phy", 0);
-	if (IS_ERR(hcd->usb_phy)) {
-		ret = PTR_ERR(hcd->usb_phy);
-		if (ret == -EPROBE_DEFER)
-			goto disable_clk;
-		hcd->usb_phy = NULL;
-	} else {
-		ret = usb_phy_init(hcd->usb_phy);
-		if (ret)
-			goto disable_clk;
+	/*
+	 * Drivers such as dwc3 manages PHYs themself (and rely on driver name
+	 * matching for the xhci platform device).
+	 */
+	of_match = of_match_device(pdev->dev.driver->of_match_table, &pdev->dev);
+	if (of_match) {
+		hcd->usb_phy = devm_usb_get_phy_by_phandle(sysdev, "usb-phy", 0);
+		if (IS_ERR(hcd->usb_phy)) {
+			ret = PTR_ERR(hcd->usb_phy);
+			if (ret == -EPROBE_DEFER)
+				goto disable_clk;
+			hcd->usb_phy = NULL;
+		} else {
+			ret = usb_phy_init(hcd->usb_phy);
+			if (ret)
+				goto disable_clk;
+		}
 	}
 
 	hcd->tpl_support = of_usb_host_tpl_support(sysdev->of_node);
@@ -285,15 +293,17 @@ int xhci_plat_probe(struct platform_device *pdev, struct device *sysdev, const s
 			goto dealloc_usb2_hcd;
 		}
 
-		xhci->shared_hcd->usb_phy = devm_usb_get_phy_by_phandle(sysdev,
-			    "usb-phy", 1);
-		if (IS_ERR(xhci->shared_hcd->usb_phy)) {
-			xhci->shared_hcd->usb_phy = NULL;
-		} else {
-			ret = usb_phy_init(xhci->shared_hcd->usb_phy);
-			if (ret)
-				dev_err(sysdev, "%s init usb3phy fail (ret=%d)\n",
-					    __func__, ret);
+		if (of_match) {
+			xhci->shared_hcd->usb_phy = devm_usb_get_phy_by_phandle(sysdev,
+										"usb-phy", 1);
+			if (IS_ERR(xhci->shared_hcd->usb_phy)) {
+				xhci->shared_hcd->usb_phy = NULL;
+			} else {
+				ret = usb_phy_init(xhci->shared_hcd->usb_phy);
+				if (ret)
+					dev_err(sysdev, "%s init usb3phy fail (ret=%d)\n",
+						__func__, ret);
+			}
 		}
 
 		xhci->shared_hcd->tpl_support = hcd->tpl_support;
-- 
2.41.0

Re: [PATCH] USB: xhci-plat: fix legacy PHY double inity

Posted by Stefan Eichenberger 2 years, 1 month ago

On Fri, Nov 03, 2023 at 05:43:23PM +0100, Johan Hovold wrote:
> Commits 7b8ef22ea547 ("usb: xhci: plat: Add USB phy support") and
> 9134c1fd0503 ("usb: xhci: plat: Add USB 3.0 phy support") added support
> for looking up legacy PHYs from the sysdev devicetree node and
> initialising them.
> 
> This broke drivers such as dwc3 which manages PHYs themself as the PHYs
> would now be initialised twice, something which specifically can lead to
> resources being left enabled during suspend (e.g. with the
> usb_phy_generic PHY driver).
> 
> As the dwc3 driver uses driver-name matching for the xhci platform
> device, fix this by only looking up and initialising PHYs for devices
> that have been matched using OF.
> 
> Note that checking that the platform device has a devicetree node would
> currently be sufficient, but that could lead to subtle breakages in case
> anyone ever tries to reuse an ancestor's node.
> 
> Fixes: 7b8ef22ea547 ("usb: xhci: plat: Add USB phy support")
> Fixes: 9134c1fd0503 ("usb: xhci: plat: Add USB 3.0 phy support")
> Cc: stable@vger.kernel.org      # 4.1
> Cc: Maxime Ripard <mripard@kernel.org>
> Cc: Stanley Chang <stanley_chang@realtek.com>
> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>

Tested-by: Stefan Eichenberger <stefan.eichenberger@toradex.com>

RE: [PATCH] USB: xhci-plat: fix legacy PHY double inity

Posted by Stanley Chang[昌育德] 2 years, 1 month ago

Hi Johan,

> > On Fri, Nov 03, 2023 at 05:43:23PM +0100, Johan Hovold wrote:
> > > Commits 7b8ef22ea547 ("usb: xhci: plat: Add USB phy support") and
> > > 9134c1fd0503 ("usb: xhci: plat: Add USB 3.0 phy support") added
> > > support for looking up legacy PHYs from the sysdev devicetree node
> > > and initialising them.
> > >
> > > This broke drivers such as dwc3 which manages PHYs themself as the
> > > PHYs would now be initialised twice, something which specifically
> > > can lead to resources being left enabled during suspend (e.g. with
> > > the usb_phy_generic PHY driver).
> > >
> > > As the dwc3 driver uses driver-name matching for the xhci platform
> > > device, fix this by only looking up and initialising PHYs for
> > > devices that have been matched using OF.
> > >
> > > Note that checking that the platform device has a devicetree node
> > > would currently be sufficient, but that could lead to subtle
> > > breakages in case anyone ever tries to reuse an ancestor's node.
> > >
> > > Fixes: 7b8ef22ea547 ("usb: xhci: plat: Add USB phy support")
> > > Fixes: 9134c1fd0503 ("usb: xhci: plat: Add USB 3.0 phy support")
> > > Cc: stable@vger.kernel.org      # 4.1
> > > Cc: Maxime Ripard <mripard@kernel.org>
> > > Cc: Stanley Chang <stanley_chang@realtek.com>
> > > Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> >
> > Tested-by: Stefan Eichenberger <stefan.eichenberger@toradex.com>
> 
> Tested-by: Stanley Chang <stanley_chang@realtek.com>
> 

I am sorry to notify you this patch is tested fail.
I test the Realtek phy driver at drivers/phy/Realtek/phy-rtk-usb2.c again.
But I can't get the phy in xhci.
It is a dwc3 generic phy driver, and it is also a usb phy driver. 

Base on you modified, I can't run on callback 
rtk_phy->phy.notify_port_status = rtk_phy_notify_port_status;
Thanks,
Stanley.

RE: [PATCH] USB: xhci-plat: fix legacy PHY double inity

Posted by Stanley Chang[昌育德] 2 years, 1 month ago

> On Fri, Nov 03, 2023 at 05:43:23PM +0100, Johan Hovold wrote:
> > Commits 7b8ef22ea547 ("usb: xhci: plat: Add USB phy support") and
> > 9134c1fd0503 ("usb: xhci: plat: Add USB 3.0 phy support") added
> > support for looking up legacy PHYs from the sysdev devicetree node and
> > initialising them.
> >
> > This broke drivers such as dwc3 which manages PHYs themself as the
> > PHYs would now be initialised twice, something which specifically can
> > lead to resources being left enabled during suspend (e.g. with the
> > usb_phy_generic PHY driver).
> >
> > As the dwc3 driver uses driver-name matching for the xhci platform
> > device, fix this by only looking up and initialising PHYs for devices
> > that have been matched using OF.
> >
> > Note that checking that the platform device has a devicetree node
> > would currently be sufficient, but that could lead to subtle breakages
> > in case anyone ever tries to reuse an ancestor's node.
> >
> > Fixes: 7b8ef22ea547 ("usb: xhci: plat: Add USB phy support")
> > Fixes: 9134c1fd0503 ("usb: xhci: plat: Add USB 3.0 phy support")
> > Cc: stable@vger.kernel.org      # 4.1
> > Cc: Maxime Ripard <mripard@kernel.org>
> > Cc: Stanley Chang <stanley_chang@realtek.com>
> > Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> 
> Tested-by: Stefan Eichenberger <stefan.eichenberger@toradex.com>

Tested-by: Stanley Chang <stanley_chang@realtek.com>