Re: [PATCH net-next] net: Correct wrong BH disable in hard-interrupt.

On 2022-02-17 15:08:55 [+0100], Marek Szyprowski wrote:

Hi All,

Hi,

quoted

quoted

Marek, does this work for you?

Yes, this fixed the issue. Thanks!

Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>

I've just noticed that there is one more issue left to fix (the $subject 
patch is already applied) - this one comes from threaded irq (if I got 
the stack trace right):

netif_rx() did only set the matching softirq bit and not more. Based on
that I don't see why NOHZ shouldn't complain about a pending softirq
once the CPU goes idle. Therefore I think the change I made is good
since it uncovered that.

------------[ cut here ]------------
WARNING: CPU: 0 PID: 147 at kernel/softirq.c:363 
__local_bh_enable_ip+0xa8/0x1c0

…

CPU: 0 PID: 147 Comm: irq/150-dwc3 Not tainted 5.17.0-rc4-next-20220217+ 
#4557
Hardware name: Samsung TM2E board (DT)
pstate: 400000c5 (nZcv daIF -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
pc : __local_bh_enable_ip+0xa8/0x1c0
lr : netif_rx+0xa4/0x2c0
...

Call trace:
  __local_bh_enable_ip+0xa8/0x1c0
  netif_rx+0xa4/0x2c0
  rx_complete+0x214/0x250
  usb_gadget_giveback_request+0x58/0x170
  dwc3_gadget_giveback+0xe4/0x200
  dwc3_gadget_endpoint_trbs_complete+0x100/0x388
  dwc3_thread_interrupt+0x46c/0xe20

So dwc3_thread_interrupt() disables interrupts here. Felipe dropped it
and then added it back in
    e5f68b4a3e7b0 ("Revert "usb: dwc3: gadget: remove unnecessary _irqsave()"")

I would suggest to revert it (the above commit) and fixing the lockdep
splat in the gadget driver and other. I don't see the g_ether warning
Felipe mentioned. It might come from f_ncm (since it uses a timer) or
something else in the network stack (that uses a timeout timer).
But not now.
As much as I hate it, I suggest:

diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 183b90923f51b..a0c883f19a417 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c

@@ -4160,9 +4160,11 @@ static irqreturn_t dwc3_thread_interrupt(int irq, void *_evt)
 	unsigned long flags;
 	irqreturn_t ret = IRQ_NONE;
 
+	local_bh_disable();
 	spin_lock_irqsave(&dwc->lock, flags);
 	ret = dwc3_process_event_buf(evt);
 	spin_unlock_irqrestore(&dwc->lock, flags);
+	local_bh_enable();
 
 	return ret;
 }

In the long run I would drop that irqsave (along with bh_disable() since
netif_rx() covers that) and make sure the there is no lockdep warning
popping up.

Marek, could you please give it a try?

Sebastian