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

[Wesley Cheng <quic_wcheng@quicinc.com>]

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