On Wed, Jun 16, 2021 at 1:31 AM Pkshih <pkshih@xxxxxxxxxxx> wrote: > > -----Original Message----- > > From: Brian Norris [mailto:briannorris@xxxxxxxxxxxx] > > On Thu, Apr 29, 2021 at 04:01:39PM +0800, Ping-Ke Shih wrote: > > > --- /dev/null > > > +++ b/drivers/net/wireless/realtek/rtw89/pci.c > > > +static irqreturn_t rtw89_pci_interrupt_threadfn(int irq, void *dev) > > > +{ > > > + struct rtw89_dev *rtwdev = dev; > > > + struct rtw89_pci *rtwpci = (struct rtw89_pci *)rtwdev->priv; > > > + u32 isrs[2]; > > > + unsigned long flags; > > > + u32 unmask0_rx = 0; > > > + > > > + isrs[0] = rtwpci->isrs[0]; > > > + isrs[1] = rtwpci->isrs[1]; By the way, I'm pretty sure you need to hold the irq_lock to safely read these. ... > By your suggestions, I trace the flow and picture them below: Nice, thanks for that! > But, three exceptions > 1. interrupt is still triggered, even we disable interrupt by step 1). > That means int_handler is executed again, but threadfn doesn't enable > interrupt yet. I think maybe that's what IRQF_ONESHOT is for? Do you need to use that? But it might not be a complete solution. > This is because interrupt event is on the way to host (I think the path is > long -- from WiFi MAC to PCI MAC to PCI bus to host). > There's race condition between disable interrupt and interrupt event. > > I don't plan to fix the race condition, but make the driver handle it properly. > > 2. napi_poll doesn't start immediately at the step 7). > I don't trace the reason yet, but I think it's reasonable that > int_threadfn and napi_poll can be ansynchronous. > Because napi_poll can run in threaded mode as well. > > 3. Since int_threadfn and napi_poll are ansynchronous (similar to exception 2), > it looks like napi_poll is done before int_threadfn in some situations. > > I'll make the driver handle these cases in next submission (v6). ACK. > Another thing is I need to do local_bh_disable() before calling napi_schedule(), > or kernel warns " NOHZ tick-stop error: Non-RCU local softirq work is pending, handler #08!!!" > I think this is because __napi_schedule() does local_irq_save(), not very sure. > > I investigate other drivers that use napi_schedule() also do local_bh_disable() > before calling napi_schedule(), or do spin_lock_bh(), or in bh context. I think > these are equivalent. OK. I'll admit I'm not that familiar with the locking and context expectations of NAPI APIs (and, they are basically undocumented), but that sounds OK. I was mostly concerned that you were trying to use BH-disable as a mutual exclusion mechanism, when that's not really what it does. > > > + spin_lock_irqsave(&rtwpci->irq_lock, flags); > > > + if (rtwpci->running) { > > > + rtw89_pci_clear_intrs(rtwdev, rtwpci); > > > > Do you really want to clear interrupts here? I'm not that familiar with > > the hardware here or anything, but that seems like a job for your ISR, > > not the NAPI poll. It also seems like you might double-clear interrupts > > without properly handling them, because you only called > > rtw89_pci_recognize_intrs() in the ISR, not here. > > This chip is an edge-trigger interrupt, so originally I'd like to make "(IMR & ISR)" > become 0, and then next RX packet can trigger the interrupt. But I believe that's racy. If you clear an interrupt now based on rtwpci->halt_c2h_isr and rtwpci->isrs[], and later reread those fields in rtw89_pci_recognize_intrs(), clobbering any saved values, then you may lose an interrupt, I think. Overall, the state you're keeping around, and all the interactions between your NAPI poll and your IRQ handler, are very complex and hard to reason about. I believe your rtw88 driver has a much cleaner interrupt dispatch logic -- it doesn't try to do smart things around reading/writing the interrupt mask in 3 different places (IRQ handler, threaded IRQ handler, and NAPI poll), but just read/stashes/clears the mask in one place (threadfn) and avoids saving that state globally. I think you might have better luck if you can imitate that. But again, maybe I'm missing something. Brian > But, it seems that enable RX interrupt (step 9 of above picture) can already > raise interrupt.
