Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <27e0335f6ed15722feff27c17428410982a02e3c.camel@redhat.com>
Subject: Re: [Patch net-next v7 06/13] net: ptp: add helper for one-step P2P
 clocks
From:   Paolo Abeni <pabeni@redhat.com>
To:     Arun Ramadoss <arun.ramadoss@microchip.com>,
        linux-kernel@vger.kernel.org, netdev@vger.kernel.org
Cc:     woojung.huh@microchip.com, UNGLinuxDriver@microchip.com,
        andrew@lunn.ch, vivien.didelot@gmail.com, f.fainelli@gmail.com,
        olteanv@gmail.com, davem@davemloft.net, edumazet@google.com,
        kuba@kernel.org, linux@armlinux.org.uk, Tristram.Ha@microchip.com,
        richardcochran@gmail.com, ceggers@arri.de
Date:   Thu, 05 Jan 2023 11:09:09 +0100
In-Reply-To: <20230104084316.4281-7-arun.ramadoss@microchip.com>
References: <20230104084316.4281-1-arun.ramadoss@microchip.com>
         <20230104084316.4281-7-arun.ramadoss@microchip.com>
Content-Type: text/plain; charset="UTF-8"
User-Agent: Evolution 3.42.4 (3.42.4-2.fc35) 
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
Precedence: bulk

Hi,

On Wed, 2023-01-04 at 14:13 +0530, Arun Ramadoss wrote:
> From: Christian Eggers <ceggers@arri.de>
> 
> 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@arri.de>
> Co-developed-by: Arun Ramadoss <arun.ramadoss@microchip.com>
> Signed-off-by: Arun Ramadoss <arun.ramadoss@microchip.com>
> ---
> v1 -> v2
> - Fixed compilation issue when PTP_CLASSIFY not selected in menuconfig
> as reported by kernel test robot <lkp@intel.com>
> ---
>  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).

Thanks!

Paolo