| Message ID | 20241009054429.3970438-1-guanyulin@google.com |
|---|---|
| Headers | show |
| Series | Support system sleep with offloaded usb transfers | expand |
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 09, 2024 at 05:42:59AM +0000, Guan-Yu Lin wrote: > Sharing a USB controller with another entity via xhci-sideband driver > creates power management complexities. To prevent the USB controller > from being inadvertently deactivated while in use by the other entity, a > usage-count based mechanism is implemented. This allows the system to > manage power effectively, ensuring the controller remains available > whenever needed. > > Signed-off-by: Guan-Yu Lin <guanyulin@google.com> > --- > drivers/usb/core/driver.c | 10 ++++++++++ > drivers/usb/core/hcd.c | 1 + > drivers/usb/core/usb.c | 1 + > drivers/usb/dwc3/core.c | 13 +++++++++++++ > drivers/usb/dwc3/core.h | 8 ++++++++ > drivers/usb/host/xhci-plat.c | 10 ++++++++++ > drivers/usb/host/xhci-plat.h | 7 +++++++ > sound/usb/qcom/qc_audio_offload.c | 3 +++ > 8 files changed, 53 insertions(+) > > diff --git a/drivers/usb/core/driver.c b/drivers/usb/core/driver.c > index c1ba5ed15214..83726bf88fb6 100644 > --- a/drivers/usb/core/driver.c > +++ b/drivers/usb/core/driver.c > @@ -1583,6 +1583,11 @@ int usb_suspend(struct device *dev, pm_message_t msg) > struct usb_device *udev = to_usb_device(dev); > int r; > > + if (msg.event == PM_EVENT_SUSPEND && usb_sideband_check(udev)) { > + dev_info(dev, "device active, skip %s", __func__); When drivers work properly, they are quiet. Why all of the loud shouting in this patch as it goes about it's business? also, __func__ is redundant in dev_*() calls :) 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 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
On Wed, Oct 9, 2024 at 8:47 PM Greg KH <gregkh@linuxfoundation.org> wrote: > > On Wed, Oct 09, 2024 at 05:42:59AM +0000, Guan-Yu Lin wrote: > > @@ -1583,6 +1583,11 @@ int usb_suspend(struct device *dev, pm_message_t msg) > > struct usb_device *udev = to_usb_device(dev); > > int r; > > > > + if (msg.event == PM_EVENT_SUSPEND && usb_sideband_check(udev)) { > > + dev_info(dev, "device active, skip %s", __func__); > > When drivers work properly, they are quiet. Why all of the loud > shouting in this patch as it goes about it's business? > > also, __func__ is redundant in dev_*() calls :) > > thanks, > > greg k-h Thanks for the suggestions. Let me switch to dev_dbg and remove __func__ in the next patch. Regards, Guan-Yu