| Message ID | 20250108012213.1659364-2-quic_wcheng@quicinc.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | Introduce QC USB SND audio offloading support | expand |
Hi, > Depending on the interrupter use case, the OS may only be used to > handle the interrupter event ring clean up. What do you mean by "cleanup"? Because I see that this patch ends up acknowledging events to the xHC and I don't know why it would do so? > 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. Right, and if the OS isn't handling those events because they are owned by a coprocessor then it shouldn't be acknowledging them either, which has the effect that the xHC considers their memory free for reuse. Also, what happens when Linux goes to sleep and this IRQ stops running? I expected that the coprocessor itself should be updating the xHC about its own progress. Is it a bug? How is this stuff supposed to work? How are future developers supposed to know how it is supposed to work? I imagine that few of them will have Qualcomm hardware for testing. > 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 function is only called from one place so the caller could perform this check and don't waste time calling it. Regards, Michalal
Hi Michal, On 1/13/2025 5:36 AM, Michał Pecio wrote: > Hi, > >> Depending on the interrupter use case, the OS may only be used to >> handle the interrupter event ring clean up. > What do you mean by "cleanup"? Because I see that this patch ends up > acknowledging events to the xHC and I don't know why it would do so? Cleanup so that we can ensure there are no pending events that were left once the secondary interrupter is disabled. Based on previous feedback, there are use cases where the OS may actually want to handle events occurring on the secondary interrupter, but that support will take some time to implement/test. >> 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. > Right, and if the OS isn't handling those events because they are owned > by a coprocessor then it shouldn't be acknowledging them either, which > has the effect that the xHC considers their memory free for reuse. This implementation was done as part of an effort to consolidate the cleanup of the pending events with the same path as the handling of events for the main/primary interrupter: https://lore.kernel.org/linux-usb/33dfa0c5-c43f-79f6-2700-beee2e5d389f@quicinc.com/ > Also, what happens when Linux goes to sleep and this IRQ stops running? > I expected that the coprocessor itself should be updating the xHC about > its own progress. Currently, Linux is not expected to go to sleep if the coprocessor is active. The coprocessor will notify when the audio stream is enabled and disabled, and the USB device runtime PM counters are incremented/decremented respectively. If Linux forcefully goes to sleep, then support is there to ensure the coprocessor's audio stream is disabled before entering suspend. > > Is it a bug? How is this stuff supposed to work? > > How are future developers supposed to know how it is supposed to work? > I imagine that few of them will have Qualcomm hardware for testing. Most of the implementation details of the overall mechanisms are highlighted in the cover letter, so can you clarify what you are suggesting that needs to be done for this statement? > >> 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 function is only called from one place so the caller could perform > this check and don't waste time calling it. I'm open to do it from either place. We have to ensure that we cycle through each pending event during the cleanup phase. Thanks Wesley Cheng
Thanks, I think I now see how this is meant to work. Cover leter mostly discusses the ALSA side of things, but not low level details of xHCI operation, such as who will be ringing doorbells and how, handling IRQs, updating event ring dequeue, or handling halted EPs. So for the record, as far as I see: 1. There is no API for ringing doorbells or even getting a pointer, the coprocessor needs to have its own access. Fair enough. 2. Same for event ring dequeue, but the driver must clean up leftover unacknowledged events after sideband operation stops. 3. Linux IRQ handler never needs to worry about sideband interrupts. 4. Resetting halted endpoints is not implemented at all, I think? So this code is currently mostly useful with isochronous. And the 'skip_events' flag only exists to enable ring cleanup when the interrupter is removed? In such case I think it's overkill. The code would be simpler and its intent more visible if 'skip_events' were a new parameter of xhci_handle_events(). Existing IRQ would call the function normally, while xhci_skip_sec_intr_events() would use the new parameter to suppress event handling in this one special case. It would be immediately clear that skipping only applies on removal. You could completely get rid of PATCH 01/32 because 02/32 would no longer need to set this flag on the interrupter, and the 'if' branch adedd by 01/32 could go into 03/32 where it logically belongs. Just a suggestion. I simply don't see any need to have a flag which causes events on a ring to always be skipped as a matter of policy. Your code doesn't seem to require it. Probably nobody ever will. Regards, Michal
Hi Michal, On 1/14/2025 6:08 AM, Michał Pecio wrote: > Thanks, I think I now see how this is meant to work. > > > Cover leter mostly discusses the ALSA side of things, but not low level > details of xHCI operation, such as who will be ringing doorbells and > how, handling IRQs, updating event ring dequeue, or handling halted EPs. > > So for the record, as far as I see: > 1. There is no API for ringing doorbells or even getting a pointer, > the coprocessor needs to have its own access. Fair enough. > 2. Same for event ring dequeue, but the driver must clean up leftover > unacknowledged events after sideband operation stops. > 3. Linux IRQ handler never needs to worry about sideband interrupts. > 4. Resetting halted endpoints is not implemented at all, I think? > So this code is currently mostly useful with isochronous. Yep, all your points about the code with respects to the xHCI perspective is correct. > > And the 'skip_events' flag only exists to enable ring cleanup when the > interrupter is removed? In such case I think it's overkill. > > The code would be simpler and its intent more visible if 'skip_events' > were a new parameter of xhci_handle_events(). Existing IRQ would call > the function normally, while xhci_skip_sec_intr_events() would use the > new parameter to suppress event handling in this one special case. > > It would be immediately clear that skipping only applies on removal. > > You could completely get rid of PATCH 01/32 because 02/32 would no > longer need to set this flag on the interrupter, and the 'if' branch > adedd by 01/32 could go into 03/32 where it logically belongs. > > Just a suggestion. I simply don't see any need to have a flag which > causes events on a ring to always be skipped as a matter of policy. > Your code doesn't seem to require it. Probably nobody ever will. > In my previous discussions with Mathias, I think the plan was that he wanted it to be built in a way where we should be able to accommodate a use case where the secondary interrupter was going to be actually handled by the Linux side. This is why the skip_events is populated/defined by the xHCI sideband calls, so that we can differentiate between the secondary interrupter use cases. Although, it is the correct assumption that this series doesn't actually implement that functionality. Thanks Wesley Cheng
diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c index 3d66ed240dc3..8982ae92adee 100644 --- a/drivers/usb/host/xhci-ring.c +++ b/drivers/usb/host/xhci-ring.c @@ -2969,14 +2969,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, xhci_trb_virt_to_dma(ir->event_ring->deq_seg, ir->event_ring->dequeue)); @@ -3066,8 +3074,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 8c164340a2c3..21e4b2d11d92 100644 --- a/drivers/usb/host/xhci.h +++ b/drivers/usb/host/xhci.h @@ -1442,6 +1442,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(-)