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>
 <CA+ASDXP1aY5Mm14GDA_qPK7+7Jri2xAMZ3fYiVrhur7N5EO0mQ@mail.gmail.com> <db26751dfb02499093829a6aeecadb61@realtek.com>
In-Reply-To: <db26751dfb02499093829a6aeecadb61@realtek.com>
From:   Brian Norris <briannorris@chromium.org>
Date:   Mon, 19 Jul 2021 11:50:56 -0700
Message-ID: <CA+ASDXO7SfuAsLhLU9hn4bANL5oTizwoYh5ifi2Wi-Mr-7zMDQ@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

(Late response)

On Wed, Jun 30, 2021 at 5:47 PM Pkshih <pkshih@realtek.com> wrote:
> > -----Original Message-----
> > From: Pkshih

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

BTW, I think you might be pointing out a sort of bug with rtw88 -- a
non-RX interrupt might cause RX interrupts to get re-enabled in the
midst of what's *supposed* to be a NAPI poll. It's not a fatal
functional problem or anything, but it does mean we might get excess
RX interrupts, which can defeat the purpose of polling. I suppose the
impact of such a bug depends on how frequently we're receiving other
interrupts (say, H2C?).

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

I haven't proven to myself that every line in there is good, but I
think the general layout is better. I'm more likely to get a closer
look+test when a v6 comes around.

Thanks,
Brian