linux-kernel - Re: [PATCH v32 01/32] 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: <20250114150847.1c3c9ebe@foxbook>
Date: Tue, 14 Jan 2025 15:08:47 +0100
From: Michał Pecio <michal.pecio@...il.com>
To: Wesley Cheng <quic_wcheng@...cinc.com>
Cc: <Thinh.Nguyen@...opsys.com>, <broonie@...nel.org>,
 <conor+dt@...nel.org>, <corbet@....net>, <devicetree@...r.kernel.org>,
 <dmitry.torokhov@...il.com>, <gregkh@...uxfoundation.org>,
 <krzk+dt@...nel.org>, <lgirdwood@...il.com>,
 <linux-arm-msm@...r.kernel.org>, <linux-doc@...r.kernel.org>,
 <linux-input@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
 <linux-sound@...r.kernel.org>, <linux-usb@...r.kernel.org>,
 <mathias.nyman@...el.com>, <perex@...ex.cz>,
 <pierre-louis.bossart@...ux.intel.com>, <robh@...nel.org>,
 <srinivas.kandagatla@...aro.org>, <tiwai@...e.com>
Subject: Re: [PATCH v32 01/32] usb: host: xhci: Repurpose event handler for
 skipping interrupter events

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