RE: [PATCH v4 09/19] rtw89: add pci files

[Pkshih <pkshih@realtek.com>]

> -----Original Message-----
> From: Brian Norris [mailto:briannorris@chromium.org]
> Sent: Tuesday, July 20, 2021 2:51 AM
> To: Pkshih
> Cc: kvalo@codeaurora.org; linux-wireless@vger.kernel.org
> Subject: Re: [PATCH v4 09/19] rtw89: add pci files
> 
> (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?).

Indeed. We need to add a 'doing RX' flag to rtw88. I have another thinking,
which doesn't use this flag, to refine rtw89, and I'm working on it.
If it works well, I can apply it to rtw88.

int_handler             int_threadfn              napi_poll
-----------             ------------              ---------
    |
    |
    | 1) dis. intr
    o                      |
                           | 2) read interrupt status locally
                           | 3) do unlikely things
                           | 4) schedule_napi
                           |
                           : >>>-------------------+ 5) (tasklet start immediately)
                           :                       |
                           :                       | 6) do TX completion things
                           :                       | 7) do RX things
                           :                       | 
                           :                       | 8) re-enable intr
                           :                       |
                           : <<<-------------------o
                           :
                           o

I don't change the jobs of the int_handler. The int_thread reads interrupt status
and schedules NAPI unconditionally, because only unlikely things reside in this
function, and the TX completion and RX things are done by napi_poll.
Finally, the napi_poll re-enable interrupt after it does all things.

Things I'm worried are the latency of RX and TX completion, and extra polling IO.
The latency affects performance, so I'll measure it. Also, I'll examine time cost
of polling IO, and compare the count of polling IO before/after this change.

I hope I can finish this work before v6.

--
Ping-Ke