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

[Chunfeng Yun <chunfeng.yun@mediatek.com>]

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