> From: Jason Gunthorpe <jgg@ziepe.ca>
> Sent: Tuesday, January 6, 2026 8:14 AM
> 
> On Mon, Jan 05, 2026 at 09:05:36PM +0100, Dmytro Maluka wrote:
> > > and WRITE_ONCE is pointless if the HW is promising not to
> > > read it due to non-present.
> >
> > As long as we use a barrier. And IIUC vice versa, if we use WRITE_ONCE
> > for any updates, a barrier is not necessary (on x86). And WRITE_ONCE for
> > any updates (for PASID entries) is what was already done before this
> > series.
> 
> That is an x86 ism and it shouldn't be needlessly leaked into drivers.

yeah WRITE_ONCE() is not by definition to guarantee the ordering between
CPU and device.

lots of READ_ONCE()/WRITE_ONCE() in existing code are meaningless,
as 1) between CPUs there is already lock protection; 2) between CPU and
device it requires dma_wmb() to guarantee the order.

> As this is not performance it should have the portable flow:
> 
>    WRITE_ONCE(non-present)
>    dma_wmb()
>    <cmd to flush caches>
> 
>    [..]
>    <writes to the entry>
> 
>    dma_wmb()
>    WRITE_ONCE(present)
> 
> So, it seems to me like all you need here is the one line to add the
> dma_wmb() prior to setting present.

+1

On Tue, Jan 06, 2026 at 07:48:50AM +0000, Tian, Kevin wrote:
> yeah WRITE_ONCE() is not by definition to guarantee the ordering between
> CPU and device.

Yes, WRITE_ONCE is not about HW guarantess at all, it is about compiler
guarantess. And it is not about ordering, it is about compiler's
guarantee to store the given 64-bit value once, with one instruction.
But this compiler guarantee is exactly my point (see my last reply to
Jason).

> lots of READ_ONCE()/WRITE_ONCE() in existing code are meaningless,
> as 1) between CPUs there is already lock protection; 2) between CPU and
> device it requires dma_wmb() to guarantee the order.

As I see it, those WRITE_ONCEs (maybe not READ_ONCEs) haven't been
meaningless (I mean, they have been actually useful) so long as we
haven't been using any barriers. Again, on x86, store ordering requires
just compiler ordering, and dma_wmb() is just a compiler barrier. So,
assuming this driver is only used on x86 (which is, well, true :)),
we are lacking even compiler barriers, but at least we have those
WRITE_ONCEs, which provide compiler ordering too (although only between
each other, not with any other memory accesses, but that seems enough
for our case).

And again, I agree it is not pretty to rely on arch-specific ordering
assumptions, and doing in-place updates via those context_xxx() and
pasid_xxx() helpers all over the place instead of updating whole entries
seems a strange choice. But that is how it was implemented 10 or so
years ago, and overhauling that hasn't been my goal.

> From: Dmytro Maluka <dmaluka@chromium.org>
> Sent: Tuesday, January 6, 2026 10:40 PM
> 
> On Tue, Jan 06, 2026 at 07:48:50AM +0000, Tian, Kevin wrote:
> > yeah WRITE_ONCE() is not by definition to guarantee the ordering between
> > CPU and device.
> 
> Yes, WRITE_ONCE is not about HW guarantess at all, it is about compiler
> guarantess. And it is not about ordering, it is about compiler's
> guarantee to store the given 64-bit value once, with one instruction.
> But this compiler guarantee is exactly my point (see my last reply to
> Jason).
> 
> > lots of READ_ONCE()/WRITE_ONCE() in existing code are meaningless,
> > as 1) between CPUs there is already lock protection; 2) between CPU and
> > device it requires dma_wmb() to guarantee the order.
> 
> As I see it, those WRITE_ONCEs (maybe not READ_ONCEs) haven't been
> meaningless (I mean, they have been actually useful) so long as we
> haven't been using any barriers. Again, on x86, store ordering requires
> just compiler ordering, and dma_wmb() is just a compiler barrier. So,
> assuming this driver is only used on x86 (which is, well, true :)),
> we are lacking even compiler barriers, but at least we have those
> WRITE_ONCEs, which provide compiler ordering too (although only between
> each other, not with any other memory accesses, but that seems enough
> for our case).
> 
> And again, I agree it is not pretty to rely on arch-specific ordering
> assumptions, and doing in-place updates via those context_xxx() and
> pasid_xxx() helpers all over the place instead of updating whole entries
> seems a strange choice. But that is how it was implemented 10 or so
> years ago, and overhauling that hasn't been my goal.

sure. the point here is to align on what is the right thing to do. then
we could have short-term fixes plus bigger refactoring later.