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
