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;
MIME-Version: 1.0
References: <20230104084316.4281-1-arun.ramadoss@microchip.com>
 <20230104084316.4281-7-arun.ramadoss@microchip.com> <27e0335f6ed15722feff27c17428410982a02e3c.camel@redhat.com>
In-Reply-To: <27e0335f6ed15722feff27c17428410982a02e3c.camel@redhat.com>
From:   Eric Dumazet <edumazet@google.com>
Date:   Thu, 5 Jan 2023 12:27:59 +0100
Message-ID: <CANn89i+XJzpXycUE_iiyRYQ-f-EkkBCD5FtdvbYfBy8pvOZ5qw@mail.gmail.com>
Subject: Re: [Patch net-next v7 06/13] net: ptp: add helper for one-step P2P clocks
To:     Paolo Abeni <pabeni@redhat.com>
Cc:     Arun Ramadoss <arun.ramadoss@microchip.com>,
        linux-kernel@vger.kernel.org, netdev@vger.kernel.org,
        woojung.huh@microchip.com, UNGLinuxDriver@microchip.com,
        andrew@lunn.ch, vivien.didelot@gmail.com, f.fainelli@gmail.com,
        olteanv@gmail.com, davem@davemloft.net, kuba@kernel.org,
        linux@armlinux.org.uk, Tristram.Ha@microchip.com,
        richardcochran@gmail.com, ceggers@arri.de
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Thu, Jan 5, 2023 at 11:09 AM Paolo Abeni <pabeni@redhat.com> wrote:
>
> 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.

Presumably skb->ip_summed could be forced to CHECKSUM_NONE

Note: if IPV4 UDP checksum is zero, we are not supposed to change it.

(Not sure if this point is already checked in caller)

>
> 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
>