| Message ID | 20230220122050.1639299-1-saikrishnag@marvell.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] octeontx2-pf: Recalculate UDP checksum for ptp 1-step sync packet | expand |
On Mon, 2023-02-20 at 17:50 +0530, Sai Krishna wrote: > From: Geetha sowjanya <gakula@marvell.com> > > 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@marvell.com> > Signed-off-by: Hariprasad Kelam <hkelam@marvell.com> > Signed-off-by: Sunil Kovvuri Goutham <sgoutham@marvell.com> > Signed-off-by: Sai Krishna <saikrishnag@marvell.com> > --- > .../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
Hi Paolo, Please see the responses inline. > -----Original Message----- > From: Paolo Abeni <pabeni@redhat.com> > Sent: Tuesday, February 21, 2023 5:10 PM > To: Sai Krishna Gajula <saikrishnag@marvell.com>; davem@davemloft.net; > edumazet@google.com; kuba@kernel.org; netdev@vger.kernel.org; linux- > kernel@vger.kernel.org; Sunil Kovvuri Goutham <sgoutham@marvell.com>; > Geethasowjanya Akula <gakula@marvell.com>; richardcochran@gmail.com > Cc: Hariprasad Kelam <hkelam@marvell.com> > Subject: Re: [net PATCH] octeontx2-pf: Recalculate UDP checksum for > ptp 1-step sync packet > > On Mon, 2023-02-20 at 17:50 +0530, Sai Krishna wrote: > > From: Geetha sowjanya <gakula@marvell.com> > > > > 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@marvell.com> > > Signed-off-by: Hariprasad Kelam <hkelam@marvell.com> > > Signed-off-by: Sunil Kovvuri Goutham <sgoutham@marvell.com> > > Signed-off-by: Sai Krishna <saikrishnag@marvell.com> > > --- > > .../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); > I will make changes and submit v2 patch. > 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. > I will make changes and submit v2 patch. > > + } 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. > We don't expect any significant performance improvement with this for PTP sync packets. So we didn't consider incremental csum updates. Thanks, Sai > Cheers, > > Paolo
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; + if (is_sync) { + if (udp_hdr_present) + *udp_csum_crt = true; + } 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); + } } } else { skb_shinfo(skb)->tx_flags |= SKBTX_IN_PROGRESS; } iova = sq->timestamps->iova + (sq->head * sizeof(u64)); otx2_sqe_add_mem(sq, offset, NIX_SENDMEMALG_E_SETTSTMP, iova, - ptp_offset, pfvf->ptp->base_ns, udp_csum); + ptp_offset, pfvf->ptp->base_ns, udp_csum_crt); } else { skb_tx_timestamp(skb); }