The existing sideband driver only registers sidebands without tracking
their active usage. To address this, sideband will now record its active
usage when it creates/removes interrupters. In addition, a new api is
introduced to provide a means for other dirvers to fetch sideband
activity information on a USB host controller.
Signed-off-by: Guan-Yu Lin <guanyulin@google.com>
---
drivers/usb/host/Kconfig | 11 +++++++++
drivers/usb/host/xhci-sideband.c | 38 +++++++++++++++++++++++++++++++
include/linux/usb/xhci-sideband.h | 9 ++++++++
3 files changed, 58 insertions(+)
diff --git a/drivers/usb/host/Kconfig b/drivers/usb/host/Kconfig
index 109100cc77a3..49b9cdc11298 100644
--- a/drivers/usb/host/Kconfig
+++ b/drivers/usb/host/Kconfig
@@ -113,6 +113,17 @@ config USB_XHCI_SIDEBAND
xHCI USB endpoints directly, allowing CPU to sleep while playing
audio.
+config USB_XHCI_SIDEBAND_SUSPEND
+ bool "xHCI support for sideband during system suspend"
+ depends on USB_XHCI_SIDEBAND
+ depends on USB_XHCI_PLATFORM
+ depends on SUSPEND
+ help
+ Say 'Y' to enable the support for the xHCI sideband capability
+ after system suspended. In addition to USB_XHCI_SIDEBAND, this
+ config allows endpoints and interrupters associated with the
+ sideband function when system is suspended.
+
config USB_XHCI_TEGRA
tristate "xHCI support for NVIDIA Tegra SoCs"
depends on PHY_TEGRA_XUSB
diff --git a/drivers/usb/host/xhci-sideband.c b/drivers/usb/host/xhci-sideband.c
index d49f9886dd84..15d72c23c0e0 100644
--- a/drivers/usb/host/xhci-sideband.c
+++ b/drivers/usb/host/xhci-sideband.c
@@ -266,6 +266,33 @@ xhci_sideband_get_event_buffer(struct xhci_sideband *sb)
}
EXPORT_SYMBOL_GPL(xhci_sideband_get_event_buffer);
+#if IS_ENABLED(CONFIG_USB_XHCI_SIDEBAND_SUSPEND)
+/**
+ * xhci_sideband_check - check the existence of active sidebands
+ * @hcd: the host controller driver associated with the target host controller
+ *
+ * Allow other drivers, such as usb controller driver, to check if there are
+ * any sideband activity on the host controller. This information could be used
+ * for power management or other forms of resource management. The caller should
+ * ensure downstream usb devices are all either suspended or marked as
+ * "offload_at_suspend" to ensure the correctness of the return value.
+ *
+ * Returns true on any active sideband existence, false otherwise.
+ */
+bool xhci_sideband_check(struct usb_hcd *hcd)
+{
+ struct usb_device *udev = hcd->self.root_hub;
+ bool active;
+
+ usb_lock_device(udev);
+ active = usb_offload_check(udev);
+ usb_unlock_device(udev);
+
+ return active;
+}
+EXPORT_SYMBOL_GPL(xhci_sideband_check);
+#endif /* IS_ENABLED(CONFIG_USB_XHCI_SIDEBAND_SUSPEND) */
+
/**
* xhci_sideband_create_interrupter - creates a new interrupter for this sideband
* @sb: sideband instance for this usb device
@@ -286,6 +313,7 @@ xhci_sideband_create_interrupter(struct xhci_sideband *sb, int num_seg,
bool ip_autoclear, u32 imod_interval, int intr_num)
{
int ret = 0;
+ struct usb_device *udev;
if (!sb || !sb->xhci)
return -ENODEV;
@@ -304,6 +332,9 @@ xhci_sideband_create_interrupter(struct xhci_sideband *sb, int num_seg,
goto out;
}
+ udev = sb->vdev->udev;
+ ret = usb_offload_get(udev);
+
sb->ir->ip_autoclear = ip_autoclear;
out:
@@ -323,6 +354,8 @@ EXPORT_SYMBOL_GPL(xhci_sideband_create_interrupter);
void
xhci_sideband_remove_interrupter(struct xhci_sideband *sb)
{
+ struct usb_device *udev;
+
if (!sb || !sb->ir)
return;
@@ -330,6 +363,11 @@ xhci_sideband_remove_interrupter(struct xhci_sideband *sb)
xhci_remove_secondary_interrupter(xhci_to_hcd(sb->xhci), sb->ir);
sb->ir = NULL;
+ udev = sb->vdev->udev;
+
+ if (udev->state != USB_STATE_NOTATTACHED)
+ usb_offload_put(udev);
+
mutex_unlock(&sb->mutex);
}
EXPORT_SYMBOL_GPL(xhci_sideband_remove_interrupter);
diff --git a/include/linux/usb/xhci-sideband.h b/include/linux/usb/xhci-sideband.h
index 45288c392f6e..5174cc5afc98 100644
--- a/include/linux/usb/xhci-sideband.h
+++ b/include/linux/usb/xhci-sideband.h
@@ -11,6 +11,7 @@
#include <linux/scatterlist.h>
#include <linux/usb.h>
+#include <linux/usb/hcd.h>
#define EP_CTX_PER_DEV 31 /* FIXME defined twice, from xhci.h */
@@ -83,6 +84,14 @@ xhci_sideband_get_endpoint_buffer(struct xhci_sideband *sb,
struct usb_host_endpoint *host_ep);
struct sg_table *
xhci_sideband_get_event_buffer(struct xhci_sideband *sb);
+
+#if IS_ENABLED(CONFIG_USB_XHCI_SIDEBAND_SUSPEND)
+bool xhci_sideband_check(struct usb_hcd *hcd);
+#else
+static inline bool xhci_sideband_check(struct usb_hcd *hcd)
+{ return false; }
+#endif /* IS_ENABLED(CONFIG_USB_XHCI_SIDEBAND_SUSPEND) */
+
int
xhci_sideband_create_interrupter(struct xhci_sideband *sb, int num_seg,
bool ip_autoclear, u32 imod_interval, int intr_num);
--
2.50.1.565.gc32cd1483b-googOn Fri, Aug 01, 2025 at 03:39:32AM +0000, Guan-Yu Lin wrote:
> The existing sideband driver only registers sidebands without tracking
> their active usage. To address this, sideband will now record its active
> usage when it creates/removes interrupters. In addition, a new api is
> introduced to provide a means for other dirvers to fetch sideband
> activity information on a USB host controller.
>
> Signed-off-by: Guan-Yu Lin <guanyulin@google.com>
> ---
> drivers/usb/host/Kconfig | 11 +++++++++
> drivers/usb/host/xhci-sideband.c | 38 +++++++++++++++++++++++++++++++
> include/linux/usb/xhci-sideband.h | 9 ++++++++
> 3 files changed, 58 insertions(+)
>
> diff --git a/drivers/usb/host/Kconfig b/drivers/usb/host/Kconfig
> index 109100cc77a3..49b9cdc11298 100644
> --- a/drivers/usb/host/Kconfig
> +++ b/drivers/usb/host/Kconfig
> @@ -113,6 +113,17 @@ config USB_XHCI_SIDEBAND
> xHCI USB endpoints directly, allowing CPU to sleep while playing
> audio.
>
> +config USB_XHCI_SIDEBAND_SUSPEND
> + bool "xHCI support for sideband during system suspend"
> + depends on USB_XHCI_SIDEBAND
> + depends on USB_XHCI_PLATFORM
> + depends on SUSPEND
> + help
> + Say 'Y' to enable the support for the xHCI sideband capability
> + after system suspended. In addition to USB_XHCI_SIDEBAND, this
> + config allows endpoints and interrupters associated with the
> + sideband function when system is suspended.
Again, why is a new config option needed here? Why can't we just use
the existing one? Why would you ever want one and not the other?
> +
> config USB_XHCI_TEGRA
> tristate "xHCI support for NVIDIA Tegra SoCs"
> depends on PHY_TEGRA_XUSB
> diff --git a/drivers/usb/host/xhci-sideband.c b/drivers/usb/host/xhci-sideband.c
> index d49f9886dd84..15d72c23c0e0 100644
> --- a/drivers/usb/host/xhci-sideband.c
> +++ b/drivers/usb/host/xhci-sideband.c
> @@ -266,6 +266,33 @@ xhci_sideband_get_event_buffer(struct xhci_sideband *sb)
> }
> EXPORT_SYMBOL_GPL(xhci_sideband_get_event_buffer);
>
> +#if IS_ENABLED(CONFIG_USB_XHCI_SIDEBAND_SUSPEND)
> +/**
> + * xhci_sideband_check - check the existence of active sidebands
> + * @hcd: the host controller driver associated with the target host controller
> + *
> + * Allow other drivers, such as usb controller driver, to check if there are
> + * any sideband activity on the host controller. This information could be used
> + * for power management or other forms of resource management. The caller should
> + * ensure downstream usb devices are all either suspended or marked as
> + * "offload_at_suspend" to ensure the correctness of the return value.
> + *
> + * Returns true on any active sideband existence, false otherwise.
> + */
> +bool xhci_sideband_check(struct usb_hcd *hcd)
> +{
> + struct usb_device *udev = hcd->self.root_hub;
> + bool active;
> +
> + usb_lock_device(udev);
> + active = usb_offload_check(udev);
> + usb_unlock_device(udev);
> +
> + return active;
> +}
> +EXPORT_SYMBOL_GPL(xhci_sideband_check);
> +#endif /* IS_ENABLED(CONFIG_USB_XHCI_SIDEBAND_SUSPEND) */
#ifdef in .c files is generally not a good idea, is it really needed
here? Maybe it is, it's hard to unwind...
thanks,
greg k-h
On Wed, Aug 13, 2025 at 10:51 PM Greg KH <gregkh@linuxfoundation.org> wrote: > > On Fri, Aug 01, 2025 at 03:39:32AM +0000, Guan-Yu Lin wrote: > > +config USB_XHCI_SIDEBAND_SUSPEND > > Again, why is a new config option needed here? Why can't we just use > the existing one? Why would you ever want one and not the other? > USB_XHCI_SIDEBAND focuses on the normal use case where the system is active, while USB_XHCI_SIDEBAND_SUSPEND enables the sideband during system sleep (Suspend-to-RAM). The design allows the user to determine the degree of support for the sideband in the system. We could add the "select" keyword to automatically enable USB_XHCI_SIDEBAND once USB_XHCI_SIDEBAND_SUSPEND is enabled. > > > +EXPORT_SYMBOL_GPL(xhci_sideband_check); > > +#endif /* IS_ENABLED(CONFIG_USB_XHCI_SIDEBAND_SUSPEND) */ > > #ifdef in .c files is generally not a good idea, is it really needed > here? Maybe it is, it's hard to unwind... > > thanks, > > greg k-h Given that CONFIG_USB_XHCI_SIDEBAND_SUSPEND depends on CONFIG_USB_XHCI_SIDEBAND and adds only a single function, I think it is preferable to place the new code in the same file. This approach prevents unnecessary code fragmentation and improves maintainability by keeping related functions together. Regards, Guan-Yu
On Tue, Aug 26, 2025 at 12:37:00PM +0800, Guan-Yu Lin wrote: > On Wed, Aug 13, 2025 at 10:51 PM Greg KH <gregkh@linuxfoundation.org> wrote: > > > > On Fri, Aug 01, 2025 at 03:39:32AM +0000, Guan-Yu Lin wrote: > > > +config USB_XHCI_SIDEBAND_SUSPEND > > > > Again, why is a new config option needed here? Why can't we just use > > the existing one? Why would you ever want one and not the other? > > > USB_XHCI_SIDEBAND focuses on the normal use case where the system is > active, while USB_XHCI_SIDEBAND_SUSPEND enables the sideband during > system sleep (Suspend-to-RAM). The design allows the user to determine > the degree of support for the sideband in the system. We could add the > "select" keyword to automatically enable USB_XHCI_SIDEBAND once > USB_XHCI_SIDEBAND_SUSPEND is enabled. But why would you want only one of these options and not both? The whole goal of this feature is for power savings, which means that suspend is needed by everyone. Don't increase the config variable combinations for no good reason. > > > +EXPORT_SYMBOL_GPL(xhci_sideband_check); > > > +#endif /* IS_ENABLED(CONFIG_USB_XHCI_SIDEBAND_SUSPEND) */ > > > > #ifdef in .c files is generally not a good idea, is it really needed > > here? Maybe it is, it's hard to unwind... > > > > thanks, > > > > greg k-h > > Given that CONFIG_USB_XHCI_SIDEBAND_SUSPEND depends on > CONFIG_USB_XHCI_SIDEBAND and adds only a single function, I think it > is preferable to place the new code in the same file. This approach > prevents unnecessary code fragmentation and improves maintainability > by keeping related functions together. We put #ifdefs in .h files. That's a long-time-rule for decades to ensure that we can maintain this codebase for even more decades to come. Please do not break that rule just to keep things close if it's not required. thanks, greg k-h
On Sat, Sep 6, 2025 at 9:11 PM Greg KH <gregkh@linuxfoundation.org> wrote: > > On Tue, Aug 26, 2025 at 12:37:00PM +0800, Guan-Yu Lin wrote: > > On Wed, Aug 13, 2025 at 10:51 PM Greg KH <gregkh@linuxfoundation.org> wrote: > > > > > > On Fri, Aug 01, 2025 at 03:39:32AM +0000, Guan-Yu Lin wrote: > > > > +config USB_XHCI_SIDEBAND_SUSPEND > > > > > > Again, why is a new config option needed here? Why can't we just use > > > the existing one? Why would you ever want one and not the other? > > > > > USB_XHCI_SIDEBAND focuses on the normal use case where the system is > > active, while USB_XHCI_SIDEBAND_SUSPEND enables the sideband during > > system sleep (Suspend-to-RAM). The design allows the user to determine > > the degree of support for the sideband in the system. We could add the > > "select" keyword to automatically enable USB_XHCI_SIDEBAND once > > USB_XHCI_SIDEBAND_SUSPEND is enabled. > > But why would you want only one of these options and not both? The > whole goal of this feature is for power savings, which means that > suspend is needed by everyone. Don't increase the config variable > combinations for no good reason. > Thanks for the suggestions. I'll remove USB_XHCI_SIDEBAND_SUSPEND in the next version. > > > > +EXPORT_SYMBOL_GPL(xhci_sideband_check); > > > > +#endif /* IS_ENABLED(CONFIG_USB_XHCI_SIDEBAND_SUSPEND) */ > > > > > > #ifdef in .c files is generally not a good idea, is it really needed > > > here? Maybe it is, it's hard to unwind... > > > > > > thanks, > > > > > > greg k-h > > > > Given that CONFIG_USB_XHCI_SIDEBAND_SUSPEND depends on > > CONFIG_USB_XHCI_SIDEBAND and adds only a single function, I think it > > is preferable to place the new code in the same file. This approach > > prevents unnecessary code fragmentation and improves maintainability > > by keeping related functions together. > > We put #ifdefs in .h files. That's a long-time-rule for decades to > ensure that we can maintain this codebase for even more decades to come. > Please do not break that rule just to keep things close if it's not > required. > > thanks, > > greg k-h Thanks for the suggestions. This concern would be addressed once we use only CONFIG_USB_XHCI_SIDEBAND to control all features. Regards, Guan-Yu
© 2016 - 2025 Red Hat, Inc.