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

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

[mheib]

From: Mohammad Heib <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.

Add a dma_wmb() in ionic_txq_post() to ensure all descriptor writes are
visible to the device before the doorbell MMIO write.

Fixes: 0f3154e6bcb3 ("ionic: Add Tx and Rx handling")
Signed-off-by: Mohammad Heib <mheib@redhat.com>
---
 drivers/net/ethernet/pensando/ionic/ionic_txrx.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/net/ethernet/pensando/ionic/ionic_txrx.c b/drivers/net/ethernet/pensando/ionic/ionic_txrx.c
index d10b58ebf603..2e571d0a0d8a 100644
--- a/drivers/net/ethernet/pensando/ionic/ionic_txrx.c
+++ b/drivers/net/ethernet/pensando/ionic/ionic_txrx.c
@@ -29,6 +29,10 @@ static void ionic_tx_clean(struct ionic_queue *q,
 
 static inline void ionic_txq_post(struct ionic_queue *q, bool ring_dbell)
 {
+	/* Ensure TX descriptor writes reach memory before NIC reads them.
+	 * Prevents device from fetching stale descriptors.
+	 */
+	dma_wmb();
 	ionic_q_post(q, ring_dbell);
 }
 
-- 
2.50.1

[PATCH net 2/2] net: ionic: map SKB after pseudo-header checksum prep

[mheib]

From: Mohammad Heib <mheib@redhat.com>

The TSO path called ionic_tx_map_skb() before preparing the TCP pseudo
checksum (ionic_tx_tcp_[inner_]pseudo_csum()), which may perform
skb_cow_head() and might modifies bytes in the linear header area.

Mapping first and then mutating the header risks:
  - Using a stale DMA address if skb_cow_head() relocates the head, and/or
  - Device reading stale header bytes on weakly-ordered systems
    (CPU writes after mapping are not guaranteed visible without an
    explicit dma_sync_single_for_device()).

Reorder the TX path to perform all header mutations (including
skb_cow_head()) *before* DMA mapping. Mapping is now done only after the
skb layout and header contents are final. This removes the need for any
post-mapping dma_sync and prevents on-wire corruption observed under
VLAN+TSO load after repeated runs.

This change is purely an ordering fix; no functional behavior change
otherwise.

Fixes: 0f3154e6bcb3 ("ionic: Add Tx and Rx handling")
Signed-off-by: Mohammad Heib <mheib@redhat.com>
---
 .../net/ethernet/pensando/ionic/ionic_txrx.c  | 30 ++++++++-----------
 1 file changed, 13 insertions(+), 17 deletions(-)

diff --git a/drivers/net/ethernet/pensando/ionic/ionic_txrx.c b/drivers/net/ethernet/pensando/ionic/ionic_txrx.c
index 2e571d0a0d8a..301ebee2fdc5 100644
--- a/drivers/net/ethernet/pensando/ionic/ionic_txrx.c
+++ b/drivers/net/ethernet/pensando/ionic/ionic_txrx.c
@@ -1448,19 +1448,6 @@ static int ionic_tx_tso(struct net_device *netdev, struct ionic_queue *q,
 	bool encap;
 	int err;
 
-	desc_info = &q->tx_info[q->head_idx];
-
-	if (unlikely(ionic_tx_map_skb(q, skb, desc_info)))
-		return -EIO;
-
-	len = skb->len;
-	mss = skb_shinfo(skb)->gso_size;
-	outer_csum = (skb_shinfo(skb)->gso_type & (SKB_GSO_GRE |
-						   SKB_GSO_GRE_CSUM |
-						   SKB_GSO_IPXIP4 |
-						   SKB_GSO_IPXIP6 |
-						   SKB_GSO_UDP_TUNNEL |
-						   SKB_GSO_UDP_TUNNEL_CSUM));
 	has_vlan = !!skb_vlan_tag_present(skb);
 	vlan_tci = skb_vlan_tag_get(skb);
 	encap = skb->encapsulation;
@@ -1474,12 +1461,21 @@ static int ionic_tx_tso(struct net_device *netdev, struct ionic_queue *q,
 		err = ionic_tx_tcp_inner_pseudo_csum(skb);
 	else
 		err = ionic_tx_tcp_pseudo_csum(skb);
-	if (unlikely(err)) {
-		/* clean up mapping from ionic_tx_map_skb */
-		ionic_tx_desc_unmap_bufs(q, desc_info);
+	if (unlikely(err))
 		return err;
-	}
 
+	desc_info = &q->tx_info[q->head_idx];
+	if (unlikely(ionic_tx_map_skb(q, skb, desc_info)))
+		return -EIO;
+
+	len = skb->len;
+	mss = skb_shinfo(skb)->gso_size;
+	outer_csum = (skb_shinfo(skb)->gso_type & (SKB_GSO_GRE |
+						   SKB_GSO_GRE_CSUM |
+						   SKB_GSO_IPXIP4 |
+						   SKB_GSO_IPXIP6 |
+						   SKB_GSO_UDP_TUNNEL |
+						   SKB_GSO_UDP_TUNNEL_CSUM));
 	if (encap)
 		hdrlen = skb_inner_tcp_all_headers(skb);
 	else
-- 
2.50.1

Re: [PATCH net 2/2] net: ionic: map SKB after pseudo-header checksum prep

[Brett Creeley]

On 10/31/2025 8:52 AM, mheib@redhat.com wrote:
> Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
> 
> 
> From: Mohammad Heib <mheib@redhat.com>
> 
> The TSO path called ionic_tx_map_skb() before preparing the TCP pseudo
> checksum (ionic_tx_tcp_[inner_]pseudo_csum()), which may perform
> skb_cow_head() and might modifies bytes in the linear header area.
> 
> Mapping first and then mutating the header risks:
>    - Using a stale DMA address if skb_cow_head() relocates the head, and/or
>    - Device reading stale header bytes on weakly-ordered systems
>      (CPU writes after mapping are not guaranteed visible without an
>      explicit dma_sync_single_for_device()).
> 
> Reorder the TX path to perform all header mutations (including
> skb_cow_head()) *before* DMA mapping. Mapping is now done only after the
> skb layout and header contents are final. This removes the need for any
> post-mapping dma_sync and prevents on-wire corruption observed under
> VLAN+TSO load after repeated runs.
> 
> This change is purely an ordering fix; no functional behavior change
> otherwise.
> 
> Fixes: 0f3154e6bcb3 ("ionic: Add Tx and Rx handling")
> Signed-off-by: Mohammad Heib <mheib@redhat.com>
> ---
>   .../net/ethernet/pensando/ionic/ionic_txrx.c  | 30 ++++++++-----------
>   1 file changed, 13 insertions(+), 17 deletions(-)

Thanks for the fix! LGTM.

Reviewed-by: Brett Creeley <brett.creeley@amd.com>

> 
> diff --git a/drivers/net/ethernet/pensando/ionic/ionic_txrx.c b/drivers/net/ethernet/pensando/ionic/ionic_txrx.c
> index 2e571d0a0d8a..301ebee2fdc5 100644
> --- a/drivers/net/ethernet/pensando/ionic/ionic_txrx.c
> +++ b/drivers/net/ethernet/pensando/ionic/ionic_txrx.c
> @@ -1448,19 +1448,6 @@ static int ionic_tx_tso(struct net_device *netdev, struct ionic_queue *q,
>          bool encap;
>          int err;
> 
> -       desc_info = &q->tx_info[q->head_idx];
> -
> -       if (unlikely(ionic_tx_map_skb(q, skb, desc_info)))
> -               return -EIO;
> -
> -       len = skb->len;
> -       mss = skb_shinfo(skb)->gso_size;
> -       outer_csum = (skb_shinfo(skb)->gso_type & (SKB_GSO_GRE |
> -                                                  SKB_GSO_GRE_CSUM |
> -                                                  SKB_GSO_IPXIP4 |
> -                                                  SKB_GSO_IPXIP6 |
> -                                                  SKB_GSO_UDP_TUNNEL |
> -                                                  SKB_GSO_UDP_TUNNEL_CSUM));
>          has_vlan = !!skb_vlan_tag_present(skb);
>          vlan_tci = skb_vlan_tag_get(skb);
>          encap = skb->encapsulation;
> @@ -1474,12 +1461,21 @@ static int ionic_tx_tso(struct net_device *netdev, struct ionic_queue *q,
>                  err = ionic_tx_tcp_inner_pseudo_csum(skb);
>          else
>                  err = ionic_tx_tcp_pseudo_csum(skb);
> -       if (unlikely(err)) {
> -               /* clean up mapping from ionic_tx_map_skb */
> -               ionic_tx_desc_unmap_bufs(q, desc_info);
> +       if (unlikely(err))
>                  return err;
> -       }
> 
> +       desc_info = &q->tx_info[q->head_idx];
> +       if (unlikely(ionic_tx_map_skb(q, skb, desc_info)))
> +               return -EIO;
> +
> +       len = skb->len;
> +       mss = skb_shinfo(skb)->gso_size;
> +       outer_csum = (skb_shinfo(skb)->gso_type & (SKB_GSO_GRE |
> +                                                  SKB_GSO_GRE_CSUM |
> +                                                  SKB_GSO_IPXIP4 |
> +                                                  SKB_GSO_IPXIP6 |
> +                                                  SKB_GSO_UDP_TUNNEL |
> +                                                  SKB_GSO_UDP_TUNNEL_CSUM));
>          if (encap)
>                  hdrlen = skb_inner_tcp_all_headers(skb);
>          else
> --
> 2.50.1
> 

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

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Fri, 31 Oct 2025 17:52:02 +0200 you wrote:
> From: Mohammad Heib <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.
> 
> [...]

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
  - [net,2/2] net: ionic: map SKB after pseudo-header checksum prep
    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

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

[Brett Creeley]

On 11/3/2025 5:30 PM, patchwork-bot+netdevbpf@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>:
> 
> On Fri, 31 Oct 2025 17:52:02 +0200 you wrote:
>> From: Mohammad Heib <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
>    - [net,2/2] net: ionic: map SKB after pseudo-header checksum prep
>      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
> 
> 

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

[mohammad heib]

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>
>      >
>      >
>