Re: [net PATCH] octeontx2-pf: Recalculate UDP checksum for ptp 1-step sync packet
From: Paolo Abeni
Date: Tue Feb 21 2023 - 06:40:55 EST
On Mon, 2023-02-20 at 17:50 +0530, Sai Krishna wrote:
> From: Geetha sowjanya <gakula@xxxxxxxxxxx>
>
> When checksum offload is disabled in the driver via ethtool,
> the PTP 1-step sync packets contain incorrect checksum, since
> the stack calculates the checksum before driver updates
> PTP timestamp field in the packet. This results in PTP packets
> getting dropped at the other end. This patch fixes the issue by
> re-calculating the UDP checksum after updating PTP
> timestamp field in the driver.
>
> Fixes: 2958d17a8984 ("octeontx2-pf: Add support for ptp 1-step mode on CN10K silicon")
> Signed-off-by: Geetha sowjanya <gakula@xxxxxxxxxxx>
> Signed-off-by: Hariprasad Kelam <hkelam@xxxxxxxxxxx>
> Signed-off-by: Sunil Kovvuri Goutham <sgoutham@xxxxxxxxxxx>
> Signed-off-by: Sai Krishna <saikrishnag@xxxxxxxxxxx>
> ---
> .../marvell/octeontx2/nic/otx2_txrx.c | 78 ++++++++++++++-----
> 1 file changed, 59 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_txrx.c b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_txrx.c
> index ef10aef3cda0..67345a3e2bba 100644
> --- a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_txrx.c
> +++ b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_txrx.c
> @@ -10,6 +10,7 @@
> #include <net/tso.h>
> #include <linux/bpf.h>
> #include <linux/bpf_trace.h>
> +#include <net/ip6_checksum.h>
>
> #include "otx2_reg.h"
> #include "otx2_common.h"
> @@ -699,7 +700,7 @@ static void otx2_sqe_add_ext(struct otx2_nic *pfvf, struct otx2_snd_queue *sq,
>
> static void otx2_sqe_add_mem(struct otx2_snd_queue *sq, int *offset,
> int alg, u64 iova, int ptp_offset,
> - u64 base_ns, int udp_csum)
> + u64 base_ns, bool udp_csum_crt)
> {
> struct nix_sqe_mem_s *mem;
>
> @@ -711,7 +712,7 @@ static void otx2_sqe_add_mem(struct otx2_snd_queue *sq, int *offset,
>
> if (ptp_offset) {
> mem->start_offset = ptp_offset;
> - mem->udp_csum_crt = udp_csum;
> + mem->udp_csum_crt = !!udp_csum_crt;
> mem->base_ns = base_ns;
> mem->step_type = 1;
> }
> @@ -986,10 +987,11 @@ static bool otx2_validate_network_transport(struct sk_buff *skb)
> return false;
> }
>
> -static bool otx2_ptp_is_sync(struct sk_buff *skb, int *offset, int *udp_csum)
> +static bool otx2_ptp_is_sync(struct sk_buff *skb, int *offset, bool *udp_csum_crt)
> {
> struct ethhdr *eth = (struct ethhdr *)(skb->data);
> u16 nix_offload_hlen = 0, inner_vhlen = 0;
> + bool udp_hdr_present = false, is_sync;
> u8 *data = skb->data, *msgtype;
> __be16 proto = eth->h_proto;
> int network_depth = 0;
> @@ -1029,45 +1031,83 @@ static bool otx2_ptp_is_sync(struct sk_buff *skb, int *offset, int *udp_csum)
> if (!otx2_validate_network_transport(skb))
> return false;
>
> - *udp_csum = 1;
> *offset = nix_offload_hlen + skb_transport_offset(skb) +
> sizeof(struct udphdr);
> + udp_hdr_present = true;
> +
> }
>
> msgtype = data + *offset;
> -
> /* Check PTP messageId is SYNC or not */
> - return (*msgtype & 0xf) == 0;
> + is_sync = ((*msgtype & 0xf) == 0) ? true : false;
the above could be:
is_sync = !(*msgtype & 0xf);
possibly more readable.
> + if (is_sync) {
> + if (udp_hdr_present)
> + *udp_csum_crt = true;
or:
*udp_csum_crt = udp_hdr_present;
that will make the code more compact.
> + } else {
> + *offset = 0;
> + }
> +
> + return is_sync;
> }
>
> static void otx2_set_txtstamp(struct otx2_nic *pfvf, struct sk_buff *skb,
> struct otx2_snd_queue *sq, int *offset)
> {
> + struct ethhdr *eth = (struct ethhdr *)(skb->data);
> struct ptpv2_tstamp *origin_tstamp;
> - int ptp_offset = 0, udp_csum = 0;
> + bool udp_csum_crt = false;
> + unsigned int udphoff;
> struct timespec64 ts;
> + int ptp_offset = 0;
> + __wsum skb_csum;
> u64 iova;
>
> if (unlikely(!skb_shinfo(skb)->gso_size &&
> (skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP))) {
> - if (unlikely(pfvf->flags & OTX2_FLAG_PTP_ONESTEP_SYNC)) {
> - if (otx2_ptp_is_sync(skb, &ptp_offset, &udp_csum)) {
> - origin_tstamp = (struct ptpv2_tstamp *)
> - ((u8 *)skb->data + ptp_offset +
> - PTP_SYNC_SEC_OFFSET);
> - ts = ns_to_timespec64(pfvf->ptp->tstamp);
> - origin_tstamp->seconds_msb = htons((ts.tv_sec >> 32) & 0xffff);
> - origin_tstamp->seconds_lsb = htonl(ts.tv_sec & 0xffffffff);
> - origin_tstamp->nanoseconds = htonl(ts.tv_nsec);
> - /* Point to correction field in PTP packet */
> - ptp_offset += 8;
> + if (unlikely(pfvf->flags & OTX2_FLAG_PTP_ONESTEP_SYNC &&
> + otx2_ptp_is_sync(skb, &ptp_offset, &udp_csum_crt))) {
> + origin_tstamp = (struct ptpv2_tstamp *)
> + ((u8 *)skb->data + ptp_offset +
> + PTP_SYNC_SEC_OFFSET);
> + ts = ns_to_timespec64(pfvf->ptp->tstamp);
> + origin_tstamp->seconds_msb = htons((ts.tv_sec >> 32) & 0xffff);
> + origin_tstamp->seconds_lsb = htonl(ts.tv_sec & 0xffffffff);
> + origin_tstamp->nanoseconds = htonl(ts.tv_nsec);
> + /* Point to correction field in PTP packet */
> + ptp_offset += 8;
> +
> + /* When user disables hw checksum, stack calculates the csum,
> + * but it does not cover ptp timestamp which is added later.
> + * Recalculate the checksum manually considering the timestamp.
> + */
> + if (udp_csum_crt) {
> + struct udphdr *uh = udp_hdr(skb);
> +
> + if (skb->ip_summed != CHECKSUM_PARTIAL && uh->check != 0) {
> + udphoff = skb_transport_offset(skb);
> + uh->check = 0;
> + skb_csum = skb_checksum(skb, udphoff, skb->len - udphoff,
> + 0);
> + if (ntohs(eth->h_proto) == ETH_P_IPV6)
> + uh->check = csum_ipv6_magic(&ipv6_hdr(skb)->saddr,
> + &ipv6_hdr(skb)->daddr,
> + skb->len - udphoff,
> + ipv6_hdr(skb)->nexthdr,
> + skb_csum);
> + else
> + uh->check = csum_tcpudp_magic(ip_hdr(skb)->saddr,
> + ip_hdr(skb)->daddr,
> + skb->len - udphoff,
> + IPPROTO_UDP,
> + skb_csum);
Have you considered incremental csum updates instead? Possibly the code
could be simpler and likely faster - not sure if the latter part is
relevant in this case.
Cheers,
Paolo