linux-kernel - Re: [PATCH v30 01/30] usb: host: xhci: Repurpose event handler for skipping interrupter events

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <a7bca9f6-06f9-4e14-a1ab-761e92a68ceb@quicinc.com>
Date: Wed, 20 Nov 2024 10:48:00 -0800
From: Wesley Cheng <quic_wcheng@...cinc.com>
To: Mathias Nyman <mathias.nyman@...ux.intel.com>,
        <srinivas.kandagatla@...aro.org>, <mathias.nyman@...el.com>,
        <perex@...ex.cz>, <conor+dt@...nel.org>, <dmitry.torokhov@...il.com>,
        <corbet@....net>, <broonie@...nel.org>, <lgirdwood@...il.com>,
        <krzk+dt@...nel.org>, <pierre-louis.bossart@...ux.intel.com>,
        <Thinh.Nguyen@...opsys.com>, <tiwai@...e.com>, <robh@...nel.org>,
        <gregkh@...uxfoundation.org>
CC: <linux-kernel@...r.kernel.org>, <devicetree@...r.kernel.org>,
        <linux-sound@...r.kernel.org>, <linux-usb@...r.kernel.org>,
        <linux-input@...r.kernel.org>, <linux-arm-msm@...r.kernel.org>,
        <linux-doc@...r.kernel.org>
Subject: Re: [PATCH v30 01/30] usb: host: xhci: Repurpose event handler for
 skipping interrupter events

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@...cinc.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