[RFT PATCH 1/2] xhci: Fix leaking USB3 shared_hcd at xhci removal

[RFT PATCH 1/2] xhci: Fix leaking USB3 shared_hcd at xhci removal

[Mathias Nyman]

Ensure that the shared_hcd pointer is valid when calling usb_put_hcd()

The shared_hcd is removed and freed in xhci by first calling
usb_remove_hcd(xhci->shared_hcd), and later
usb_put_hcd(xhci->shared_hcd)

Afer commit fe190ed0d602 ("xhci: Do not halt the host until both HCD have
disconnected their devices.") the shared_hcd was never properly put as
xhci->shared_hcd was set to NULL before usb_put_hcd(xhci->shared_hcd) was
called.

shared_hcd (USB3) is removed before primary hcd (USB2).
While removing the primary hcd we might need to handle xhci interrupts
to cleanly remove last USB2 devices, therefore we need to set
xhci->shared_hcd to NULL before removing the primary hcd to let xhci
interrupt handler know shared_hcd is no longer available.

xhci-plat.c, xhci-histb.c and xhci-mtk first create both their hcd's before
adding them. so to keep the correct reverse removal order use a temporary
shared_hcd variable for them.
For more details see commit 4ac53087d6d4 ("usb: xhci: plat: Create both
HCDs before adding them")

Fixes: fe190ed0d602 ("xhci: Do not halt the host until both HCD have disconnected their devices.")
Cc: Joel Stanley <joel@jms.id.au>
Cc: Chunfeng Yun <chunfeng.yun@mediatek.com>
Cc: Thierry Reding <treding@nvidia.com>
Cc: Jianguo Sun <sunjianguo1@huawei.com>
Cc: <stable@vger.kernel.org>
Reported-by: Jack Pham <jackp@codeaurora.org>
Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
---
 drivers/usb/host/xhci-histb.c | 6 ++++--
 drivers/usb/host/xhci-mtk.c   | 6 ++++--
 drivers/usb/host/xhci-pci.c   | 1 +
 drivers/usb/host/xhci-plat.c  | 6 ++++--
 drivers/usb/host/xhci-tegra.c | 1 +
 drivers/usb/host/xhci.c       | 2 --
 6 files changed, 14 insertions(+), 8 deletions(-)

diff --git a/drivers/usb/host/xhci-histb.c b/drivers/usb/host/xhci-histb.c
index 27f0016..3c4abb5 100644
--- a/drivers/usb/host/xhci-histb.c
+++ b/drivers/usb/host/xhci-histb.c
@@ -325,14 +325,16 @@ static int xhci_histb_remove(struct platform_device *dev)
 	struct xhci_hcd_histb *histb = platform_get_drvdata(dev);
 	struct usb_hcd *hcd = histb->hcd;
 	struct xhci_hcd	*xhci = hcd_to_xhci(hcd);
+	struct usb_hcd *shared_hcd = xhci->shared_hcd;
 
 	xhci->xhc_state |= XHCI_STATE_REMOVING;
 
-	usb_remove_hcd(xhci->shared_hcd);
+	usb_remove_hcd(shared_hcd);
+	xhci->shared_hcd = NULL;
 	device_wakeup_disable(&dev->dev);
 
 	usb_remove_hcd(hcd);
-	usb_put_hcd(xhci->shared_hcd);
+	usb_put_hcd(shared_hcd);
 
 	xhci_histb_host_disable(histb);
 	usb_put_hcd(hcd);
diff --git a/drivers/usb/host/xhci-mtk.c b/drivers/usb/host/xhci-mtk.c
index 71d0d33..60987c7 100644
--- a/drivers/usb/host/xhci-mtk.c
+++ b/drivers/usb/host/xhci-mtk.c
@@ -590,12 +590,14 @@ static int xhci_mtk_remove(struct platform_device *dev)
 	struct xhci_hcd_mtk *mtk = platform_get_drvdata(dev);
 	struct usb_hcd	*hcd = mtk->hcd;
 	struct xhci_hcd	*xhci = hcd_to_xhci(hcd);
+	struct usb_hcd  *shared_hcd = xhci->shared_hcd;
 
-	usb_remove_hcd(xhci->shared_hcd);
+	usb_remove_hcd(shared_hcd);
+	xhci->shared_hcd = NULL;
 	device_init_wakeup(&dev->dev, false);
 
 	usb_remove_hcd(hcd);
-	usb_put_hcd(xhci->shared_hcd);
+	usb_put_hcd(shared_hcd);
 	usb_put_hcd(hcd);
 	xhci_mtk_sch_exit(mtk);
 	xhci_mtk_clks_disable(mtk);
diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c
index 51dd8e0..92fd6b6 100644
--- a/drivers/usb/host/xhci-pci.c
+++ b/drivers/usb/host/xhci-pci.c
@@ -356,6 +356,7 @@ static void xhci_pci_remove(struct pci_dev *dev)
 	if (xhci->shared_hcd) {
 		usb_remove_hcd(xhci->shared_hcd);
 		usb_put_hcd(xhci->shared_hcd);
+		xhci->shared_hcd = NULL;
 	}
 
 	/* Workaround for spurious wakeups at shutdown with HSW */
diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
index 94e9392..e5da8ce 100644
--- a/drivers/usb/host/xhci-plat.c
+++ b/drivers/usb/host/xhci-plat.c
@@ -359,14 +359,16 @@ static int xhci_plat_remove(struct platform_device *dev)
 	struct xhci_hcd	*xhci = hcd_to_xhci(hcd);
 	struct clk *clk = xhci->clk;
 	struct clk *reg_clk = xhci->reg_clk;
+	struct usb_hcd *shared_hcd = xhci->shared_hcd;
 
 	xhci->xhc_state |= XHCI_STATE_REMOVING;
 
-	usb_remove_hcd(xhci->shared_hcd);
+	usb_remove_hcd(shared_hcd);
+	xhci->shared_hcd = NULL;
 	usb_phy_shutdown(hcd->usb_phy);
 
 	usb_remove_hcd(hcd);
-	usb_put_hcd(xhci->shared_hcd);
+	usb_put_hcd(shared_hcd);
 
 	clk_disable_unprepare(clk);
 	clk_disable_unprepare(reg_clk);
diff --git a/drivers/usb/host/xhci-tegra.c b/drivers/usb/host/xhci-tegra.c
index 4b463e5..b1cce98 100644
--- a/drivers/usb/host/xhci-tegra.c
+++ b/drivers/usb/host/xhci-tegra.c
@@ -1240,6 +1240,7 @@ static int tegra_xusb_remove(struct platform_device *pdev)
 
 	usb_remove_hcd(xhci->shared_hcd);
 	usb_put_hcd(xhci->shared_hcd);
+	xhci->shared_hcd = NULL;
 	usb_remove_hcd(tegra->hcd);
 	usb_put_hcd(tegra->hcd);
 
diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index 0420eef..c928dbb 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -719,8 +719,6 @@ static void xhci_stop(struct usb_hcd *hcd)
 
 	/* Only halt host and free memory after both hcds are removed */
 	if (!usb_hcd_is_primary_hcd(hcd)) {
-		/* usb core will free this hcd shortly, unset pointer */
-		xhci->shared_hcd = NULL;
 		mutex_unlock(&xhci->mutex);
 		return;
 	}
-- 
2.7.4

[RFT PATCH 2/2] xhci: handle port status events for removed USB3 hcd

[Mathias Nyman]

At xhci removal the USB3 hcd (shared_hcd) is removed before the primary
USB2 hcd. Interrupts for port status changes may still occur for USB3
ports after the shared_hcd is freed, causing  NULL pointer dereference.

Check if xhci->shared_hcd is still valid before handing USB3 port events

Cc: <stable@vger.kernel.org>
Reported-by: Peter Chen <peter.chen@nxp.com>
Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
---
 drivers/usb/host/xhci-ring.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index f0a99aa..3d314b8 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -1552,6 +1552,13 @@ static void handle_port_status(struct xhci_hcd *xhci,
 		goto cleanup;
 	}
 
+	/* We might get interrupts after shared_hcd is removed */
+	if (port->rhub == &xhci->usb3_rhub && xhci->shared_hcd == NULL) {
+		xhci_dbg(xhci, "ignore port event for removed USB3 hcd\n");
+		bogus_port_status = true;
+		goto cleanup;
+	}
+
 	hcd = port->rhub->hcd;
 	bus_state = &xhci->bus_state[hcd_index(hcd)];
 	hcd_portnum = port->hcd_portnum;
-- 
2.7.4

Re: [RFT PATCH 2/2] xhci: handle port status events for removed USB3 hcd

[Mathias Nyman]

On 27.09.2018 19:26, Mathias Nyman wrote:
> At xhci removal the USB3 hcd (shared_hcd) is removed before the primary
> USB2 hcd. Interrupts for port status changes may still occur for USB3
> ports after the shared_hcd is freed, causing  NULL pointer dereference.
> 
> Check if xhci->shared_hcd is still valid before handing USB3 port events
> 
> Cc: <stable@vger.kernel.org>
> Reported-by: Peter Chen <peter.chen@nxp.com>
> Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
> ---
>   drivers/usb/host/xhci-ring.c | 7 +++++++
>   1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
> index f0a99aa..3d314b8 100644
> --- a/drivers/usb/host/xhci-ring.c
> +++ b/drivers/usb/host/xhci-ring.c
> @@ -1552,6 +1552,13 @@ static void handle_port_status(struct xhci_hcd *xhci,
>   		goto cleanup;
>   	}
>   
> +	/* We might get interrupts after shared_hcd is removed */
> +	if (port->rhub == &xhci->usb3_rhub && xhci->shared_hcd == NULL) {
> +		xhci_dbg(xhci, "ignore port event for removed USB3 hcd\n");
> +		bogus_port_status = true;
> +		goto cleanup;
> +	}
> +
>   	hcd = port->rhub->hcd;
>   	bus_state = &xhci->bus_state[hcd_index(hcd)];
>   	hcd_portnum = port->hcd_portnum;
> 

This probably only applies from 4.18 onwards, to test on older kernel try something
like this instead:

diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index 6996235..7925da9 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -1606,7 +1606,11 @@ static void handle_port_status(struct xhci_hcd *xhci,
         hcd = xhci_to_hcd(xhci);
         if ((major_revision == 0x03) != (hcd->speed >= HCD_USB3))
                 hcd = xhci->shared_hcd;
-
+       if (!hcd) {
+               bogus_port_status = true;
+               goto cleanup;
+       }
         if (major_revision == 0) {
                 xhci_warn(xhci, "Event for port %u not in "
                                 "Extended Capabilities, ignoring.\n",


Jack, Peter, do these patches solve the remove issues you are seeing?

Thanks
-Mathias

Re: [RFT PATCH 1/2] xhci: Fix leaking USB3 shared_hcd at xhci removal

[Chunfeng Yun]

On Thu, 2018-09-27 at 19:26 +0300, Mathias Nyman wrote:
> Ensure that the shared_hcd pointer is valid when calling usb_put_hcd()
> 
> The shared_hcd is removed and freed in xhci by first calling
> usb_remove_hcd(xhci->shared_hcd), and later
> usb_put_hcd(xhci->shared_hcd)
> 
> Afer commit fe190ed0d602 ("xhci: Do not halt the host until both HCD have
> disconnected their devices.") the shared_hcd was never properly put as
> xhci->shared_hcd was set to NULL before usb_put_hcd(xhci->shared_hcd) was
> called.
> 
> shared_hcd (USB3) is removed before primary hcd (USB2).
> While removing the primary hcd we might need to handle xhci interrupts
> to cleanly remove last USB2 devices, therefore we need to set
> xhci->shared_hcd to NULL before removing the primary hcd to let xhci
> interrupt handler know shared_hcd is no longer available.
> 
> xhci-plat.c, xhci-histb.c and xhci-mtk first create both their hcd's before
> adding them. so to keep the correct reverse removal order use a temporary
> shared_hcd variable for them.
> For more details see commit 4ac53087d6d4 ("usb: xhci: plat: Create both
> HCDs before adding them")
> 
> Fixes: fe190ed0d602 ("xhci: Do not halt the host until both HCD have disconnected their devices.")
> Cc: Joel Stanley <joel@jms.id.au>
> Cc: Chunfeng Yun <chunfeng.yun@mediatek.com>
> Cc: Thierry Reding <treding@nvidia.com>
> Cc: Jianguo Sun <sunjianguo1@huawei.com>
> Cc: <stable@vger.kernel.org>
> Reported-by: Jack Pham <jackp@codeaurora.org>
> Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
> ---
>  drivers/usb/host/xhci-histb.c | 6 ++++--
>  drivers/usb/host/xhci-mtk.c   | 6 ++++--
>  drivers/usb/host/xhci-pci.c   | 1 +
>  drivers/usb/host/xhci-plat.c  | 6 ++++--
>  drivers/usb/host/xhci-tegra.c | 1 +
>  drivers/usb/host/xhci.c       | 2 --
>  6 files changed, 14 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/usb/host/xhci-histb.c b/drivers/usb/host/xhci-histb.c
> index 27f0016..3c4abb5 100644
> --- a/drivers/usb/host/xhci-histb.c
> +++ b/drivers/usb/host/xhci-histb.c
> @@ -325,14 +325,16 @@ static int xhci_histb_remove(struct platform_device *dev)
>  	struct xhci_hcd_histb *histb = platform_get_drvdata(dev);
>  	struct usb_hcd *hcd = histb->hcd;
>  	struct xhci_hcd	*xhci = hcd_to_xhci(hcd);
> +	struct usb_hcd *shared_hcd = xhci->shared_hcd;
>  
>  	xhci->xhc_state |= XHCI_STATE_REMOVING;
>  
> -	usb_remove_hcd(xhci->shared_hcd);
> +	usb_remove_hcd(shared_hcd);
> +	xhci->shared_hcd = NULL;
>  	device_wakeup_disable(&dev->dev);
>  
>  	usb_remove_hcd(hcd);
> -	usb_put_hcd(xhci->shared_hcd);
> +	usb_put_hcd(shared_hcd);
>  
>  	xhci_histb_host_disable(histb);
>  	usb_put_hcd(hcd);
> diff --git a/drivers/usb/host/xhci-mtk.c b/drivers/usb/host/xhci-mtk.c
> index 71d0d33..60987c7 100644
> --- a/drivers/usb/host/xhci-mtk.c
> +++ b/drivers/usb/host/xhci-mtk.c
> @@ -590,12 +590,14 @@ static int xhci_mtk_remove(struct platform_device *dev)
>  	struct xhci_hcd_mtk *mtk = platform_get_drvdata(dev);
>  	struct usb_hcd	*hcd = mtk->hcd;
>  	struct xhci_hcd	*xhci = hcd_to_xhci(hcd);
> +	struct usb_hcd  *shared_hcd = xhci->shared_hcd;
>  
> -	usb_remove_hcd(xhci->shared_hcd);
> +	usb_remove_hcd(shared_hcd);
> +	xhci->shared_hcd = NULL;
>  	device_init_wakeup(&dev->dev, false);
>  
>  	usb_remove_hcd(hcd);
> -	usb_put_hcd(xhci->shared_hcd);
> +	usb_put_hcd(shared_hcd);
>  	usb_put_hcd(hcd);
>  	xhci_mtk_sch_exit(mtk);
>  	xhci_mtk_clks_disable(mtk);
> diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c
> index 51dd8e0..92fd6b6 100644
> --- a/drivers/usb/host/xhci-pci.c
> +++ b/drivers/usb/host/xhci-pci.c
> @@ -356,6 +356,7 @@ static void xhci_pci_remove(struct pci_dev *dev)
>  	if (xhci->shared_hcd) {
>  		usb_remove_hcd(xhci->shared_hcd);
>  		usb_put_hcd(xhci->shared_hcd);
> +		xhci->shared_hcd = NULL;
>  	}
>  
>  	/* Workaround for spurious wakeups at shutdown with HSW */
> diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
> index 94e9392..e5da8ce 100644
> --- a/drivers/usb/host/xhci-plat.c
> +++ b/drivers/usb/host/xhci-plat.c
> @@ -359,14 +359,16 @@ static int xhci_plat_remove(struct platform_device *dev)
>  	struct xhci_hcd	*xhci = hcd_to_xhci(hcd);
>  	struct clk *clk = xhci->clk;
>  	struct clk *reg_clk = xhci->reg_clk;
> +	struct usb_hcd *shared_hcd = xhci->shared_hcd;
>  
>  	xhci->xhc_state |= XHCI_STATE_REMOVING;
>  
> -	usb_remove_hcd(xhci->shared_hcd);
> +	usb_remove_hcd(shared_hcd);
> +	xhci->shared_hcd = NULL;
>  	usb_phy_shutdown(hcd->usb_phy);
>  
>  	usb_remove_hcd(hcd);
> -	usb_put_hcd(xhci->shared_hcd);
> +	usb_put_hcd(shared_hcd);
>  
>  	clk_disable_unprepare(clk);
>  	clk_disable_unprepare(reg_clk);
> diff --git a/drivers/usb/host/xhci-tegra.c b/drivers/usb/host/xhci-tegra.c
> index 4b463e5..b1cce98 100644
> --- a/drivers/usb/host/xhci-tegra.c
> +++ b/drivers/usb/host/xhci-tegra.c
> @@ -1240,6 +1240,7 @@ static int tegra_xusb_remove(struct platform_device *pdev)
>  
>  	usb_remove_hcd(xhci->shared_hcd);
>  	usb_put_hcd(xhci->shared_hcd);
> +	xhci->shared_hcd = NULL;
>  	usb_remove_hcd(tegra->hcd);
>  	usb_put_hcd(tegra->hcd);
>  
> diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
> index 0420eef..c928dbb 100644
> --- a/drivers/usb/host/xhci.c
> +++ b/drivers/usb/host/xhci.c
> @@ -719,8 +719,6 @@ static void xhci_stop(struct usb_hcd *hcd)
>  
>  	/* Only halt host and free memory after both hcds are removed */
>  	if (!usb_hcd_is_primary_hcd(hcd)) {
> -		/* usb core will free this hcd shortly, unset pointer */
> -		xhci->shared_hcd = NULL;
>  		mutex_unlock(&xhci->mutex);
>  		return;
>  	}

Tested-by: Chunfeng Yun <chunfeng.yun@mediatek.com>

Thanks

RE: [RFT PATCH 2/2] xhci: handle port status events for removed USB3 hcd

[Peter Chen]

 
> > Cc: <stable@vger.kernel.org>
> > Reported-by: Peter Chen <peter.chen@nxp.com>
> > Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
> > ---
> >   drivers/usb/host/xhci-ring.c | 7 +++++++
> >   1 file changed, 7 insertions(+)
> >
> > diff --git a/drivers/usb/host/xhci-ring.c
> > b/drivers/usb/host/xhci-ring.c index f0a99aa..3d314b8 100644
> > --- a/drivers/usb/host/xhci-ring.c
> > +++ b/drivers/usb/host/xhci-ring.c
> > @@ -1552,6 +1552,13 @@ static void handle_port_status(struct xhci_hcd *xhci,
> >   		goto cleanup;
> >   	}
> >
> > +	/* We might get interrupts after shared_hcd is removed */
> > +	if (port->rhub == &xhci->usb3_rhub && xhci->shared_hcd == NULL) {
> > +		xhci_dbg(xhci, "ignore port event for removed USB3 hcd\n");
> > +		bogus_port_status = true;
> > +		goto cleanup;
> > +	}
> > +
> >   	hcd = port->rhub->hcd;
> >   	bus_state = &xhci->bus_state[hcd_index(hcd)];
> >   	hcd_portnum = port->hcd_portnum;
> >
> 
> This probably only applies from 4.18 onwards, to test on older kernel try something
> like this instead:
> 
> diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c index
> 6996235..7925da9 100644
> --- a/drivers/usb/host/xhci-ring.c
> +++ b/drivers/usb/host/xhci-ring.c
> @@ -1606,7 +1606,11 @@ static void handle_port_status(struct xhci_hcd *xhci,
>          hcd = xhci_to_hcd(xhci);
>          if ((major_revision == 0x03) != (hcd->speed >= HCD_USB3))
>                  hcd = xhci->shared_hcd;
> -
> +       if (!hcd) {
> +               bogus_port_status = true;
> +               goto cleanup;
> +       }
>          if (major_revision == 0) {
>                  xhci_warn(xhci, "Event for port %u not in "
>                                  "Extended Capabilities, ignoring.\n",
> 
> 
> Jack, Peter, do these patches solve the remove issues you are seeing?

At my two USB3 platforms, only apply the 1st patch can fix my problem.  Maybe
my USB3 port change interrupt occurs always before removing USB2 HCD.

Peter

RE: [RFT PATCH 1/2] xhci: Fix leaking USB3 shared_hcd at xhci removal

[Peter Chen]

 
> Ensure that the shared_hcd pointer is valid when calling usb_put_hcd()
> 
> The shared_hcd is removed and freed in xhci by first calling usb_remove_hcd(xhci-
> >shared_hcd), and later
> usb_put_hcd(xhci->shared_hcd)
> 
> Afer commit fe190ed0d602 ("xhci: Do not halt the host until both HCD have
> disconnected their devices.") the shared_hcd was never properly put as
> xhci->shared_hcd was set to NULL before usb_put_hcd(xhci->shared_hcd)
> xhci->was
> called.
> 
> shared_hcd (USB3) is removed before primary hcd (USB2).
> While removing the primary hcd we might need to handle xhci interrupts to cleanly
> remove last USB2 devices, therefore we need to set
> xhci->shared_hcd to NULL before removing the primary hcd to let xhci
> interrupt handler know shared_hcd is no longer available.
> 
> xhci-plat.c, xhci-histb.c and xhci-mtk first create both their hcd's before adding them.
> so to keep the correct reverse removal order use a temporary shared_hcd variable
> for them.
> For more details see commit 4ac53087d6d4 ("usb: xhci: plat: Create both HCDs
> before adding them")
> 
> Fixes: fe190ed0d602 ("xhci: Do not halt the host until both HCD have disconnected
> their devices.")
> Cc: Joel Stanley <joel@jms.id.au>
> Cc: Chunfeng Yun <chunfeng.yun@mediatek.com>
> Cc: Thierry Reding <treding@nvidia.com>
> Cc: Jianguo Sun <sunjianguo1@huawei.com>
> Cc: <stable@vger.kernel.org>
> Reported-by: Jack Pham <jackp@codeaurora.org>
> Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
> ---
>  drivers/usb/host/xhci-histb.c | 6 ++++--
>  drivers/usb/host/xhci-mtk.c   | 6 ++++--
>  drivers/usb/host/xhci-pci.c   | 1 +
>  drivers/usb/host/xhci-plat.c  | 6 ++++--  drivers/usb/host/xhci-tegra.c | 1 +
>  drivers/usb/host/xhci.c       | 2 --
>  6 files changed, 14 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/usb/host/xhci-histb.c b/drivers/usb/host/xhci-histb.c index
> 27f0016..3c4abb5 100644
> --- a/drivers/usb/host/xhci-histb.c
> +++ b/drivers/usb/host/xhci-histb.c
> @@ -325,14 +325,16 @@ static int xhci_histb_remove(struct platform_device *dev)
>  	struct xhci_hcd_histb *histb = platform_get_drvdata(dev);
>  	struct usb_hcd *hcd = histb->hcd;
>  	struct xhci_hcd	*xhci = hcd_to_xhci(hcd);
> +	struct usb_hcd *shared_hcd = xhci->shared_hcd;
> 
>  	xhci->xhc_state |= XHCI_STATE_REMOVING;
> 
> -	usb_remove_hcd(xhci->shared_hcd);
> +	usb_remove_hcd(shared_hcd);
> +	xhci->shared_hcd = NULL;
>  	device_wakeup_disable(&dev->dev);
> 
>  	usb_remove_hcd(hcd);
> -	usb_put_hcd(xhci->shared_hcd);
> +	usb_put_hcd(shared_hcd);
> 
>  	xhci_histb_host_disable(histb);
>  	usb_put_hcd(hcd);
> diff --git a/drivers/usb/host/xhci-mtk.c b/drivers/usb/host/xhci-mtk.c index
> 71d0d33..60987c7 100644
> --- a/drivers/usb/host/xhci-mtk.c
> +++ b/drivers/usb/host/xhci-mtk.c
> @@ -590,12 +590,14 @@ static int xhci_mtk_remove(struct platform_device *dev)
>  	struct xhci_hcd_mtk *mtk = platform_get_drvdata(dev);
>  	struct usb_hcd	*hcd = mtk->hcd;
>  	struct xhci_hcd	*xhci = hcd_to_xhci(hcd);
> +	struct usb_hcd  *shared_hcd = xhci->shared_hcd;
> 
> -	usb_remove_hcd(xhci->shared_hcd);
> +	usb_remove_hcd(shared_hcd);
> +	xhci->shared_hcd = NULL;
>  	device_init_wakeup(&dev->dev, false);
> 
>  	usb_remove_hcd(hcd);
> -	usb_put_hcd(xhci->shared_hcd);
> +	usb_put_hcd(shared_hcd);
>  	usb_put_hcd(hcd);
>  	xhci_mtk_sch_exit(mtk);
>  	xhci_mtk_clks_disable(mtk);
> diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c index
> 51dd8e0..92fd6b6 100644
> --- a/drivers/usb/host/xhci-pci.c
> +++ b/drivers/usb/host/xhci-pci.c
> @@ -356,6 +356,7 @@ static void xhci_pci_remove(struct pci_dev *dev)
>  	if (xhci->shared_hcd) {
>  		usb_remove_hcd(xhci->shared_hcd);
>  		usb_put_hcd(xhci->shared_hcd);
> +		xhci->shared_hcd = NULL;
>  	}
> 
>  	/* Workaround for spurious wakeups at shutdown with HSW */ diff --git
> a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c index 94e9392..e5da8ce
> 100644
> --- a/drivers/usb/host/xhci-plat.c
> +++ b/drivers/usb/host/xhci-plat.c
> @@ -359,14 +359,16 @@ static int xhci_plat_remove(struct platform_device *dev)
>  	struct xhci_hcd	*xhci = hcd_to_xhci(hcd);
>  	struct clk *clk = xhci->clk;
>  	struct clk *reg_clk = xhci->reg_clk;
> +	struct usb_hcd *shared_hcd = xhci->shared_hcd;
> 
>  	xhci->xhc_state |= XHCI_STATE_REMOVING;
> 
> -	usb_remove_hcd(xhci->shared_hcd);
> +	usb_remove_hcd(shared_hcd);
> +	xhci->shared_hcd = NULL;
>  	usb_phy_shutdown(hcd->usb_phy);
> 
>  	usb_remove_hcd(hcd);
> -	usb_put_hcd(xhci->shared_hcd);
> +	usb_put_hcd(shared_hcd);
> 
>  	clk_disable_unprepare(clk);
>  	clk_disable_unprepare(reg_clk);
> diff --git a/drivers/usb/host/xhci-tegra.c b/drivers/usb/host/xhci-tegra.c index
> 4b463e5..b1cce98 100644
> --- a/drivers/usb/host/xhci-tegra.c
> +++ b/drivers/usb/host/xhci-tegra.c
> @@ -1240,6 +1240,7 @@ static int tegra_xusb_remove(struct platform_device
> *pdev)
> 
>  	usb_remove_hcd(xhci->shared_hcd);
>  	usb_put_hcd(xhci->shared_hcd);
> +	xhci->shared_hcd = NULL;
>  	usb_remove_hcd(tegra->hcd);
>  	usb_put_hcd(tegra->hcd);
> 
> diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c index 0420eef..c928dbb
> 100644
> --- a/drivers/usb/host/xhci.c
> +++ b/drivers/usb/host/xhci.c
> @@ -719,8 +719,6 @@ static void xhci_stop(struct usb_hcd *hcd)
> 
>  	/* Only halt host and free memory after both hcds are removed */
>  	if (!usb_hcd_is_primary_hcd(hcd)) {
> -		/* usb core will free this hcd shortly, unset pointer */
> -		xhci->shared_hcd = NULL;
>  		mutex_unlock(&xhci->mutex);
>  		return;
>  	}
> --
> 2.7.4

Tested-by: Peter Chen <peter.chen@nxp.com>

Since it can't apply to v4.14 directly, I did change manually.

Peter

Re: [RFT PATCH 2/2] xhci: handle port status events for removed USB3 hcd

[Jack Pham]

Hi Mathias,

On Fri, Sep 28, 2018 at 03:35:10AM +0000, Peter Chen wrote:
>  
> > > Cc: <stable@vger.kernel.org>
> > > Reported-by: Peter Chen <peter.chen@nxp.com>
> > > Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
> > > ---
> > >   drivers/usb/host/xhci-ring.c | 7 +++++++
> > >   1 file changed, 7 insertions(+)
> > >
> > > diff --git a/drivers/usb/host/xhci-ring.c
> > > b/drivers/usb/host/xhci-ring.c index f0a99aa..3d314b8 100644
> > > --- a/drivers/usb/host/xhci-ring.c
> > > +++ b/drivers/usb/host/xhci-ring.c
> > > @@ -1552,6 +1552,13 @@ static void handle_port_status(struct xhci_hcd *xhci,
> > >   		goto cleanup;
> > >   	}
> > >
> > > +	/* We might get interrupts after shared_hcd is removed */
> > > +	if (port->rhub == &xhci->usb3_rhub && xhci->shared_hcd == NULL) {
> > > +		xhci_dbg(xhci, "ignore port event for removed USB3 hcd\n");
> > > +		bogus_port_status = true;
> > > +		goto cleanup;
> > > +	}
> > > +
> > >   	hcd = port->rhub->hcd;
> > >   	bus_state = &xhci->bus_state[hcd_index(hcd)];
> > >   	hcd_portnum = port->hcd_portnum;
> > >
> > 
> > This probably only applies from 4.18 onwards, to test on older kernel try something
> > like this instead:
> > 
> > diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c index
> > 6996235..7925da9 100644
> > --- a/drivers/usb/host/xhci-ring.c
> > +++ b/drivers/usb/host/xhci-ring.c
> > @@ -1606,7 +1606,11 @@ static void handle_port_status(struct xhci_hcd *xhci,
> >          hcd = xhci_to_hcd(xhci);
> >          if ((major_revision == 0x03) != (hcd->speed >= HCD_USB3))
> >                  hcd = xhci->shared_hcd;
> > -
> > +       if (!hcd) {

For testing on 4.14 I also added the debug print "ignore port event"
here as well. Maybe it should be there in the final -stable patch as
well.

> > +               bogus_port_status = true;
> > +               goto cleanup;
> > +       }
> >          if (major_revision == 0) {
> >                  xhci_warn(xhci, "Event for port %u not in "
> >                                  "Extended Capabilities, ignoring.\n",
> > 
> > 
> > Jack, Peter, do these patches solve the remove issues you are seeing?
> 
> At my two USB3 platforms, only apply the 1st patch can fix my problem.  Maybe
> my USB3 port change interrupt occurs always before removing USB2 HCD.
> 
> Peter

Ditto. I think the xhci_irq() is getting triggered by something during
usb_remove_hcd() (usb_disconnect on the root hub?) but is able to
complete before it returns. That is, the NULL pointer dereference is
resolved yet I don't see that "ignore port event for removed USB3 hcd"
message at all.

Regardless, it's good to have here just in case, so
Tested-by: Jack Pham <jackp@codeaurora.org>

Will you be sending this as separate patches for -rc vs -stable?

Thanks,
Jack
-- 
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [RFT PATCH 1/2] xhci: Fix leaking USB3 shared_hcd at xhci removal

[Jack Pham]

On Thu, Sep 27, 2018 at 07:26:26PM +0300, Mathias Nyman wrote:
> Ensure that the shared_hcd pointer is valid when calling usb_put_hcd()
> 
> The shared_hcd is removed and freed in xhci by first calling
> usb_remove_hcd(xhci->shared_hcd), and later
> usb_put_hcd(xhci->shared_hcd)
> 
> Afer commit fe190ed0d602 ("xhci: Do not halt the host until both HCD have
> disconnected their devices.") the shared_hcd was never properly put as
> xhci->shared_hcd was set to NULL before usb_put_hcd(xhci->shared_hcd) was
> called.
> 
> shared_hcd (USB3) is removed before primary hcd (USB2).
> While removing the primary hcd we might need to handle xhci interrupts
> to cleanly remove last USB2 devices, therefore we need to set
> xhci->shared_hcd to NULL before removing the primary hcd to let xhci
> interrupt handler know shared_hcd is no longer available.
> 
> xhci-plat.c, xhci-histb.c and xhci-mtk first create both their hcd's before
> adding them. so to keep the correct reverse removal order use a temporary
> shared_hcd variable for them.
> For more details see commit 4ac53087d6d4 ("usb: xhci: plat: Create both
> HCDs before adding them")
> 
> Fixes: fe190ed0d602 ("xhci: Do not halt the host until both HCD have disconnected their devices.")
> Cc: Joel Stanley <joel@jms.id.au>
> Cc: Chunfeng Yun <chunfeng.yun@mediatek.com>
> Cc: Thierry Reding <treding@nvidia.com>
> Cc: Jianguo Sun <sunjianguo1@huawei.com>
> Cc: <stable@vger.kernel.org>
> Reported-by: Jack Pham <jackp@codeaurora.org>
> Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
> ---
>  drivers/usb/host/xhci-histb.c | 6 ++++--
>  drivers/usb/host/xhci-mtk.c   | 6 ++++--
>  drivers/usb/host/xhci-pci.c   | 1 +
>  drivers/usb/host/xhci-plat.c  | 6 ++++--
>  drivers/usb/host/xhci-tegra.c | 1 +
>  drivers/usb/host/xhci.c       | 2 --
>  6 files changed, 14 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/usb/host/xhci-histb.c b/drivers/usb/host/xhci-histb.c
> index 27f0016..3c4abb5 100644
> --- a/drivers/usb/host/xhci-histb.c
> +++ b/drivers/usb/host/xhci-histb.c
> @@ -325,14 +325,16 @@ static int xhci_histb_remove(struct platform_device *dev)
>  	struct xhci_hcd_histb *histb = platform_get_drvdata(dev);
>  	struct usb_hcd *hcd = histb->hcd;
>  	struct xhci_hcd	*xhci = hcd_to_xhci(hcd);
> +	struct usb_hcd *shared_hcd = xhci->shared_hcd;
>  
>  	xhci->xhc_state |= XHCI_STATE_REMOVING;
>  
> -	usb_remove_hcd(xhci->shared_hcd);
> +	usb_remove_hcd(shared_hcd);
> +	xhci->shared_hcd = NULL;
>  	device_wakeup_disable(&dev->dev);
>  
>  	usb_remove_hcd(hcd);
> -	usb_put_hcd(xhci->shared_hcd);
> +	usb_put_hcd(shared_hcd);
>  
>  	xhci_histb_host_disable(histb);
>  	usb_put_hcd(hcd);
> diff --git a/drivers/usb/host/xhci-mtk.c b/drivers/usb/host/xhci-mtk.c
> index 71d0d33..60987c7 100644
> --- a/drivers/usb/host/xhci-mtk.c
> +++ b/drivers/usb/host/xhci-mtk.c
> @@ -590,12 +590,14 @@ static int xhci_mtk_remove(struct platform_device *dev)
>  	struct xhci_hcd_mtk *mtk = platform_get_drvdata(dev);
>  	struct usb_hcd	*hcd = mtk->hcd;
>  	struct xhci_hcd	*xhci = hcd_to_xhci(hcd);
> +	struct usb_hcd  *shared_hcd = xhci->shared_hcd;
>  
> -	usb_remove_hcd(xhci->shared_hcd);
> +	usb_remove_hcd(shared_hcd);
> +	xhci->shared_hcd = NULL;
>  	device_init_wakeup(&dev->dev, false);
>  
>  	usb_remove_hcd(hcd);
> -	usb_put_hcd(xhci->shared_hcd);
> +	usb_put_hcd(shared_hcd);
>  	usb_put_hcd(hcd);
>  	xhci_mtk_sch_exit(mtk);
>  	xhci_mtk_clks_disable(mtk);
> diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c
> index 51dd8e0..92fd6b6 100644
> --- a/drivers/usb/host/xhci-pci.c
> +++ b/drivers/usb/host/xhci-pci.c
> @@ -356,6 +356,7 @@ static void xhci_pci_remove(struct pci_dev *dev)
>  	if (xhci->shared_hcd) {
>  		usb_remove_hcd(xhci->shared_hcd);
>  		usb_put_hcd(xhci->shared_hcd);
> +		xhci->shared_hcd = NULL;
>  	}
>  
>  	/* Workaround for spurious wakeups at shutdown with HSW */
> diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
> index 94e9392..e5da8ce 100644
> --- a/drivers/usb/host/xhci-plat.c
> +++ b/drivers/usb/host/xhci-plat.c
> @@ -359,14 +359,16 @@ static int xhci_plat_remove(struct platform_device *dev)
>  	struct xhci_hcd	*xhci = hcd_to_xhci(hcd);
>  	struct clk *clk = xhci->clk;
>  	struct clk *reg_clk = xhci->reg_clk;
> +	struct usb_hcd *shared_hcd = xhci->shared_hcd;
>  
>  	xhci->xhc_state |= XHCI_STATE_REMOVING;
>  
> -	usb_remove_hcd(xhci->shared_hcd);
> +	usb_remove_hcd(shared_hcd);
> +	xhci->shared_hcd = NULL;
>  	usb_phy_shutdown(hcd->usb_phy);
>  
>  	usb_remove_hcd(hcd);
> -	usb_put_hcd(xhci->shared_hcd);
> +	usb_put_hcd(shared_hcd);
>  
>  	clk_disable_unprepare(clk);
>  	clk_disable_unprepare(reg_clk);
> diff --git a/drivers/usb/host/xhci-tegra.c b/drivers/usb/host/xhci-tegra.c
> index 4b463e5..b1cce98 100644
> --- a/drivers/usb/host/xhci-tegra.c
> +++ b/drivers/usb/host/xhci-tegra.c
> @@ -1240,6 +1240,7 @@ static int tegra_xusb_remove(struct platform_device *pdev)
>  
>  	usb_remove_hcd(xhci->shared_hcd);
>  	usb_put_hcd(xhci->shared_hcd);
> +	xhci->shared_hcd = NULL;
>  	usb_remove_hcd(tegra->hcd);
>  	usb_put_hcd(tegra->hcd);
>  
> diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
> index 0420eef..c928dbb 100644
> --- a/drivers/usb/host/xhci.c
> +++ b/drivers/usb/host/xhci.c
> @@ -719,8 +719,6 @@ static void xhci_stop(struct usb_hcd *hcd)
>  
>  	/* Only halt host and free memory after both hcds are removed */
>  	if (!usb_hcd_is_primary_hcd(hcd)) {
> -		/* usb core will free this hcd shortly, unset pointer */
> -		xhci->shared_hcd = NULL;
>  		mutex_unlock(&xhci->mutex);
>  		return;
>  	}

Tested-by: Jack Pham <jackp@codeaurora.org>

Re: [RFT PATCH 2/2] xhci: handle port status events for removed USB3 hcd

[Mathias Nyman]

On 28.09.2018 21:10, Jack Pham wrote:
> Hi Mathias,
> 
>>> Jack, Peter, do these patches solve the remove issues you are seeing?
>>
>> At my two USB3 platforms, only apply the 1st patch can fix my problem.  Maybe
>> my USB3 port change interrupt occurs always before removing USB2 HCD.
>>
It's possible yes.

>> Peter
> 
> Ditto. I think the xhci_irq() is getting triggered by something during
> usb_remove_hcd() (usb_disconnect on the root hub?) but is able to
> complete before it returns. That is, the NULL pointer dereference is
> resolved yet I don't see that "ignore port event for removed USB3 hcd"
> message at all.
> 
> Regardless, it's good to have here just in case, so
> Tested-by: Jack Pham <jackp@codeaurora.org>
> 
> Will you be sending this as separate patches for -rc vs -stable?
> 
> Thanks,
> Jack
> 

Thanks, adding tested-by tags.

I'll send them to -rc with stable tag, and then later send a backported version
to older kernel once I have a upstream commit ID I can refer to.

Thanks
-Mathias