| Message ID | 20220424165307.591145-1-jonathan.lemon@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | acac0541d1d65e81e599ec399d34d184d2424401 |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next,RESEND] net: bcmgenet: hide status block before TX timestamping | expand |
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!
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!
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 */
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(+)