Re: [PATCH v4 3/5] usb: xhci: sideband: allocate sideband ring segments from a dedicated pool

[Wesley Cheng <wesley.cheng@oss.qualcomm.com>]

On 10/5/2026 4:03 AM, Mathias Nyman wrote:
> On 9/10/26 04:50, Wesley Cheng wrote:
>> A sideband client that maps a ring buffer directly via the IOMMU
>> (which operates at page granularity) needs to know exactly which
>> page(s) back the ring, and only pages that are actually intended to
>> be exposed to that client should ever be mapped this way.
>>
>> Allow each xhci_sideband endpoint to pass its own segment_pool, allocated
>> separately from the core xhci->segment_pool, so every segment backing
>> a sideband-tagged endpoint always comes from a page that is meant to
>> be visible by the entity handling the offloaded endpoints. Normal
>> (non-offloaded) endpoints are unaffected, as they keep allocating from
>> xhci->segment_pool.
>>
>> The offload client owns the pool's full lifetime, and since
>> that lifetime is no longer tied to the sideband instance itself,
>> xhci_sideband_unregister() must free any ring still backed by a
>> client-supplied pool before returning, rather than leaving it for xhci
>> to free later when the client and its pool may already be gone.
> 
> xhci is the one allocating the ring when adding an endpoint, and telling
> xHC hardware to write to that ring. It seems more approriate that xhci should
> be the one freeing the ring after it told xHC hardware to drop the endpoint,
> and there no longer is a risk on hardware accessing it.
> 

Hi Mathias,

Sorry for the late response.  Was out last week.

Specifically in the disconnect path (while audio offload is active), when 
we utilize the usb_set_interface() path, it'll return early due to the 
iface->unregistering check.  This would lead to the drop_endpoint not to be 
called, and rely on the xhci_free_dev() to handle the transfer ring 
cleanup.  This wouldn't work in the offload situation, because 
qc_audio_offload is the only entity managing the DMA pool, and that would 
be freed during the .disconnect callback, which happens BEFORE xhci_free_dev().

Would you be more inclined to prefer a solution where we can defer the DMA 
pool free'ing to the xHCI core during xhci_free_dev, or maybe adding 
another sideband notifier (sb->notify_client) enum to tell the client 
driver that it can release the pool?

> usb audio (class) driver controls both when and endpoint is added or dropped
> by setting configurations and interfaces, and also the one registering and
> unregistering sideband, and adding and removing endpoint to/from sideband use.
> 
> I think usb audio driver should ensure endpoints are dropped before 
> unregistering
> sideband.
> 
>>
>> Assisted-by: Claude:claude-sonnet-5
>> Signed-off-by: Wesley Cheng <wesley.cheng@oss.qualcomm.com>
>> ---
>>   drivers/usb/host/xhci-sideband.c  | 38 ++++++++++++++++++++++++++++++++ 
>> +++---
>>   drivers/usb/host/xhci.h           | 10 +++-------
>>   include/linux/usb/xhci-sideband.h | 20 +++++++++++++++++---
>>   sound/usb/qcom/qc_audio_offload.c | 32 ++++++++++++++++++++++++++++----
>>   4 files changed, 83 insertions(+), 17 deletions(-)
>>
>> diff --git a/drivers/usb/host/xhci-sideband.c b/drivers/usb/host/xhci- 
>> sideband.c
>> index 274da38f333d..1bb6e5034b58 100644
>> --- a/drivers/usb/host/xhci-sideband.c
>> +++ b/drivers/usb/host/xhci-sideband.c
>> @@ -9,6 +9,7 @@
>>    */
>>   #include <linux/usb/xhci-sideband.h>
>> +#include <linux/dmapool.h>
>>   #include "xhci.h"
>> @@ -67,6 +68,7 @@ __xhci_sideband_remove_endpoint(struct xhci_sideband 
>> *sb, struct xhci_virt_ep *e
>>       xhci_stop_endpoint_sync(sb->xhci, ep, 0, GFP_KERNEL);
>>       ep->sideband = NULL;
>> +    ep->priv_seg_pool = NULL;
>>       sb->eps[ep->ep_index] = NULL;
>>   }
>> @@ -113,6 +115,8 @@ EXPORT_SYMBOL_GPL(xhci_sideband_notify_ep_ring_free);
>>    * xhci_sideband_add_endpoint - add endpoint to sideband access list
>>    * @sb: sideband instance for this usb device
>>    * @host_ep: usb host endpoint
>> + * @pool: dma pool to allocate this endpoint's ring segments from, or NULL
>> + *        to leave the endpoint's current pool selection untouched
> 
> Add some comment about if pool is set then xhci_sideband_add_endpoint() needs
> to be called before setting configuration or interface that enables as
> allocated the endpoint.
> 
> Also add note that this is not suitable for ep0 as ring is allocated 
> immedialely
> when device is created.
> 

Will do.

> 
>>    *
>>    * Adds an endpoint to the list of sideband accessed endpoints for this 
>> usb
>>    * device.
>> @@ -123,7 +127,8 @@ EXPORT_SYMBOL_GPL(xhci_sideband_notify_ep_ring_free);
>>    */
>>   int
>>   xhci_sideband_add_endpoint(struct xhci_sideband *sb,
>> -               struct usb_host_endpoint *host_ep)
>> +               struct usb_host_endpoint *host_ep,
>> +               struct dma_pool *pool)
>>   {
>>       struct xhci_virt_ep *ep;
>>       unsigned int ep_index;
>> @@ -153,6 +158,9 @@ xhci_sideband_add_endpoint(struct xhci_sideband *sb,
>>       ep->sideband = sb;
>>       sb->eps[ep_index] = ep;
>> +    if (pool)
>> +        ep->priv_seg_pool = pool;
>> +
> 
> Fail here ring is already allocated from another pool:
> 
> if (pool) {
>      if (ep->ring && ep->ring->segment_pool != pool)
>          return -SOME_SUITABLE_ERROR;
>      ep->priv_seg_pool = pool
> }
> 

Got it.

> 
> 
> 
>>       return 0;
>>   }
>>   EXPORT_SYMBOL_GPL(xhci_sideband_add_endpoint);
>> @@ -288,6 +296,7 @@ EXPORT_SYMBOL_GPL(xhci_sideband_check);
>>    * xhci_sideband_create_interrupter - creates a new interrupter for 
>> this sideband
>>    * @sb: sideband instance for this usb device
>>    * @num_seg: number of event ring segments to allocate
>> + * @pool: dma pool to allocate the interrupter's event ring segments from
>>    * @ip_autoclear: IP autoclearing support such as MSI implemented
>>    *
>>    * Sets up a xhci interrupter that can be used for this sideband 
>> accessed usb
>> @@ -301,7 +310,8 @@ EXPORT_SYMBOL_GPL(xhci_sideband_check);
>>    */
>>   int
>>   xhci_sideband_create_interrupter(struct xhci_sideband *sb, int num_seg,
>> -                 bool ip_autoclear, u32 imod_interval, int intr_num)
>> +                 struct dma_pool *pool, bool ip_autoclear,
>> +                 u32 imod_interval, int intr_num)
>>   {
>>       if (!sb || !sb->xhci)
>>           return -ENODEV;
>> @@ -315,7 +325,7 @@ xhci_sideband_create_interrupter(struct xhci_sideband 
>> *sb, int num_seg,
>>           return -EBUSY;
>>       sb->ir = xhci_create_secondary_interrupter(xhci_to_hcd(sb->xhci),
>> -                           num_seg, NULL,
>> +                           num_seg, pool,
>>                              imod_interval, intr_num);
>>       if (!sb->ir)
>>           return -ENOMEM;
>> @@ -370,6 +380,8 @@ EXPORT_SYMBOL_GPL(xhci_sideband_interrupter_id);
>>   /**
>>    * xhci_sideband_register - register a sideband for a usb device
>>    * @intf: usb interface associated with the sideband device
>> + * @type: xHCI sideband type
>> + * @notify_client: callback for xHCI sideband sequences
>>    *
>>    * Allows for clients to utilize XHCI interrupters and fetch transfer 
>> and event
>>    * ring parameters for executing data transfers.
>> @@ -436,6 +448,15 @@ EXPORT_SYMBOL_GPL(xhci_sideband_register);
>>    * After this the endpoint and interrupter event buffers should no longer
>>    * be accessed via sideband. The xhci driver can now take over handling
>>    * the buffers.
>> + * Any transfer ring allocated from a client supplied dma pool is freed 
>> here
>> + * as well, as the client is not expected to keep that pool alive any 
>> longer
>> + * than this call. This includes rings of endpoints already removed with
>> + * xhci_sideband_remove_endpoint(), which xhci would otherwise only free 
>> once
>> + * the device is reconfigured or torn down, i.e. after the client is gone.
>> + *
>> + * The caller must ensure the usb device is no longer streaming through the
>> + * normal, non-sideband path when calling this, as the freed rings are 
>> still
>> + * referenced by the endpoint contexts until xhci reconfigures the device.
>>    */
>>   void
>>   xhci_sideband_unregister(struct xhci_sideband *sb)
>> @@ -458,6 +479,17 @@ xhci_sideband_unregister(struct xhci_sideband *sb)
>>               if (sb->eps[i])
>>                   __xhci_sideband_remove_endpoint(sb, sb->eps[i]);
>> +        spin_lock_irq(&xhci->lock);
>> +        for (i = 0; i < EP_CTX_PER_DEV; i++) {
>> +            struct xhci_ring *ring = vdev->eps[i].ring;
>> +
>> +            if (ring && ring->segment_pool != xhci->segment_pool) {
>> +                xhci_ring_free(xhci, ring);
>> +                vdev->eps[i].ring = NULL;
> 
> 
> Sideband should not make decisions for all endpoints of a device based on the
> pool memory is _not_ allocated from.
> 
> Endpoint may not even belong to audio interface, for example can be part if 
> HID.
> 
> Can't assume that just because ring isn't allocatd from xhci->segment_pool 
> that
> it means it belongs to sideband. It's true today, but dedicated pools may 
> be used
> for VTIO/virtualization in future.
>

Make sense.  If we end up keeping this change (based on the first comment), 
I'll rework it to only apply this to rings owned by the sideband entity 
being unregistered.

> Maybe add some warning to xhci_unregister_sideband() if an endpoint assined
> to sideband  still has a ring allocated before calling
> __xhci_sideband_remove_endpoint(sb, sb->eps[i]);
> 

Sure.

>> +            }
>> +        }
>> +        spin_unlock_irq(&xhci->lock);
>> +
>>           __xhci_sideband_remove_interrupter(sb);
>>           sb->vdev = NULL;
>> diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
>> index 15ce0bb7aa3f..1353d6fa2776 100644
>> --- a/drivers/usb/host/xhci.h
>> +++ b/drivers/usb/host/xhci.h
>> @@ -19,6 +19,7 @@
>>   #include <linux/usb/hcd.h>
>>   #include <linux/io-64-nonatomic-lo-hi.h>
>>   #include <linux/io-64-nonatomic-hi-lo.h>
>> +#include <linux/usb/xhci-sideband.h>
>>   /* Code sharing between pci-quirks and xhci hcd */
>>   #include    "xhci-ext-caps.h"
>> @@ -740,8 +741,6 @@ struct xhci_interval_bw_table {
>>       unsigned int        ss_bw_out;
>>   };
>> -#define EP_CTX_PER_DEV        31
>> -
>>   struct xhci_virt_device {
>>       int                slot_id;
>>       struct usb_device        *udev;
>> @@ -1255,14 +1254,11 @@ static inline const char *xhci_trb_type_string(u8 
>> type)
>>   #define NEC_FW_MAJOR(p)        (((p) >> 8) & 0xff)
>>   /*
>> - * TRBS_PER_SEGMENT must be a multiple of 4,
>> - * since the command ring is 64-byte aligned.
>> - * It must also be greater than 16.
>> + * TRBS_PER_SEGMENT and TRB_SEGMENT_SIZE are defined in
>> + * <linux/usb/xhci-sideband.h>, shared with sideband client drivers.
> 
> This seems backwards.
> 
> Maybe we should just add ring_seg_size and xHC device pointer to struct 
> xhci__sideband
> 
> Or optionally helper(s) that provides those

Yeah...I think you mentioned you wanted to explicitly wanted to keep 
sideband out of the core.  Will look at a better way to address it.

> 
>>    */
>> -#define TRBS_PER_SEGMENT    256
>>   /* Allow two commands + a link TRB, along with any reserved command 
>> TRBs */
>>   #define MAX_RSVD_CMD_TRBS    (TRBS_PER_SEGMENT - 3)
>> -#define TRB_SEGMENT_SIZE    (TRBS_PER_SEGMENT*16)
>>   #define TRB_SEGMENT_SHIFT    (ilog2(TRB_SEGMENT_SIZE))
>>   /* TRB buffer pointers can't cross 64KB boundaries */
>>   #define TRB_MAX_BUFF_SHIFT        16
>> diff --git a/include/linux/usb/xhci-sideband.h b/include/linux/usb/xhci- 
>> sideband.h
>> index 005257085dcb..6d318e6a3bf6 100644
>> --- a/include/linux/usb/xhci-sideband.h
>> +++ b/include/linux/usb/xhci-sideband.h
>> @@ -13,7 +13,19 @@
>>   #include <linux/usb.h>
>>   #include <linux/usb/hcd.h>
>> -#define    EP_CTX_PER_DEV        31    /* FIXME defined twice, from 
>> xhci.h */
>> +/*
>> + * Constants shared with the xHCI host driver (drivers/usb/host/xhci.h),
>> + * which includes this header for its canonical definitions.
>> + */
>> +#define EP_CTX_PER_DEV      31
>> +
>> +/*
>> + * TRBS_PER_SEGMENT must be a multiple of 4,
>> + * since the command ring is 64-byte aligned.
>> + * It must also be greater than 16.
>> + */
>> +#define TRBS_PER_SEGMENT    256
>> +#define TRB_SEGMENT_SIZE    (TRBS_PER_SEGMENT * 16)
>>   struct xhci_sideband;
>> @@ -72,7 +84,8 @@ void
>>   xhci_sideband_unregister(struct xhci_sideband *sb);
>>   int
>>   xhci_sideband_add_endpoint(struct xhci_sideband *sb,
>> -               struct usb_host_endpoint *host_ep);
>> +               struct usb_host_endpoint *host_ep,
>> +               struct dma_pool *pool);
>>   int
>>   xhci_sideband_remove_endpoint(struct xhci_sideband *sb,
>>                     struct usb_host_endpoint *host_ep);
>> @@ -94,7 +107,8 @@ static inline bool xhci_sideband_check(struct usb_hcd 
>> *hcd)
>>   int
>>   xhci_sideband_create_interrupter(struct xhci_sideband *sb, int num_seg,
>> -                 bool ip_autoclear, u32 imod_interval, int intr_num);
>> +                 struct dma_pool *pool, bool ip_autoclear,
>> +                 u32 imod_interval, int intr_num);
>>   void
>>   xhci_sideband_remove_interrupter(struct xhci_sideband *sb);
>>   int
>> diff --git a/sound/usb/qcom/qc_audio_offload.c b/sound/usb/qcom/ 
>> qc_audio_offload.c
>> index e4bfd43a2488..c94b15423a9a 100644
>> --- a/sound/usb/qcom/qc_audio_offload.c
>> +++ b/sound/usb/qcom/qc_audio_offload.c
>> @@ -7,6 +7,7 @@
>>   #include <linux/ctype.h>
>>   #include <linux/dma-mapping.h>
>>   #include <linux/dma-map-ops.h>
>> +#include <linux/dmapool.h>
>>   #include <linux/init.h>
>>   #include <linux/iommu.h>
>>   #include <linux/module.h>
>> @@ -131,6 +132,7 @@ struct uaudio_dev {
>>       /* xhci sideband */
>>       struct xhci_sideband *sb;
>> +    struct dma_pool *segment_pool;
>>       /* SoC USB device */
>>       struct snd_soc_usb_device *sdev;
>> @@ -1140,7 +1142,8 @@ uaudio_endpoint_setup(struct snd_usb_substream *subs,
>>       memcpy(ep_desc, &ep->desc, sizeof(ep->desc));
>> -    ret = xhci_sideband_add_endpoint(uadev[card_num].sb, ep);
>> +    ret = xhci_sideband_add_endpoint(uadev[card_num].sb, ep,
>> +                     uadev[card_num].segment_pool);
>>       if (ret < 0) {
>>           dev_err(&subs->dev->dev,
>>               "failed to add data ep to sec intr: %d\n", ret);
>> @@ -1211,8 +1214,9 @@ static int uaudio_event_ring_setup(struct 
>> snd_usb_substream *subs,
>>           goto exit;
>>       /* event ring */
>> -    ret = xhci_sideband_create_interrupter(uadev[card_num].sb, 1, false,
>> -                           0, uaudio_qdev->data->intr_num);
>> +    ret = xhci_sideband_create_interrupter(uadev[card_num].sb, 1,
>> +                           uadev[card_num].segment_pool,
>> +                           false, 0, uaudio_qdev->data->intr_num);
>>       if (ret < 0) {
>>           dev_err(&subs->dev->dev, "failed to fetch interrupter\n");
>>           goto put_offload;
>> @@ -1786,6 +1790,7 @@ static void qc_usb_audio_offload_probe(struct 
>> snd_usb_audio *chip)
>>       struct usb_interface_descriptor *altsd;
>>       struct usb_host_interface *alts;
>>       struct snd_soc_usb_device *sdev;
>> +    struct dma_pool *segment_pool;
>>       struct xhci_sideband *sb;
>>       /*
>> @@ -1804,10 +1809,21 @@ static void qc_usb_audio_offload_probe(struct 
>> snd_usb_audio *chip)
>>           if (!sdev)
>>               return;
>> +        segment_pool = dma_pool_create("xHCI sideband ring segments",
>> +                           interface_to_usbdev(intf)->bus->sysdev,
>> +                           TRB_SEGMENT_SIZE, TRB_SEGMENT_SIZE,
>> +                           TRB_SEGMENT_SIZE);
>> +        if (!segment_pool)
>> +            goto free_sdev;
>> +
>>           sb = xhci_sideband_register(intf, XHCI_SIDEBAND_VENDOR,
>>                           uaudio_sideband_notifier);
>> -        if (!sb)
>> +        if (!sb) {
>> +            dma_pool_destroy(segment_pool);
>>               goto free_sdev;
>> +        }
>> +
>> +        uadev[chip->card->number].segment_pool = segment_pool;
> 
> If struct xhci_sideband had dev pointer and segment size then we would just 
> swap the order:
> 
> 
> sb = xhci_sideband_register(intf, XHCI_SIDEBAND_VENDOR,
>                  uaudio_sideband_notifier);
> 
> 
> segment_pool = dma_pool_create("xHCI sideband ring segments",
>                  sb->hcdev,
>                  sb->ring_seg_size, sb->ring_seg_size,
>                  sb->ring_seg_size);
> 
> segment_pool isn't needed yet when resistering the sideband.
>
This could be a good way.  Let me take a look and get back to you.

Thanks
Wesley Cheng