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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <2025042518-shoplift-garnish-3a69@gregkh>
Date: Fri, 25 Apr 2025 13:14:26 +0200
From: Greg KH <gregkh@...uxfoundation.org>
To: Guan-Yu Lin <guanyulin@...gle.com>
Cc: mathias.nyman@...el.com, stern@...land.harvard.edu,
	sumit.garg@...nel.org, gargaditya08@...e.com, kekrby@...il.com,
	jeff.johnson@....qualcomm.com, quic_zijuhu@...cinc.com,
	andriy.shevchenko@...ux.intel.com, ben@...adent.org.uk,
	broonie@...nel.org, quic_wcheng@...cinc.com,
	linux-usb@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v12 3/4] xhci: sideband: add api to trace sideband usage

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@...gle.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