> [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@xxxxxxxxxx>
> > ---
> > 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
> >
> >
