Re: [RFT PATCH 1/2] xhci: Fix leaking USB3 shared_hcd at xhci removal
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 > Cc: Chunfeng Yun > Cc: Thierry Reding > Cc: Jianguo Sun > Cc: > Reported-by: Jack Pham > Signed-off-by: Mathias Nyman > --- > 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 >
RE: [RFT PATCH 1/2] xhci: Fix leaking USB3 shared_hcd at xhci removal
> 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 > Cc: Chunfeng Yun > Cc: Thierry Reding > Cc: Jianguo Sun > Cc: > Reported-by: Jack Pham > Signed-off-by: Mathias Nyman > --- > 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->sha
Re: [RFT PATCH 1/2] xhci: Fix leaking USB3 shared_hcd at xhci removal
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 > Cc: Chunfeng Yun > Cc: Thierry Reding > Cc: Jianguo Sun > Cc: > Reported-by: Jack Pham > Signed-off-by: Mathias Nyman > --- > 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)