> -----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. Thank you -- Ping-Ke
Attachment:
pci.patch
Description: pci.patch
