diff mbox series

[v20,03/41] usb: host: xhci: Repurpose event handler for skipping interrupter events

Message ID 20240425215125.29761-4-quic_wcheng@quicinc.com
State Superseded
Headers show
Series Introduce QC USB SND audio offloading support | expand

Commit Message

Wesley Cheng April 25, 2024, 9:50 p.m. UTC
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@quicinc.com>
---
 drivers/usb/host/xhci-ring.c | 17 +++++++++++++----
 drivers/usb/host/xhci.h      |  1 +
 2 files changed, 14 insertions(+), 4 deletions(-)

Comments

Mathias Nyman April 30, 2024, 11:02 a.m. UTC | #1
On 26.4.2024 0.50, 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.

Could you elaborate on this a bit.

If I understood correctly the whole point of requesting a secondary xhci interrupter
for the sideband device without ever requesting a real interrupt for it was to avoid
waking up the cpu and calling the interrupt handler.

with this flag is seems the normal xhci interrupt handler does get called for
sideband transfer events.

> 
> Signed-off-by: Wesley Cheng <quic_wcheng@quicinc.com>
> ---
>   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 52278afea94b..6c7a21f522cd 100644
> --- a/drivers/usb/host/xhci-ring.c
> +++ b/drivers/usb/host/xhci-ring.c
> @@ -2973,14 +2973,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;
> +

I think we need another solution than a skip_events flag.

To make secondary xhci interrupters more useful in general it would make more
sense to add an interrupt handler function pointer to struct xhci_interrupter.

Then call that function instead of xhci_handle_event_trb()

--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -3098,8 +3098,8 @@ static int xhci_handle_events(struct xhci_hcd *xhci, struct xhci_interrupter *ir
  
         /* Process all OS owned event TRBs on this event ring */
         while (unhandled_event_trb(ir->event_ring)) {
-               err = xhci_handle_event_trb(xhci, ir, ir->event_ring->dequeue);
-
+               if (ir->handle_event_trb)
+                       err = ir->handle_event_trb(xhci, ir, ir->event_ring->dequeue);
                 /*
                  * If half a segment of events have been handled in one go then
                  * update ERDP, and force isoc trbs to interrupt more often

The handler function would be passed to, and function pointer set in
xhci_create_secondary_interrupter()

For primary interrupter it would always be set to xhci_handle_event_trb()

Thanks
Mathias
Wesley Cheng April 30, 2024, 9:57 p.m. UTC | #2
Hi Mathias,

On 4/30/2024 4:02 AM, Mathias Nyman wrote:
> On 26.4.2024 0.50, 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.
> 
> Could you elaborate on this a bit.
> 
> If I understood correctly the whole point of requesting a secondary xhci 
> interrupter
> for the sideband device without ever requesting a real interrupt for it 
> was to avoid
> waking up the cpu and calling the interrupt handler.
> 

Yes, this is the correct understanding.  We don't currently register the 
separate interrupt line (from GIC) for the secondary interrupter, so the 
main apps proc doesn't get interrupted on events generated on the 
secondary interrupter.

> with this flag is seems the normal xhci interrupt handler does get 
> called for
> sideband transfer events.
> 

Main intention was to utilize the refactoring you did to expose the 
xhci_handle_event_trb() for both handling events on the main 
interrupter, as well as the logic to skip events on the secondary 
interrupter.

https://lore.kernel.org/linux-usb/44a3d4db-7759-dd93-782a-1efbebfdb22c@linux.intel.com/

>>
>> Signed-off-by: Wesley Cheng <quic_wcheng@quicinc.com>
>> ---
>>   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 52278afea94b..6c7a21f522cd 100644
>> --- a/drivers/usb/host/xhci-ring.c
>> +++ b/drivers/usb/host/xhci-ring.c
>> @@ -2973,14 +2973,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;
>> +
> 
> I think we need another solution than a skip_events flag.
> 
> To make secondary xhci interrupters more useful in general it would make 
> more
> sense to add an interrupt handler function pointer to struct 
> xhci_interrupter.
> 
> Then call that function instead of xhci_handle_event_trb()
>

I agree that is how it should be for when support for actually utilizing 
secondary interrupters for routing events to different targets (instead 
of offloading).  However, since I don't have an existing use case that 
will exercise this functionality, its a bit difficult to verify that it 
should be working the way it was intended.

> --- a/drivers/usb/host/xhci-ring.c
> +++ b/drivers/usb/host/xhci-ring.c
> @@ -3098,8 +3098,8 @@ static int xhci_handle_events(struct xhci_hcd 
> *xhci, struct xhci_interrupter *ir
> 
>          /* Process all OS owned event TRBs on this event ring */
>          while (unhandled_event_trb(ir->event_ring)) {
> -               err = xhci_handle_event_trb(xhci, ir, 
> ir->event_ring->dequeue);
> -
> +               if (ir->handle_event_trb)
> +                       err = ir->handle_event_trb(xhci, ir, 
> ir->event_ring->dequeue);
>                  /*
>                   * If half a segment of events have been handled in one 
> go then
>                   * update ERDP, and force isoc trbs to interrupt more 
> often
> 
> The handler function would be passed to, and function pointer set in
> xhci_create_secondary_interrupter()
> 
> For primary interrupter it would always be set to xhci_handle_event_trb()
> 

Yes, definitely agree with this for when we introduce support for 
handling the secondary interrupter GIC line within the apps proc itself. 
  Would prefer if we took up that effort in another series, but willing 
to go back to the skip events loop previously implemented if the above 
change isn't where you want to go with this.

Thanks
Wesley Cheng

> Thanks
> Mathias
>
diff mbox series

Patch

diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index 52278afea94b..6c7a21f522cd 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -2973,14 +2973,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;
+
 	trace_xhci_handle_event(ir->event_ring, &event->generic);
 
 	/*
@@ -3069,8 +3077,9 @@  static void xhci_clear_interrupt_pending(struct xhci_hcd *xhci,
 }
 
 /*
- * Handle all OS-owned events on an interrupter event ring. It may drop
- * and reaquire xhci->lock between event processing.
+ * Handle all OS-owned events on an interrupter event ring, or skip pending events
+ * for non OS owned interrupter event ring. It may drop and reaquire xhci->lock
+ * between event processing.
  */
 static int xhci_handle_events(struct xhci_hcd *xhci, struct xhci_interrupter *ir)
 {
diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
index dbed0199aa5c..67d729635d56 100644
--- a/drivers/usb/host/xhci.h
+++ b/drivers/usb/host/xhci.h
@@ -1432,6 +1432,7 @@  struct xhci_interrupter {
 	struct xhci_intr_reg __iomem *ir_set;
 	unsigned int		intr_num;
 	bool			ip_autoclear;
+	bool			skip_events;
 	u32			isoc_bei_interval;
 	/* For interrupter registers save and restore over suspend/resume */
 	u32	s3_irq_pending;