Message-ID: <a618ada1582c82b58d2503ecf777ea2d726f9399.camel@kylinos.cn>
Date: Tue, 10 Sep 2024 17:36:56 +0800
From: duanchenghao <duanchenghao@...inos.cn>
To: Alan Stern <stern@...land.harvard.edu>
Cc: gregkh@...uxfoundation.org, pavel@....cz, linux-pm@...r.kernel.org, 
	niko.mauno@...sala.com, stanley_chang@...ltek.com, tj@...nel.org, 
	linux-kernel@...r.kernel.org, linux-usb@...r.kernel.org
Subject: Re: [PATCH] USB: Fix the issue of task recovery failure caused by
 USB status when S4 wakes up


> [Please make sure that the lines in your email message don't extend 
> beyond 76 columns or so.]
> 

OK. Later, I will modify the patch format. V2 patch will be released
later

> Lots of things here seem to be wrong.
> 
> On Fri, Sep 06, 2024 at 11:05:48AM +0800, Duan Chenghao wrote:
> > When a device is inserted into the USB port and an S4 wakeup is
> > initiated,
> 
> There is no such thing as an S4 wakeup.  Do you mean wakeup from an
> S4 
> suspend state?

Yes, waking up from the S4 suspend state.

> 
> > after the USB-hub initialization is completed, it will
> > automatically enter suspend mode.
> 
> What will enter suspend mode?  The hub that the device was plugged
> into?
> That should not happen.  The hub initialization code should detect
> that 
> a new device was plugged in and prevent the hub from suspending.
> 

Yes, the current issue is that the hub detects a new device during the
resuming process. However, the S4 wakeup is attempting to put the hub
into suspend mode, and during the suspend process, it detects that the
HCD_FLAG_WAKEUP_PENDING flag has already been set, resulting in the
return of an EBUSY status.

> > Upon detecting a device on the USB port, it will proceed with
> > resume and set the hcd to the HCD_FLAG_WAKEUP_PENDING state.
> 
> HCD_FLAG_WAKEUP_PENDING is not a state.  It is a flag.
> 
> > During the S4 wakeup process, peripherals are put into suspend
> > mode, followed by task recovery.
> 
> What do you mean by "task recovery"?  We don't need to recover any 
> tasks.
> 

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.)
    |
    v
    restore image(task recovery)


> What do you mean by "peripherals are put into suspend mode"?  That's
> not 
> what happens.  Peripherals are set back to full power.
> 
> > However, upon detecting that the hcd is in the
> > HCD_FLAG_WAKEUP_PENDING state,
> > it will return an EBUSY status, causing the S4 suspend to fail and
> > subsequent task recovery to not proceed.
> 
> What will return an EBUSY status?

if HCD_FLAG_WAKEUP_PENDING flag is set_bit, will return EBUSY.

> 
> Why do you say that S4 suspend will fail?  Aren't you talking about
> S4 
> wakeup?

After returning EBUSY, the subsequent restore image operation will not
be executed.

> 
> Can you provide a kernel log that explains these points and shows
> what 
> problem you are trying to solve?

[    9.009166][ 2] [  T403] PM: Image signature found, resuming
[    9.009167][ 2] [  T403] PM: resume from hibernation
[    9.009243][ 2] [  T403] inno-codec inno-codec.16.auto:
[inno_vpu][vpu_notifier:1540]vpu_notifier: untested action 5...
[    9.009244][ 2] [  T403] Freezing user space processes ... (elapsed
0.001 seconds) done.
[    9.010355][ 2] [  T403] OOM killer disabled.
[    9.010355][ 2] [  T403] Freezing remaining freezable tasks ...
(elapsed 0.000 seconds) done.
[    9.012152][ 2] [  T403] PM: Basic memory bitmaps created
[    9.073333][ 2] [  T403] PM: Using 3 thread(s) for decompression
[    9.073334][ 2] [  T403] PM: Loading and decompressing image data
(486874 pages)...
[    9.073335][ 2] [  T403] hibernate: Hibernated on CPU 0 [mpidr:0x0]
[    9.095928][ 2] [  T403] PM: Image loading progress:   0%
[    9.664803][ 2] [  T403] PM: Image loading progress:  10%
[    9.794156][ 2] [  T403] PM: Image loading progress:  20%
[    9.913001][ 2] [  T403] PM: Image loading progress:  30%
[   10.034331][ 2] [  T403] PM: Image loading progress:  40%
[   10.154070][ 2] [  T403] PM: Image loading progress:  50%
[   10.277096][ 2] [  T403] PM: Image loading progress:  60%
[   10.398860][ 2] [  T403] PM: Image loading progress:  70%
[   10.533760][ 2] [  T403] PM: Image loading progress:  80%
[   10.659874][ 2] [  T403] PM: Image loading progress:  90%
[   10.760681][ 2] [  T403] PM: Image loading progress: 100%
[   10.760693][ 2] [  T403] PM: Image loading done
[   10.760718][ 2] [  T403] PM: Read 1947496 kbytes in 1.68 seconds
(1159.22 MB/s)
[   10.761982][ 2] [  T403] PM: Image successfully loaded
[   10.761988][ 2] [  T403] printk: Suspending console(s) (use
no_console_suspend to debug)
[   10.864973][ 2] [  T403] innovpu_freeze:1782
[   10.864974][ 2] [  T403] innovpu_suspend:1759
[   11.168871][ 2] [  T189] PM: pci_pm_freeze():
hcd_pci_suspend+0x0/0x38 returns -16
[   11.168875][ 2] [  T189] PM: dpm_run_callback():
pci_pm_freeze+0x0/0x108 returns -16
[   11.168876][ 2] [  T189] PM: Device 0000:05:00.0 failed to quiesce
async: error -16
[   12.270452][ 2] [  T403] innovpu_thaw:1792
[   12.405296][ 2] [  T403] PM: Failed to load hibernation image,
recovering.
[   12.486859][ 2] [  T403] PM: Basic memory bitmaps freed
[   12.486860][ 2] [  T403] OOM killer enabled.
[   12.486861][ 2] [  T403] Restarting tasks ... 

> 
> > This patch makes two modifications in total:
> > 1. The set_bit and clean_bit operations for the
> > HCD_FLAG_WAKEUP_PENDING flag of Hcd,
> > which were previously split between the top half and bottom half of
> > the interrupt,
> > are now unified and executed solely in the bottom half of the
> > interrupt.
> > This prevents the bottom half tasks from being frozen during the S4
> > process,
> > ensuring that the clean_bit process can proceed without
> > interruption.
> 
> The name is "clear_bit" (with an 'r'), not "clean_bit".
> 
> > 2. Add a condition to the set_bit operation for the hcd status
> > HCD_FLAG_WAKEUP_PENDING.
> > When the hcd status is HC_STATE_SUSPENDED, perform the setting of
> > the aforementioned status bit.
> > This prevents a subsequent set_bit from occurring after the
> > clean_bit if the hcd is in the resuming process.
> 
> hcd_bus_resume() clears that HCD_FLAG_WAKEUP_PENDING bit after
> calling 
> hcd->driver->bus_resume().  After that point,
> usb_hcd_resume_root_hub() 
> won't be called, so how can HCD_FLAG_WAKEUP_PENDING get set again?
> 
> Alan Stern
> 
> > Signed-off-by: Duan Chenghao <duanchenghao@...inos.cn>
> > ---
> >  drivers/usb/core/hcd.c | 1 -
> >  drivers/usb/core/hub.c | 3 +++
> >  2 files changed, 3 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
> > index 1ff7d901fede..a6bd0fbd82f4 100644
> > --- a/drivers/usb/core/hcd.c
> > +++ b/drivers/usb/core/hcd.c
> > @@ -2389,7 +2389,6 @@ void usb_hcd_resume_root_hub (struct usb_hcd
> > *hcd)
> >         spin_lock_irqsave (&hcd_root_hub_lock, flags);
> >         if (hcd->rh_registered) {
> >                 pm_wakeup_event(&hcd->self.root_hub->dev, 0);
> > -               set_bit(HCD_FLAG_WAKEUP_PENDING, &hcd->flags);
> >                 queue_work(pm_wq, &hcd->wakeup_work);
> >         }
> >         spin_unlock_irqrestore (&hcd_root_hub_lock, flags);
> > diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
> > index 4b93c0bd1d4b..7f847c4afc0d 100644
> > --- a/drivers/usb/core/hub.c
> > +++ b/drivers/usb/core/hub.c
> > @@ -3835,11 +3835,14 @@ int usb_port_resume(struct usb_device
> > *udev, pm_message_t msg)
> >  
> >  int usb_remote_wakeup(struct usb_device *udev)
> >  {
> > +       struct usb_hcd  *hcd = bus_to_hcd(udev->bus);
> >         int     status = 0;
> >  
> >         usb_lock_device(udev);
> >         if (udev->state == USB_STATE_SUSPENDED) {
> >                 dev_dbg(&udev->dev, "usb %sresume\n", "wakeup-");
> > +               if (hcd->state == HC_STATE_SUSPENDED)
> > +                       set_bit(HCD_FLAG_WAKEUP_PENDING, &hcd-
> > > flags);
> >                 status = usb_autoresume_device(udev);
> >                 if (status == 0) {
> >                         /* Let the drivers do their thing, then...
> > */
> > -- 
> > 2.34.1
> > 
> > 

Powered by blists - more mailing lists