Re: [PATCH v24 03/34] xhci: sideband: add initial api to register a sideband entity

["Amadeusz Sławiński" <amadeuszx.slawinski@linux.intel.com>]

On 8/1/2024 3:16 AM, Wesley Cheng wrote:
> From: Mathias Nyman <mathias.nyman@linux.intel.com>
> 
> Introduce XHCI sideband, which manages the USB endpoints being requested by
> a client driver.  This is used for when client drivers are attempting to
> offload USB endpoints to another entity for handling USB transfers.  XHCI
> sideband will allow for drivers to fetch the required information about the
> transfer ring, so the user can submit transfers independently.  Expose the
> required APIs for drivers to register and request for a USB endpoint and to
> manage XHCI secondary interrupters.
> 
> Multiple ring segment page linking, proper endpoint clean up, and allowing
> module compliation added by Wesley Cheng to complete original concept code
> by Mathias Nyman.
> 
> Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
> Co-developed-by: Wesley Cheng <quic_wcheng@quicinc.com>
> Signed-off-by: Wesley Cheng <quic_wcheng@quicinc.com>
> ---
>   drivers/usb/host/Kconfig          |   9 +
>   drivers/usb/host/Makefile         |   2 +
>   drivers/usb/host/xhci-sideband.c  | 419 ++++++++++++++++++++++++++++++
>   drivers/usb/host/xhci.h           |   4 +
>   include/linux/usb/xhci-sideband.h |  68 +++++
>   5 files changed, 502 insertions(+)
>   create mode 100644 drivers/usb/host/xhci-sideband.c
>   create mode 100644 include/linux/usb/xhci-sideband.h
> 
> diff --git a/drivers/usb/host/Kconfig b/drivers/usb/host/Kconfig
> index 4448d0ab06f0..6135603c5dc4 100644
> --- a/drivers/usb/host/Kconfig
> +++ b/drivers/usb/host/Kconfig
> @@ -104,6 +104,15 @@ config USB_XHCI_RZV2M
>   	  Say 'Y' to enable the support for the xHCI host controller
>   	  found in Renesas RZ/V2M SoC.
>   
> +config USB_XHCI_SIDEBAND
> +	tristate "xHCI support for sideband"
> +	help
> +	  Say 'Y' to enable the support for the xHCI sideband capability.
> +	  provide a mechanism for a sideband datapath for payload associated

Sentence should start from capital letter, so provide -> Provide

> +	  with audio class endpoints. This allows for an audio DSP to use
> +	  xHCI USB endpoints directly, allowing CPU to sleep while playing
> +	  audio

Missing '.' at the end of sentence.

(...)

> +/**
> + * xhci_sideband_remove_endpoint - remove endpoint from sideband access list
> + * @sb: sideband instance for this usb device
> + * @host_ep: usb host endpoint
> + *
> + * Removes an endpoint from the list of sideband accessed endpoints for this usb
> + * device.
> + * sideband client should no longer touch the endpoint transfer buffer after
> + * calling this.
> + *
> + * Return: 0 on success, negative error otherwise.
> + */
> +int
> +xhci_sideband_remove_endpoint(struct xhci_sideband *sb,
> +			      struct usb_host_endpoint *host_ep)
> +{
> +	struct xhci_virt_ep *ep;
> +	unsigned int ep_index;
> +
> +	mutex_lock(&sb->mutex);
> +	ep_index = xhci_get_endpoint_index(&host_ep->desc);
> +	ep = sb->eps[ep_index];
> +
> +	if (!ep || !ep->sideband) {
> +		mutex_unlock(&sb->mutex);
> +		return -ENODEV;
> +	}
> +
> +	__xhci_sideband_remove_endpoint(sb, ep);
> +	xhci_initialize_ring_info(ep->ring, 1);
> +	mutex_unlock(&sb->mutex);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(xhci_sideband_remove_endpoint);
> +
> +int
> +xhci_sideband_stop_endpoint(struct xhci_sideband *sb,
> +			    struct usb_host_endpoint *host_ep)
> +{
> +	struct xhci_virt_ep *ep;
> +	unsigned int ep_index;
> +
> +	ep_index = xhci_get_endpoint_index(&host_ep->desc);
> +	ep = sb->eps[ep_index];
> +
> +	if (!ep || ep->sideband != sb)

Any reason why we check if ep->sideband != sb only on stop but not on 
remove above?

> +		return -EINVAL;
> +
> +	return xhci_stop_endpoint_sync(sb->xhci, ep, 0, GFP_KERNEL);
> +}
> +EXPORT_SYMBOL_GPL(xhci_sideband_stop_endpoint);
> +
> +/**
> + * xhci_sideband_get_endpoint_buffer - gets the endpoint transfer buffer address
> + * @sb: sideband instance for this usb device
> + * @host_ep: usb host endpoint
> + *
> + * Returns the address of the endpoint buffer where xHC controller reads queued
> + * transfer TRBs from. This is the starting address of the ringbuffer where the
> + * sideband client should write TRBs to.
> + *
> + * Caller needs to free the returned sg_table
> + *
> + * Return: struct sg_table * if successful. NULL otherwise.
> + */
> +struct sg_table *
> +xhci_sideband_get_endpoint_buffer(struct xhci_sideband *sb,
> +			      struct usb_host_endpoint *host_ep)
> +{
> +	struct xhci_virt_ep *ep;
> +	unsigned int ep_index;
> +
> +	ep_index = xhci_get_endpoint_index(&host_ep->desc);
> +	ep = sb->eps[ep_index];
> +
> +	if (!ep)

And here there is none of checks done in above 2 functions? Seems bit weird.

> +		return NULL;
> +
> +	return xhci_ring_to_sgtable(sb, ep->ring);
> +}
> +EXPORT_SYMBOL_GPL(xhci_sideband_get_endpoint_buffer);
> +

(...)

> +MODULE_DESCRIPTION("XHCI sideband driver for secondary interrupter management");

XHCI -> xHCI

> +MODULE_LICENSE("GPL");
> diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
> index efbd1f651da4..9232c53d204a 100644
> --- a/drivers/usb/host/xhci.h
> +++ b/drivers/usb/host/xhci.h
> @@ -693,6 +693,8 @@ struct xhci_virt_ep {
>   	int			next_frame_id;
>   	/* Use new Isoch TRB layout needed for extended TBC support */
>   	bool			use_extended_tbc;
> +	/* set if this endpoint is controlled via sideband access*/
> +	struct xhci_sideband			*sideband;
>   };
>   
>   enum xhci_overhead_type {
> @@ -755,6 +757,8 @@ struct xhci_virt_device {
>   	u16				current_mel;
>   	/* Used for the debugfs interfaces. */
>   	void				*debugfs_private;
> +	/* set if this device is registered for sideband access */
> +	struct xhci_sideband			*sideband;
>   };
>   
>   /*
> diff --git a/include/linux/usb/xhci-sideband.h b/include/linux/usb/xhci-sideband.h
> new file mode 100644
> index 000000000000..1035dae43cee
> --- /dev/null
> +++ b/include/linux/usb/xhci-sideband.h
> @@ -0,0 +1,68 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * xHCI host controller sideband support
> + *
> + * Copyright (c) 2023, Intel Corporation.
> + *
> + * Author: Mathias Nyman <mathias.nyman@linux.intel.com>
> + */
> +
> +#ifndef __LINUX_XHCI_SIDEBAND_H
> +#define __LINUX_XHCI_SIDEBAND_H
> +
> +#include <linux/scatterlist.h>
> +#include <linux/usb.h>
> +
> +#define	EP_CTX_PER_DEV		31	/* FIMXME defined twice, from xhci.h */

If it is left for later, FIMXME -> FIXME

(...)