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

[Sebastian Andrzej Siewior <bigeasy@linutronix.de>]

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

> >> 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