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/14/2026 7:26 AM, Mathias Nyman wrote:
> On 10/13/26 12:11, Michal Pecio wrote:
>> On Mon, 12 Oct 2026 18:01:50 -0700, Wesley Cheng wrote:
>>> On 10/5/2026 4:03 AM, Mathias Nyman wrote:
>>>> 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().
> 
> You are right, audio class driver .disconnect is called, driver unbound
> and interface device gone before xhci_free_dev() is called.
> 
> If sideband is unregistered as part of .disconnect callback it also means
> ring/pool would be destroyed before xHC has dropped the endpoint/disabled
> the slot. Would be better to free the ring after we know xHC controller
> won't access it.
> 
>>>
>>> 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?
> 
> sb->notify_client callback has the risk that this sideband entity is
> unregistered and gone by the time we want to call it
> 
> Deferring freeing the DMA poolby some kind of reference counting again adds
> complexity, and maybe not a suitable solution for this issue.
> (taking a reference at sideband register and every dma_pool_zalloc(), putting
> when freeing or sideband unregister, finally freeing pool when reaching zero.)
> 
> I'm trying to figure out a simpler solution.
> 
> How about skipping dma pools completely for endpoints in these cases?
> (page size over 4k with offloaded endpoints)
> 
> Instead of sideband client creating a custom dma pool it could just set a
> 'no dma pool' flag for the endpoint. xhci driver would allocate the
> ring segments using dma_alloc_coherent() instead of dma_pool_zalloc() for
> these, possibly even allocate a full page for each segment?
> Freeing would be done similarly.
> 
> Would this be a viable simple solution?
> 

I think so, this was something similar I proposed on v0:
https://lore.kernel.org/all/20260824-16k_offload_v1_b4-v1-0-49a6be60ca30@oss.qualcomm.com/

In that case, the client offload driver wasn't responsible for freeing 
anything and that was all taken care of by xHCI core.  The sideband just 
set the alignment_req.  Might need to make some adjustments after the 
further discussions we've had.

Thanks
Wesley Cheng

>>
>> I'm not Mathias but my opinion is the same as always: none of these
>> problems would exist if the sideband client allocated and freed all
>> transfer/event ring memory, and xhci-sideband only owned a mapping.
>>
>> The mapping would be freed by xhci_sideband_unregister() or similar
>> calls, which should ensure that the xHC stops using the rings, and
>> memory would only be freed by the client driver after unregistering
>> the sideband and stopping the coprocessor. Nothing is ever UAF or
>> leaked, and ensuring this is *easy*. And if xhci-hcd randomly wants
>> to get rid of some endpoint, it simply stops using the mapping and
>> lets the sideband client clean it up two milliseconds later.
>>
>> One thing I'm not entirely sure about is how we could create this
>> mapping. Using bare iommu_map() creates a hard dependency on IOMMU.
>> Documentation/core-api/dma-attributes.rst mentions possibility of
>> using generic dma_map_() helpers on multiple devices, so maybe this
>> can work, but I haven't seen existing examples of such use.
>>
> Could be an option, but maybe not for this specific issue.
> It's a larger, complex change without any real need (yet).
> 
> I'm hoping vendors would move away from this sideband implementation
> instead of evolving it, adapting the 'xHCI Audio Sideband Capability'
> defined in xHCI  specification section 7.9 instead.
> 
> Thanks
> Mathias