Received-SPF: pass (google.com: domain of linux-wireless-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20210429080149.7068-1-pkshih@realtek.com> <20210429080149.7068-10-pkshih@realtek.com>
 <YMFzAZUysQ5HxZlK@google.com> <45dd7da687a444d5acbc13eb67dcee97@realtek.com>
In-Reply-To: <45dd7da687a444d5acbc13eb67dcee97@realtek.com>
From:   Brian Norris <briannorris@chromium.org>
Date:   Fri, 18 Jun 2021 12:13:16 -0700
Message-ID: <CA+ASDXP1aY5Mm14GDA_qPK7+7Jri2xAMZ3fYiVrhur7N5EO0mQ@mail.gmail.com>
Subject: Re: [PATCH v4 09/19] rtw89: add pci files
To:     Pkshih <pkshih@realtek.com>
Cc:     "kvalo@codeaurora.org" <kvalo@codeaurora.org>,
        "linux-wireless@vger.kernel.org" <linux-wireless@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

 On Wed, Jun 16, 2021 at 1:31 AM Pkshih <pkshih@realtek.com> wrote:
> > -----Original Message-----
> > From: Brian Norris [mailto:briannorris@chromium.org]

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