-----Original Message-----
From: Pkshih
Sent: Friday, June 25, 2021 6:07 PM
To: 'Brian Norris'
Cc: kvalo@codeaurora.org; linux-wireless@vger.kernel.org
Subject: RE: [PATCH v4 09/19] rtw89: add pci files

quoted

-----Original Message-----
From: Brian Norris [mailto:briannorris@chromium.org]
Sent: Saturday, June 19, 2021 3:13 AM
To: Pkshih
Cc: kvalo@codeaurora.org; linux-wireless@vger.kernel.org
Subject: Re: [PATCH v4 09/19] rtw89: add pci files

 On Wed, Jun 16, 2021 at 1:31 AM Pkshih [off-list ref] wrote:

quoted

quoted

-----Original Message-----
From: Brian Norris [mailto:briannorris@chromium.org]

quoted

quoted

On Thu, Apr 29, 2021 at 04:01:39PM +0800, Ping-Ke Shih wrote:

quoted

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

quoted

...

quoted

By your suggestions, I trace the flow and picture them below:

Nice, thanks for that!

quoted

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.

quoted

quoted

   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.

quoted

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.

quoted

quoted

quoted

+           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