Re: [PATCH v4 03/13] rtw88: hci files

On Sun, Feb 10, 2019 at 9:48 PM Tony Chuang [off-list ref] wrote:

quoted

From: Brian Norris [mailto:briannorris@chromium.org]

quoted

+   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