On Mon, Oct 14, 2024 at 11:56 PM Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> wrote:
>
> On Mon, Oct 14, 2024 at 08:50:29AM +0000, Guan-Yu Lin wrote:
> >
> > diff --git a/drivers/usb/core/driver.c b/drivers/usb/core/driver.c
> > index e713cf9b3dd2..eb85cbb1a2ff 100644
> > --- a/drivers/usb/core/driver.c
> > +++ b/drivers/usb/core/driver.c
> > @@ -1583,6 +1583,11 @@ int usb_suspend(struct device *dev, pm_message_t msg)
> > struct usb_device *udev = to_usb_device(dev);
> > int r;
> >
> > + if (msg.event == PM_EVENT_SUSPEND && usb_sideband_check(udev)) {
> > + dev_dbg(dev, "device accessed via sideband\n");
> > + return 0;
> > + }
>
> I'm not so sure about this. By returning early, you prevent the drivers
> bound to this device from suspending. But they can't operate properly
> when the system is in a low-power mode. Won't that cause problems?
>
> Maybe this really belongs in usb_suspend_device(), and its counterpart
> belongs in usb_resume_device().
>
To my understanding, after the system is suspended, the USB driver
will do nothing as the main processor has been suspended. May I check
what forms of low power mode and operation we are discussing here?
usb_suspend_device() did close the required port/bus to maintain usb
transfer. However, there are additional usb_hcd_flush_endpoint() calls
aside from usb_suspend_device(). Maybe we should consider not doing
those also since some of the endpoints are now handled by the
sideband.
> > diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
> > index 1ff7d901fede..9876b3940281 100644
> > --- a/drivers/usb/core/hcd.c
> > +++ b/drivers/usb/core/hcd.c
> > @@ -2593,6 +2593,7 @@ struct usb_hcd *__usb_create_hcd(const struct hc_driver *driver,
> > timer_setup(&hcd->rh_timer, rh_timer_func, 0);
> > #ifdef CONFIG_PM
> > INIT_WORK(&hcd->wakeup_work, hcd_resume_work);
> > + refcount_set(&hcd->sb_usage_count, 0);
>
> Did I miss something? I didn't notice this field in any of the earlier
> patches. Was it already created by the prerequisite series? If so, why
> didn't that series do this initialization?
>
> > #endif
> >
> > INIT_WORK(&hcd->died_work, hcd_died_work);
> > diff --git a/drivers/usb/core/usb.c b/drivers/usb/core/usb.c
> > index 0b4685aad2d5..d315d066a56b 100644
> > --- a/drivers/usb/core/usb.c
> > +++ b/drivers/usb/core/usb.c
> > @@ -671,6 +671,7 @@ struct usb_device *usb_alloc_dev(struct usb_device *parent,
> > dev->state = USB_STATE_ATTACHED;
> > dev->lpm_disable_count = 1;
> > atomic_set(&dev->urbnum, 0);
> > + refcount_set(&dev->sb_usage_count, 0);
>
> And doesn't this belong in the 3/5 patch, the one that creates the
> sb_usage_count field?
>
> Alan Stern
Thanks for the suggestion, I'll move this, as well as the
initialization of hcd->sb_usage_count, to corresponding earlier
patches.
Regards,
Guan-Yu
