Re: [PATCH v30 01/30] usb: host: xhci: Repurpose event handler for skipping interrupter events

[Wesley Cheng <quic_wcheng@xxxxxxxxxxx>]

Hi Mathias,

On 11/20/2024 3:48 AM, Mathias Nyman wrote:
> On 6.11.2024 21.33, Wesley Cheng wrote:
>> Depending on the interrupter use case, the OS may only be used to handle
>> the interrupter event ring clean up.  In these scenarios, event TRBs don't
>> need to be handled by the OS, so introduce an xhci interrupter flag to tag
>> if the events from an interrupter needs to be handled or not.
>>
>> Signed-off-by: Wesley Cheng <quic_wcheng@xxxxxxxxxxx>
>> ---
>>   drivers/usb/host/xhci-ring.c | 17 +++++++++++++----
>>   drivers/usb/host/xhci.h      |  1 +
>>   2 files changed, 14 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
>> index 9f1e150a1c76..b8f6983b7369 100644
>> --- a/drivers/usb/host/xhci-ring.c
>> +++ b/drivers/usb/host/xhci-ring.c
>> @@ -2931,14 +2931,22 @@ static int handle_tx_event(struct xhci_hcd *xhci,
>>   }
>>     /*
>> - * This function handles one OS-owned event on the event ring. It may drop
>> - * xhci->lock between event processing (e.g. to pass up port status changes).
>> + * This function handles one OS-owned event on the event ring, or ignores one event
>> + * on interrupters which are non-OS owned. It may drop xhci->lock between event
>> + * processing (e.g. to pass up port status changes).
>>    */
>>   static int xhci_handle_event_trb(struct xhci_hcd *xhci, struct xhci_interrupter *ir,
>>                    union xhci_trb *event)
>>   {
>>       u32 trb_type;
>>   +    /*
>> +     * Some interrupters do not need to handle event TRBs, as they may be
>> +     * managed by another entity, but rely on the OS to clean up.
>> +     */
>> +    if (ir->skip_events)
>> +        return 0;
>
> This works for your special case but is a small step sideways from other possible xhci
> secondary interrupter usecases.
>
> We currently support just one event handler function even if we support several secondary
> interrupters. Idea was to add support to pass dedicated handlers for each secondary interrupter,
> set when the secondary interrupter is requested.
>
> In your case this dedicated handler wouldn't do anything.
>
> This patch again has a different approach, it keeps the default handler, and instead adds
> flags to it, preventing it from handling the event trb.
>
> Not sure if we should take the time and implement dedicated handlers now, even if we don't
> have any real users yet, or just take this quick change and rework it later when needed.
>
>
Yes, I think we had a small discussion on this on v20:

https://lore.kernel.org/linux-usb/a88b41f4-7e53-e162-5a6a-2d470e29c0bb@xxxxxxxxxxx/

Since I didn't have an environment that exercised the path where we'd actually want to handle secondary interrupter events, I wasn't sure if it was valid to add bits and pieces of it to support such use cases w/o proper testing.  I think having this driver (as is) is still a step forward into the right direction, as these APIs are still going to be required if enabling secondary interrupter events in the Linux environment.

Thanks

Wesley Cheng