> -----Original Message----- > From: Pkshih > Sent: Friday, June 25, 2021 6:07 PM > To: 'Brian Norris' > Cc: kvalo@xxxxxxxxxxxxxx; linux-wireless@xxxxxxxxxxxxxxx > Subject: RE: [PATCH v4 09/19] rtw89: add pci files > > > > > -----Original Message----- > > From: Brian Norris [mailto:briannorris@xxxxxxxxxxxx] > > Sent: Saturday, June 19, 2021 3:13 AM > > To: Pkshih > > Cc: kvalo@xxxxxxxxxxxxxx; linux-wireless@xxxxxxxxxxxxxxx > > Subject: Re: [PATCH v4 09/19] rtw89: add pci files > > > > 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. > > > > Will do it. > > > ... > > > > > 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. > > > > I tried IRQF_ONESHOT and it works well. But this flag is mutual exclusive with > IRQF_SHARED that is in use. > > I compare the interrupt count between these two flags, there is no significant > difference when I running TCP/UDP TX/RX stress test. Surprisingly, interrupt > count of using IRQF_SHARED is a little lower. > > Since new flow (see below) can properly handle this case, I decide to use > original flag IRQF_SHARED. > > > > > 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. > > > > I read IRQ handler of rtw88 that looks much simpler, and I picture the > new flow: > > int_handler int_threadfn napi_poll > ----------- ------------ --------- > | > | > | 1) dis. intr > o | > | 2) read interrupt status locally > | 3) do TX reclaim > | 4) check if RX? > | 5) re-enable intr > | (RX is optional) > | 6) schedule_napi > | (if RX) > : >>>-------------------+ 7) (tasklet start immediately) > : | > : | 8) set 'doing RX' flag > : | 9) do RX things > : | 10) clear 'doing RX' flag > : | 11) re-enable intr of RX > : | > : <<<-------------------o > : > o > > Step 2) read and clear interrupt status with spin_lock_irqsave, and use local > variables to save the status. Then, the status will be correct, even more > interrupts are triggered. > > Step 8)/10) add a 'doing RX' flag we don't enable RX interrupt in this period, so > step 5) will not make a mistake to enable RX interrupt improperly. > > I attach the patch based on v5, and these changes will be included in v6. > Further suggestions are welcome. > Sorry, I missed the changes of pci.h, so send reference patch again. -- Ping-Ke
Attachment:
pci.patch
Description: pci.patch
