> -----Original Message----- > From: Sinan Kaya [mailto:ok...@codeaurora.org] > Sent: Friday, March 23, 2018 10:44 PM > To: David Miller <da...@davemloft.net> > Cc: netdev@vger.kernel.org; ti...@codeaurora.org; sulr...@codeaurora.org; > linux-arm-...@vger.kernel.org; linux-arm-ker...@lists.infradead.org; Elior, > Ariel <ariel.el...@cavium.com>; Dept-Eng Everest Linux L2 <Dept- > engeverestlinu...@cavium.com>; linux-ker...@vger.kernel.org > Subject: Re: [PATCH v5 3/5] bnx2x: Eliminate duplicate barriers on weakly- > ordered archs > > On 3/23/2018 1:04 PM, David Miller wrote: > > From: Sinan Kaya <ok...@codeaurora.org> > > Date: Fri, 23 Mar 2018 12:51:47 -0400 > > > >> It could if txdata->tx_db was not a union. There is a data dependency > >> between txdata->tx_db.data.prod and txdata->tx_db.raw. > >> > >> So, no reordering. > > > > I don't see it that way, the code requires that: > > > > txdata->tx_db.data.prod += nbd; > > > > is visible before the doorbell update.> > > barrier() doesn't provide that. > > > > Neither does writel_relaxed(). However plain writel() does. > > Correct for some architectures including ARM but not correct universally. > > writel() just guarantees register read/writes before and after to be ordered > when HW observes it. > > writel() doesn't guarantee that the memory update is visible to the HW on all > architectures. > > If you need memory update visibility, that barrier() should have been a > wmb() > > A correct multi-arch pattern is > > wmb() > writel_relaxed() > mmiowb() >
Sinan, Since you have mentioned the use of mmiowb() here after writel_relaxed(). I believe this is not always correct for all types of IO mapped memory [Specially if IO memory is mapped using write combined (for ex. Ioremap_wc())]. We have a current issue on our NIC (qede) driver on x86 for which the patch is already been sent more than a week ago [Still awaiting to hear from David on that]. where mmiowb() seems to be useless since we use write combined mapped doorbell and mmiowb() just seems to be a compiler barrier() there. So in order to flush write combined buffer we really need writel_relaxed() followed by a wmb() to synchronize writes among CPU cores. I think the correct pattern in such cases (for write combined IO) would have been like below - wmb(); writel_relaxed(); wmb(); -> To flush the writes actually. Thanks.
