Re: [RFC PATCH net-next 7/9] net: dsa: microchip: ksz9477: add hardware time stamping support

[Christian Eggers <ceggers@arri.de>]

Hi Vladimir,

On Thursday, 22 October 2020, 13:32:43 CET, Vladimir Oltean wrote:
> On Thu, Oct 22, 2020 at 01:11:40PM +0200, Christian Eggers wrote:
> > On Thursday, 22 October 2020, 12:50:14 CEST, Vladimir Oltean wrote:
> > after applying the RX timestamp correctly to the correction field
> > (shifting
> > the nanoseconds by 16),
> 
> That modification should have been done anyway, since the unit of
> measurement for correctionField is scaled ppb (48 bits nanoseconds, 16
> bits scaled nanoseconds), and not nanoseconds.
> 
> > it seems that "moving" the timestamp back to the tail tag on TX is not
> > required anymore. Keeping the RX timestamp simply in the correction
> > field (negative value), works fine now. So this halves the effort in
> > the tag_ksz driver.

unfortunately I made a mistake when testing. Actually the timestamp *must* be
moved from the correction field (negative) to the egress tail tag.

> Ok, this makes sense.
> Depending on what Richard responds, it now looks like the cleanest
> approach would be to move your implementation that is currently in
> ksz9477_update_ptp_correction_field() into a generic function called
> 
> static inline void ptp_onestep_p2p_move_t2_to_correction(struct sk_buff
> *skb, unsigned int ptp_type,
>  struct ptp_header *ptp_header,
> ktime_t t2)
I have implemented this in ptp_classify.h. Passing t2 instead of the correction
field itself is fine for rx, but as this function is now still required for
transmit, it looks a little bit misused there (see below).

Shall I keep it as below, or revert it to passing value of the correction field
itself?

regards
Christian


static void ksz9477_xmit_timestamp(struct sk_buff *skb)
{
	struct sk_buff *clone = DSA_SKB_CB(skb)->clone;
	struct ptp_header *ptp_hdr;
	u32 tstamp_raw = 0;
	u64 correction;

	if (!clone)
		goto out_put_tag;

	/* Use cached PTP header and type from ksz9477_ptp_should_tstamp(). Note
	 * that KSZ9477_SKB_CB(clone)->ptp_header != NULL implies that this is a
	 * Pdelay_resp message.
	 */
	ptp_hdr = KSZ9477_SKB_CB(clone)->ptp_header;
	if (!ptp_hdr)
		goto out_put_tag;

	correction = get_unaligned_be64(&ptp_hdr->correction);

	/* For PDelay_Resp messages we will likely have a negative value in the
	 * correction field (see ksz9477_rcv()). The switch hardware cannot
	 * correctly update such values, so it must be moved to the time stamp
	 * field in the tail tag.
	 */
	if ((s64)correction < 0) {
		unsigned int ptp_type = KSZ9477_SKB_CB(clone)->ptp_type;
		struct timespec64 ts;
		u64 ns;

		/* Move ingress time stamp from PTP header's correction field to
		 * tail tag. Format of the correction filed is 48 bit ns + 16
		 * bit fractional ns.  Avoid shifting negative numbers.
		 */
		ns = -((s64)correction) >> 16;
		ts = ns_to_timespec64(ns);
		tstamp_raw = ((ts.tv_sec & 3) << 30) | ts.tv_nsec;

>>>		/* Set correction field to 0 (by subtracting the negative value)
>>>		 * and update UDP checksum.
>>>		 */
>>>		ptp_onestep_p2p_move_t2_to_correction(skb, ptp_type, ptp_hdr, ns_to_ktime(-ns));
	}

out_put_tag:
	put_unaligned_be32(tstamp_raw, skb_put(skb, KSZ9477_PTP_TAG_LEN));
}


Addtionally ptp_onestep_p2p_move_t2_to_correction() must be able to handle negative values:

static inline
void ptp_onestep_p2p_move_t2_to_correction(struct sk_buff *skb,
					   unsigned int type,
					   struct ptp_header *hdr,
					   ktime_t t2)
{
	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 bit
	 * fractional nanoseconds.  Avoid shifting negative numbers.
	 */
>>>	if (ns >= 0)
>>>		correction -= ns << 16;
>>>	else
>>>		correction += -ns << 16;

	/* write new correction value */
	put_unaligned_be64((u64)correction, &hdr->correction);
...
}