netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2 net 0/2] dpaa2-eth: various fixes
@ 2023-11-24 10:28 Ioana Ciornei
  2023-11-24 10:28 ` [PATCH v2 net 1/2] dpaa2-eth: increase the needed headroom to account for alignment Ioana Ciornei
                   ` (2 more replies)
  0 siblings, 3 replies; 5+ messages in thread
From: Ioana Ciornei @ 2023-11-24 10:28 UTC (permalink / raw)
  To: davem, edumazet, kuba, pabeni; +Cc: andrew, netdev, Ioana Ciornei

The first patch fixes a memory corruption issue happening between the Tx
and Tx confirmation of a packet by making the Tx alignment at 64bytes
mandatory instead of optional as it was previously.

The second patch fixes the Rx copybreak code path which recycled the
initial data buffer before all processing was done on the packet.

Changes in v2:
- squashed patches #1 and #2

Ioana Ciornei (2):
  dpaa2-eth: increase the needed headroom to account for alignment
  dpaa2-eth: recycle the RX buffer only after all processing done

 drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c | 16 ++++++++++------
 drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.h |  2 +-
 2 files changed, 11 insertions(+), 7 deletions(-)

-- 
2.25.1


^ permalink raw reply	[flat|nested] 5+ messages in thread

* [PATCH v2 net 1/2] dpaa2-eth: increase the needed headroom to account for alignment
  2023-11-24 10:28 [PATCH v2 net 0/2] dpaa2-eth: various fixes Ioana Ciornei
@ 2023-11-24 10:28 ` Ioana Ciornei
  2024-01-24  6:02   ` Mathew McBride
  2023-11-24 10:28 ` [PATCH v2 net 2/2] dpaa2-eth: recycle the RX buffer only after all processing done Ioana Ciornei
  2023-11-26 15:20 ` [PATCH v2 net 0/2] dpaa2-eth: various fixes patchwork-bot+netdevbpf
  2 siblings, 1 reply; 5+ messages in thread
From: Ioana Ciornei @ 2023-11-24 10:28 UTC (permalink / raw)
  To: davem, edumazet, kuba, pabeni; +Cc: andrew, netdev, Ioana Ciornei

Increase the needed headroom to account for a 64 byte alignment
restriction which, with this patch, we make mandatory on the Tx path.
The case in which the amount of headroom needed is not available is
already handled by the driver which instead sends a S/G frame with the
first buffer only holding the SW and HW annotation areas.

Without this patch, we can empirically see data corruption happening
between Tx and Tx confirmation which sometimes leads to the SW
annotation area being overwritten.

Since this is an old IP where the hardware team cannot help to
understand the underlying behavior, we make the Tx alignment mandatory
for all frames to avoid the crash on Tx conf. Also, remove the comment
that suggested that this is just an optimization.

This patch also sets the needed_headroom net device field to the usual
value that the driver would need on the Tx path:
	- 64 bytes for the software annotation area
	- 64 bytes to account for a 64 byte aligned buffer address

Fixes: 6e2387e8f19e ("staging: fsl-dpaa2/eth: Add Freescale DPAA2 Ethernet driver")
Closes: https://lore.kernel.org/netdev/aa784d0c-85eb-4e5d-968b-c8f74fa86be6@gin.de/
Signed-off-by: Ioana Ciornei <ioana.ciornei@nxp.com>
---
Changes in v2:
- squashed patches #1 and #2

 drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c | 8 ++++----
 drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.h | 2 +-
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c b/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c
index 15bab41cee48..774377db0b4b 100644
--- a/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c
+++ b/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c
@@ -1073,14 +1073,12 @@ static int dpaa2_eth_build_single_fd(struct dpaa2_eth_priv *priv,
 	dma_addr_t addr;
 
 	buffer_start = skb->data - dpaa2_eth_needed_headroom(skb);
-
-	/* If there's enough room to align the FD address, do it.
-	 * It will help hardware optimize accesses.
-	 */
 	aligned_start = PTR_ALIGN(buffer_start - DPAA2_ETH_TX_BUF_ALIGN,
 				  DPAA2_ETH_TX_BUF_ALIGN);
 	if (aligned_start >= skb->head)
 		buffer_start = aligned_start;
+	else
+		return -ENOMEM;
 
 	/* Store a backpointer to the skb at the beginning of the buffer
 	 * (in the private data area) such that we can release it
@@ -4967,6 +4965,8 @@ static int dpaa2_eth_probe(struct fsl_mc_device *dpni_dev)
 	if (err)
 		goto err_dl_port_add;
 
+	net_dev->needed_headroom = DPAA2_ETH_SWA_SIZE + DPAA2_ETH_TX_BUF_ALIGN;
+
 	err = register_netdev(net_dev);
 	if (err < 0) {
 		dev_err(dev, "register_netdev() failed\n");
diff --git a/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.h b/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.h
index bfb6c96c3b2f..834cba8c3a41 100644
--- a/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.h
+++ b/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.h
@@ -740,7 +740,7 @@ static inline bool dpaa2_eth_rx_pause_enabled(u64 link_options)
 
 static inline unsigned int dpaa2_eth_needed_headroom(struct sk_buff *skb)
 {
-	unsigned int headroom = DPAA2_ETH_SWA_SIZE;
+	unsigned int headroom = DPAA2_ETH_SWA_SIZE + DPAA2_ETH_TX_BUF_ALIGN;
 
 	/* If we don't have an skb (e.g. XDP buffer), we only need space for
 	 * the software annotation area
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 5+ messages in thread

* [PATCH v2 net 2/2] dpaa2-eth: recycle the RX buffer only after all processing done
  2023-11-24 10:28 [PATCH v2 net 0/2] dpaa2-eth: various fixes Ioana Ciornei
  2023-11-24 10:28 ` [PATCH v2 net 1/2] dpaa2-eth: increase the needed headroom to account for alignment Ioana Ciornei
@ 2023-11-24 10:28 ` Ioana Ciornei
  2023-11-26 15:20 ` [PATCH v2 net 0/2] dpaa2-eth: various fixes patchwork-bot+netdevbpf
  2 siblings, 0 replies; 5+ messages in thread
From: Ioana Ciornei @ 2023-11-24 10:28 UTC (permalink / raw)
  To: davem, edumazet, kuba, pabeni; +Cc: andrew, netdev, Ioana Ciornei

The blamed commit added support for Rx copybreak. This meant that for
certain frame sizes, a new skb was allocated and the initial data buffer
was recycled. Instead of waiting to recycle the Rx buffer only after all
processing was done on it (like accessing the parse results or timestamp
information), the code path just went ahead and re-used the buffer right
away.

This sometimes lead to corrupted HW and SW annotation areas.
Fix this by delaying the moment when the buffer is recycled.

Fixes: 50f826999a80 ("dpaa2-eth: add rx copybreak support")
Signed-off-by: Ioana Ciornei <ioana.ciornei@nxp.com>
---
Changes in v2:
- none

 drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c b/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c
index 774377db0b4b..888509cf1f21 100644
--- a/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c
+++ b/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c
@@ -516,8 +516,6 @@ struct sk_buff *dpaa2_eth_alloc_skb(struct dpaa2_eth_priv *priv,
 
 	memcpy(skb->data, fd_vaddr + fd_offset, fd_length);
 
-	dpaa2_eth_recycle_buf(priv, ch, dpaa2_fd_get_addr(fd));
-
 	return skb;
 }
 
@@ -589,6 +587,7 @@ void dpaa2_eth_rx(struct dpaa2_eth_priv *priv,
 	struct rtnl_link_stats64 *percpu_stats;
 	struct dpaa2_eth_drv_stats *percpu_extras;
 	struct device *dev = priv->net_dev->dev.parent;
+	bool recycle_rx_buf = false;
 	void *buf_data;
 	u32 xdp_act;
 
@@ -618,6 +617,8 @@ void dpaa2_eth_rx(struct dpaa2_eth_priv *priv,
 			dma_unmap_page(dev, addr, priv->rx_buf_size,
 				       DMA_BIDIRECTIONAL);
 			skb = dpaa2_eth_build_linear_skb(ch, fd, vaddr);
+		} else {
+			recycle_rx_buf = true;
 		}
 	} else if (fd_format == dpaa2_fd_sg) {
 		WARN_ON(priv->xdp_prog);
@@ -637,6 +638,9 @@ void dpaa2_eth_rx(struct dpaa2_eth_priv *priv,
 		goto err_build_skb;
 
 	dpaa2_eth_receive_skb(priv, ch, fd, vaddr, fq, percpu_stats, skb);
+
+	if (recycle_rx_buf)
+		dpaa2_eth_recycle_buf(priv, ch, dpaa2_fd_get_addr(fd));
 	return;
 
 err_build_skb:
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH v2 net 0/2] dpaa2-eth: various fixes
  2023-11-24 10:28 [PATCH v2 net 0/2] dpaa2-eth: various fixes Ioana Ciornei
  2023-11-24 10:28 ` [PATCH v2 net 1/2] dpaa2-eth: increase the needed headroom to account for alignment Ioana Ciornei
  2023-11-24 10:28 ` [PATCH v2 net 2/2] dpaa2-eth: recycle the RX buffer only after all processing done Ioana Ciornei
@ 2023-11-26 15:20 ` patchwork-bot+netdevbpf
  2 siblings, 0 replies; 5+ messages in thread
From: patchwork-bot+netdevbpf @ 2023-11-26 15:20 UTC (permalink / raw)
  To: Ioana Ciornei; +Cc: davem, edumazet, kuba, pabeni, andrew, netdev

Hello:

This series was applied to netdev/net.git (main)
by David S. Miller <davem@davemloft.net>:

On Fri, 24 Nov 2023 12:28:03 +0200 you wrote:
> The first patch fixes a memory corruption issue happening between the Tx
> and Tx confirmation of a packet by making the Tx alignment at 64bytes
> mandatory instead of optional as it was previously.
> 
> The second patch fixes the Rx copybreak code path which recycled the
> initial data buffer before all processing was done on the packet.
> 
> [...]

Here is the summary with links:
  - [v2,net,1/2] dpaa2-eth: increase the needed headroom to account for alignment
    https://git.kernel.org/netdev/net/c/f422abe3f23d
  - [v2,net,2/2] dpaa2-eth: recycle the RX buffer only after all processing done
    https://git.kernel.org/netdev/net/c/beb1930f966d

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html



^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH v2 net 1/2] dpaa2-eth: increase the needed headroom to account for alignment
  2023-11-24 10:28 ` [PATCH v2 net 1/2] dpaa2-eth: increase the needed headroom to account for alignment Ioana Ciornei
@ 2024-01-24  6:02   ` Mathew McBride
  0 siblings, 0 replies; 5+ messages in thread
From: Mathew McBride @ 2024-01-24  6:02 UTC (permalink / raw)
  To: Ioana Ciornei; +Cc: netdev

Hi Ioana,

On Fri, Nov 24, 2023, at 9:28 PM, Ioana Ciornei wrote:
> Increase the needed headroom to account for a 64 byte alignment
> restriction which, with this patch, we make mandatory on the Tx path.
> The case in which the amount of headroom needed is not available is
> already handled by the driver which instead sends a S/G frame with the
> first buffer only holding the SW and HW annotation areas.

This change appears to have broken data flow from virtual machines, via vhost-net that egress (across a bridge) via a dpaa2 Ethernet port

For example:

brctl addbr br-vm
brctl addif br-vm eth0
ip link set br-vm up
ip link set eth0 up
echo 'allow br-vm' > /etc/qemu/bridge.conf

qemu-system-aarch64 -nographic -M virt -cpu host --enable-kvm \
        -drive file=openwrt-armsr-armv8-generic-ext4-combined.img,format=raw,index=0,media=disk \
        -device "virtio-net,netdev=landev,disable-legacy=off,disable-modern=off" \
        -netdev "tap,id=landev,helper=/usr/lib/qemu/qemu-bridge-helper --br=br-vm,vhost=on"

I have reproduced the issue across different series of kernels (e.g the issue appears in 6.1.66 when this change was backported) and different host and guest userspaces (Debian and OpenWrt). The platform is our LS1088A board (Ten64).

The VM is able to receive frames from the bridged interface, but no Ethernet frames that originate from the VM will be transmitted by the dpaa2_eth interface.

If vhost-net acceleration is disabled (vhost=off on the QEMU command line), the VM is able to communicate with the network, but with reduced performance.

The frames in question fail the TX alignment check in dpaa2_eth_build_single_fd [if (aligned_start >= skb->head)] ( https://elixir.bootlin.com/linux/v6.8-rc1/source/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c#L1084 )

fsl_dpaa2_eth dpni.9: dpaa2_eth_build_single_fd alignment issue, aligned_start=ffff008002e09140 skb->head=ffff008002e09180
fsl_dpaa2_eth dpni.9: dpaa2_eth_build_single_fd data=ffff008002e09200
fsl_dpaa2_eth dpni.9: dpaa2_eth_build_single_fd is cloned=0
dpaa2_eth_build_single_fdskb len=150 headroom=128 headlen=150 tailroom=42
mac=(128,14) net=(142,48) trans=190
shinfo(txflags=0 nr_frags=0 gso(size=0 type=0 segs=0))
csum(0xd19f ip_summed=0 complete_sw=1 valid=1 level=0)
hash(0x0 sw=0 l4=0) proto=0x86dd pkttype=2 iif=15
dpaa2_eth_build_single_fddev name=eth0 feat=0x0002010000015833
dpaa2_eth_build_single_fdskb headroom: 00000000: 80 02 00 00 00 00 00 00 00 00 00 00 00 00 00 00
dpaa2_eth_build_single_fdskb headroom: 00000010: 00 00 00 00 00 00 00 02 41 a4 01 00 00 00 00 00
dpaa2_eth_build_single_fdskb headroom: 00000020: 01 00 00 0d 14 00 00 00 00 00 00 00 38 a3 01 00
dpaa2_eth_build_single_fdskb headroom: 00000030: 00 00 00 00 00 00 00 02 43 a4 01 00 00 00 00 00
dpaa2_eth_build_single_fdskb headroom: 00000040: 02 00 00 0d 14 00 00 00 00 00 00 00 38 a3 01 00
dpaa2_eth_build_single_fdskb headroom: 00000050: 00 00 00 00 8c a2 01 00 00 00 00 00 00 00 00 02
dpaa2_eth_build_single_fdskb headroom: 00000060: 45 a4 01 00 00 00 00 00 02 00 00 0d 00 00 00 00
dpaa2_eth_build_single_fdskb headroom: 00000070: 00 00 00 00 38 a3 01 00 00 00 00 00 8c a2 01 00
dpaa2_eth_build_single_fdskb linear:   00000000: 33 33 00 00 00 16 52 54 00 12 34 56 86 dd 60 00
dpaa2_eth_build_single_fdskb linear:   00000010: 00 00 00 60 00 01 00 00 00 00 00 00 00 00 00 00
dpaa2_eth_build_single_fdskb linear:   00000020: 00 00 00 00 00 00 ff 02 00 00 00 00 00 00 00 00
dpaa2_eth_build_single_fdskb linear:   00000030: 00 00 00 00 00 16 3a 00 05 02 00 00 01 00 8f 00
dpaa2_eth_build_single_fdskb linear:   00000040: 33 d1 00 00 00 04 03 00 00 00 ff 02 00 00 00 00
dpaa2_eth_build_single_fdskb linear:   00000050: 00 00 00 00 00 01 ff 00 00 00 04 00 00 00 ff 02
dpaa2_eth_build_single_fdskb linear:   00000060: 00 00 00 00 00 00 00 00 00 01 ff 12 34 56 04 00
dpaa2_eth_build_single_fdskb linear:   00000070: 00 00 ff 05 00 00 00 00 00 00 00 00 00 00 00 00
dpaa2_eth_build_single_fdskb linear:   00000080: 00 02 04 00 00 00 ff 02 00 00 00 00 00 00 00 00
dpaa2_eth_build_single_fdskb linear:   00000090: 00 00 00 00 00 02
dpaa2_eth_build_single_fdskb tailroom: 00000000: 08 00 45 10 00 68 cf fa 40 00 40 06 fd 66 ac 10
dpaa2_eth_build_single_fdskb tailroom: 00000010: 0a 80 ac 10 0a 7e 00 16 48 2a 6a 6d 3f 42 26 8c
dpaa2_eth_build_single_fdskb tailroom: 00000020: ae 31 50 18 07 7a 6d 79 00 00

From the patch description, it sounds like this situation should be handled by converting to a S/G frame in __dpaa2_eth_tx ? Or is the problem elsewhere?

Best Regards,
Matt

> Without this patch, we can empirically see data corruption happening
> between Tx and Tx confirmation which sometimes leads to the SW
> annotation area being overwritten.
> 
> Since this is an old IP where the hardware team cannot help to
> understand the underlying behavior, we make the Tx alignment mandatory
> for all frames to avoid the crash on Tx conf. Also, remove the comment
> that suggested that this is just an optimization.
> 
> This patch also sets the needed_headroom net device field to the usual
> value that the driver would need on the Tx path:
> - 64 bytes for the software annotation area
> - 64 bytes to account for a 64 byte aligned buffer address
> 
> Fixes: 6e2387e8f19e ("staging: fsl-dpaa2/eth: Add Freescale DPAA2 Ethernet driver")
> Closes: https://lore.kernel.org/netdev/aa784d0c-85eb-4e5d-968b-c8f74fa86be6@gin.de/
> Signed-off-by: Ioana Ciornei <ioana.ciornei@nxp.com>
> ---
> Changes in v2:
> - squashed patches #1 and #2
> 
> drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c | 8 ++++----
> drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.h | 2 +-
> 2 files changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c b/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c
> index 15bab41cee48..774377db0b4b 100644
> --- a/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c
> +++ b/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c
> @@ -1073,14 +1073,12 @@ static int dpaa2_eth_build_single_fd(struct dpaa2_eth_priv *priv,
> dma_addr_t addr;
>  
> buffer_start = skb->data - dpaa2_eth_needed_headroom(skb);
> -
> - /* If there's enough room to align the FD address, do it.
> - * It will help hardware optimize accesses.
> - */
> aligned_start = PTR_ALIGN(buffer_start - DPAA2_ETH_TX_BUF_ALIGN,
>   DPAA2_ETH_TX_BUF_ALIGN);
> if (aligned_start >= skb->head)
> buffer_start = aligned_start;
> + else
> + return -ENOMEM;
>  
> /* Store a backpointer to the skb at the beginning of the buffer
> * (in the private data area) such that we can release it
> @@ -4967,6 +4965,8 @@ static int dpaa2_eth_probe(struct fsl_mc_device *dpni_dev)
> if (err)
> goto err_dl_port_add;
>  
> + net_dev->needed_headroom = DPAA2_ETH_SWA_SIZE + DPAA2_ETH_TX_BUF_ALIGN;
> +
> err = register_netdev(net_dev);
> if (err < 0) {
> dev_err(dev, "register_netdev() failed\n");
> diff --git a/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.h b/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.h
> index bfb6c96c3b2f..834cba8c3a41 100644
> --- a/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.h
> +++ b/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.h
> @@ -740,7 +740,7 @@ static inline bool dpaa2_eth_rx_pause_enabled(u64 link_options)
>  
> static inline unsigned int dpaa2_eth_needed_headroom(struct sk_buff *skb)
> {
> - unsigned int headroom = DPAA2_ETH_SWA_SIZE;
> + unsigned int headroom = DPAA2_ETH_SWA_SIZE + DPAA2_ETH_TX_BUF_ALIGN;
>  
> /* If we don't have an skb (e.g. XDP buffer), we only need space for
> * the software annotation area
> 

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2024-01-24  6:03 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-11-24 10:28 [PATCH v2 net 0/2] dpaa2-eth: various fixes Ioana Ciornei
2023-11-24 10:28 ` [PATCH v2 net 1/2] dpaa2-eth: increase the needed headroom to account for alignment Ioana Ciornei
2024-01-24  6:02   ` Mathew McBride
2023-11-24 10:28 ` [PATCH v2 net 2/2] dpaa2-eth: recycle the RX buffer only after all processing done Ioana Ciornei
2023-11-26 15:20 ` [PATCH v2 net 0/2] dpaa2-eth: various fixes patchwork-bot+netdevbpf

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).