Re: refine xhci-plat-Fix-xhci_plat-shutdown-sequence

Re: refine xhci-plat-Fix-xhci_plat-shutdown-sequence

[leiwan]

在 2018-04-17 18:32，Greg KH 写道：
> On Tue, Apr 17, 2018 at 11:32:42AM +0800, leiwan@codeaurora.org wrote:
>> 
>> xhci-plat Shutdown callback should check HCD_FLAG_HW_ACCESSIBLE
>> before accessing any register. This should avoid hung with access
>> controllers which support runtime suspend
>> 
>> This can fix for issue of https://patchwork.kernel.org/patch/10339317/
>> corresponding upload in CAF:
>> https://source.codeaurora.org/quic/la/kernel/msm-4.4/commit/?h=LV.HB.1.1.5-03810-8x96.0&id=a7a5307ee04ad349d365ad50f304605a9cd9bd0a
>> 
>> full patch refer attachment.
>> diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
>> index 9b27798..bdf914d 100644
>> --- a/drivers/usb/host/xhci.c
>> +++ b/drivers/usb/host/xhci.c
>> @@ -702,6 +702,10 @@ static void xhci_shutdown(struct usb_hcd *hcd)
>>                 usb_disable_xhci_ports(to_pci_dev(hcd->self.sysdev));
>> 
>>         spin_lock_irq(&xhci->lock);
>> +       if (!HCD_HW_ACCESSIBLE(hcd)) {
>> +               spin_unlock_irq(&xhci->lock);
>> +               return;
>> +       }
>>         xhci_halt(xhci);
> 
> A blank line after the if statement?
> >> [lei]yes
> What about all of the other places in this driver that you should also
> check for this?  Look at the other host controllers, shouldn't you
> mirror what they are doing?
> >> [lei]I checked other usb host module shutdown and suspend workflow.
   >> All usb host driver need to check hw accessable before
   >> read/write usb register especially in runtime PM case..
> And this needs a Fixes: tag, along with a cc: stable so as to properly
> get backported as this is broken in some stable kernels right now.
> >> [lei] Added by v2 patch
> thanks,
> 
> greg k-h


 From c03697fa259ab38d1002598ec2ccfac37607ca0b Mon Sep 17 00:00:00 2001
 From: Lei wang <leiwan@codeaurora.org>
Date: Tue, 17 Apr 2018 10:55:35 +0800
Subject: [PATCH v2] xhci: plat: Fix xhci_plat shutdown hung

xhci-plat Shutdown callback should check HCD_FLAG_HW_ACCESSIBLE
before accessing any register. This should avoid hung with access
controllers which support runtime suspend

Fixes: b07c12517f2a ("xhci: plat: Register shutdown for xhci_plat")
Cc: <stable@vger.kernel.org>
Signed-off-by: Lei wang <leiwan@codeaurora.org>
---
  drivers/usb/host/xhci.c | 4 ++++
  1 file changed, 4 insertions(+)

diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index 9b27798..bdf914d 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -702,6 +702,10 @@ static void xhci_shutdown(struct usb_hcd *hcd)
         usb_disable_xhci_ports(to_pci_dev(hcd->self.sysdev));

     spin_lock_irq(&xhci->lock);
+   if (!HCD_HW_ACCESSIBLE(hcd)) {
+       spin_unlock_irq(&xhci->lock);
+       return;
+   }
     xhci_halt(xhci);
     /* Workaround for spurious wakeups at shutdown with HSW */
     if (xhci->quirks & XHCI_SPURIOUS_WAKEUP)
--
1.9.1

Re: refine xhci-plat-Fix-xhci_plat-shutdown-sequence

[Greg KH]

On Tue, Apr 17, 2018 at 10:32:28PM +0800, leiwan@codeaurora.org wrote:
> 在 2018-04-17 18:32，Greg KH 写道：
> > On Tue, Apr 17, 2018 at 11:32:42AM +0800, leiwan@codeaurora.org wrote:
> > > 
> > > xhci-plat Shutdown callback should check HCD_FLAG_HW_ACCESSIBLE
> > > before accessing any register. This should avoid hung with access
> > > controllers which support runtime suspend
> > > 
> > > This can fix for issue of https://patchwork.kernel.org/patch/10339317/
> > > corresponding upload in CAF:
> > > https://source.codeaurora.org/quic/la/kernel/msm-4.4/commit/?h=LV.HB.1.1.5-03810-8x96.0&id=a7a5307ee04ad349d365ad50f304605a9cd9bd0a
> > > 
> > > full patch refer attachment.
> > > diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
> > > index 9b27798..bdf914d 100644
> > > --- a/drivers/usb/host/xhci.c
> > > +++ b/drivers/usb/host/xhci.c
> > > @@ -702,6 +702,10 @@ static void xhci_shutdown(struct usb_hcd *hcd)
> > >                 usb_disable_xhci_ports(to_pci_dev(hcd->self.sysdev));
> > > 
> > >         spin_lock_irq(&xhci->lock);
> > > +       if (!HCD_HW_ACCESSIBLE(hcd)) {
> > > +               spin_unlock_irq(&xhci->lock);
> > > +               return;
> > > +       }
> > >         xhci_halt(xhci);
> > 
> > A blank line after the if statement?
> > >> [lei]yes
> > What about all of the other places in this driver that you should also
> > check for this?  Look at the other host controllers, shouldn't you
> > mirror what they are doing?
> > >> [lei]I checked other usb host module shutdown and suspend workflow.
>   >> All usb host driver need to check hw accessable before
>   >> read/write usb register especially in runtime PM case..
> > And this needs a Fixes: tag, along with a cc: stable so as to properly
> > get backported as this is broken in some stable kernels right now.
> > >> [lei] Added by v2 patch
> > thanks,
> > 
> > greg k-h
> 
> 
> From c03697fa259ab38d1002598ec2ccfac37607ca0b Mon Sep 17 00:00:00 2001
> From: Lei wang <leiwan@codeaurora.org>
> Date: Tue, 17 Apr 2018 10:55:35 +0800
> Subject: [PATCH v2] xhci: plat: Fix xhci_plat shutdown hung
> 
> xhci-plat Shutdown callback should check HCD_FLAG_HW_ACCESSIBLE
> before accessing any register. This should avoid hung with access
> controllers which support runtime suspend
> 
> Fixes: b07c12517f2a ("xhci: plat: Register shutdown for xhci_plat")
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Lei wang <leiwan@codeaurora.org>
> ---
>  drivers/usb/host/xhci.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
> index 9b27798..bdf914d 100644
> --- a/drivers/usb/host/xhci.c
> +++ b/drivers/usb/host/xhci.c
> @@ -702,6 +702,10 @@ static void xhci_shutdown(struct usb_hcd *hcd)
>         usb_disable_xhci_ports(to_pci_dev(hcd->self.sysdev));
> 
>     spin_lock_irq(&xhci->lock);
> +   if (!HCD_HW_ACCESSIBLE(hcd)) {
> +       spin_unlock_irq(&xhci->lock);
> +       return;
> +   }
>     xhci_halt(xhci);
>     /* Workaround for spurious wakeups at shutdown with HSW */
>     if (xhci->quirks & XHCI_SPURIOUS_WAKEUP)
> --
> 1.9.1

I totally do not understand, why did you forward a broken patch that was
not accepted upstream to the stable list?

Please go read Documentation/SubmittingPatches for how to do this
correctly.  Your first email was close, but it was attached, which isn't
ok.  And then I provided code review comments which you seem to have
ignored here...

thanks,

greg k-h