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

Wesley Cheng
2023-01-13T00:35:49Z

Hi Mathias,

On 1/12/2023 1:24 AM, Mathias Nyman wrote:
> On 11.1.2023 5.11, Wesley Cheng wrote:
>> 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)
>
> Tried on an Intel host and it seems to work fine.
> I created a few secondary interrupters while xHC was running without
> issues.
> DMA mask is 64 bits.
> Only created the interrupters, no events on those new event rings, and
> didn't actually
> check that the values written to ERSTBA were 64 bit.
>
> Does temporarily setting DMA mask to 32 bits while allocating erst help
> in your case?
>

Yes, that works fine. Another thing I found which works is that, the
XHCI spec mentions that we should always write the lower 32 bits first
before the upper bits for a 64 bit register. Just as an experiment, I
flipped the write order (upper first, lower second), and that seemed to
not trigger the event ring state machine, and I was able to verify that
our audio offloading was working.

At the moment, I exposed/exported the
xhci_remove_secondary_interrupter() and
xhci_create_secondary_interrupter() into a header file
(include/linux/usb/xhci-intr.h), along with some other APIs to fetch the
transfer resources. (same mechanism I have in my patchset) This allowed
me to only reference the required APIs from the USB audio offload class
driver w/o having to include all of xhci.h (where some of the things are
currently defined in your changes)

Thanks
Wesley Cheng