Re: [PATCH v12 2/4] usb: add apis for offload usage tracking

[Greg KH <gregkh@linuxfoundation.org>]

On Wed, Apr 16, 2025 at 02:43:02PM +0000, Guan-Yu Lin wrote:
> Introduce offload_usage and corresponding apis to track offload usage
> on each USB device. Offload denotes that there is another co-processor
> accessing the USB device via the same USB host controller. To optimize
> power usage, it's essential to monitor whether the USB device is
> actively used by other co-processor. This information is vital when
> determining if a USB device can be safely suspended during system power
> state transitions.
> 
> Signed-off-by: Guan-Yu Lin <guanyulin@google.com>
> ---
>  drivers/usb/core/driver.c | 109 ++++++++++++++++++++++++++++++++++++++
>  drivers/usb/core/usb.c    |   1 +
>  drivers/usb/host/Kconfig  |  11 ++++
>  include/linux/usb.h       |  17 ++++++
>  4 files changed, 138 insertions(+)
> 
> diff --git a/drivers/usb/core/driver.c b/drivers/usb/core/driver.c
> index 460d4dde5994..76372690add0 100644
> --- a/drivers/usb/core/driver.c
> +++ b/drivers/usb/core/driver.c
> @@ -2036,6 +2036,115 @@ int usb_disable_usb2_hardware_lpm(struct usb_device *udev)
>  
>  #endif /* CONFIG_PM */
>  
> +#if IS_ENABLED(CONFIG_USB_XHCI_SIDEBAND_SUSPEND)
> +
> +/**
> + * usb_offload_get - increment the offload_usage of a USB device
> + * @udev: the USB device to increment its offload_usage
> + *
> + * Incrementing the offload_usage of a usb_device indicates that offload is
> + * enabled on this usb_device; that is, another entity is actively handling USB
> + * transfers. This information allows the USB driver to adjust its power
> + * management policy based on offload activity.
> + *
> + * The caller must hold @udev's device lock.
> + *
> + * Return: 0 on success. A negative error code otherwise.
> + */
> +int usb_offload_get(struct usb_device *udev)
> +{
> +	int ret;
> +
> +	if (udev->state == USB_STATE_NOTATTACHED ||
> +			udev->state == USB_STATE_SUSPENDED)
> +		return -EAGAIN;
> +
> +	/*
> +	 * offload_usage could only be modified when the device is active, since
> +	 * it will alter the suspend flow of the device.
> +	 */
> +	ret = usb_autoresume_device(udev);
> +
> +	if (ret < 0)
> +		return ret;
> +
> +	udev->offload_usage++;
> +	usb_autosuspend_device(udev);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(usb_offload_get);
> +
> +/**
> + * usb_offload_put - drop the offload_usage of a USB device
> + * @udev: the USB device to drop its offload_usage
> + *
> + * The inverse operation of usb_offload_get, which drops the offload_usage of
> + * a USB device. This information allows the USB driver to adjust its power
> + * management policy based on offload activity.
> + *
> + * The caller must hold @udev's device lock.
> + *
> + * Return: 0 on success. A negative error code otherwise.
> + */
> +int usb_offload_put(struct usb_device *udev)
> +{
> +	int ret;
> +
> +	if (udev->state == USB_STATE_NOTATTACHED ||
> +			udev->state == USB_STATE_SUSPENDED)
> +		return -EAGAIN;

What's to prevent the state of the device from changing right after you
check for this?

And why -EAGAIN, you don't mention that in the comment for the function.

Also, to pile on, sorry, the coding style needs to be fixed up here :)

> +
> +	/*
> +	 * offload_usage could only be modified when the device is active, since
> +	 * it will alter the suspend flow of the device.
> +	 */
> +	ret = usb_autoresume_device(udev);
> +
> +	if (ret < 0)
> +		return ret;
> +
> +	/* Drop the count when it wasn't 0, ignore the operation otherwise. */
> +	if (udev->offload_usage)
> +		udev->offload_usage--;
> +	usb_autosuspend_device(udev);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(usb_offload_put);
> +
> +/**
> + * usb_offload_check - check offload activities on a USB device
> + * @udev: the USB device to check its offload activity.
> + *
> + * Check if there are any offload activity on the USB device right now. This
> + * information could be used for power management or other forms of resource
> + * management.
> + *
> + * The caller must hold @udev's device lock.
> + *
> + * Returns true on any offload activity, false otherwise.
> + */
> +bool usb_offload_check(struct usb_device *udev)
> +{
> +	struct usb_device *child;
> +	bool active;
> +	int port1;
> +
> +	usb_hub_for_each_child(udev, port1, child) {

No locking is needed for this loop at all?  What happens if a device is
added or removed while it is looping?

> +		device_lock(&child->dev);
> +		active = usb_offload_check(child);
> +		device_unlock(&child->dev);
> +		if (active)
> +			return true;
> +	}
> +
> +	return !!udev->offload_usage;

But the state can change right afterwards, so no one can do anything
with this value, right?  What is is used for?

thanks,

greg k-h