Hi Amadeusz,
On 8/6/2024 7:49 AM, Amadeusz Sławiński wrote:
> On 8/1/2024 3:16 AM, Wesley Cheng wrote:
>> From: Mathias Nyman <mathias.nyman@xxxxxxxxxxxxxxx>
>>
>> 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@xxxxxxxxxxxxxxx>
>> Co-developed-by: Wesley Cheng <quic_wcheng@xxxxxxxxxxx>
>> Signed-off-by: Wesley Cheng <quic_wcheng@xxxxxxxxxxx>
>> ---
>> 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
Sorry for the late reply. Been going through addressing the other comments. Will fix.
>
>
>> + 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.
Same.
>
> (...)
>
>> +/**
>> + * 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?
>
I'll add the needed checks across all the APIs you've pointed out. There wasn't a specific reason for leaving this check out.
>> + 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
>
Fixed.
>> +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@xxxxxxxxxxxxxxx>
>> + */
>> +
>> +#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
Ack.
Thanks
Wesley Cheng
>
> (...)
