RE: [PATCH] USB: Fix the issue of task recovery failure caused by USB status when S4 wakes up

[Gopal, Saranya]

Hi Duanchenghao,

> -----Original Message-----
> From: duanchenghao <duanchenghao@xxxxxxxxxx>
> Sent: Wednesday, October 09, 2024 8:05 AM
> To: Alan Stern <stern@xxxxxxxxxxxxxxxxxxx>
> Cc: Hongyu Xie <xy521521@xxxxxxxxx>;
> gregkh@xxxxxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; linux-
> pm@xxxxxxxxxxxxxxx; linux-usb@xxxxxxxxxxxxxxx;
> niko.mauno@xxxxxxxxxxx; pavel@xxxxxx;
> stanley_chang@xxxxxxxxxxx; tj@xxxxxxxxxx; Hongyu Xie
> <xiehongyu1@xxxxxxxxxx>
> Subject: Re: [PATCH] USB: Fix the issue of task recovery failure caused
> by USB status when S4 wakes up
> 
> Hi Alan,
> 
> These are two patches, each addressing the same issue. The current
> patch is a direct solution to the problem of the interrupt bottom half
> being frozen. The patch you replied with is another, alternative
> solution to the same problem. Please review which solution is more
> suitable, or if there are any other revised proposals.
> 
> 
> Please review the patch I mentioned:
> https://lore.kernel.org/all/0a4dc46ae767c28dd207ae29511ede747f
> 05539a.camel@xxxxxxxxxx/
> 

I was able to test 1500 S4 cycles in my system with your patch and all cycles passed with your patch.
I was facing an issue similar to what Kai-Heng was observing in the below mail thread:
https://lore.kernel.org/lkml/b8553def-19ea-41d5-b665-4859ddb7b6d5@xxxxxxxxxx/T/
He was observing issues due to USB touchscreen. And my issue was due to Bluetooth USB controller.
But, xHCI error logs were exactly same.
It looks like your patch is fixing both the issues.
It would be nice to see this patch land upstream.

Thanks,
Saranya

> Duanchenghao
> 
> 在 2024-09-24星期二的 09:38 -0400，Alan Stern写道：
> > On Mon, Sep 23, 2024 at 04:00:35PM +0800, duanchenghao wrote:
> > > Hi Alan,
> > >
> > > Do you think this plan is feasible, or is there any unclear part in
> > > my
> > > description that needs to be supplemented?
> >
> > I apologize for not getting back to you earlier -- I've been
> > incredibly
> > busy during the last few weeks.
> >
> > I still haven't had time to go over this throroughly.  If I don't
> > respond by the end of this week, remind me again.
> >
> > Alan Stern
> >
> > > duanchenghao
> > >
> > >
> > > 在 2024-09-14星期六的 10:43 +0800，Hongyu Xie写道：
> > > > From: Hongyu Xie <xiehongyu1@xxxxxxxxxx>
> > > >
> > > >
> > > > Hi Alan,
> > > > On 2024/9/12 23:00, Alan Stern wrote:
> > > > > On Thu, Sep 12, 2024 at 11:21:26AM +0800, duanchenghao
> wrote:
> > > > > > 在 2024-09-11星期三的 10:40 -0400，Alan Stern写道：
> > > > > > > On Tue, Sep 10, 2024 at 05:36:56PM +0800,
> duanchenghao
> > > > > > > wrote:
> > > > > > > > S4 wakeup restores the image that was saved before the
> > > > > > > > system
> > > > > > > > entered
> > > > > > > > the S4 sleep state.
> > > > > > > >
> > > > > > > >      S4 waking up from hibernation
> > > > > > > >      =============================
> > > > > > > >      kernel initialization
> > > > > > > >      |
> > > > > > > >      v
> > > > > > > >      freeze user task and kernel thread
> > > > > > > >      |
> > > > > > > >      v
> > > > > > > >      load saved image
> > > > > > > >      |
> > > > > > > >      v
> > > > > > > >      freeze the peripheral device and controller
> > > > > > > >      (Check the HCD_FLAG_WAKEUP_ PENDING flag of
> the USB.
> > > > > > > > If
> > > > > > > > it is
> > > > > > > > set,
> > > > > > > >       return to EBUSY and do not perform the following
> > > > > > > > restore
> > > > > > > > image.)
> > > > > > >
> > > > > > > Why is the flag set at this point?  It should not be; the
> > > > > > > device and
> > > > > > > controller should have been frozen with wakeup disabled.
> > > > > > >
> > > > > > This is check point, not set point.
> > > > >
> > > > > Yes, I know that.  But when the flag was checked, why did the
> > > > > code
> > > > > find
> > > > > that it was set?  The flag should have been clear.
> > > > Maybe duanchenghao means this,
> > > > freeze_kernel_threads
> > > > load_image_and_restore
> > > >    suspend roothub
> > > >                                       interrupt occurred
> > > >                                         usb_hcd_resume_root_hub
> > > >                                           set
> > > > HCD_FLAG_WAKEUP_PENDING
> > > >                                           queue_work // freezed
> > > >    suspend pci
> > > >      return -EBUSY  because HCD_FLAG_WAKEUP_PENDING
> > > >
> > > > So s4 resume failed.
> > > > >
> > > > > > > Is your problem related to the one discussed in this email
> > > > > > > thread?
> > > > > > >
> > > > > > > https://lore.kernel.org/linux-usb/d8600868-6e4b-45ab-
> b328-852b6ac8ecb5@xxxxxxxxxxxxxxxxxxx/
> > > > > > >
> > > > > > > Would the suggestion I made there -- i.e., have the xhci-
> > > > > > > hcd
> > > > > > > interrupt handler skip calling usb_hcd_resume_root_hub()
> if
> > > > > > > the
> > > > > > > root
> > > > > > > hub
> > > > > > > was suspended with wakeup = 0 -- fix your problem?
> > > > > >
> > > > > > Skipping usb_hcd_resume_root_hub() should generally be
> > > > > > possible,
> > > > > > but
> > > > > > it's important to ensure that normal remote wakeup
> > > > > > functionality
> > > > > > is not
> > > > > > compromised. Is it HUB_SUSPEND that the hub you are
> referring
> > > > > > to
> > > > > > is in
> > > > > > a suspended state?
> > > > >
> > > > > I don't understand this question.  hub_quiesce() gets called
> > > > > with
> > > > > HUB_SUSPEND when the hub enters a suspended state.
> > > > >
> > > > > You are correct about the need for normal remote wakeup to
> work
> > > > > properly.  The interrupt handler should skip calling
> > > > > usb_hcd_resume_root_hub() for port connect or disconnect
> > > > > changes
> > > > > and for
> > > > > port overcurrent changes (when the root hub is suspended
> with
> > > > > wakeup =
> > > > > 0).  But it should _not_ skip calling
> usb_hcd_resume_root_hub()
> > > > > for
> > > > > port
> > > > > resume events.
> > > > >
> > > > > Alan Stern
> > > > >
> > > >
> > > > Hongyu Xie
> > >
>