MIME-Version: 1.0
References: <1548820940-15237-1-git-send-email-yhchuang@realtek.com>
 <1548820940-15237-4-git-send-email-yhchuang@realtek.com> <20190209021426.GA163159@google.com>
 <F7CD281DE3E379468C6D07993EA72F84D17A2AC2@RTITMBSVM04.realtek.com.tw>
In-Reply-To: <F7CD281DE3E379468C6D07993EA72F84D17A2AC2@RTITMBSVM04.realtek.com.tw>
From:   Brian Norris <briannorris@chromium.org>
Date:   Mon, 11 Feb 2019 09:56:22 -0800
Message-ID: <CA+ASDXNmU3_nZ1VXeGZ4GMijgQKSL+LJMvy44FP9u1THBohSbA@mail.gmail.com>
Subject: Re: [PATCH v4 03/13] rtw88: hci files
To:     Tony Chuang <yhchuang@realtek.com>
Cc:     "kvalo@codeaurora.org" <kvalo@codeaurora.org>,
        "johannes@sipsolutions.net" <johannes@sipsolutions.net>,
        "Larry.Finger@lwfinger.net" <Larry.Finger@lwfinger.net>,
        Pkshih <pkshih@realtek.com>, Andy Huang <tehuang@realtek.com>,
        "sgruszka@redhat.com" <sgruszka@redhat.com>,
        "linux-wireless@vger.kernel.org" <linux-wireless@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-wireless-owner@vger.kernel.org
Precedence: bulk

On Sun, Feb 10, 2019 at 9:48 PM Tony Chuang <yhchuang@realtek.com> wrote:
> > From: Brian Norris [mailto:briannorris@chromium.org]
> > > +   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