On Sun, Feb 10, 2019 at 9:48 PM Tony Chuang <yhchuang@xxxxxxxxxxx> wrote: > > From: Brian Norris [mailto:briannorris@xxxxxxxxxxxx] > > > + tx_data = rtw_pci_get_tx_data(skb); > > > + tx_data->dma = dma; > > > + skb_queue_tail(&ring->queue, skb); > > > > IIUC, you have no locking for this queue. That seems like a bad idea. It > > then gets pulled off this queue in your ISR, again without a lock. So > > for example, if the only packet in your queue gets completed while you > > are trying to queue another one, you might corrupt the list. > > > > I think skb_queue_tail already has its own spinlock to protect the queue? > Cannot see why the list might be corrupted. Or I misunderstand you. Ah, no of course you're correct. I think I kept looking at the definition of __skb_queue_tail() instead. It also doesn't help that, in skbuff.h, the kerneldoc comments for __skb_queue_tail() are put above skb_queue_tail(). So it's extra easy to confuse them... And to be clear, so far I don't think I've seen actual corruption of this queue yet. Just bad TX status reporting, including plenty of driver WARN()s. So my comment was only theoretical (and incorrect). Regards, Brian
