Re: [PATCH net v6] usbnet: fix smp_processor_id() use in preemptible context
From: Paolo Abeni
Date: Tue Oct 13 2026 - 05:16:49 EST
On 10/13/26 04:09, Alan Stern wrote:
On Mon, Oct 12, 2026 at 11:25:27PM +0200, Sebastian Andrzej Siewior wrote:Please help me wrapping this up. I read the above as a more generic
On 2026-10-12 16:26:28 [-0400], Alan Stern wrote:
On Mon, Oct 12, 2026 at 09:49:02PM +0200, Sebastian Andrzej Siewior wrote:
On 2026-10-09 13:31:17 [+0200], Greg Kroah-Hartman wrote:
As far as I am aware, there aren't really any guarantees on the
context of a USB URB-completion callback. The kerneldoc for struct urb
in include/linux/usb.h says "The completion callback is made
in_interrupt()", but that is most definitely out of date.
I don't think so, I think some platforms still have those callbacks in
irq context, unless we changed to threaded irq handlers everywhere? I
could have missed that, but we should still write the callbacks to
assume that and then we should be fine even if we aren't in irq context,
right?
The HCD invokes the completion callback usually in IRQ context. I think
the VHCI is an exception here.
For the forced-threaded mode we disable BH before the IRQ handler is
invoked. I think even dummy_hcd invokes them from the timer if my memory
is right.
Drivers shouldn't rely on any particular context guarantees. Not even
whether local irqs are enabled/disabled.
Agreed.
I would suggest to make VHCI invoke the completion callback with BH
disabled. usbnet's tx_complete() does also defer_bh() which schedules a
BH worker via queue_work(). While this works without disabling BH it
leads to waking ksoftirqd and an additional context switch which we
could avoid by dealing with it directly.
Today you fix VHCI, tomorrow some other virtual HCD will cause problems.
We don't have other virtual HCDs, do we?
Well, we've got dummy-hcd. And just recently there was an RFC post with
a virtio USB HCD implementation. It wouldn't be surprising to encounter
other ones.
Also, don't forget, URBs can complete in several different ways.
There's normal completion, explicit unlinking, unlinking because the
device was unplugged, unlinking because the device or HC is going into
suspend, and so on. And there's a difference between URBs sent to
root hubs as opposed to devices actually on the bus. Not to mention
that in some cases completions may occur synchronously with submission
or unlinking, even though they usually are asynchronous.
The other driver is drivers/net/mctp/mctp-usb.c.
mctp_usb_out_complete() -> … -> mctp_usblib_tx_stats_update() does
get_cpu_ptr() for stats. But mctp_usb_in_complete() has
mctp_usblib_rx_complete() -> mctp_usblib_rx_stats_single_drop() and
mctp_usblib_rx() which do just this_cpu_ptr(). Plus the queue wakes &
netif_rx() which wake ksoftirqd.
That makes two drivers from a quick grep. But I guess it is just
networking for the per-CPU stats.
Should we document any requirements that all HCDs will have to follow
and that all completion handlers can rely on?
It used to complete in IRQ context. Then you added the possibility to
complete BH for those HCDs marked as such (except for the RH). I think
it was always reasonable to assume that it is either IRQ-context
(force-threaded or not for !HCD_BH) or BH (for HCD_BH). It makes hardly
a difference, BH is always off and networking uses to rely on it.
So yes, documenting would make sense.
Do you want to document it yourself and have me do the VHCI bits?
At the moment, the only thing I think we can actually guarantee is that
completion handlers will be called with local interrupts disabled. No
commitments about process context vs. interrupt context vs. BH context
or anything else.
Of course, in special cases you might know more about how a particular
HCD works. But except for platform-specific settings, it's hard to be
certain that your driver will always be used with that one particular
HCD.
If this is okay with you, I can write a documentation patch.
patch is under discussion, am I correct?
Thanks,
Paolo