| Message ID | 20241106193413.1730413-2-quic_wcheng@quicinc.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | Introduce QC USB SND audio offloading support | expand |
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@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 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. Thanks Mathias
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@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 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@quicinc.com/ 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
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; + trace_xhci_handle_event(ir->event_ring, &event->generic); /* @@ -3026,8 +3034,9 @@ static void xhci_clear_interrupt_pending(struct xhci_interrupter *ir) } /* - * 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 reacquire 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 55a81073cf1e..92cde885acd3 100644 --- a/drivers/usb/host/xhci.h +++ b/drivers/usb/host/xhci.h @@ -1430,6 +1430,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;
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(-)