Re: [PATCH v4 3/8] rtw88: add napi support

[Pkshih <pkshih@realtek.com>]

On Mon, 2021-02-01 at 06:38 +0000, Pkshih wrote:
> > -----Original Message-----
> > From: Brian Norris [mailto:briannorris@chromium.org]
> > Sent: Saturday, January 30, 2021 7:39 AM
> > To: Pkshih
> > Cc: Yan-Hsuan Chuang; Kalle Valo; linux-wireless; Bernie Huang
> > Subject: Re: [PATCH v4 3/8] rtw88: add napi support
> > 
> > On Thu, Jan 28, 2021 at 1:45 AM Pkshih <pkshih@realtek.com> wrote:
> > > > -----Original Message-----
> > > > From: Brian Norris [mailto:briannorris@chromium.org]
> > > > On Fri, Jan 15, 2021 at 1:26 AM Ping-Ke Shih <pkshih@realtek.com> wrote:
> > > > > +static u32 rtw_pci_rx_napi(struct rtw_dev *rtwdev, struct rtw_pci
> *rtwpci,
> > > > >                            u8 hw_queue)
> > > > ...
> > > >
> > > > Are you sure you don't want any locking in rtw_pci_rx_napi()?
> > > > Previously, you held irq_lock for the entirety of rtw_pci_rx_isr(),
> > > > but now all the RX work is being deferred to a NAPI context, without
> > > > any additional lock. IIUC, that means you can be both handling RX and
> > > > other ISR operations at the same time. Is that intentional?
> > > >
> > >
> > > irq_lock is used to protect TX ring->queue. The TX skb(s) are queued into
> the
> > > queue, and unlink the skb until TX_OK_ISR is received. So, RX doesn't need
> to
> > > hold this lock.
> > 
> > I could be misunderstanding your locking model, but IIUC, you're left
> > with zero locking between NAPI RX and all other operations (H2C, link
> > up/down -- including DMA free, etc.). irq_lock used to protect you
> > from that.
> > 
> 
> Sorry, I'm wrong. I think irq_lock is used to protect not only TX ring->queue
> but also TX/RX rings. The RX ring rtwpci->rx_rings[RTW_RX_QUEUE_MPDU] is reset
> by rtw_pci_reset_buf_desc() when pci_stop(), and napi_poll() also uses it to
> know how many RX packets are needed to be received. Therefore, we plan to
> use irq_lock to protect napi_poll(), and then see if it affects performance.
> 

I change my mind, because using irq_lock to protect napi_poll causes deadlock.
I think that it's disallowed to hold a spin_lock_bh and call napi APIs that uses
RCU lock.

Then, I have another simple thinking -- enable NAPI only if interrupt is
enabled. Other operations with RX ring are working only if interrupt is
disabled. So, we don't need a lock to protect RX ring at all.

The irq_lock is still used to protect TX ring/queue, and now it also used
to protect switching IMR. Some comments are added to describe about this.

Above is implemented in v5.

---
Ping-Ke