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);
>
> 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, 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)
>
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);
>
> 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, 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);
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);
>
> 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, 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)
>
>
