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

[Wesley Cheng <quic_wcheng@xxxxxxxxxxx>]

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

>
> (...)