On Mon, Sep 23, 2024 at 06:05:53PM +0800, dengjie03 wrote:
> Reproduction of the problem: During the S4 stress test,
> when a USB device is inserted or removed, there is a
> probability that the S4 wakeup will turn into a reboot.
> The following two points describe how to analyze and
> locate the problem points:
>
> 1. During the boot stage when S4 is awakened, after
> the USB RootHub is initialized,it will enter the
> runtime suspend state. From then on, whenever an
> xhci port change event occurs, it will trigger a
> remote wakeup request event and add wakeup_work to
> pm_wq, where the subsequent RootHub runtime resume
> process will be handled by pm_wq.
>
> xhci runtime suspend flow：
> S4 boot
> |->xhci init
> |->register_root_hub
> |->hub_probe
> |->callback = RPM_GET_CALLBACK(dev, runtime_suspend) /* xhci RootHub runtime suspend */
>
> xhci runtime resume flow ：
> xhci_irq()
> |->xhci_handle_event()
> |->handle_port_status()
> |->if(hcd->state == HC_STATE_SUSPENDED)
> |->usb_hcd_resume_root_hub()
> |->set_bit(HCD_FLAG_WAKEUP_PENDING, &hcd->flags) /* wakeup pending signal to be set */
> |->queue_work(pm_wq, &hcd->wakeup_work)
> |->hcd_resume_work() /* hcd->wakeup_work */
> |->usb_remote_wakeup()
> |->callback = RPM_GET_CALLBACK(dev, runtime_resume)
> |->usb_runtime_resume() /* usb runtime resume */
> |->generic_resume()
> |->hcd_bus_resume()
> |->clear_bit(HCD_FLAG_WAKEUP_PENDING, &hcd->flags);
> /* wakeup pending signal to be clear */
>
> 2. However, during the quiesce phase of S4 wakeup,
> freeze_kernel_threads() will freeze this pm_wq,
> and between freeze_kernel_threads() and dpm_suspend_start(),
> there exists a very time-consuming S4 image loading process.
> This leads to a situation where, if an xhci port change event occurs
> after freeze_kernel_threads(), triggering the wakeup pending
> signal to be set,but it cannot be processed by pm_wq to clear
> this wakeup_pending bit, it will result in a subsequent
> dpm_suspend_start() where USB suspend_common() detects the
> wakeup pending signal being set and returns an -EBUSY error,
> interrupting the S4 quiesce process and reverting to a reboot.
>
> S4 wakeup
> |->resume_store
> |->software_resume()
> |->freeze_kernel_threads() /* will freeze pm_wq */
> |->load_image_and_restore()
> |->swsusp_read() /* S4 image loading: time-consuming .
> When an xhci port change event occurs at this point, it triggers the wakeup pending signal to be set.
> However, since the pm_wq is in a frozen state, the wakeup_pending bit cannot be cleared.*/
> |->hibernation_restore
> |->dpm_suspend_start(PMSG_QUIESCE)
> |->hcd_pci_suspend()
> |->suspend_common()
> |->if (do_wakeup && HCD_WAKEUP_PENDING(hcd)) return -EBUSY;
>
> Below is a description of the countermeasures taken to address this issue:
> 1. Considering the restore process that occurs after
> the quiesce phase during S4 wakeup, which essentially
> resets all root hubs,checking this wakeup pending status
> in USB suspend_common() during the quiesce phase is of
> little significance and should therefore be filtered out.
>
> S4 wakeup restore phase
> |->dpm_resume(PMSG_RESTORE)
> |->hcd_pci_restore()
> |->xhci_resume() /* reset all root hubs */
>
> Fixes: 3904bdf0821c40352495f632862637080e6bd744(PM: hibernate: Freeze kernel threads in software_resume())
Please read the documentation for how to list a Fixes: tag, it should be
a bit smaller than this line :)
> Signed-off-by: xiongxin <xiongxin@xxxxxxxxxx>
> Co-developed-by: dengjie03 <dengjie03@xxxxxxxxxx>
As the documentation states, we need real names, not email aliases.
And fix up how you use co-developed-by please, again, the documentation
shows how to do so.
> ---
> drivers/base/power/main.c | 5 +++++
> drivers/usb/core/hcd-pci.c | 21 ++++++++++++++-------
> include/linux/pm.h | 1 +
> 3 files changed, 20 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c
> index fb4d18a0b185..264d08b9e331 100644
> --- a/drivers/base/power/main.c
> +++ b/drivers/base/power/main.c
> @@ -559,6 +559,11 @@ bool dev_pm_may_skip_resume(struct device *dev)
> return !dev->power.must_resume && pm_transition.event != PM_EVENT_RESTORE;
> }
>
> +bool is_pm_queisce(void)
Bad name for a global function :(
> +{
> + return pm_transition.event == PM_EVENT_QUIESCE;
What happens if it changes right after this? Where is the locking
involved? And why does USB only care about this and no other subsystem?
thanks,
greg k-h
