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

[Greg KH]

On 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@xxxxxxxxxx>
> ---
>  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