Received-SPF: pass (google.com: domain of linux-kernel+bounces-47728-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Precedence: bulk
MIME-Version: 1.0
References: <20240130064819.1362642-1-guanyulin@google.com> <2024013056-fidelity-sandy-0353@gregkh>
In-Reply-To: <2024013056-fidelity-sandy-0353@gregkh>
From: Guan-Yu Lin <guanyulin@google.com>
Date: Thu, 1 Feb 2024 15:22:00 +0800
Message-ID: <CAOuDEK1o_oGRcfQqCo5yjDRGibKAWa2NvZrK=hjim8GuNqJ9yQ@mail.gmail.com>
Subject: Re: [PATCH] [RFC] usb: host: Allow userspace to control usb suspend flows
To: Greg KH <gregkh@linuxfoundation.org>
Cc: mathias.nyman@intel.com, stern@rowland.harvard.edu, royluo@google.com, 
	hadess@hadess.net, benjamin.tissoires@redhat.com, 
	heikki.krogerus@linux.intel.com, oneukum@suse.com, grundler@chromium.org, 
	yajun.deng@linux.dev, dianders@chromium.org, linux-kernel@vger.kernel.org, 
	linux-usb@vger.kernel.org, badhri@google.com, albertccwang@google.com, 
	pumahsu@google.com
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Wed, Jan 31, 2024 at 12:41=E2=80=AFAM Greg KH <gregkh@linuxfoundation.or=
g> 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 shoul=
d
> > 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@google.com>
> > ---
> >  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/AB=
I/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@google.com>
> > +Description:
> > +             Deprecates the device power management functionality of U=
SB devices
> > +             and their host contorller under this usb bus. The modific=
ation of
> > +             this entry should be done when the system is active to en=
sure 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 =3D 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", __fu=
nc__);
>
> 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 =3D to_usb_device(dev);
> >       int                     status;
> >
> > +     /*
> > +      * Skip the entire resume process under the same usb bus if its s=
ysfs
> > +      * entry `deprecate_device_pm` is set.
> > +      */
> > +     if (udev->bus->deprecate_device_pm) {
> > +             dev_vdbg(&udev->dev, "deprecating dev_pm_ops: %s\n", __fu=
nc__);
>
> 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 =3D to_usb_device(dev);
> > +     int                     val, rc;
> > +
> > +     if (sscanf(buf, "%d", &val) !=3D 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 additi=
on,
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