[PATCH net-next RESEND] net: bcmgenet: hide status block before TX timestamping

[PATCH net-next RESEND] net: bcmgenet: hide status block before TX timestamping

[Jonathan Lemon]

The hardware checksum offloading requires use of a transmit
status block inserted before the outgoing frame data, this was
updated in '9a9ba2a4aaaa ("net: bcmgenet: always enable status blocks")'

However, skb_tx_timestamp() assumes that it is passed a raw frame
and PTP parsing chokes on this status block.

Fix this by calling __skb_pull(), which hides the TSB before calling
skb_tx_timestamp(), so an outgoing PTP packet is parsed correctly.

As the data in the skb has already been set up for DMA, and the
dma_unmap_* calls use a separately stored address, there is no
no effective change in the data transmission.

Signed-off-by: Jonathan Lemon <jonathan.lemon@gmail.com>
---
 drivers/net/ethernet/broadcom/genet/bcmgenet.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
index 9a41145dadfc..bf1ec8fdc2ad 100644
--- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
+++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
@@ -2035,6 +2035,11 @@ static struct sk_buff *bcmgenet_add_tsb(struct net_device *dev,
 	return skb;
 }
 
+static void bcmgenet_hide_tsb(struct sk_buff *skb)
+{
+	__skb_pull(skb, sizeof(struct status_64));
+}
+
 static netdev_tx_t bcmgenet_xmit(struct sk_buff *skb, struct net_device *dev)
 {
 	struct bcmgenet_priv *priv = netdev_priv(dev);
@@ -2141,6 +2146,8 @@ static netdev_tx_t bcmgenet_xmit(struct sk_buff *skb, struct net_device *dev)
 	}
 
 	GENET_CB(skb)->last_cb = tx_cb_ptr;
+
+	bcmgenet_hide_tsb(skb);
 	skb_tx_timestamp(skb);
 
 	/* Decrement total BD count and advance our write pointer */
-- 
2.31.1

Re: [PATCH net-next RESEND] net: bcmgenet: hide status block before TX timestamping

[Florian Fainelli]

On 4/24/22 09:53, Jonathan Lemon wrote:
> The hardware checksum offloading requires use of a transmit
> status block inserted before the outgoing frame data, this was
> updated in '9a9ba2a4aaaa ("net: bcmgenet: always enable status blocks")'
> 
> However, skb_tx_timestamp() assumes that it is passed a raw frame
> and PTP parsing chokes on this status block.
> 
> Fix this by calling __skb_pull(), which hides the TSB before calling
> skb_tx_timestamp(), so an outgoing PTP packet is parsed correctly.
> 
> As the data in the skb has already been set up for DMA, and the
> dma_unmap_* calls use a separately stored address, there is no
> no effective change in the data transmission.
> 
> Signed-off-by: Jonathan Lemon <jonathan.lemon@gmail.com>

Without restructuring the way we do time-stamping within the driver to 
be before bcmgenet_insert_tsb() this appears to get you what you want 
without being too intrusive. I would slap a:

Fixes: d03825fba459 ("net: bcmgenet: add skb_tx_timestamp call")

since this is a bug fix that should be fixed back when this was added. 
It was only made recently problematic by default with the enabling of 
the transmit checksum offload which implicit forces the use of a 64 byte 
transmit status block.

With that said:

Acked-by: Florian Fainelli <f.fainelli@gmail.com>

Now, if we wanted to avoid playing tricks with the sk_buff, we could 
place skb_tx_timestamp() earlier in the transmit path with the risk that 
we are no longer as close as possible from hitting the DMA, and subject 
to DMA mapping or allocation failures. Food for thought.

Thanks!
-- 
Florian

Re: [PATCH net-next RESEND] net: bcmgenet: hide status block before TX timestamping

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net.git (master)
by Paolo Abeni <pabeni@redhat.com>:

On Sun, 24 Apr 2022 09:53:07 -0700 you wrote:
> The hardware checksum offloading requires use of a transmit
> status block inserted before the outgoing frame data, this was
> updated in '9a9ba2a4aaaa ("net: bcmgenet: always enable status blocks")'
> 
> However, skb_tx_timestamp() assumes that it is passed a raw frame
> and PTP parsing chokes on this status block.
> 
> [...]

Here is the summary with links:
  - [net-next,RESEND] net: bcmgenet: hide status block before TX timestamping
    https://git.kernel.org/netdev/net/c/acac0541d1d6

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