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);
...
}
