| Message ID | 20240130064819.1362642-1-guanyulin@google.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [RFC] usb: host: Allow userspace to control usb suspend flows | expand |
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@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/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@google.com> > +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()? > + 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. 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
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 What is a sub-system and how does it become engaged? > 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. This description and the comments and documentation in the patch all talk about "device power management". But in fact the patch only affects system power management; it does not affect runtime power management. Also, "deprecate_device_pm" does not seem like a very good name. You're not deprecating power management; you're just disabling it temporarily. You should find a better name. Do you really want your new flag to affect device suspend during hibernation? Does the co-processor remain powered when the system is powered off and unplugged? Do you really want the new sysfs flag to be present even on systems that don't have a co-processor? Why does this affect only the USB subsystem? Can't the co-processor use other, non-USB, devices on the system? Alan Stern
On Wed, Jan 31, 2024 at 12:41 AM Greg KH <gregkh@linuxfoundation.org> 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@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/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@google.com> > > +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
On Wed, Jan 31, 2024 at 1:12 AM Alan Stern <stern@rowland.harvard.edu> 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 > > What is a sub-system and how does it become engaged? > The subsystem, run by the co-processor, provides basic functionality even when the main system is suspended or otherwise occupied. In our design, the subsystem becomes engaged when the main system is powered on. The userspace will interact with and control both main system (main processor) and the sub system (co-processor). That way, the controllers will not be occupied by both processors simultaneously. > > > 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. > > This description and the comments and documentation in the patch all > talk about "device power management". But in fact the patch only > affects system power management; it does not affect runtime power > management. > This description does introduce ambiguity, In addition, the implementation does affect more functionality than I expected. I'll re-design the feature to let it only affect `suspend to RAM`. The related comments/commit message will also be updated in the next version. > > Also, "deprecate_device_pm" does not seem like a very good name. > You're not deprecating power management; you're just disabling it > temporarily. You should find a better name. > Thanks for pointing that out. I'll use `disable_suspend2ram` in the next version to better reflect what this feature does. I'll also update other wordings accordingly. > > Do you really want your new flag to affect device suspend during > hibernation? Does the co-processor remain powered when the system is > powered off and unplugged? > The implementation will be modified in the next version. This feature will only focus on `suspend to RAM`. The co-processor will not be powered when the system is powered off. > > Do you really want the new sysfs flag to be present even on systems > that don't have a co-processor? > We've identified a use case where this feature would be essential, which is a system with both the main processor and the co-processor. However, other use cases may also benefit from this feature. So, I think we could create a build flag for related data structures and codes. > > Why does this affect only the USB subsystem? Can't the co-processor > use other, non-USB, devices on the system? > In our use case, the co-processor only supports USB subsystem. There might be other co-processors support more subsystems, but we're not sure about how they will interact with the system. > > Alan Stern
On 01.02.24 10:02, Guan-Yu Lin wrote: > On Wed, Jan 31, 2024 at 1:12 AM Alan Stern <stern@rowland.harvard.edu> wrote: >> >> On Tue, Jan 30, 2024 at 06:47:13AM +0000, Guan-Yu Lin wrote: >> Why does this affect only the USB subsystem? Can't the co-processor >> use other, non-USB, devices on the system? >> > In our use case, the co-processor only supports USB subsystem. There might be > other co-processors support more subsystems, but we're not sure about how they > will interact with the system. Hi, it would be very good if you decided this now, before we add attributes. The reason is that if this feature is needed for multiple subsystems, the attribute should be added to the generic device structure, so that the naming and semantics are consistent. You really don't want to repeat this discussion for every subsystem. Regards Oliver
On Thu, Feb 1, 2024 at 5:38 PM Oliver Neukum <oneukum@suse.com> wrote: > > > > On 01.02.24 10:02, Guan-Yu Lin wrote: > > On Wed, Jan 31, 2024 at 1:12 AM Alan Stern <stern@rowland.harvard.edu> wrote: > >> > >> On Tue, Jan 30, 2024 at 06:47:13AM +0000, Guan-Yu Lin wrote: > > >> Why does this affect only the USB subsystem? Can't the co-processor > >> use other, non-USB, devices on the system? > >> > > In our use case, the co-processor only supports USB subsystem. There might be > > other co-processors support more subsystems, but we're not sure about how they > > will interact with the system. > > Hi, > > it would be very good if you decided this now, before we add attributes. > > The reason is that if this feature is needed for multiple subsystems, > the attribute should be added to the generic device structure, so that > the naming and semantics are consistent. > You really don't want to repeat this discussion for every subsystem. > > Regards > Oliver > Hi, Given that in our use case the co-processor only supports USB subsystem, I'd like to proceed with adding the attribute exclusively within the USB subsystem. Please let me know if there is any further consideration, thanks.
On Fri, Feb 02, 2024 at 12:00:00AM +0800, Guan-Yu Lin wrote: > On Thu, Feb 1, 2024 at 5:38 PM Oliver Neukum <oneukum@suse.com> wrote: > > > > > > > > On 01.02.24 10:02, Guan-Yu Lin wrote: > > > On Wed, Jan 31, 2024 at 1:12 AM Alan Stern <stern@rowland.harvard.edu> wrote: > > >> > > >> On Tue, Jan 30, 2024 at 06:47:13AM +0000, Guan-Yu Lin wrote: > > > > >> Why does this affect only the USB subsystem? Can't the co-processor > > >> use other, non-USB, devices on the system? > > >> > > > In our use case, the co-processor only supports USB subsystem. There might be > > > other co-processors support more subsystems, but we're not sure about how they > > > will interact with the system. > > > > Hi, > > > > it would be very good if you decided this now, before we add attributes. > > > > The reason is that if this feature is needed for multiple subsystems, > > the attribute should be added to the generic device structure, so that > > the naming and semantics are consistent. > > You really don't want to repeat this discussion for every subsystem. > > > > Regards > > Oliver > > > > Hi, > > Given that in our use case the co-processor only supports USB subsystem, I'd > like to proceed with adding the attribute exclusively within the USB subsystem. > Please let me know if there is any further consideration, thanks. Please do it properly as Oliver states here. Otherwise it will require more work in the future for you to modify this again for all other subsystems. thanks, greg k-h
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@google.com> +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__); + 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__); + return 0; + } + /* For all calls, take the device back to full power and * tell the PM core in case it was autosuspended previously. * Unbind the interfaces that will need rebinding later, diff --git a/drivers/usb/core/sysfs.c b/drivers/usb/core/sysfs.c index 5d21718afb05..b4520e5c8974 100644 --- a/drivers/usb/core/sysfs.c +++ b/drivers/usb/core/sysfs.c @@ -68,6 +68,33 @@ static ssize_t bMaxPower_show(struct device *dev, } static DEVICE_ATTR_RO(bMaxPower); +static ssize_t deprecate_device_pm_show(struct device *dev, + struct device_attribute *attr, char *buf) +{ + struct usb_device *udev; + + udev = to_usb_device(dev); + return sysfs_emit(buf, "%d\n", !!(udev->bus->deprecate_device_pm)); +} + +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; + rc = usb_lock_device_interruptible(udev); + if (rc < 0) + return -EINTR; + udev->bus->deprecate_device_pm = !!(val); + usb_unlock_device(udev); + return count; +} +static DEVICE_ATTR_RW(deprecate_device_pm); + static ssize_t configuration_show(struct device *dev, struct device_attribute *attr, char *buf) { @@ -984,6 +1011,7 @@ static DEVICE_ATTR_RW(interface_authorized_default); static struct attribute *usb_bus_attrs[] = { &dev_attr_authorized_default.attr, &dev_attr_interface_authorized_default.attr, + &dev_attr_deprecate_device_pm.attr, NULL, }; diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c index f04fde19f551..bc05d83d1c0b 100644 --- a/drivers/usb/host/xhci-plat.c +++ b/drivers/usb/host/xhci-plat.c @@ -442,6 +442,15 @@ static int xhci_plat_suspend(struct device *dev) struct xhci_hcd *xhci = hcd_to_xhci(hcd); int ret; + /* + * Skip the entire suspend process under the same usb bus if its sysfs + * entry `deprecate_device_pm` is set. + */ + if (hcd->self.deprecate_device_pm) { + dev_vdbg(dev, "deprecating dev_pm_ops: %s\n", __func__); + return 0; + } + if (pm_runtime_suspended(dev)) pm_runtime_resume(dev); @@ -507,6 +516,17 @@ static int xhci_plat_resume_common(struct device *dev, struct pm_message pmsg) static int xhci_plat_resume(struct device *dev) { + struct usb_hcd *hcd = dev_get_drvdata(dev); + + /* + * Skip the entire resume process under the same usb bus if its sysfs + * entry `deprecate_device_pm` is set. + */ + if (hcd->self.deprecate_device_pm) { + dev_vdbg(dev, "deprecating dev_pm_ops: %s\n", __func__); + return 0; + } + return xhci_plat_resume_common(dev, PMSG_RESUME); } diff --git a/include/linux/usb.h b/include/linux/usb.h index 9e52179872a5..1a1873104663 100644 --- a/include/linux/usb.h +++ b/include/linux/usb.h @@ -465,6 +465,10 @@ struct usb_bus { * the ep queue on a short transfer * with the URB_SHORT_NOT_OK flag set. */ + unsigned deprecate_device_pm:1; /* Deprecates the device power management + * functionality of USB devices on this + * bus and their hcd. + */ unsigned no_sg_constraint:1; /* no sg constraint */ unsigned sg_tablesize; /* 0 or largest number of sg list entries */
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@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(+)