Date:   Fri, 22 Mar 2019 14:36:21 +0000
From:   Brian Norris <briannorris@chromium.org>
To:     Tony Chuang <yhchuang@realtek.com>
Cc:     Grant Grundler <grundler@chromium.org>,
        Kalle Valo <kvalo@codeaurora.org>,
        Johannes Berg <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>
Subject: Re: [PATCH v4 03/13] rtw88: hci files
Message-ID: <20190322143621.2rtyzysglv4fqocf@penguin>
References: <1548820940-15237-1-git-send-email-yhchuang@realtek.com>
 <1548820940-15237-4-git-send-email-yhchuang@realtek.com>
 <20190131223609.GA191502@google.com>
 <CANEJEGt4xJ5pTXgqazTVV2MNZy9+SWa6uE+wbE_94aT+FvL-+A@mail.gmail.com>
 <F7CD281DE3E379468C6D07993EA72F84D17AA49B@RTITMBSVM04.realtek.com.tw>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <F7CD281DE3E379468C6D07993EA72F84D17AA49B@RTITMBSVM04.realtek.com.tw>
User-Agent: NeoMutt/20170113 (1.7.2)
Sender: linux-wireless-owner@vger.kernel.org
Precedence: bulk

[Following up a little late on this one]

Hi Grant and Tony,

On Wed, Feb 20, 2019 at 11:19:10AM +0000, Tony Chuang wrote:
> > -----Original Message-----
> > From: Grant Grundler [mailto:grundler@chromium.org]

> > This is another bug in the code actually: the code accessing tx
> > descriptor rings should be declared volatile since they are updated by
> > the device which is not visible to the compiler.  Compiler won't
> > optimize (or reorder) volatile code (by "volatile code" I mean code
> > that is accessing data marked "volatile").

If I'm understanding the code correctly, the TX ISR assumes that
everything between the last entry we processed (ring->r.rp) and a
certain point (cur_rp) is complete. The former index was saved during
the previous interrupt and the latter is determined via a register read.
So assuming properly synchronized interrupts (such as with PCIe MSI) and
no device-/firmware-related ordering bugs, then I think we're OK -- the
only "volatile" piece is the register read, which already embeds
volatile in the IO accessors.

> This is fixed in PATCH v5. We reset and use rx tag to sync dma.
> So the polling can be removed and the bus timeout has not happened again.

Yes, I tested the later versions of this series, and I didn't see this
problem so far. I'll follow up there with my Review/Test information
eventually.

Thanks,
Brian