Re: [PATCH net v6] usbnet: fix smp_processor_id() use in preemptible context

[Paolo Abeni <pabeni@redhat.com>]

On 10/13/26 04:09, Alan Stern wrote:
> On Mon, Oct 12, 2026 at 11:25:27PM +0200, Sebastian Andrzej Siewior wrote:
>> 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.
Please help me wrapping this up. I read the above as a more generic
patch is under discussion, am I correct?

Thanks,

Paolo