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

[Brian Norris <briannorris@chromium.org>]

(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