Re: [PATCH net 1/2] net: ionic: add dma_wmb() before ringing TX doorbell

[mohammad heib <mheib@redhat.com>]

Hi Brett,

I was looking at Documentation/memory-barriers.txt, specifically this 
example:

1928         if (desc->status != DEVICE_OWN) {
1929                 /* do not read data until we own descriptor */
1930                 dma_rmb();
1931
1932                 /* read/modify data */
1933                 read_data = desc->data;
1934                 desc->data = write_data;
1935
1936                 /* flush modifications before status update */
1937                 dma_wmb();
1938
1939                 /* assign ownership */
1940                 desc->status = DEVICE_OWN;
1941
1942                 /* Make descriptor status visible to the device 
followed by
1943                  * notify device of new descriptor
1944                  */
1945                 writel(DESC_NOTIFY, doorbell);
1946         }
1947
1948      The dma_rmb() allows us to guarantee that the device has 
released ownership
1949      before we read the data from the descriptor, and the dma_wmb() 
allows
1950      us to guarantee the data is written to the descriptor before 
the device
1951      can see it now has ownership.  The dma_mb() implies both a 
dma_rmb() and
1952      a dma_wmb().
1953


As described there, dma_wmb() guarantees that all descriptor writes are 
visible to the device before ownership (or notification) is handed over 
via the doorbell.

In our case, the original confusion came from the fact that 
ionic_tx_tcp_inner_pseudo_csum() was updating the SKB data after 
mapping, which indeed could require a DMA barrier to make sure those 
writes are visible.
After applying patch 2 from the same series, all data modifications now 
happen before the DMA mapping, so the descriptors and buffers are 
already consistent when the doorbell is written.

That’s why I added the barrier initially, to handle the case where the 
CPU might update memory visible to the device right before ringing the 
doorbell.
With the current order (after patch 2), the need for it is less clear, 
but the change is harmless and ensures correctness on weakly ordered 
architectures.

DMA ordering is not exactly my strongest area, so it’s possible that I 
added it out of caution rather than necessity. Sorry if that caused 
confusion

On 11/10/25 9:21 PM, Mohammad Heib wrote:
> Hi Brett,
> 
> I was looking at Documentation/memory-barriers.txt, specifically this 
> example:
> 
> 1928         if (desc->status != DEVICE_OWN) {
> 1929                 /* do not read data until we own descriptor */
> 1930                 dma_rmb();
> 1931
> 1932                 /* read/modify data */
> 1933                 read_data = desc->data;
> 1934                 desc->data = write_data;
> 1935
> 1936                 /* flush modifications before status update */
> 1937                 dma_wmb();
> 1938
> 1939                 /* assign ownership */
> 1940                 desc->status = DEVICE_OWN;
> 1941
> 1942                 /* Make descriptor status visible to the device 
> followed by
> 1943                  * notify device of new descriptor
> 1944                  */
> 1945                 writel(DESC_NOTIFY, doorbell);
> 1946         }
> 1947
> 1948      The dma_rmb() allows us to guarantee that the device has 
> released ownership
> 1949      before we read the data from the descriptor, and the dma_wmb() 
> allows
> 1950      us to guarantee the data is written to the descriptor before 
> the device
> 1951      can see it now has ownership.  The dma_mb() implies both a 
> dma_rmb() and
> 1952      a dma_wmb().
> 1953
> 
> 
> As described there, |dma_wmb()| guarantees that all descriptor writes 
> are visible to the device before ownership (or notification) is handed 
> over via the doorbell.
> 
> In our case, the original confusion came from the fact that | 
> ionic_tx_tcp_inner_pseudo_csum()| was updating the SKB data after 
> mapping, which indeed could require a DMA barrier to make sure those 
> writes are visible.
> After applying patch 2 from the same series, all data modifications now 
> happen before the DMA mapping, so the descriptors and buffers are 
> already consistent when the doorbell is written.
> 
> That’s why I added the barrier initially, to handle the case where the 
> CPU might update memory visible to the device right before ringing the 
> doorbell.
> With the current order (after patch 2), the need for it is less clear, 
> but the change is harmless and ensures correctness on weakly ordered 
> architectures.
> 
> DMA ordering is not exactly my strongest area, so it’s possible that I 
> added it out of caution rather than necessity. Sorry if that caused 
> confusion
> 
> 
> 
> On Mon, Nov 10, 2025 at 7:28 PM Brett Creeley <bcreeley@amd.com 
> <mailto:bcreeley@amd.com>> wrote:
> 
> 
> 
>     On 11/3/2025 5:30 PM, patchwork-bot+netdevbpf@kernel.org
>     <mailto:patchwork-bot%2Bnetdevbpf@kernel.org> wrote:
>      > Caution: This message originated from an External Source. Use
>     proper caution when opening attachments, clicking links, or responding.
>      >
>      >
>      > Hello:
>      >
>      > This series was applied to netdev/net.git (main)
>      > by Jakub Kicinski <kuba@kernel.org <mailto:kuba@kernel.org>>:
>      >
>      > On Fri, 31 Oct 2025 17:52:02 +0200 you wrote:
>      >> From: Mohammad Heib <mheib@redhat.com <mailto:mheib@redhat.com>>
>      >>
>      >> The TX path currently writes descriptors and then immediately
>     writes to
>      >> the MMIO doorbell register to notify the NIC.  On weakly ordered
>      >> architectures, descriptor writes may still be pending in CPU or DMA
>      >> write buffers when the doorbell is issued, leading to the device
>      >> fetching stale or incomplete descriptors.
> 
>     Apologies for the late response, but it's not clear to me why this is
>     necessary.
> 
>     In other vendors the "doorbell record" (dbr) is writing another
>     location
>     in system memory, not an mmio write. These cases do use a dma_wmb().
> 
>     Why isn't the writeq() sufficient in our case? According to
>     Documentation/memory-barriers.txt it seems like writeq() should be
>     sufficient.
> 
>     Thanks,
> 
>     Brett
>      >>
>      >> [...]
>      >
>      > Here is the summary with links:
>      >    - [net,1/2] net: ionic: add dma_wmb() before ringing TX doorbell
>      > https://git.kernel.org/netdev/net/c/d261f5b09c28 <https://
>     git.kernel.org/netdev/net/c/d261f5b09c28>
>      >    - [net,2/2] net: ionic: map SKB after pseudo-header checksum prep
>      > https://git.kernel.org/netdev/net/c/de0337d641bf <https://
>     git.kernel.org/netdev/net/c/de0337d641bf>
>      >
>      > You are awesome, thank you!
>      > --
>      > Deet-doot-dot, I am a bot.
>      > https://korg.docs.kernel.org/patchwork/pwbot.html <https://
>     korg.docs.kernel.org/patchwork/pwbot.html>
>      >
>      >
>