On Mon, 2021-02-01 at 06:38 +0000, Pkshih wrote: > > -----Original Message----- > > From: Brian Norris [mailto:briannorris@xxxxxxxxxxxx] > > 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@xxxxxxxxxxx> wrote: > > > > -----Original Message----- > > > > From: Brian Norris [mailto:briannorris@xxxxxxxxxxxx] > > > > On Fri, Jan 15, 2021 at 1:26 AM Ping-Ke Shih <pkshih@xxxxxxxxxxx> 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
