Re: [RFC PATCH 07/14] usb: host: xhci: Add XHCI secondary interrupter support

Wesley Cheng
2023-01-11T03:12:51Z

Hi Mathias,

On 1/10/2023 12:03 PM, Wesley Cheng wrote:
> Hi Mathias,
>
> On 1/10/2023 11:47 AM, Mathias Nyman wrote:
>> On 9.1.2023 22.24, Wesley Cheng wrote:
>>> Hi Mathias,
>>>
>>> On 1/2/2023 8:38 AM, Mathias Nyman wrote:
>>>> On 29.12.2022 23.14, Wesley Cheng wrote:
>>>>> Hi Mathias,
>>>>>
>>>>> On 12/28/2022 7:47 AM, Mathias Nyman wrote:
>>>>>> On 24.12.2022 1.31, Wesley Cheng wrote:
>>>>>>> Implement the XHCI operations for allocating and requesting for a
>>>>>>> secondary
>>>>>>> interrupter.  The secondary interrupter can allow for events for a
>>>>>>> particular endpoint to be routed to a separate event ring.  The
>>>>>>> event
>>>>>>> routing is defined when submitting a transfer descriptor to the
>>>>>>> USB HW.
>>>>>>> There is a specific field which denotes which interrupter ring to
>>>>>>> route the
>>>>>>> event to when the transfer is completed.
>>>>>>>
>>>>>>> An example use case, such as audio packet offloading can utilize
>>>>>>> a separate
>>>>>>> event ring, so that these events can be routed to a different
>>>>>>> processor
>>>>>>> within the system.  The processor would be able to independently
>>>>>>> submit
>>>>>>> transfers and handle its completions without intervention from
>>>>>>> the main
>>>>>>> processor.
>>>>>>>
>>>>>>
>>>>>> Adding support for more xHCI interrupters than just the primary
>>>>>> one make sense for
>>>>>> both the offloading and virtualization cases.
>>>>>>
>>>>>> xHCI support for several interrupters was probably added to
>>>>>> support virtualization,
>>>>>> to hand over usb devices to virtual machines and give them their
>>>>>> own event ring and
>>>>>> MSI/MSI-X vector.
>>>>>>
>>>>>> In this offloading case you probably want to avoid xHC interrupts
>>>>>> from this device
>>>>>> completely, making sure it doesn't wake up the main CPU
>>>>>> unnecessarily.
>>>>>>
>>>>>> So is the idea here to let xhci driver set up the new interrupter,
>>>>>> its event ring,
>>>>>> and the endpoint transfer rings. Then pass the address of the
>>>>>> endpoint transfer rings
>>>>>> and the new event ring to the separate processor.
>>>>>>
>>>>>> This separate processor then both polls the event ring for new
>>>>>> events, sets its dequeue
>>>>>> pointer, clears EHB bit, and queues new TRBs on the transfer ring.
>>>>>>
>>>>>> so xhci driver does not handle any events for the audio part, and
>>>>>> no audio data URBs
>>>>>> are sent to usb core?
>>>>>
>>>>> Your entire description is correct.  To clarify, the interfaces
>>>>> which are non-audio will still be handled by the main processor.
>>>>> For example, a USB headset can have a HID interface as well for
>>>>> volume control.  The HID interface will still be handled by the
>>>>> main processor, and events routed to the main event ring.
>>>>>
>>>>>>
>>>>>> How about the control part?
>>>>>> Is the control endpoint for this device still handled normally by
>>>>>> usb core/xhci?
>>>>>>
>>>>>
>>>>> Control transfers are always handled on the main processor.  Only
>>>>> audio interface's endpoints.
>>>>
>>>> Good to know, that means interrupter should be chosen per endpoint,
>>>> not per device.
>>>>
>>>>>
>>>>>> For the xhci parts I think we should start start by adding generic
>>>>>> support for several
>>>>>> interrupters, then add parts needed for offloading.
>>>>>
>>>> I can split up the patchsets to add interrupters first, then adding
>>>> the offloading APIs in a separate patch.
>>>>
>>>>
>>>> I started looking at supporting secondary interrupters myself.
>>>> Let me work on that part a bit first. We have a bit different end
>>>> goals.
>>>> I want to handle interrupts from a secondary interrupter, while this
>>>> audio offload
>>>> really just wants to mask some interrupts.
>>>>
>>>
>>> I was looking at how we could possibly split up the XHCI secondary
>>> interrupter, and offloading parts.  Since the XHCI secondary
>>> interrupter is a feature that is defined in the XHCI spec (and we
>>> aren't doing anything outside of what is defined), I was thinking of
>>> having a separate XHCI driver (ie xhci-sec.c/h) that can be used to
>>> define all APIs related to setting up the event ring and ring
>>> management. (interrupt support can be added here)  This aligns a bit
>>> with what Alan suggested, and removing the APIs in the USB HCD, since
>>> this is XHCI specific stuff. (
>>> https://lore.kernel.org/linux-usb/Y6zwZOquZOTZfnvP@rowland.harvard.edu/
>>> )
>>
>> Already started working on the interrupter, that part fits well into
>> current driver.
>>
>> Code (untested, will be randomly rebased etc) can be found in my
>> feature_interrupters branch:
>> git://git.kernel.org/pub/scm/linux/kernel/git/mnyman/xhci.git
>> feature_interrupters
>> https://git.kernel.org/pub/scm/linux/kernel/git/mnyman/xhci.git/log/?h=feature_interrupters
>>
>
> Oh perfect, let me take a look.  Thanks for this!
>

I actually tried to see if I could get our audio offloading to work with
your current series. (I understand its still work in progress) I did
have to make some changes to expose the APIs to our class driver, but I
wanted to let you know about one of the issues I saw when developing my
implementation, because I am seeing the same behavior w/ yours. (and
there's a discrepancy w/ what's stated in the XHCI spec :))

So the reason why my initial submission did the event ring allocation
and set up before the run/stop bit was set, is that I found that when
writing to the ir_set->erst_base in this scenario (for the secondary
interrupter), it lead to a SMMU fault from the DWC3 controller. One
thing I noticed, was that the SMMU fault address was the lower 32 bits
of the segment table base address allocated. The XHCI driver utilizes
the xhci_write_64() api which first writes the lower 32 bits then the
upper 32 bits. The XHCI spec states that:

Table 5-41: Event Ring Segment Table Base Address Register Bit
Definitions (ERSTBA)

"Event Ring Segment Table Base Address Register – RW. Default = ‘0’.
This field defines the
high order bits of the start address of the Event Ring Segment Table.
Writing this register sets the Event Ring State Machine:EREP Advancement
to the Start state.
Refer to Figure 4-12 for more information.
**For Secondary Interrupters: This field may be modified at any time.**"

I'm not sure if this is an issue with the specific controller we're
using, so maybe I will wait until you can give this a try on your set
up. However, it doesn't seem to be true that we can write the ERSTBA
any time we want to. My assumption is that once I made the lower 32 bit
write, the controller attempted to enable the Event Ring State machine
(Figure 4-12), and this led to a SMMU fault, since the upper 64 bits
haven't been written. I also did some bit banging manually as well
(using devmem) and any time I write to the secondary ring ERSTBA
register it generates a fault. (before any offloading has started)

Thanks
Wesley Cheng