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

[Wesley Cheng <quic_wcheng@quicinc.com>]

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