Re: [Patch net-next v7 06/13] net: ptp: add helper for one-step P2P clocks

[Paolo Abeni]

On Thu, 2023-01-05 at 11:09 +0100, Paolo Abeni wrote:
> Hi,
> 
> On Wed, 2023-01-04 at 14:13 +0530, Arun Ramadoss wrote:
> > From: Christian Eggers <ceggers@xxxxxxx>
> > 
> > For P2P delay measurement, the ingress time stamp of the PDelay_Req is
> > required for the correction field of the PDelay_Resp. The application
> > echoes back the correction field of the PDelay_Req when sending the
> > PDelay_Resp.
> > 
> > Some hardware (like the ZHAW InES PTP time stamping IP core) subtracts
> > the ingress timestamp autonomously from the correction field, so that
> > the hardware only needs to add the egress timestamp on tx. Other
> > hardware (like the Microchip KSZ9563) reports the ingress time stamp via
> > an interrupt and requires that the software provides this time stamp via
> > tail-tag on tx.
> > 
> > In order to avoid introducing a further application interface for this,
> > the driver can simply emulate the behavior of the InES device and
> > subtract the ingress time stamp in software from the correction field.
> > 
> > On egress, the correction field can either be kept as it is (and the
> > time stamp field in the tail-tag is set to zero) or move the value from
> > the correction field back to the tail-tag.
> > 
> > Changing the correction field requires updating the UDP checksum (if UDP
> > is used as transport).
> > 
> > Signed-off-by: Christian Eggers <ceggers@xxxxxxx>
> > Co-developed-by: Arun Ramadoss <arun.ramadoss@xxxxxxxxxxxxx>
> > Signed-off-by: Arun Ramadoss <arun.ramadoss@xxxxxxxxxxxxx>
> > ---
> > v1 -> v2
> > - Fixed compilation issue when PTP_CLASSIFY not selected in menuconfig
> > as reported by kernel test robot <lkp@xxxxxxxxx>
> > ---
> >  include/linux/ptp_classify.h | 71 ++++++++++++++++++++++++++++++++++++
> >  1 file changed, 71 insertions(+)
> > 
> > diff --git a/include/linux/ptp_classify.h b/include/linux/ptp_classify.h
> > index 2b6ea36ad162..6e5869c2504c 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 */
> > @@ -129,6 +133,67 @@ 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_header_update_correction - Update PTP header's correction field
> > + * @skb: packet buffer
> > + * @type: type of the packet (see ptp_classify_raw())
> > + * @hdr: ptp header
> > + * @correction: new correction value
> > + *
> > + * This updates 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_header_update_correction(struct sk_buff *skb, unsigned int type,
> > +				  struct ptp_header *hdr, s64 correction)
> > +{
> > +	__be64 correction_old;
> > +	struct udphdr *uhdr;
> > +
> > +	/* previous correction value is required for checksum update. */
> > +	memcpy(&correction_old,  &hdr->correction, sizeof(correction_old));
> > +
> > +	/* write new correction value */
> > +	put_unaligned_be64((u64)correction, &hdr->correction);
> > +
> > +	switch (type & PTP_CLASS_PMASK) {
> > +	case PTP_CLASS_IPV4:
> > +	case PTP_CLASS_IPV6:
> > +		/* locate udp header */
> > +		uhdr = (struct udphdr *)((char *)hdr - sizeof(struct udphdr));
> > +		break;
> > +	default:
> > +		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;
> 
> AFAICS the above works under the assumption that skb->ip_summed !=
> CHECKSUM_COMPLETE, and such assumption is true for the existing DSA
> devices.
> 
> Still the new helper is a generic one, so perhaps it should take care
> of CHECKSUM_COMPLETE, too? Or at least add a big fat warning in the
> helper documentation and/or a warn_on_once(CHECKSUM_COMPLETE).

I see this helper is used later even in the tx path, so even packet
with ip_summed == CHECKSUM_PARTIAL could reach here and should be
accomodated accordingly.

Thanks,

Paolo