Re: [PATCH v12 3/4] xhci: sideband: add api to trace sideband usage

[Greg KH <gregkh@linuxfoundation.org>]

On 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