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. If I'm right, maybe it needs a rename and/or some additional comments. Brian
