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

Alan Stern
2024-10-09T15:50:46Z

On Wed, Oct 09, 2024 at 10:35:05AM +0800, duanchenghao wrote:
> 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/0a4dc46ae767c28dd207ae29511ede747f05539a.camel@kylinos.cn/

I still think your whole approach is wrong. There is no need to change
the way the HCD_FLAG_WAKEUP_PENDING flag gets set or cleared; that's not
the reason for the problem you're seeing.

The problem occurs because when suspend_common() in
drivers/usb/core/hcd-pci.c sets do_wakeup, it does so by calling
device_may_wakeup(), and the value that function returns is not what we
want. The value is based on whether the controller's power/wakeup
attribute is set, but we also have to take into account the type of
suspend that's happening.

Basically, if msg is one of the suspend types for which wakeups should
always be disabled, then do_wakeup should be set to false. There isn't
a macro to test for these things, but there should be. Something like
PMSG_IS_AUTO() in include/linux/pm.h; maybe call it PMSG_NO_WAKEUP().
This macro should return true if the PM_EVENT_FREEZE or PM_EVENT_QUIESCE
bits are set in msg.event.

Rafael, please correct me if I got any of this wrong.

So the right way to fix the problem is to add to pm.h:

#define PMSG_NO_WAKEUP(msg) (((msg.event) & \
(PM_EVENT_FREEZE | PM_EVENT_QUIESCE)) != 0)

and in suspend_common():

if (PMSG_IS_AUTO(msg))
do_wakeup = true;
else if (PMSG_NO_WAKEUP(msg))
do_wakeup = false;
else
do_wakeup = device_may_wakeup(dev);

Then with do_wakeup set to false, none of the HCD_WAKEUP_PENDING() tests
in the following code will be executed, so the routine won't fail during
the freeze or restore phase with -EBUSY.

You'll also have to define an hcd_pci_freeze() routine, just
like hcd_pci_suspend() except that it uses PMSG_FREEZE instead of
PMSG_SUSPEND. And the .freeze callback in usb_hcd_pci_pm_ops should be
changed to hcd_pci_freeze.

In fact, it looks like choose_wakeup() in drivers/usb/core/driver.c
could also use the new macro, instead of checking for FREEZE or QUIESCE
directly the way it does now.

Alan Stern