On Tue, Nov 16, 2021 at 04:28:16PM -0800, Brian Norris wrote: > On Mon, Nov 01, 2021 at 01:23:41PM +0530, Sandeep Maheswaram wrote: > > Avoiding phy powerdown when wakeup capable devices are connected > > by checking wakeup property of xhci plat device. > > Phy should be on to wake up the device from suspend using wakeup capable > > devices such as keyboard and mouse. > > > > Signed-off-by: Sandeep Maheswaram <quic_c_sanm@xxxxxxxxxxx> > > Because you've now factored in a device_may_wakeup() (which evaluates > 'false' for me), this no longer breaks my Rockchip RK3399 systems > (previous versions did). So from that angle: > > Tested-by: Brian Norris <briannorris@xxxxxxxxxxxx> > > > --- > > drivers/usb/dwc3/core.c | 7 +++++-- > > 1 file changed, 5 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c > > index 643239d..a6ad0ed 100644 > > --- a/drivers/usb/dwc3/core.c > > +++ b/drivers/usb/dwc3/core.c > > @@ -1787,7 +1787,7 @@ static int dwc3_suspend_common(struct dwc3 *dwc, pm_message_t msg) > > dwc3_core_exit(dwc); > > break; > > case DWC3_GCTL_PRTCAP_HOST: > > - if (!PMSG_IS_AUTO(msg)) { > > + if (!PMSG_IS_AUTO(msg) && !device_may_wakeup(&dwc->xhci->dev)) { > > I still find it odd to use device_may_wakeup(), since that's something > controlled by user space (sysfs) and doesn't fully factor in hardware > support. But that's what xhci-plat.c is doing, so I guess I see why > you're imitating it... > ...still, feels wrong to me. But so does a lot of how dwc3 works. device_may_wakeup() actually factors in hardware support, at least if the driver does the right thing (TM). The (current) implementation is: static inline bool device_may_wakeup(struct device *dev) { return dev->power.can_wakeup && !!dev->power.wakeup; } '.can_wakeup' should describe the hardware capability to wake up and the other flag whether wakeup is enabled (which can be altered by userspace). What this series currently does with the .can_wakeup flag is still wrong though IMO. Patch "[1/5] usb: host: xhci: plat: Add suspend quirk for dwc3 controller" [1] dynamically sets the flag with a value that depends on what is connected to the bus, so it doesn't specify any longer whether the hardware supports wakeup or not. [1] https://patchwork.kernel.org/project/linux-usb/patch/1635753224-23975-2-git-send-email-quic_c_sanm@xxxxxxxxxxx/