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
