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

[Pkshih <pkshih@realtek.com>]

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

> If I'm right, maybe it needs a rename and/or some additional comments.
> 

The original name and comment:
	/* Used for PCI TX queueing. */
	spinlock_t irq_lock;
Will change to
	/* Used for PCI TX/RX rings and TX queueing. */
	spinlock_t irq_lock;

---
Ping-Ke