Re: [PATCH net-next v2 08/11] net: ptp: add helper for one-step P2P clocks
From: Vladimir Oltean <olteanv@gmail.com>
Date: 2020-11-13 00:51:33
Also in:
linux-devicetree, lkml
On Thu, Nov 12, 2020 at 04:35:34PM +0100, Christian Eggers wrote:
This function subtracts the ingress hardware time stamp from the correction field of a PTP header and updates the UDP checksum (if UDP is used as transport. It is needed for hardware capable of one-step P2P
Please close the parenthesis.
that does not already modify the correction field of Pdelay_Req event messages on ingress. Signed-off-by: Christian Eggers <ceggers@arri.de>
Please add more verbiage here, giving the reader as much context as possible. You are establishing a de-facto approach for one-step peer delay timestamping for hardware like yours, you need to explain why it is done this way, for people to understand just by looking at git blame.
quoted hunk ↗ jump to hunk
--- include/linux/ptp_classify.h | 97 ++++++++++++++++++++++++++++++++++++ 1 file changed, 97 insertions(+)diff --git a/include/linux/ptp_classify.h b/include/linux/ptp_classify.h index 56b2d7d66177..f27b512e1abd 100644 --- a/include/linux/ptp_classify.h +++ b/include/linux/ptp_classify.h@@ -10,8 +10,12 @@ #ifndef _PTP_CLASSIFY_H_ #define _PTP_CLASSIFY_H_ +#include <asm/unaligned.h> #include <linux/ip.h> +#include <linux/ktime.h> #include <linux/skbuff.h> +#include <linux/udp.h> +#include <net/checksum.h> #define PTP_CLASS_NONE 0x00 /* not a PTP event message */ #define PTP_CLASS_V1 0x01 /* protocol version 1 */@@ -118,6 +122,91 @@ static inline u8 ptp_get_msgtype(const struct ptp_header *hdr, return msgtype; } +/** + * ptp_check_diff8 - Computes new checksum (when altering a 64-bit field) + * @old: old field value + * @new: new field value + * @oldsum: previous checksum + * + * This function can be used to calculate a new checksum when only a single + * field is changed. Similar as ip_vs_check_diff*() in ip_vs.h. + * + * Return: Updated checksum + */ +static inline __wsum ptp_check_diff8(__be64 old, __be64 new, __wsum oldsum) +{ + __be64 diff[2] = { ~old, new }; + + return csum_partial(diff, sizeof(diff), oldsum); +} + +/** + * ptp_onestep_p2p_move_t2_to_correction - Update PTP header's correction field + * @skb: packet buffer + * @type: type of the packet (see ptp_classify_raw()) + * @hdr: ptp header + * @t2: ingress hardware time stamp + * + * This function subtracts the ingress hardware time stamp from the correction + * field of a PTP header and updates the UDP checksum (if UDP is used as + * transport). It is needed for hardware capable of one-step P2P that does not + * already modify the correction field of Pdelay_Req event messages on ingress. + */ +static inline +void ptp_onestep_p2p_move_t2_to_correction(struct sk_buff *skb, + unsigned int type, + struct ptp_header *hdr, + ktime_t t2)
The way this function is coded up right now, there's no reason to call it: - onestep - p2p - move_t2_to_correction As it is, it would be better named ptp_header_update_correction.
+{
+ u8 *ptr = skb_mac_header(skb);
+ struct udphdr *uhdr = NULL;
+ s64 ns = ktime_to_ns(t2);
+ __be64 correction_old;
+ s64 correction;
+
+ /* previous correction value is required for checksum update. */
+ memcpy(&correction_old, &hdr->correction, sizeof(correction_old));
+ correction = (s64)be64_to_cpu(correction_old);
+
+ /* PTP correction field consists of 32 bit nanoseconds and 16 bit48 bit nanoseconds
+ * fractional nanoseconds. Avoid shifting negative numbers. + */ + if (ns >= 0) + correction -= ns << 16; + else + correction += -ns << 16;
Again? Why? There's nothing wrong with left-shifting negative numbers, two's complement works the same (note that right-shifting is a different story, but that's not the case here).
+ + /* write new correction value */ + put_unaligned_be64((u64)correction, &hdr->correction); + + /* locate udp header */ + if (type & PTP_CLASS_VLAN) + ptr += VLAN_HLEN;
Can't you just go back from the struct ptp_header pointer and avoid this check?
+ ptr += ETH_HLEN;
+
+ switch (type & PTP_CLASS_PMASK) {
+ case PTP_CLASS_IPV4:
+ ptr += ((struct iphdr *)ptr)->ihl << 2;
+ uhdr = (struct udphdr *)ptr;
+ break;
+ case PTP_CLASS_IPV6:
+ ptr += IP6_HLEN;
+ uhdr = (struct udphdr *)ptr;
+ break;
+ }
+
+ if (!uhdr)
+ return;
+
+ /* update checksum */
+ uhdr->check = csum_fold(ptp_check_diff8(correction_old,
+ hdr->correction,
+ ~csum_unfold(uhdr->check)));
+ if (!uhdr->check)
+ uhdr->check = CSUM_MANGLED_0;
+}
+