| Message ID | 20241009054429.3970438-4-guanyulin@google.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Support system sleep with offloaded usb transfers | expand |
On 10/9/2024 7:42 AM, Guan-Yu Lin wrote: > Introduce sb_usage_count and corresponding apis to track sideband usage > on each usb_device. A sideband refers to the co-processor that accesses > the usb_device via shared control on the same USB host controller. To > optimize power usage, it's essential to monitor whether ther sideband is > actively using the usb_device. This information is vital when > determining if a usb_device can be safely suspended during system power > state transitions. > > Signed-off-by: Guan-Yu Lin <guanyulin@google.com> > --- > drivers/usb/core/driver.c | 54 +++++++++++++++++++++++++++++++++++++++ > include/linux/usb.h | 13 ++++++++++ > 2 files changed, 67 insertions(+) > > diff --git a/drivers/usb/core/driver.c b/drivers/usb/core/driver.c > index 0c3f12daac79..c1ba5ed15214 100644 > --- a/drivers/usb/core/driver.c > +++ b/drivers/usb/core/driver.c > @@ -1673,6 +1673,60 @@ void usb_disable_autosuspend(struct usb_device *udev) > } > EXPORT_SYMBOL_GPL(usb_disable_autosuspend); > > +/** > + * usb_sideband_get - notify usb driver there's a new active sideband > + * @udev: the usb_device which has an active sideband > + * > + * An active sideband indicates that another entity is currently using the usb > + * device. Notify the usb device by increasing the sb_usage_count. This allows > + * usb driver to adjust power management policy based on sideband activities. > + */ > +void usb_sideband_get(struct usb_device *udev) > +{ > + struct usb_device *parent = udev; Is it really "parent" in this case? Perhaps better variable name would just be "device". > + > + do { > + atomic_inc(&parent->sb_usage_count); > + parent = parent->parent; > + } while (parent); > +} > +EXPORT_SYMBOL_GPL(usb_sideband_get); > + > +/** > + * usb_sideband_put - notify usb driver there's a sideband deactivated > + * @udev: the usb_device which has a sideband deactivated > + * > + * The inverse operation of usb_sideband_get, which notifies the usb device by > + * decreasing the sb_usage_count. This allows usb driver to adjust power > + * management policy based on sideband activities. > + */ > +void usb_sideband_put(struct usb_device *udev) > +{ > + struct usb_device *parent = udev; Similarly here. > + > + do { > + atomic_dec(&parent->sb_usage_count); > + parent = parent->parent; > + } while (parent); > +} > +EXPORT_SYMBOL_GPL(usb_sideband_put); > +
On Wed, Oct 9, 2024 at 3:33 PM Amadeusz Sławiński <amadeuszx.slawinski@linux.intel.com> wrote: > > On 10/9/2024 7:42 AM, Guan-Yu Lin wrote: > > > > +void usb_sideband_get(struct usb_device *udev) > > +{ > > + struct usb_device *parent = udev; > > Is it really "parent" in this case? Perhaps better variable name would > just be "device". > Thanks for the heads-up, will change it to "device" in the next patchset. > > + > > + do { > > + atomic_inc(&parent->sb_usage_count); > > + parent = parent->parent; > > + } while (parent); > > +} > > +EXPORT_SYMBOL_GPL(usb_sideband_get); > > + > > Similarly here. > > > + > > + do { > > + atomic_dec(&parent->sb_usage_count); > > + parent = parent->parent; > > + } while (parent); > > +} > > +EXPORT_SYMBOL_GPL(usb_sideband_put); > > + Regards, Guan-Yu
On Wed, Oct 09, 2024 at 05:42:57AM +0000, Guan-Yu Lin wrote: > Introduce sb_usage_count and corresponding apis to track sideband usage > on each usb_device. A sideband refers to the co-processor that accesses > the usb_device via shared control on the same USB host controller. To > optimize power usage, it's essential to monitor whether ther sideband is > actively using the usb_device. This information is vital when > determining if a usb_device can be safely suspended during system power > state transitions. > > Signed-off-by: Guan-Yu Lin <guanyulin@google.com> > --- > drivers/usb/core/driver.c | 54 +++++++++++++++++++++++++++++++++++++++ > include/linux/usb.h | 13 ++++++++++ > 2 files changed, 67 insertions(+) > > diff --git a/drivers/usb/core/driver.c b/drivers/usb/core/driver.c > index 0c3f12daac79..c1ba5ed15214 100644 > --- a/drivers/usb/core/driver.c > +++ b/drivers/usb/core/driver.c > @@ -1673,6 +1673,60 @@ void usb_disable_autosuspend(struct usb_device *udev) > } > EXPORT_SYMBOL_GPL(usb_disable_autosuspend); > > +/** > + * usb_sideband_get - notify usb driver there's a new active sideband > + * @udev: the usb_device which has an active sideband > + * > + * An active sideband indicates that another entity is currently using the usb > + * device. Notify the usb device by increasing the sb_usage_count. This allows > + * usb driver to adjust power management policy based on sideband activities. > + */ > +void usb_sideband_get(struct usb_device *udev) > +{ > + struct usb_device *parent = udev; > + > + do { > + atomic_inc(&parent->sb_usage_count); As this is a reference count, use refcount_t please. > + parent = parent->parent; > + } while (parent); Woah, walking up the device chain? That should not be needed, or if so, then each device's "usage count" is pointless. > +} > +EXPORT_SYMBOL_GPL(usb_sideband_get); > + > +/** > + * usb_sideband_put - notify usb driver there's a sideband deactivated > + * @udev: the usb_device which has a sideband deactivated > + * > + * The inverse operation of usb_sideband_get, which notifies the usb device by > + * decreasing the sb_usage_count. This allows usb driver to adjust power > + * management policy based on sideband activities. > + */ > +void usb_sideband_put(struct usb_device *udev) > +{ > + struct usb_device *parent = udev; > + > + do { > + atomic_dec(&parent->sb_usage_count); > + parent = parent->parent; > + } while (parent); > +} > +EXPORT_SYMBOL_GPL(usb_sideband_put); > + > +/** > + * usb_sideband_check - check sideband activities on the host controller > + * @udev: the usb_device which has a sideband deactivated > + * > + * Check if there are any sideband activity on the USB device right now. This > + * information could be used for power management or other forms or resource > + * management. > + * > + * Returns true on any active sideband existence, false otherwise > + */ > +bool usb_sideband_check(struct usb_device *udev) > +{ > + return !!atomic_read(&udev->sb_usage_count); And what happens if it changes right after you make this call? This feels racy and broken. > +} > +EXPORT_SYMBOL_GPL(usb_sideband_check); > + > /** > * usb_autosuspend_device - delayed autosuspend of a USB device and its interfaces > * @udev: the usb_device to autosuspend > diff --git a/include/linux/usb.h b/include/linux/usb.h > index 672d8fc2abdb..5b9fea378960 100644 > --- a/include/linux/usb.h > +++ b/include/linux/usb.h > @@ -645,6 +645,7 @@ struct usb3_lpm_parameters { > * parent->hub_delay + wHubDelay + tTPTransmissionDelay (40ns) > * Will be used as wValue for SetIsochDelay requests. > * @use_generic_driver: ask driver core to reprobe using the generic driver. > + * @sb_usage_count: number of active sideband accessing this usb device. > * > * Notes: > * Usbcore drivers should not set usbdev->state directly. Instead use > @@ -731,6 +732,8 @@ struct usb_device { > > u16 hub_delay; > unsigned use_generic_driver:1; > + > + atomic_t sb_usage_count; Why is this on the device and not the interface that is bound to the driver that is doing this work? thanks, greg k-h
On Wed, Oct 9, 2024 at 8:44 PM Greg KH <gregkh@linuxfoundation.org> wrote: > > On Wed, Oct 09, 2024 at 05:42:57AM +0000, Guan-Yu Lin wrote: > > +void usb_sideband_get(struct usb_device *udev) > > +{ > > + struct usb_device *parent = udev; > > + > > + do { > > + atomic_inc(&parent->sb_usage_count); > > As this is a reference count, use refcount_t please. Acknowledged, will change it in the next patch. Thanks for the guidance. > > > + parent = parent->parent; > > + } while (parent); > > Woah, walking up the device chain? That should not be needed, or if so, > then each device's "usage count" is pointless. > Say a hub X with usb devices A,B,C attached on it, where usb device A is actively used by sideband now. We'd like to introduce a mechanism so that hub X won't have to iterate through all its children to determine sideband activities under this usb device tree. This problem is similar to runtime suspending a device, where rpm uses power.usage_count for tracking activity of the device itself and power.child_count to check the children's activity. In our scenario, we don't see the need to separate activities on the device itself or on its children. So we combine two counters in rpm as sb_usage_count, denoting the sideband activities under a specific usb device. We have to keep a counter in each device so that we won't influence the usb devices that aren't controlled by a sideband. When sideband activity changes on a usb device, its usb device parents should all get notified to maintain the correctness of sb_usage_count. This notifying process creates the procedure to walk up the device chain. > > +bool usb_sideband_check(struct usb_device *udev) > > +{ > > + return !!atomic_read(&udev->sb_usage_count); > > And what happens if it changes right after you make this call? This > feels racy and broken. > Seems like we need a mechanism to block any new sideband access after the usb device has been suspended. How about adding a lock during the period when the usb device is suspended? Do you think this is the correct direction to address the race condition? > > @@ -731,6 +732,8 @@ struct usb_device { > > > > u16 hub_delay; > > unsigned use_generic_driver:1; > > + > > + atomic_t sb_usage_count; > > Why is this on the device and not the interface that is bound to the > driver that is doing this work? > > thanks, > > greg k-h If the count is bound on the usb interface, I'm afraid that the sideband information couldn't be broadcasted across its usb device parents. Do you have some insight on how we can achieve this? Regards, Guan-Yu
On Thu, Oct 10, 2024 at 01:30:00PM +0800, Guan-Yu Lin wrote: > On Wed, Oct 9, 2024 at 8:44 PM Greg KH <gregkh@linuxfoundation.org> wrote: > > > > On Wed, Oct 09, 2024 at 05:42:57AM +0000, Guan-Yu Lin wrote: > > > +void usb_sideband_get(struct usb_device *udev) > > > +{ > > > + struct usb_device *parent = udev; > > > + > > > + do { > > > + atomic_inc(&parent->sb_usage_count); > > > > As this is a reference count, use refcount_t please. > > Acknowledged, will change it in the next patch. Thanks for the guidance. > > > > > > + parent = parent->parent; > > > + } while (parent); > > > > Woah, walking up the device chain? That should not be needed, or if so, > > then each device's "usage count" is pointless. > > > > Say a hub X with usb devices A,B,C attached on it, where usb device A > is actively used by sideband now. We'd like to introduce a mechanism > so that hub X won't have to iterate through all its children to > determine sideband activities under this usb device tree. Why would a hub care? > This problem > is similar to runtime suspending a device, where rpm uses > power.usage_count for tracking activity of the device itself and > power.child_count to check the children's activity. In our scenario, > we don't see the need to separate activities on the device itself or > on its children. But that's exactly what is needed here, if a hub wants to know what is happening on a child device, it should just walk the list of children and look :) > So we combine two counters in rpm as sb_usage_count, Combining counters is almost always never a good idea and will come back to bite you in the end. Memory isn't an issue here, speed isn't an issue here, so why not just do it properly? > denoting the sideband activities under a specific usb device. We have > to keep a counter in each device so that we won't influence the usb > devices that aren't controlled by a sideband. I can understand that for the device being "controlled" by a sideband, but that's it. > When sideband activity changes on a usb device, its usb device parents > should all get notified to maintain the correctness of sb_usage_count. Why "should" they? They shouldn't really care. > This notifying process creates the procedure to walk up the device > chain. You aren't notifying anyone, you are just incrementing a count that can change at any moment in time. > > > +bool usb_sideband_check(struct usb_device *udev) > > > +{ > > > + return !!atomic_read(&udev->sb_usage_count); > > > > And what happens if it changes right after you make this call? This > > feels racy and broken. > > > > Seems like we need a mechanism to block any new sideband access after > the usb device has been suspended. How about adding a lock during the > period when the usb device is suspended? Do you think this is the > correct direction to address the race condition? I don't know, as I don't know exactly what you are going to do with this information. But as-is, you can't just go "let's put an atomic variable in there to make it race free" as that's just not going to work. > > > @@ -731,6 +732,8 @@ struct usb_device { > > > > > > u16 hub_delay; > > > unsigned use_generic_driver:1; > > > + > > > + atomic_t sb_usage_count; > > > > Why is this on the device and not the interface that is bound to the > > driver that is doing this work? > > > > thanks, > > > > greg k-h > > If the count is bound on the usb interface, I'm afraid that the > sideband information couldn't be broadcasted across its usb device > parents. Do you have some insight on how we can achieve this? But the driver that is "sideband" is bound to the interface, not the device, right? Or is it the device? And again, nothing is being "broadcasted" here, it's just a variable to be read at random times and can change randomly :) thanks, greg k-h
On Thu, Oct 10, 2024 at 2:33 PM Greg KH <gregkh@linuxfoundation.org> wrote: > > On Thu, Oct 10, 2024 at 01:30:00PM +0800, Guan-Yu Lin wrote: > > On Wed, Oct 9, 2024 at 8:44 PM Greg KH <gregkh@linuxfoundation.org> wrote: > > > > > > On Wed, Oct 09, 2024 at 05:42:57AM +0000, Guan-Yu Lin wrote: > > > > + parent = parent->parent; > > > > + } while (parent); > > > > > > Woah, walking up the device chain? That should not be needed, or if so, > > > then each device's "usage count" is pointless. > > > > > > > Say a hub X with usb devices A,B,C attached on it, where usb device A > > is actively used by sideband now. We'd like to introduce a mechanism > > so that hub X won't have to iterate through all its children to > > determine sideband activities under this usb device tree. > > Why would a hub care? > Without the information of sideband activities on the usb devices connected to the hub, the hub couldn't determine if it could suspend or not. > > This problem > > is similar to runtime suspending a device, where rpm uses > > power.usage_count for tracking activity of the device itself and > > power.child_count to check the children's activity. In our scenario, > > we don't see the need to separate activities on the device itself or > > on its children. > > But that's exactly what is needed here, if a hub wants to know what is > happening on a child device, it should just walk the list of children > and look :) > > > So we combine two counters in rpm as sb_usage_count, > > Combining counters is almost always never a good idea and will come back > to bite you in the end. Memory isn't an issue here, speed isn't an > issue here, so why not just do it properly? > By combining the two comments above, my understanding is that we should either: 1. separating the counter to one recording the sideband activity of itself, one for its children. 2. walk the list of children to check sideband activities on demand. Please correct me if I mistake your messages. > > denoting the sideband activities under a specific usb device. We have > > to keep a counter in each device so that we won't influence the usb > > devices that aren't controlled by a sideband. > > I can understand that for the device being "controlled" by a sideband, > but that's it. > > > When sideband activity changes on a usb device, its usb device parents > > should all get notified to maintain the correctness of sb_usage_count. > > Why "should" they? They shouldn't really care. > Hubs need the sideband activity information on downstream usb devices, so that the hub won't suspend the upstream usb port when there is a sideband accessing the usb device connected to it. > > This notifying process creates the procedure to walk up the device > > chain. > > You aren't notifying anyone, you are just incrementing a count that can > change at any moment in time. > Apologies for the misleading word selection, by notify I mean changing the counter. > > > > +bool usb_sideband_check(struct usb_device *udev) > > > > +{ > > > > + return !!atomic_read(&udev->sb_usage_count); > > > > > > And what happens if it changes right after you make this call? This > > > feels racy and broken. > > > > > > > Seems like we need a mechanism to block any new sideband access after > > the usb device has been suspended. How about adding a lock during the > > period when the usb device is suspended? Do you think this is the > > correct direction to address the race condition? > > I don't know, as I don't know exactly what you are going to do with this > information. But as-is, you can't just go "let's put an atomic variable > in there to make it race free" as that's just not going to work. > Agree that changing the variable to atomic wouldn't resolve race conditions. Let me identify and mark critical sections in the next patchset. > > > > @@ -731,6 +732,8 @@ struct usb_device { > > > > > > > > u16 hub_delay; > > > > unsigned use_generic_driver:1; > > > > + > > > > + atomic_t sb_usage_count; > > > > > > Why is this on the device and not the interface that is bound to the > > > driver that is doing this work? > > > > > > thanks, > > > > > > greg k-h > > > > If the count is bound on the usb interface, I'm afraid that the > > sideband information couldn't be broadcasted across its usb device > > parents. Do you have some insight on how we can achieve this? > > But the driver that is "sideband" is bound to the interface, not the > device, right? Or is it the device? > > And again, nothing is being "broadcasted" here, it's just a variable to > be read at random times and can change randomly :) > > thanks, > > greg k-h By looking at the comment in xhci-sideband.c, I think the sideband is bound to an usb device instead of an usb interface. Maybe Mathias or Wesley could provide more details on this. /** * xhci_sideband_register - register a sideband for a usb device * @udev: usb device to be accessed via sideband ... */ Regards, Guan-Yu
On Fri, Oct 11, 2024 at 12:14:00AM +0800, Guan-Yu Lin wrote: > On Thu, Oct 10, 2024 at 2:33 PM Greg KH <gregkh@linuxfoundation.org> wrote: > > > > On Thu, Oct 10, 2024 at 01:30:00PM +0800, Guan-Yu Lin wrote: > > > On Wed, Oct 9, 2024 at 8:44 PM Greg KH <gregkh@linuxfoundation.org> wrote: > > > > > > > > On Wed, Oct 09, 2024 at 05:42:57AM +0000, Guan-Yu Lin wrote: > > > > > + parent = parent->parent; > > > > > + } while (parent); > > > > > > > > Woah, walking up the device chain? That should not be needed, or if so, > > > > then each device's "usage count" is pointless. > > > > > > > > > > Say a hub X with usb devices A,B,C attached on it, where usb device A > > > is actively used by sideband now. We'd like to introduce a mechanism > > > so that hub X won't have to iterate through all its children to > > > determine sideband activities under this usb device tree. > > > > Why would a hub care? > > > > Without the information of sideband activities on the usb devices > connected to the hub, the hub couldn't determine if it could suspend > or not. You are talking about an "internal" hub, right? And isn't this already covered by the original sideband patchset? If not, how is power management being handled there? > > > This problem > > > is similar to runtime suspending a device, where rpm uses > > > power.usage_count for tracking activity of the device itself and > > > power.child_count to check the children's activity. In our scenario, > > > we don't see the need to separate activities on the device itself or > > > on its children. > > > > But that's exactly what is needed here, if a hub wants to know what is > > happening on a child device, it should just walk the list of children > > and look :) > > > > > So we combine two counters in rpm as sb_usage_count, > > > > Combining counters is almost always never a good idea and will come back > > to bite you in the end. Memory isn't an issue here, speed isn't an > > issue here, so why not just do it properly? > > > > By combining the two comments above, my understanding is that we should either: > 1. separating the counter to one recording the sideband activity of > itself, one for its children. > 2. walk the list of children to check sideband activities on demand. > Please correct me if I mistake your messages. I think 2 is better, as this is infrequent and should be pretty fast to do when needed, right? But I really don't care, just don't combine things together that shouldn't be combined. > > > denoting the sideband activities under a specific usb device. We have > > > to keep a counter in each device so that we won't influence the usb > > > devices that aren't controlled by a sideband. > > > > I can understand that for the device being "controlled" by a sideband, > > but that's it. > > > > > When sideband activity changes on a usb device, its usb device parents > > > should all get notified to maintain the correctness of sb_usage_count. > > > > Why "should" they? They shouldn't really care. > > > > Hubs need the sideband activity information on downstream usb devices, > so that the hub won't suspend the upstream usb port when there is a > sideband accessing the usb device connected to it. Then why doesn't the sideband code just properly mark the "downstream" device a being used like any other normal device? Why is this acting "special"? thanks, greg k-h
On Fri, Oct 11, 2024 at 12:40 PM Greg KH <gregkh@linuxfoundation.org> wrote: > > On Fri, Oct 11, 2024 at 12:14:00AM +0800, Guan-Yu Lin wrote: > > On Thu, Oct 10, 2024 at 2:33 PM Greg KH <gregkh@linuxfoundation.org> wrote: > > > > > > On Thu, Oct 10, 2024 at 01:30:00PM +0800, Guan-Yu Lin wrote: > > > > On Wed, Oct 9, 2024 at 8:44 PM Greg KH <gregkh@linuxfoundation.org> wrote: > > > > > > > > > > On Wed, Oct 09, 2024 at 05:42:57AM +0000, Guan-Yu Lin wrote: > > > > > > + parent = parent->parent; > > > > > > + } while (parent); > > > > > > > > > > Woah, walking up the device chain? That should not be needed, or if so, > > > > > then each device's "usage count" is pointless. > > > > > > > > > > > > > Say a hub X with usb devices A,B,C attached on it, where usb device A > > > > is actively used by sideband now. We'd like to introduce a mechanism > > > > so that hub X won't have to iterate through all its children to > > > > determine sideband activities under this usb device tree. > > > > > > Why would a hub care? > > > > > > > Without the information of sideband activities on the usb devices > > connected to the hub, the hub couldn't determine if it could suspend > > or not. > > You are talking about an "internal" hub, right? And isn't this already > covered by the original sideband patchset? If not, how is power > management being handled there? > I'm referring to both internal and external hubs. As a sideband is designed to handle the transfers on specific endpoints, I think there's a possibility the usb device controlled by the sideband is connected to the host controller by a hierarchy of usb hubs. The current proposal of sideband, AFAIK, only supports sideband accessing usb devices when the system is active, whereas now we're proposing patchset to enable sideband access when the system is in sleep. > > > > This problem > > > > is similar to runtime suspending a device, where rpm uses > > > > power.usage_count for tracking activity of the device itself and > > > > power.child_count to check the children's activity. In our scenario, > > > > we don't see the need to separate activities on the device itself or > > > > on its children. > > > > > > But that's exactly what is needed here, if a hub wants to know what is > > > happening on a child device, it should just walk the list of children > > > and look :) > > > > > > > So we combine two counters in rpm as sb_usage_count, > > > > > > Combining counters is almost always never a good idea and will come back > > > to bite you in the end. Memory isn't an issue here, speed isn't an > > > issue here, so why not just do it properly? > > > > > > > By combining the two comments above, my understanding is that we should either: > > 1. separating the counter to one recording the sideband activity of > > itself, one for its children. > > 2. walk the list of children to check sideband activities on demand. > > Please correct me if I mistake your messages. > > I think 2 is better, as this is infrequent and should be pretty fast to > do when needed, right? But I really don't care, just don't combine > things together that shouldn't be combined. > Thanks for the clarification, I'll renew the patchset based on the discussions. > > > > denoting the sideband activities under a specific usb device. We have > > > > to keep a counter in each device so that we won't influence the usb > > > > devices that aren't controlled by a sideband. > > > > > > I can understand that for the device being "controlled" by a sideband, > > > but that's it. > > > > > > > When sideband activity changes on a usb device, its usb device parents > > > > should all get notified to maintain the correctness of sb_usage_count. > > > > > > Why "should" they? They shouldn't really care. > > > > > > > Hubs need the sideband activity information on downstream usb devices, > > so that the hub won't suspend the upstream usb port when there is a > > sideband accessing the usb device connected to it. > > Then why doesn't the sideband code just properly mark the "downstream" > device a being used like any other normal device? Why is this acting > "special"? > > thanks, > > greg k-h In runtime power management, sidebands could mark a device active by runtime pm apis as we usually did. However, there will be ambiguity when we're doing device power management in system suspend. A usb device being marked as runtime active could be either: 1) actively function through the existing usb driver stacks. 2) accessed by a sideband, where the usb driver stacks in the linux system aren't involved. In 1) system should suspend the devices, ports, controllers as normal because usb transfers are also suspended during system suspend. On the other hand, in 2) we should keep the necessary device, port, controller active to support sideband access during system suspend. Hence, we need the sideband access information of each usb_device during system power state transition. Regards, Guan-Yu
diff --git a/drivers/usb/core/driver.c b/drivers/usb/core/driver.c index 0c3f12daac79..c1ba5ed15214 100644 --- a/drivers/usb/core/driver.c +++ b/drivers/usb/core/driver.c @@ -1673,6 +1673,60 @@ void usb_disable_autosuspend(struct usb_device *udev) } EXPORT_SYMBOL_GPL(usb_disable_autosuspend); +/** + * usb_sideband_get - notify usb driver there's a new active sideband + * @udev: the usb_device which has an active sideband + * + * An active sideband indicates that another entity is currently using the usb + * device. Notify the usb device by increasing the sb_usage_count. This allows + * usb driver to adjust power management policy based on sideband activities. + */ +void usb_sideband_get(struct usb_device *udev) +{ + struct usb_device *parent = udev; + + do { + atomic_inc(&parent->sb_usage_count); + parent = parent->parent; + } while (parent); +} +EXPORT_SYMBOL_GPL(usb_sideband_get); + +/** + * usb_sideband_put - notify usb driver there's a sideband deactivated + * @udev: the usb_device which has a sideband deactivated + * + * The inverse operation of usb_sideband_get, which notifies the usb device by + * decreasing the sb_usage_count. This allows usb driver to adjust power + * management policy based on sideband activities. + */ +void usb_sideband_put(struct usb_device *udev) +{ + struct usb_device *parent = udev; + + do { + atomic_dec(&parent->sb_usage_count); + parent = parent->parent; + } while (parent); +} +EXPORT_SYMBOL_GPL(usb_sideband_put); + +/** + * usb_sideband_check - check sideband activities on the host controller + * @udev: the usb_device which has a sideband deactivated + * + * Check if there are any sideband activity on the USB device right now. This + * information could be used for power management or other forms or resource + * management. + * + * Returns true on any active sideband existence, false otherwise + */ +bool usb_sideband_check(struct usb_device *udev) +{ + return !!atomic_read(&udev->sb_usage_count); +} +EXPORT_SYMBOL_GPL(usb_sideband_check); + /** * usb_autosuspend_device - delayed autosuspend of a USB device and its interfaces * @udev: the usb_device to autosuspend diff --git a/include/linux/usb.h b/include/linux/usb.h index 672d8fc2abdb..5b9fea378960 100644 --- a/include/linux/usb.h +++ b/include/linux/usb.h @@ -645,6 +645,7 @@ struct usb3_lpm_parameters { * parent->hub_delay + wHubDelay + tTPTransmissionDelay (40ns) * Will be used as wValue for SetIsochDelay requests. * @use_generic_driver: ask driver core to reprobe using the generic driver. + * @sb_usage_count: number of active sideband accessing this usb device. * * Notes: * Usbcore drivers should not set usbdev->state directly. Instead use @@ -731,6 +732,8 @@ struct usb_device { u16 hub_delay; unsigned use_generic_driver:1; + + atomic_t sb_usage_count; }; #define to_usb_device(__dev) container_of_const(__dev, struct usb_device, dev) @@ -798,6 +801,9 @@ static inline int usb_acpi_port_lpm_incapable(struct usb_device *hdev, int index #ifdef CONFIG_PM extern void usb_enable_autosuspend(struct usb_device *udev); extern void usb_disable_autosuspend(struct usb_device *udev); +extern void usb_sideband_get(struct usb_device *udev); +extern void usb_sideband_put(struct usb_device *udev); +extern bool usb_sideband_check(struct usb_device *udev); extern int usb_autopm_get_interface(struct usb_interface *intf); extern void usb_autopm_put_interface(struct usb_interface *intf); @@ -818,6 +824,13 @@ static inline int usb_enable_autosuspend(struct usb_device *udev) static inline int usb_disable_autosuspend(struct usb_device *udev) { return 0; } +static inline int usb_sideband_get(struct usb_device *udev) +{ return 0; } +static inline int usb_sideband_put(struct usb_device *udev) +{ return 0; } +static inline bool usb_sideband_check(struct usb_device *udev) +{ return false; } + static inline int usb_autopm_get_interface(struct usb_interface *intf) { return 0; } static inline int usb_autopm_get_interface_async(struct usb_interface *intf)
Introduce sb_usage_count and corresponding apis to track sideband usage on each usb_device. A sideband refers to the co-processor that accesses the usb_device via shared control on the same USB host controller. To optimize power usage, it's essential to monitor whether ther sideband is actively using the usb_device. This information is vital when determining if a usb_device can be safely suspended during system power state transitions. Signed-off-by: Guan-Yu Lin <guanyulin@google.com> --- drivers/usb/core/driver.c | 54 +++++++++++++++++++++++++++++++++++++++ include/linux/usb.h | 13 ++++++++++ 2 files changed, 67 insertions(+)