Re: [PATCH] [RFC] usb: host: Allow userspace to control usb suspend flows

[Guan-Yu Lin]

On Wed, Jan 31, 2024 at 12:41 AM Greg KH <gregkh@xxxxxxxxxxxxxxxxxxx> wrote:
>
> On Tue, Jan 30, 2024 at 06:47:13AM +0000, Guan-Yu Lin wrote:
> > In a system with sub-system engaged, the controllers are controlled by
> > both the main processor and the co-processor. Chances are when the main
> > processor decides to suspend the USB device, the USB device might still
> > be used by the co-processor. In this scenario, we need a way to let
> > system know whether it can suspend the USB device or not. We introduce a
> > new sysfs entry "deprecate_device_pm" to allow userspace to control the
> > device power management functionality on demand. As the userspace should
> > possess the information of both the main processor and the co-processor,
> > it should be able to address the conflict mentioned above.
> >
> > Signed-off-by: Guan-Yu Lin <guanyulin@xxxxxxxxxx>
> > ---
> >  Documentation/ABI/testing/sysfs-bus-usb | 10 +++++++++
> >  drivers/usb/core/driver.c               | 18 ++++++++++++++++
> >  drivers/usb/core/sysfs.c                | 28 +++++++++++++++++++++++++
> >  drivers/usb/host/xhci-plat.c            | 20 ++++++++++++++++++
> >  include/linux/usb.h                     |  4 ++++
> >  5 files changed, 80 insertions(+)
> >
> > diff --git a/Documentation/ABI/testing/sysfs-bus-usb b/Documentation/ABI/testing/sysfs-bus-usb
> > index 2b7108e21977..3f3d6c14201f 100644
> > --- a/Documentation/ABI/testing/sysfs-bus-usb
> > +++ b/Documentation/ABI/testing/sysfs-bus-usb
> > @@ -19,6 +19,16 @@ Description:
> >               would be authorized by default.
> >               The value can be 1 or 0. It's by default 1.
> >
> > +What:                /sys/bus/usb/devices/usbX/deprecate_device_pm
> > +Date:                January 2024
> > +Contact:     Guan-Yu Lin <guanyulin@xxxxxxxxxx>
> > +Description:
> > +             Deprecates the device power management functionality of USB devices
> > +             and their host contorller under this usb bus. The modification of
> > +             this entry should be done when the system is active to ensure the
> > +             correctness of system power state transitions.
> > +             The value can be 1 or 0. It's by default 0.
> > +
> >  What:                /sys/bus/usb/device/.../authorized
> >  Date:                July 2008
> >  KernelVersion:       2.6.26
> > diff --git a/drivers/usb/core/driver.c b/drivers/usb/core/driver.c
> > index e02ba15f6e34..e03cf972160d 100644
> > --- a/drivers/usb/core/driver.c
> > +++ b/drivers/usb/core/driver.c
> > @@ -1569,6 +1569,15 @@ int usb_suspend(struct device *dev, pm_message_t msg)
> >       struct usb_device       *udev = to_usb_device(dev);
> >       int r;
> >
> > +     /*
> > +      * Skip the entire suspend process under the same usb bus if its sysfs
> > +      * entry `deprecate_device_pm` is set.
> > +      */
> > +     if (udev->bus->deprecate_device_pm) {
> > +             dev_vdbg(&udev->dev, "deprecating dev_pm_ops: %s\n", __func__);
>
> Nit, dev_dbg() already contains __func__ by default, so no need for that
> at all.  And please use dev_dbg(), why are you using dev_vdbg()?
>
Thanks for the suggestion. I'll change it to dev_dbg() in the next version.
>
> > +             return 0;
> > +     }
> > +
> >       unbind_no_pm_drivers_interfaces(udev);
> >
> >       /* From now on we are sure all drivers support suspend/resume
> > @@ -1605,6 +1614,15 @@ int usb_resume(struct device *dev, pm_message_t msg)
> >       struct usb_device       *udev = to_usb_device(dev);
> >       int                     status;
> >
> > +     /*
> > +      * Skip the entire resume process under the same usb bus if its sysfs
> > +      * entry `deprecate_device_pm` is set.
> > +      */
> > +     if (udev->bus->deprecate_device_pm) {
> > +             dev_vdbg(&udev->dev, "deprecating dev_pm_ops: %s\n", __func__);
>
> Same as above.  And for all other instances you added.
>
> > +static ssize_t deprecate_device_pm_store(struct device *dev,
> > +                                      struct device_attribute *attr,
> > +                                      const char *buf, size_t count)
> > +{
> > +     struct usb_device       *udev = to_usb_device(dev);
> > +     int                     val, rc;
> > +
> > +     if (sscanf(buf, "%d", &val) != 1 || val < 0 || val > 1)
> > +             return -EINVAL;
>
> Please use the builtin function for determining if a boolean has been
> written through sysfs, don't roll your own.
>
Thanks for the advice. I'll use kstrtobool() in the next version. In addition,
I'll prepare another patch to update other self-rolled implementations.
>
> Note, these are just cosmetic things, I'm not taking the time yet to
> comment on the contents here, I'll let others do that first :)
>
> thanks,
>
> greg k-h