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);
+		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)
-- 
2.47.0.rc0.187.ge670bccf7e-goog

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

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