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/xhci-sideband.c | 43 +++++++++++++++++++++++++++++++
include/linux/usb/xhci-sideband.h | 9 +++++++
2 files changed, 52 insertions(+)
diff --git a/drivers/usb/host/xhci-sideband.c b/drivers/usb/host/xhci-sideband.c
index d49f9886dd84..67f31b0ec726 100644
--- a/drivers/usb/host/xhci-sideband.c
+++ b/drivers/usb/host/xhci-sideband.c
@@ -266,6 +266,31 @@ 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.
+ *
+ * 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;
+
+ device_lock(&udev->dev);
+ active = usb_offload_check(udev);
+ device_unlock(&udev->dev);
+
+ 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 +311,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 +330,11 @@ xhci_sideband_create_interrupter(struct xhci_sideband *sb, int num_seg,
goto out;
}
+ udev = sb->vdev->udev;
+ device_lock(&udev->dev);
+ ret = usb_offload_get(udev);
+ device_unlock(&udev->dev);
+
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,16 @@ 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);
+ } else {
+ device_lock(&udev->dev);
+ usb_offload_put(udev);
+ device_unlock(&udev->dev);
+ }
+
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.49.0.604.gff1f9ca942-googOn Wed, Apr 16, 2025 at 02:43:03PM +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/xhci-sideband.c | 43 +++++++++++++++++++++++++++++++
> include/linux/usb/xhci-sideband.h | 9 +++++++
> 2 files changed, 52 insertions(+)
>
> diff --git a/drivers/usb/host/xhci-sideband.c b/drivers/usb/host/xhci-sideband.c
> index d49f9886dd84..67f31b0ec726 100644
> --- a/drivers/usb/host/xhci-sideband.c
> +++ b/drivers/usb/host/xhci-sideband.c
> @@ -266,6 +266,31 @@ 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.
> + *
> + * 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;
> +
> + device_lock(&udev->dev);
> + active = usb_offload_check(udev);
> + device_unlock(&udev->dev);
> +
> + return active;
What happens if the value changes right after reading it? What are you
going to do with the value?
> +}
> +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 +311,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 +330,11 @@ xhci_sideband_create_interrupter(struct xhci_sideband *sb, int num_seg,
> goto out;
> }
>
> + udev = sb->vdev->udev;
> + device_lock(&udev->dev);
> + ret = usb_offload_get(udev);
> + device_unlock(&udev->dev);
A "raw" call to device_lock/unlock feels rough, and harsh, why doesn't
the function do that itself?
thanks,
greg k-h
On Fri, Apr 25, 2025 at 7:14 PM Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Wed, Apr 16, 2025 at 02:43:03PM +0000, Guan-Yu Lin wrote:
> > +bool xhci_sideband_check(struct usb_hcd *hcd)
> > +{
> > + struct usb_device *udev = hcd->self.root_hub;
> > + bool active;
> > +
> > + device_lock(&udev->dev);
> > + active = usb_offload_check(udev);
> > + device_unlock(&udev->dev);
> > +
> > + return active;
>
> What happens if the value changes right after reading it? What are you
> going to do with the value?
>
Currently xhci_sideband_check() is only called when the xhci platform
device is going to suspend. Given that the usb devices should be
either already suspended or being marked as "offload_at_suspend" right
now, it should be safe if we ensure that "offload_usage" doesn't
change at this moment. Let me update
usb_offload_get()/usb_offload_put() to achieve this.
> >
> > + udev = sb->vdev->udev;
> > + device_lock(&udev->dev);
> > + ret = usb_offload_get(udev);
> > + device_unlock(&udev->dev);
>
> A "raw" call to device_lock/unlock feels rough, and harsh, why doesn't
> the function do that itself?
>
> thanks,
>
> greg k-h
The design is to align with usb_offload_put(). For usb_offload_put(), we
don't need to lock the device when the device state is
USB_STATE_NOTATTACHED. Hence, we put the device_lock()/device_unlock()
outside of usb_offload_get()/usb_offload_put(). Let me also change the
functions to usb_lock_device()/usb_unlock_device() for better coding
style.
Regards,
Guan-Yu
© 2016 - 2025 Red Hat, Inc.