[PATCH net-next v1 0/6] ptp: Support hardware clocks with additional free running time

[PATCH net-next v1 0/6] ptp: Support hardware clocks with additional free running time

[Gerhard Engleder]

ptp vclocks require a clock with free running time for the timecounter.
Currently only a physical clock forced to free running is supported.
If vclocks are used, then the physical clock cannot be synchronized
anymore. The synchronized time is not available in hardware in this
case. As a result, timed transmission with TAPRIO hardware support
is not possible anymore.

If hardware would support a free running time additionally to the
physical clock, then the physical clock does not need to be forced to
free running. Thus, the physical clocks can still be synchronized while
vclocks are in use.

The physical clock could be used to synchronize the time domain of the
TSN network and trigger TAPRIO. In parallel vclocks can be used to
synchronize other time domains.

One year ago I thought for two time domains within a TSN network also
two physical clocks are required. This would lead to new kernel
interfaces for asking for the second clock, ... . But actually for a
time triggered system like TSN there can be only one time domain that
controls the system itself. All other time domains belong to other
layers, but not to the time triggered system itself. So other time
domains can be based on a free running counter if similar mechanisms
like 2 step synchroisation are used.

Synchronisation was tested with two time domains between two directly
connected hosts. Each host run two ptp4l instances, the first used the
physical clock and the second used the virtual clock. I used my FPGA
based network controller as network device. ptp4l was used in
combination with the virtual clock support patches from Miroslav
Lichvar.

v1:
- comlete rework based on feedback to RFC (Richard Cochran)

Gerhard Engleder (6):
  ptp: Add cycles support for virtual clocks
  ptp: Request cycles for TX timestamp
  ptp: Pass hwtstamp to ptp_convert_timestamp()
  ethtool: Add kernel API for PHC index
  ptp: Support late timestamp determination
  tsnep: Add physical clock cycles support

 drivers/net/ethernet/engleder/tsnep_hw.h   |  9 ++-
 drivers/net/ethernet/engleder/tsnep_main.c | 27 ++++++---
 drivers/net/ethernet/engleder/tsnep_ptp.c  | 44 ++++++++++++++
 drivers/ptp/ptp_clock.c                    | 58 +++++++++++++++++--
 drivers/ptp/ptp_private.h                  | 10 ++++
 drivers/ptp/ptp_sysfs.c                    | 10 ++--
 drivers/ptp/ptp_vclock.c                   | 18 +++---
 include/linux/ethtool.h                    |  8 +++
 include/linux/ptp_clock_kernel.h           | 67 ++++++++++++++++++++--
 include/linux/skbuff.h                     | 11 +++-
 net/core/skbuff.c                          |  2 +
 net/ethtool/common.c                       | 13 +++++
 net/socket.c                               | 45 +++++++++++----
 13 files changed, 275 insertions(+), 47 deletions(-)

-- 
2.20.1

[PATCH net-next v1 1/6] ptp: Add cycles support for virtual clocks

[Gerhard Engleder]

ptp vclocks require a free running time for their timecounter.
Currently only a physical clock forced to free running is supported.
If vclocks are used, then the physical clock cannot be synchronized
anymore. The synchronized time is not available in hardware in this
case. As a result, timed transmission with TAPRIO hardware support
is not possible anymore.

If hardware would support a free running time additionally to the
physical clock, then the physical clock does not need to be forced to
free running. Thus, the physical clocks can still be synchronized
while vclocks are in use.

The physical clock could be used to synchronize the time domain of the
TSN network and trigger TAPRIO. In parallel vclocks can be used to
synchronize other time domains.

Introduce support for a free running time called cycles to physical
clocks. Rework ptp vclocks to use cycles. Default implementation of
cycles is based on time of physical clock. Thus, behavior of ptp vclocks
based on physical clocks without cycles is identical to previous
behavior.

Signed-off-by: Gerhard Engleder <gerhard@engleder-embedded.com>
---
 drivers/ptp/ptp_clock.c          | 31 +++++++++++++++++++++++++++----
 drivers/ptp/ptp_private.h        | 10 ++++++++++
 drivers/ptp/ptp_sysfs.c          | 10 ++++++----
 drivers/ptp/ptp_vclock.c         | 13 +++++--------
 include/linux/ptp_clock_kernel.h | 30 ++++++++++++++++++++++++++++++
 5 files changed, 78 insertions(+), 16 deletions(-)

diff --git a/drivers/ptp/ptp_clock.c b/drivers/ptp/ptp_clock.c
index b6f2cfd15dd2..54b9f54ac0b2 100644
--- a/drivers/ptp/ptp_clock.c
+++ b/drivers/ptp/ptp_clock.c
@@ -77,8 +77,8 @@ static int ptp_clock_settime(struct posix_clock *pc, const struct timespec64 *tp
 {
 	struct ptp_clock *ptp = container_of(pc, struct ptp_clock, clock);
 
-	if (ptp_vclock_in_use(ptp)) {
-		pr_err("ptp: virtual clock in use\n");
+	if (ptp_clock_freerun(ptp)) {
+		pr_err("ptp: physical clock is free running\n");
 		return -EBUSY;
 	}
 
@@ -103,8 +103,8 @@ static int ptp_clock_adjtime(struct posix_clock *pc, struct __kernel_timex *tx)
 	struct ptp_clock_info *ops;
 	int err = -EOPNOTSUPP;
 
-	if (ptp_vclock_in_use(ptp)) {
-		pr_err("ptp: virtual clock in use\n");
+	if (ptp_clock_freerun(ptp)) {
+		pr_err("ptp: physical clock is free running\n");
 		return -EBUSY;
 	}
 
@@ -178,6 +178,14 @@ static void ptp_clock_release(struct device *dev)
 	kfree(ptp);
 }
 
+static int ptp_getcycles64(struct ptp_clock_info *info, struct timespec64 *ts)
+{
+	if (info->getcyclesx64)
+		return info->getcyclesx64(info, ts, NULL);
+	else
+		return info->gettime64(info, ts);
+}
+
 static void ptp_aux_kworker(struct kthread_work *work)
 {
 	struct ptp_clock *ptp = container_of(work, struct ptp_clock,
@@ -225,6 +233,21 @@ struct ptp_clock *ptp_clock_register(struct ptp_clock_info *info,
 	mutex_init(&ptp->n_vclocks_mux);
 	init_waitqueue_head(&ptp->tsev_wq);
 
+	if (!ptp->info->getcycles64 && !ptp->info->getcyclesx64) {
+		/* Cycles are based on time per default. */
+		ptp->info->getcycles64 = ptp_getcycles64;
+
+		if (ptp->info->gettimex64)
+			ptp->info->getcyclesx64 = ptp->info->gettimex64;
+
+		if (ptp->info->getcrosststamp)
+			ptp->info->getcrosscycles = ptp->info->getcrosststamp;
+	} else {
+		ptp->cycles = true;
+		if (!ptp->info->getcycles64 && ptp->info->getcyclesx64)
+			ptp->info->getcycles64 = ptp_getcycles64;
+	}
+
 	if (ptp->info->do_aux_work) {
 		kthread_init_delayed_work(&ptp->aux_work, ptp_aux_kworker);
 		ptp->kworker = kthread_create_worker(0, "ptp%d", ptp->index);
diff --git a/drivers/ptp/ptp_private.h b/drivers/ptp/ptp_private.h
index dba6be477067..ae66376def84 100644
--- a/drivers/ptp/ptp_private.h
+++ b/drivers/ptp/ptp_private.h
@@ -52,6 +52,7 @@ struct ptp_clock {
 	int *vclock_index;
 	struct mutex n_vclocks_mux; /* protect concurrent n_vclocks access */
 	bool is_virtual_clock;
+	bool cycles;
 };
 
 #define info_to_vclock(d) container_of((d), struct ptp_vclock, info)
@@ -96,6 +97,15 @@ static inline bool ptp_vclock_in_use(struct ptp_clock *ptp)
 	return in_use;
 }
 
+/* Check if ptp clock shall be free running */
+static inline bool ptp_clock_freerun(struct ptp_clock *ptp)
+{
+	if (ptp->cycles)
+		return false;
+
+	return ptp_vclock_in_use(ptp);
+}
+
 extern struct class *ptp_class;
 
 /*
diff --git a/drivers/ptp/ptp_sysfs.c b/drivers/ptp/ptp_sysfs.c
index 9233bfedeb17..8da3d374f78e 100644
--- a/drivers/ptp/ptp_sysfs.c
+++ b/drivers/ptp/ptp_sysfs.c
@@ -231,10 +231,12 @@ static ssize_t n_vclocks_store(struct device *dev,
 			*(ptp->vclock_index + ptp->n_vclocks - i) = -1;
 	}
 
-	if (num == 0)
-		dev_info(dev, "only physical clock in use now\n");
-	else
-		dev_info(dev, "guarantee physical clock free running\n");
+	if (!ptp->cycles) {
+		if (num == 0)
+			dev_info(dev, "only physical clock in use now\n");
+		else
+			dev_info(dev, "guarantee physical clock free running\n");
+	}
 
 	ptp->n_vclocks = num;
 	mutex_unlock(&ptp->n_vclocks_mux);
diff --git a/drivers/ptp/ptp_vclock.c b/drivers/ptp/ptp_vclock.c
index cb179a3ea508..3a095eab9cc5 100644
--- a/drivers/ptp/ptp_vclock.c
+++ b/drivers/ptp/ptp_vclock.c
@@ -68,7 +68,7 @@ static int ptp_vclock_gettimex(struct ptp_clock_info *ptp,
 	int err;
 	u64 ns;
 
-	err = pptp->info->gettimex64(pptp->info, &pts, sts);
+	err = pptp->info->getcyclesx64(pptp->info, &pts, sts);
 	if (err)
 		return err;
 
@@ -104,7 +104,7 @@ static int ptp_vclock_getcrosststamp(struct ptp_clock_info *ptp,
 	int err;
 	u64 ns;
 
-	err = pptp->info->getcrosststamp(pptp->info, xtstamp);
+	err = pptp->info->getcrosscycles(pptp->info, xtstamp);
 	if (err)
 		return err;
 
@@ -143,10 +143,7 @@ static u64 ptp_vclock_read(const struct cyclecounter *cc)
 	struct ptp_clock *ptp = vclock->pclock;
 	struct timespec64 ts = {};
 
-	if (ptp->info->gettimex64)
-		ptp->info->gettimex64(ptp->info, &ts, NULL);
-	else
-		ptp->info->gettime64(ptp->info, &ts);
+	ptp->info->getcycles64(ptp->info, &ts);
 
 	return timespec64_to_ns(&ts);
 }
@@ -168,11 +165,11 @@ struct ptp_vclock *ptp_vclock_register(struct ptp_clock *pclock)
 
 	vclock->pclock = pclock;
 	vclock->info = ptp_vclock_info;
-	if (pclock->info->gettimex64)
+	if (pclock->info->getcyclesx64)
 		vclock->info.gettimex64 = ptp_vclock_gettimex;
 	else
 		vclock->info.gettime64 = ptp_vclock_gettime;
-	if (pclock->info->getcrosststamp)
+	if (pclock->info->getcrosscycles)
 		vclock->info.getcrosststamp = ptp_vclock_getcrosststamp;
 	vclock->cc = ptp_vclock_cc;
 
diff --git a/include/linux/ptp_clock_kernel.h b/include/linux/ptp_clock_kernel.h
index 554454cb8693..acdcaa4a1ec2 100644
--- a/include/linux/ptp_clock_kernel.h
+++ b/include/linux/ptp_clock_kernel.h
@@ -108,6 +108,31 @@ struct ptp_system_timestamp {
  * @settime64:  Set the current time on the hardware clock.
  *              parameter ts: Time value to set.
  *
+ * @getcycles64:  Reads the current free running time from the hardware clock.
+ *                If @getcycles64 and @getcyclesx64 are not supported, then
+ *                @gettime64 or @gettimex64 will be used as default
+ *                implementation.
+ *                parameter ts: Holds the result.
+ *
+ * @getcyclesx64:  Reads the current free running time from the hardware clock
+ *                 and optionally also the system clock.
+ *                 If @getcycles64 and @getcyclesx64 are not supported, then
+ *                 @gettimex64 will be used as default implementation if
+ *                 available.
+ *                 parameter ts: Holds the PHC timestamp.
+ *                 parameter sts: If not NULL, it holds a pair of timestamps
+ *                 from the system clock. The first reading is made right before
+ *                 reading the lowest bits of the PHC timestamp and the second
+ *                 reading immediately follows that.
+ *
+ * @getcrosscycles:  Reads the current free running time from the hardware clock
+ *                   and system clock simultaneously.
+ *                   If @getcycles64 and @getcyclesx64 are not supported, then
+ *                   @getcrosststamp will be used as default implementation if
+ *                   available.
+ *                   parameter cts: Contains timestamp (device,system) pair,
+ *                   where system time is realtime and monotonic.
+ *
  * @enable:   Request driver to enable or disable an ancillary feature.
  *            parameter request: Desired resource to enable or disable.
  *            parameter on: Caller passes one to enable or zero to disable.
@@ -155,6 +180,11 @@ struct ptp_clock_info {
 	int (*getcrosststamp)(struct ptp_clock_info *ptp,
 			      struct system_device_crosststamp *cts);
 	int (*settime64)(struct ptp_clock_info *p, const struct timespec64 *ts);
+	int (*getcycles64)(struct ptp_clock_info *ptp, struct timespec64 *ts);
+	int (*getcyclesx64)(struct ptp_clock_info *ptp, struct timespec64 *ts,
+			    struct ptp_system_timestamp *sts);
+	int (*getcrosscycles)(struct ptp_clock_info *ptp,
+			      struct system_device_crosststamp *cts);
 	int (*enable)(struct ptp_clock_info *ptp,
 		      struct ptp_clock_request *request, int on);
 	int (*verify)(struct ptp_clock_info *ptp, unsigned int pin,
-- 
2.20.1

Re: [PATCH net-next v1 1/6] ptp: Add cycles support for virtual clocks

[Richard Cochran]

On Tue, Mar 22, 2022 at 10:07:17PM +0100, Gerhard Engleder wrote:

> diff --git a/drivers/ptp/ptp_private.h b/drivers/ptp/ptp_private.h
> index dba6be477067..ae66376def84 100644
> --- a/drivers/ptp/ptp_private.h
> +++ b/drivers/ptp/ptp_private.h
> @@ -52,6 +52,7 @@ struct ptp_clock {
>  	int *vclock_index;
>  	struct mutex n_vclocks_mux; /* protect concurrent n_vclocks access */
>  	bool is_virtual_clock;
> +	bool cycles;

Can we please have something more descriptive?

How about a predicate like is_xyz or has_xyz or xyz_available ?

Thanks,
Richard



>  };

Re: [PATCH net-next v1 1/6] ptp: Add cycles support for virtual clocks

[Gerhard Engleder]

> > diff --git a/drivers/ptp/ptp_private.h b/drivers/ptp/ptp_private.h
> > index dba6be477067..ae66376def84 100644
> > --- a/drivers/ptp/ptp_private.h
> > +++ b/drivers/ptp/ptp_private.h
> > @@ -52,6 +52,7 @@ struct ptp_clock {
> >       int *vclock_index;
> >       struct mutex n_vclocks_mux; /* protect concurrent n_vclocks access */
> >       bool is_virtual_clock;
> > +     bool cycles;
>
> Can we please have something more descriptive?
>
> How about a predicate like is_xyz or has_xyz or xyz_available ?

I'll improve that.

Thanks, Gerhard

[PATCH net-next v1 2/6] ptp: Request cycles for TX timestamp

[Gerhard Engleder]

The free running time of physical clocks called cycles shall be used for
hardware timestamps to enable synchronisation.

Introduce new flag SKBTX_HW_TSTAMP_USE_CYCLES, which signals driver to
provide a TX timestamp based on cycles if cycles are supported.

Signed-off-by: Gerhard Engleder <gerhard@engleder-embedded.com>
---
 include/linux/skbuff.h |  3 +++
 net/core/skbuff.c      |  2 ++
 net/socket.c           | 10 +++++++++-
 3 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index 26538ceb4b01..f494ddbfc826 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -578,6 +578,9 @@ enum {
 	/* device driver is going to provide hardware time stamp */
 	SKBTX_IN_PROGRESS = 1 << 2,
 
+	/* generate hardware time stamp based on cycles if supported */
+	SKBTX_HW_TSTAMP_USE_CYCLES = 1 << 3,
+
 	/* generate wifi status information (where possible) */
 	SKBTX_WIFI_STATUS = 1 << 4,
 
diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 10bde7c6db44..c0f8f1341c3f 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -4847,6 +4847,8 @@ void __skb_tstamp_tx(struct sk_buff *orig_skb,
 		skb_shinfo(skb)->tx_flags |= skb_shinfo(orig_skb)->tx_flags &
 					     SKBTX_ANY_TSTAMP;
 		skb_shinfo(skb)->tskey = skb_shinfo(orig_skb)->tskey;
+	} else {
+		skb_shinfo(skb)->tx_flags &= ~SKBTX_HW_TSTAMP_USE_CYCLES;
 	}
 
 	if (hwtstamps)
diff --git a/net/socket.c b/net/socket.c
index 982eecad464c..1acebcb19e8f 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -683,9 +683,17 @@ void __sock_tx_timestamp(__u16 tsflags, __u8 *tx_flags)
 {
 	u8 flags = *tx_flags;
 
-	if (tsflags & SOF_TIMESTAMPING_TX_HARDWARE)
+	if (tsflags & SOF_TIMESTAMPING_TX_HARDWARE) {
 		flags |= SKBTX_HW_TSTAMP;
 
+		/* PTP hardware clocks can provide a free running time called
+		 * cycles as base for virtual clocks. Tell driver to use cycles
+		 * for timestamp if socket is bound to virtual clock.
+		 */
+		if (tsflags & SOF_TIMESTAMPING_BIND_PHC)
+			flags |= SKBTX_HW_TSTAMP_USE_CYCLES;
+	}
+
 	if (tsflags & SOF_TIMESTAMPING_TX_SOFTWARE)
 		flags |= SKBTX_SW_TSTAMP;
 
-- 
2.20.1

Re: [PATCH net-next v1 2/6] ptp: Request cycles for TX timestamp

[Richard Cochran]

On Tue, Mar 22, 2022 at 10:07:18PM +0100, Gerhard Engleder wrote:
> The free running time of physical clocks called cycles shall be used for
> hardware timestamps to enable synchronisation.
> 
> Introduce new flag SKBTX_HW_TSTAMP_USE_CYCLES, which signals driver to
> provide a TX timestamp based on cycles if cycles are supported.
> 
> Signed-off-by: Gerhard Engleder <gerhard@engleder-embedded.com>
> ---
>  include/linux/skbuff.h |  3 +++
>  net/core/skbuff.c      |  2 ++
>  net/socket.c           | 10 +++++++++-
>  3 files changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
> index 26538ceb4b01..f494ddbfc826 100644
> --- a/include/linux/skbuff.h
> +++ b/include/linux/skbuff.h
> @@ -578,6 +578,9 @@ enum {
>  	/* device driver is going to provide hardware time stamp */
>  	SKBTX_IN_PROGRESS = 1 << 2,
>  
> +	/* generate hardware time stamp based on cycles if supported */
> +	SKBTX_HW_TSTAMP_USE_CYCLES = 1 << 3,

Bit 4 used, but 3 was unused... interesting!

>  	/* generate wifi status information (where possible) */
>  	SKBTX_WIFI_STATUS = 1 << 4,
>  
> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> index 10bde7c6db44..c0f8f1341c3f 100644
> --- a/net/core/skbuff.c
> +++ b/net/core/skbuff.c
> @@ -4847,6 +4847,8 @@ void __skb_tstamp_tx(struct sk_buff *orig_skb,
>  		skb_shinfo(skb)->tx_flags |= skb_shinfo(orig_skb)->tx_flags &
>  					     SKBTX_ANY_TSTAMP;
>  		skb_shinfo(skb)->tskey = skb_shinfo(orig_skb)->tskey;
> +	} else {
> +		skb_shinfo(skb)->tx_flags &= ~SKBTX_HW_TSTAMP_USE_CYCLES;
>  	}
>  
>  	if (hwtstamps)
> diff --git a/net/socket.c b/net/socket.c
> index 982eecad464c..1acebcb19e8f 100644
> --- a/net/socket.c
> +++ b/net/socket.c
> @@ -683,9 +683,17 @@ void __sock_tx_timestamp(__u16 tsflags, __u8 *tx_flags)
>  {
>  	u8 flags = *tx_flags;
>  
> -	if (tsflags & SOF_TIMESTAMPING_TX_HARDWARE)
> +	if (tsflags & SOF_TIMESTAMPING_TX_HARDWARE) {
>  		flags |= SKBTX_HW_TSTAMP;
>  
> +		/* PTP hardware clocks can provide a free running time called
> +		 * cycles as base for virtual clocks.

"PTP hardware clocks can provide a free running cycle counter as a
time base for virtual clocks."


Thanks,
Richard

Re: [PATCH net-next v1 2/6] ptp: Request cycles for TX timestamp

[Miroslav Lichvar]

On Thu, Mar 24, 2022 at 06:49:34AM -0700, Richard Cochran wrote:
> On Tue, Mar 22, 2022 at 10:07:18PM +0100, Gerhard Engleder wrote:
> > --- a/include/linux/skbuff.h
> > +++ b/include/linux/skbuff.h
> > @@ -578,6 +578,9 @@ enum {
> >  	/* device driver is going to provide hardware time stamp */
> >  	SKBTX_IN_PROGRESS = 1 << 2,
> >  
> > +	/* generate hardware time stamp based on cycles if supported */
> > +	SKBTX_HW_TSTAMP_USE_CYCLES = 1 << 3,
> 
> Bit 4 used, but 3 was unused... interesting!

It seems the bit 3 and 5 were removed in commit 06b4feb37e64
("net: group skb_shinfo zerocopy related bits together.").

-- 
Miroslav Lichvar

Re: [PATCH net-next v1 2/6] ptp: Request cycles for TX timestamp

[Gerhard Engleder]

> > @@ -683,9 +683,17 @@ void __sock_tx_timestamp(__u16 tsflags, __u8 *tx_flags)
> >  {
> >       u8 flags = *tx_flags;
> >
> > -     if (tsflags & SOF_TIMESTAMPING_TX_HARDWARE)
> > +     if (tsflags & SOF_TIMESTAMPING_TX_HARDWARE) {
> >               flags |= SKBTX_HW_TSTAMP;
> >
> > +             /* PTP hardware clocks can provide a free running time called
> > +              * cycles as base for virtual clocks.
>
> "PTP hardware clocks can provide a free running cycle counter as a
> time base for virtual clocks."

I'll use your wording here. I'll also update other comments and commit messages
with this wording.

Thanks,
Gerhard

[PATCH net-next v1 3/6] ptp: Pass hwtstamp to ptp_convert_timestamp()

[Gerhard Engleder]

ptp_convert_timestamp() converts only the timestamp hwtstamp, which is
a field of the argument with the type struct skb_shared_hwtstamps *. So
a pointer to the hwtstamp field of this structure is sufficient.

Rework ptp_convert_timestamp() to use an argument of type ktime_t *.
This allows to add additional timestamp manipulation stages before the
call of ptp_convert_timestamp().

Signed-off-by: Gerhard Engleder <gerhard@engleder-embedded.com>
---
 drivers/ptp/ptp_vclock.c         | 5 ++---
 include/linux/ptp_clock_kernel.h | 7 +++----
 net/socket.c                     | 2 +-
 3 files changed, 6 insertions(+), 8 deletions(-)

diff --git a/drivers/ptp/ptp_vclock.c b/drivers/ptp/ptp_vclock.c
index 3a095eab9cc5..c30bcce2bb43 100644
--- a/drivers/ptp/ptp_vclock.c
+++ b/drivers/ptp/ptp_vclock.c
@@ -232,8 +232,7 @@ int ptp_get_vclocks_index(int pclock_index, int **vclock_index)
 }
 EXPORT_SYMBOL(ptp_get_vclocks_index);
 
-ktime_t ptp_convert_timestamp(const struct skb_shared_hwtstamps *hwtstamps,
-			      int vclock_index)
+ktime_t ptp_convert_timestamp(const ktime_t *hwtstamp, int vclock_index)
 {
 	char name[PTP_CLOCK_NAME_LEN] = "";
 	struct ptp_vclock *vclock;
@@ -255,7 +254,7 @@ ktime_t ptp_convert_timestamp(const struct skb_shared_hwtstamps *hwtstamps,
 
 	vclock = info_to_vclock(ptp->info);
 
-	ns = ktime_to_ns(hwtstamps->hwtstamp);
+	ns = ktime_to_ns(*hwtstamp);
 
 	spin_lock_irqsave(&vclock->lock, flags);
 	ns = timecounter_cyc2time(&vclock->tc, ns);
diff --git a/include/linux/ptp_clock_kernel.h b/include/linux/ptp_clock_kernel.h
index acdcaa4a1ec2..cc6a7b2e267d 100644
--- a/include/linux/ptp_clock_kernel.h
+++ b/include/linux/ptp_clock_kernel.h
@@ -379,17 +379,16 @@ int ptp_get_vclocks_index(int pclock_index, int **vclock_index);
 /**
  * ptp_convert_timestamp() - convert timestamp to a ptp vclock time
  *
- * @hwtstamps:    skb_shared_hwtstamps structure pointer
+ * @hwtstamp:     timestamp
  * @vclock_index: phc index of ptp vclock.
  *
  * Returns converted timestamp, or 0 on error.
  */
-ktime_t ptp_convert_timestamp(const struct skb_shared_hwtstamps *hwtstamps,
-			      int vclock_index);
+ktime_t ptp_convert_timestamp(const ktime_t *hwtstamp, int vclock_index);
 #else
 static inline int ptp_get_vclocks_index(int pclock_index, int **vclock_index)
 { return 0; }
-static inline ktime_t ptp_convert_timestamp(const struct skb_shared_hwtstamps *hwtstamps,
+static inline ktime_t ptp_convert_timestamp(const ktime_t *hwtstamp,
 					    int vclock_index)
 { return 0; }
 
diff --git a/net/socket.c b/net/socket.c
index 1acebcb19e8f..2e932c058002 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -887,7 +887,7 @@ void __sock_recv_timestamp(struct msghdr *msg, struct sock *sk,
 	    (sk->sk_tsflags & SOF_TIMESTAMPING_RAW_HARDWARE) &&
 	    !skb_is_swtx_tstamp(skb, false_tstamp)) {
 		if (sk->sk_tsflags & SOF_TIMESTAMPING_BIND_PHC)
-			hwtstamp = ptp_convert_timestamp(shhwtstamps,
+			hwtstamp = ptp_convert_timestamp(&shhwtstamps->hwtstamp,
 							 sk->sk_bind_phc);
 		else
 			hwtstamp = shhwtstamps->hwtstamp;
-- 
2.20.1

Re: [PATCH net-next v1 3/6] ptp: Pass hwtstamp to ptp_convert_timestamp()

[Richard Cochran]

On Tue, Mar 22, 2022 at 10:07:19PM +0100, Gerhard Engleder wrote:
> ptp_convert_timestamp() converts only the timestamp hwtstamp, which is
> a field of the argument with the type struct skb_shared_hwtstamps *. So
> a pointer to the hwtstamp field of this structure is sufficient.
> 
> Rework ptp_convert_timestamp() to use an argument of type ktime_t *.
> This allows to add additional timestamp manipulation stages before the
> call of ptp_convert_timestamp().
> 
> Signed-off-by: Gerhard Engleder <gerhard@engleder-embedded.com>

Acked-by: Richard Cochran <richardcochran@gmail.com>

[PATCH net-next v1 4/6] ethtool: Add kernel API for PHC index

[Gerhard Engleder]

Add a new function, which returns the physical clock index of a
networking device. This function will be used to get the physical clock
of a device for timestamp manipulation in the receive path.

Signed-off-by: Gerhard Engleder <gerhard@engleder-embedded.com>
---
 include/linux/ethtool.h |  8 ++++++++
 net/ethtool/common.c    | 13 +++++++++++++
 2 files changed, 21 insertions(+)

diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
index 4af58459a1e7..e107069f37a4 100644
--- a/include/linux/ethtool.h
+++ b/include/linux/ethtool.h
@@ -820,6 +820,14 @@ void
 ethtool_params_from_link_mode(struct ethtool_link_ksettings *link_ksettings,
 			      enum ethtool_link_mode_bit_indices link_mode);
 
+/**
+ * ethtool_get_phc - Get phc index
+ * @dev: pointer to net_device structure
+ *
+ * Return index of phc
+ */
+int ethtool_get_phc(struct net_device *dev);
+
 /**
  * ethtool_get_phc_vclocks - Derive phc vclocks information, and caller
  *                           is responsible to free memory of vclock_index
diff --git a/net/ethtool/common.c b/net/ethtool/common.c
index 0c5210015911..8218e3b3e98a 100644
--- a/net/ethtool/common.c
+++ b/net/ethtool/common.c
@@ -557,6 +557,19 @@ int __ethtool_get_ts_info(struct net_device *dev, struct ethtool_ts_info *info)
 	return 0;
 }
 
+int ethtool_get_phc(struct net_device *dev)
+{
+	struct ethtool_ts_info info;
+	int ret;
+
+	ret = __ethtool_get_ts_info(dev, &info);
+	if (ret)
+		return ret;
+
+	return info.phc_index;
+}
+EXPORT_SYMBOL(ethtool_get_phc);
+
 int ethtool_get_phc_vclocks(struct net_device *dev, int **vclock_index)
 {
 	struct ethtool_ts_info info = { };
-- 
2.20.1

Re: [PATCH net-next v1 4/6] ethtool: Add kernel API for PHC index

[Richard Cochran]

On Tue, Mar 22, 2022 at 10:07:20PM +0100, Gerhard Engleder wrote:
> Add a new function, which returns the physical clock index of a
> networking device. This function will be used to get the physical clock
> of a device for timestamp manipulation in the receive path.
> 
> Signed-off-by: Gerhard Engleder <gerhard@engleder-embedded.com>

Acked-by: Richard Cochran <richardcochran@gmail.com>

[PATCH net-next v1 5/6] ptp: Support late timestamp determination

[Gerhard Engleder]

If a physical clock supports a free running time called cycles, then
timestamps shall be based on this time too. For TX it is known in
advance before the transmission if a timestamp based on cycles is
needed. For RX it is impossible to know which timestamp is needed before
the packet is received and assigned to a socket.

Support late timestamp determination by a physical clock. Therefore, an
address/cookie is stored within the new phc_data field of struct
skb_shared_hwtstamps. This address/cookie is provided to a new physical
clock method called gettstamp(), which returns a timestamp based on the
normal/adjustable time or based on cycles.

The previously introduced flag SKBTX_HW_TSTAMP_USE_CYCLES is reused with
an additional alias to request the late timestamp determination by the
physical clock. That is possible, because SKBTX_HW_TSTAMP_USE_CYCLES is
not used in the receive path.

Signed-off-by: Gerhard Engleder <gerhard@engleder-embedded.com>
---
 drivers/ptp/ptp_clock.c          | 27 ++++++++++++++++++++++++
 include/linux/ptp_clock_kernel.h | 30 +++++++++++++++++++++++++++
 include/linux/skbuff.h           |  8 +++++++-
 net/socket.c                     | 35 ++++++++++++++++++++++----------
 4 files changed, 88 insertions(+), 12 deletions(-)

diff --git a/drivers/ptp/ptp_clock.c b/drivers/ptp/ptp_clock.c
index 54b9f54ac0b2..b7a8cf27c349 100644
--- a/drivers/ptp/ptp_clock.c
+++ b/drivers/ptp/ptp_clock.c
@@ -450,6 +450,33 @@ void ptp_cancel_worker_sync(struct ptp_clock *ptp)
 }
 EXPORT_SYMBOL(ptp_cancel_worker_sync);
 
+ktime_t ptp_get_timestamp(int index,
+			  const struct skb_shared_hwtstamps *hwtstamps,
+			  bool cycles)
+{
+	char name[PTP_CLOCK_NAME_LEN] = "";
+	struct ptp_clock *ptp;
+	struct device *dev;
+	ktime_t ts;
+
+	snprintf(name, PTP_CLOCK_NAME_LEN, "ptp%d", index);
+	dev = class_find_device_by_name(ptp_class, name);
+	if (!dev)
+		return 0;
+
+	ptp = dev_get_drvdata(dev);
+
+	if (ptp->info->gettstamp)
+		ts = ptp->info->gettstamp(ptp->info, hwtstamps, cycles);
+	else
+		ts = hwtstamps->hwtstamp;
+
+	put_device(dev);
+
+	return ts;
+}
+EXPORT_SYMBOL(ptp_get_timestamp);
+
 /* module operations */
 
 static void __exit ptp_exit(void)
diff --git a/include/linux/ptp_clock_kernel.h b/include/linux/ptp_clock_kernel.h
index cc6a7b2e267d..f4f0d8a880c6 100644
--- a/include/linux/ptp_clock_kernel.h
+++ b/include/linux/ptp_clock_kernel.h
@@ -133,6 +133,16 @@ struct ptp_system_timestamp {
  *                   parameter cts: Contains timestamp (device,system) pair,
  *                   where system time is realtime and monotonic.
  *
+ * @gettstamp:  Get hardware timestamp based on normal/adjustable time or free
+ *              running time. If @getcycles64 or @getcyclesx64 are supported,
+ *              then this method is required to provide timestamps based on the
+ *              free running time. This method will be called if
+ *              SKBTX_HW_TSTAMP_PHC is set by the driver.
+ *              parameter hwtstamps: skb_shared_hwtstamps structure pointer.
+ *              parameter cycles: If false, then hardware timestamp based on
+ *              normal/adjustable time is requested. If true, then hardware
+ *              timestamp based on free running time is requested.
+ *
  * @enable:   Request driver to enable or disable an ancillary feature.
  *            parameter request: Desired resource to enable or disable.
  *            parameter on: Caller passes one to enable or zero to disable.
@@ -185,6 +195,9 @@ struct ptp_clock_info {
 			    struct ptp_system_timestamp *sts);
 	int (*getcrosscycles)(struct ptp_clock_info *ptp,
 			      struct system_device_crosststamp *cts);
+	ktime_t (*gettstamp)(struct ptp_clock_info *ptp,
+			     const struct skb_shared_hwtstamps *hwtstamps,
+			     bool cycles);
 	int (*enable)(struct ptp_clock_info *ptp,
 		      struct ptp_clock_request *request, int on);
 	int (*verify)(struct ptp_clock_info *ptp, unsigned int pin,
@@ -364,6 +377,19 @@ static inline void ptp_cancel_worker_sync(struct ptp_clock *ptp)
  * a loadable module.
  */
 
+/**
+ * ptp_get_timestamp() - get timestamp of ptp clock
+ *
+ * @index:     phc index of ptp pclock.
+ * @hwtstamps: skb_shared_hwtstamps structure pointer.
+ * @cycles:    true for timestamp based on cycles.
+ *
+ * Returns timestamp, or 0 on error.
+ */
+ktime_t ptp_get_timestamp(int index,
+			  const struct skb_shared_hwtstamps *hwtstamps,
+			  bool cycles);
+
 /**
  * ptp_get_vclocks_index() - get all vclocks index on pclock, and
  *                           caller is responsible to free memory
@@ -386,6 +412,10 @@ int ptp_get_vclocks_index(int pclock_index, int **vclock_index);
  */
 ktime_t ptp_convert_timestamp(const ktime_t *hwtstamp, int vclock_index);
 #else
+static inline ktime_t ptp_get_timestamp(int index,
+					const struct skb_shared_hwtstamps *hwtstamps,
+					bool cycles);
+{ return 0; }
 static inline int ptp_get_vclocks_index(int pclock_index, int **vclock_index)
 { return 0; }
 static inline ktime_t ptp_convert_timestamp(const ktime_t *hwtstamp,
diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index f494ddbfc826..38929c113953 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -564,7 +564,10 @@ static inline bool skb_frag_must_loop(struct page *p)
  * &skb_shared_info. Use skb_hwtstamps() to get a pointer.
  */
 struct skb_shared_hwtstamps {
-	ktime_t	hwtstamp;
+	union {
+		ktime_t	hwtstamp;
+		void *phc_data;
+	};
 };
 
 /* Definitions for tx_flags in struct skb_shared_info */
@@ -581,6 +584,9 @@ enum {
 	/* generate hardware time stamp based on cycles if supported */
 	SKBTX_HW_TSTAMP_USE_CYCLES = 1 << 3,
 
+	/* call PHC to get actual hardware time stamp */
+	SKBTX_HW_TSTAMP_PHC = 1 << 3,
+
 	/* generate wifi status information (where possible) */
 	SKBTX_WIFI_STATUS = 1 << 4,
 
diff --git a/net/socket.c b/net/socket.c
index 2e932c058002..fe765d559086 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -804,21 +804,17 @@ static bool skb_is_swtx_tstamp(const struct sk_buff *skb, int false_tstamp)
 	return skb->tstamp && !false_tstamp && skb_is_err_queue(skb);
 }
 
-static void put_ts_pktinfo(struct msghdr *msg, struct sk_buff *skb)
+static void put_ts_pktinfo(struct msghdr *msg, struct sk_buff *skb,
+			   int if_index)
 {
 	struct scm_ts_pktinfo ts_pktinfo;
-	struct net_device *orig_dev;
 
 	if (!skb_mac_header_was_set(skb))
 		return;
 
 	memset(&ts_pktinfo, 0, sizeof(ts_pktinfo));
 
-	rcu_read_lock();
-	orig_dev = dev_get_by_napi_id(skb_napi_id(skb));
-	if (orig_dev)
-		ts_pktinfo.if_index = orig_dev->ifindex;
-	rcu_read_unlock();
+	ts_pktinfo.if_index = if_index;
 
 	ts_pktinfo.pkt_length = skb->len - skb_mac_offset(skb);
 	put_cmsg(msg, SOL_SOCKET, SCM_TIMESTAMPING_PKTINFO,
@@ -838,6 +834,9 @@ void __sock_recv_timestamp(struct msghdr *msg, struct sock *sk,
 	int empty = 1, false_tstamp = 0;
 	struct skb_shared_hwtstamps *shhwtstamps =
 		skb_hwtstamps(skb);
+	struct net_device *orig_dev;
+	int if_index = 0;
+	int phc_index = -1;
 	ktime_t hwtstamp;
 
 	/* Race occurred between timestamp enabling and packet
@@ -886,18 +885,32 @@ void __sock_recv_timestamp(struct msghdr *msg, struct sock *sk,
 	if (shhwtstamps &&
 	    (sk->sk_tsflags & SOF_TIMESTAMPING_RAW_HARDWARE) &&
 	    !skb_is_swtx_tstamp(skb, false_tstamp)) {
-		if (sk->sk_tsflags & SOF_TIMESTAMPING_BIND_PHC)
-			hwtstamp = ptp_convert_timestamp(&shhwtstamps->hwtstamp,
-							 sk->sk_bind_phc);
+		rcu_read_lock();
+		orig_dev = dev_get_by_napi_id(skb_napi_id(skb));
+		if (orig_dev) {
+			if_index = orig_dev->ifindex;
+			if (skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP_PHC)
+				phc_index = ethtool_get_phc(orig_dev);
+		}
+		rcu_read_unlock();
+
+		if ((skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP_PHC) &&
+		    (phc_index != -1))
+			hwtstamp = ptp_get_timestamp(phc_index, shhwtstamps,
+						     sk->sk_tsflags & SOF_TIMESTAMPING_BIND_PHC);
 		else
 			hwtstamp = shhwtstamps->hwtstamp;
 
+		if (sk->sk_tsflags & SOF_TIMESTAMPING_BIND_PHC)
+			hwtstamp = ptp_convert_timestamp(&hwtstamp,
+							 sk->sk_bind_phc);
+
 		if (ktime_to_timespec64_cond(hwtstamp, tss.ts + 2)) {
 			empty = 0;
 
 			if ((sk->sk_tsflags & SOF_TIMESTAMPING_OPT_PKTINFO) &&
 			    !skb_is_err_queue(skb))
-				put_ts_pktinfo(msg, skb);
+				put_ts_pktinfo(msg, skb, if_index);
 		}
 	}
 	if (!empty) {
-- 
2.20.1

Re: [PATCH net-next v1 5/6] ptp: Support late timestamp determination

[kernel test robot]

Hi Gerhard,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on net-next/master]

url:    https://github.com/0day-ci/linux/commits/Gerhard-Engleder/ptp-Support-hardware-clocks-with-additional-free-running-time/20220323-051003
base:   https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git 4a0cb83ba6e0cd73a50fa4f84736846bf0029f2b
config: microblaze-randconfig-r022-20220321 (https://download.01.org/0day-ci/archive/20220323/202203230801.KMxkRkMZ-lkp@intel.com/config)
compiler: microblaze-linux-gcc (GCC) 11.2.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/754e870cb9699166113d6ea383e48b0207165c1a
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Gerhard-Engleder/ptp-Support-hardware-clocks-with-additional-free-running-time/20220323-051003
        git checkout 754e870cb9699166113d6ea383e48b0207165c1a
        # save the config file to linux build tree
        mkdir build_dir
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross O=build_dir ARCH=microblaze SHELL=/bin/bash drivers/spi/

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   In file included from drivers/spi/spi.c:36:
>> include/linux/ptp_clock_kernel.h:418:1: error: expected identifier or '(' before '{' token
     418 | { return 0; }
         | ^
   include/linux/ptp_clock_kernel.h:415:23: warning: 'ptp_get_timestamp' declared 'static' but never defined [-Wunused-function]
     415 | static inline ktime_t ptp_get_timestamp(int index,
         |                       ^~~~~~~~~~~~~~~~~


vim +418 include/linux/ptp_clock_kernel.h

   404	
   405	/**
   406	 * ptp_convert_timestamp() - convert timestamp to a ptp vclock time
   407	 *
   408	 * @hwtstamp:     timestamp
   409	 * @vclock_index: phc index of ptp vclock.
   410	 *
   411	 * Returns converted timestamp, or 0 on error.
   412	 */
   413	ktime_t ptp_convert_timestamp(const ktime_t *hwtstamp, int vclock_index);
   414	#else
   415	static inline ktime_t ptp_get_timestamp(int index,
   416						const struct skb_shared_hwtstamps *hwtstamps,
   417						bool cycles);
 > 418	{ return 0; }
   419	static inline int ptp_get_vclocks_index(int pclock_index, int **vclock_index)
   420	{ return 0; }
   421	static inline ktime_t ptp_convert_timestamp(const ktime_t *hwtstamp,
   422						    int vclock_index)
   423	{ return 0; }
   424	

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp

Re: [PATCH net-next v1 5/6] ptp: Support late timestamp determination

[kernel test robot]

Hi Gerhard,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on net-next/master]

url:    https://github.com/0day-ci/linux/commits/Gerhard-Engleder/ptp-Support-hardware-clocks-with-additional-free-running-time/20220323-051003
base:   https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git 4a0cb83ba6e0cd73a50fa4f84736846bf0029f2b
config: hexagon-buildonly-randconfig-r004-20220320 (https://download.01.org/0day-ci/archive/20220323/202203230811.oaI4Q7IP-lkp@intel.com/config)
compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project 902f4708fe1d03b0de7e5315ef875006a6adc319)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/754e870cb9699166113d6ea383e48b0207165c1a
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Gerhard-Engleder/ptp-Support-hardware-clocks-with-additional-free-running-time/20220323-051003
        git checkout 754e870cb9699166113d6ea383e48b0207165c1a
        # save the config file to linux build tree
        mkdir build_dir
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=hexagon SHELL=/bin/bash drivers/spi/

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   In file included from drivers/spi/spi.c:36:
>> include/linux/ptp_clock_kernel.h:418:1: error: expected identifier or '('
   { return 0; }
   ^
   1 error generated.


vim +418 include/linux/ptp_clock_kernel.h

   404	
   405	/**
   406	 * ptp_convert_timestamp() - convert timestamp to a ptp vclock time
   407	 *
   408	 * @hwtstamp:     timestamp
   409	 * @vclock_index: phc index of ptp vclock.
   410	 *
   411	 * Returns converted timestamp, or 0 on error.
   412	 */
   413	ktime_t ptp_convert_timestamp(const ktime_t *hwtstamp, int vclock_index);
   414	#else
   415	static inline ktime_t ptp_get_timestamp(int index,
   416						const struct skb_shared_hwtstamps *hwtstamps,
   417						bool cycles);
 > 418	{ return 0; }
   419	static inline int ptp_get_vclocks_index(int pclock_index, int **vclock_index)
   420	{ return 0; }
   421	static inline ktime_t ptp_convert_timestamp(const ktime_t *hwtstamp,
   422						    int vclock_index)
   423	{ return 0; }
   424	

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp

Re: [PATCH net-next v1 5/6] ptp: Support late timestamp determination

[kernel test robot]

Hi Gerhard,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on net-next/master]

url:    https://github.com/0day-ci/linux/commits/Gerhard-Engleder/ptp-Support-hardware-clocks-with-additional-free-running-time/20220323-051003
base:   https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git 4a0cb83ba6e0cd73a50fa4f84736846bf0029f2b
config: nios2-randconfig-r023-20220321 (https://download.01.org/0day-ci/archive/20220323/202203231015.YRlUe3av-lkp@intel.com/config)
compiler: nios2-linux-gcc (GCC) 11.2.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/754e870cb9699166113d6ea383e48b0207165c1a
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Gerhard-Engleder/ptp-Support-hardware-clocks-with-additional-free-running-time/20220323-051003
        git checkout 754e870cb9699166113d6ea383e48b0207165c1a
        # save the config file to linux build tree
        mkdir build_dir
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross O=build_dir ARCH=nios2 SHELL=/bin/bash

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

   In file included from net/socket.c:108:
   include/linux/ptp_clock_kernel.h:418:1: error: expected identifier or '(' before '{' token
     418 | { return 0; }
         | ^
   net/socket.c: In function '__sys_getsockopt':
   net/socket.c:2227:13: warning: variable 'max_optlen' set but not used [-Wunused-but-set-variable]
    2227 |         int max_optlen;
         |             ^~~~~~~~~~
   In file included from net/socket.c:108:
   net/socket.c: At top level:
>> include/linux/ptp_clock_kernel.h:415:23: warning: 'ptp_get_timestamp' used but never defined
     415 | static inline ktime_t ptp_get_timestamp(int index,
         |                       ^~~~~~~~~~~~~~~~~


vim +/ptp_get_timestamp +415 include/linux/ptp_clock_kernel.h

   404	
   405	/**
   406	 * ptp_convert_timestamp() - convert timestamp to a ptp vclock time
   407	 *
   408	 * @hwtstamp:     timestamp
   409	 * @vclock_index: phc index of ptp vclock.
   410	 *
   411	 * Returns converted timestamp, or 0 on error.
   412	 */
   413	ktime_t ptp_convert_timestamp(const ktime_t *hwtstamp, int vclock_index);
   414	#else
 > 415	static inline ktime_t ptp_get_timestamp(int index,
   416						const struct skb_shared_hwtstamps *hwtstamps,
   417						bool cycles);
   418	{ return 0; }
   419	static inline int ptp_get_vclocks_index(int pclock_index, int **vclock_index)
   420	{ return 0; }
   421	static inline ktime_t ptp_convert_timestamp(const ktime_t *hwtstamp,
   422						    int vclock_index)
   423	{ return 0; }
   424	

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp

Re: [PATCH net-next v1 5/6] ptp: Support late timestamp determination

[Richard Cochran]

On Tue, Mar 22, 2022 at 10:07:21PM +0100, Gerhard Engleder wrote:
> diff --git a/drivers/ptp/ptp_clock.c b/drivers/ptp/ptp_clock.c
> index 54b9f54ac0b2..b7a8cf27c349 100644
> --- a/drivers/ptp/ptp_clock.c
> +++ b/drivers/ptp/ptp_clock.c
> @@ -450,6 +450,33 @@ void ptp_cancel_worker_sync(struct ptp_clock *ptp)
>  }
>  EXPORT_SYMBOL(ptp_cancel_worker_sync);
>  
> +ktime_t ptp_get_timestamp(int index,
> +			  const struct skb_shared_hwtstamps *hwtstamps,
> +			  bool cycles)
> +{
> +	char name[PTP_CLOCK_NAME_LEN] = "";
> +	struct ptp_clock *ptp;
> +	struct device *dev;
> +	ktime_t ts;
> +
> +	snprintf(name, PTP_CLOCK_NAME_LEN, "ptp%d", index);
> +	dev = class_find_device_by_name(ptp_class, name);

This seems expensive for every single Rx frame in a busy PTP network.
Can't this be cached in the socket?

> +	if (!dev)
> +		return 0;
> +
> +	ptp = dev_get_drvdata(dev);
> +
> +	if (ptp->info->gettstamp)
> +		ts = ptp->info->gettstamp(ptp->info, hwtstamps, cycles);
> +	else
> +		ts = hwtstamps->hwtstamp;
> +
> +	put_device(dev);
> +
> +	return ts;
> +}
> +EXPORT_SYMBOL(ptp_get_timestamp);
> +
>  /* module operations */
>  
>  static void __exit ptp_exit(void)
> diff --git a/include/linux/ptp_clock_kernel.h b/include/linux/ptp_clock_kernel.h
> index cc6a7b2e267d..f4f0d8a880c6 100644
> --- a/include/linux/ptp_clock_kernel.h
> +++ b/include/linux/ptp_clock_kernel.h
> @@ -133,6 +133,16 @@ struct ptp_system_timestamp {
>   *                   parameter cts: Contains timestamp (device,system) pair,
>   *                   where system time is realtime and monotonic.
>   *
> + * @gettstamp:  Get hardware timestamp based on normal/adjustable time or free
> + *              running time. If @getcycles64 or @getcyclesx64 are supported,
> + *              then this method is required to provide timestamps based on the
> + *              free running time. This method will be called if
> + *              SKBTX_HW_TSTAMP_PHC is set by the driver.
> + *              parameter hwtstamps: skb_shared_hwtstamps structure pointer.
> + *              parameter cycles: If false, then hardware timestamp based on
> + *              normal/adjustable time is requested. If true, then hardware
> + *              timestamp based on free running time is requested.
> + *
>   * @enable:   Request driver to enable or disable an ancillary feature.
>   *            parameter request: Desired resource to enable or disable.
>   *            parameter on: Caller passes one to enable or zero to disable.
> @@ -185,6 +195,9 @@ struct ptp_clock_info {
>  			    struct ptp_system_timestamp *sts);
>  	int (*getcrosscycles)(struct ptp_clock_info *ptp,
>  			      struct system_device_crosststamp *cts);
> +	ktime_t (*gettstamp)(struct ptp_clock_info *ptp,
> +			     const struct skb_shared_hwtstamps *hwtstamps,
> +			     bool cycles);
>  	int (*enable)(struct ptp_clock_info *ptp,
>  		      struct ptp_clock_request *request, int on);
>  	int (*verify)(struct ptp_clock_info *ptp, unsigned int pin,
> @@ -364,6 +377,19 @@ static inline void ptp_cancel_worker_sync(struct ptp_clock *ptp)
>   * a loadable module.
>   */
>  
> +/**
> + * ptp_get_timestamp() - get timestamp of ptp clock
> + *
> + * @index:     phc index of ptp pclock.
> + * @hwtstamps: skb_shared_hwtstamps structure pointer.
> + * @cycles:    true for timestamp based on cycles.
> + *
> + * Returns timestamp, or 0 on error.
> + */
> +ktime_t ptp_get_timestamp(int index,
> +			  const struct skb_shared_hwtstamps *hwtstamps,
> +			  bool cycles);
> +
>  /**
>   * ptp_get_vclocks_index() - get all vclocks index on pclock, and
>   *                           caller is responsible to free memory
> @@ -386,6 +412,10 @@ int ptp_get_vclocks_index(int pclock_index, int **vclock_index);
>   */
>  ktime_t ptp_convert_timestamp(const ktime_t *hwtstamp, int vclock_index);
>  #else
> +static inline ktime_t ptp_get_timestamp(int index,
> +					const struct skb_shared_hwtstamps *hwtstamps,
> +					bool cycles);
> +{ return 0; }
>  static inline int ptp_get_vclocks_index(int pclock_index, int **vclock_index)
>  { return 0; }
>  static inline ktime_t ptp_convert_timestamp(const ktime_t *hwtstamp,
> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
> index f494ddbfc826..38929c113953 100644
> --- a/include/linux/skbuff.h
> +++ b/include/linux/skbuff.h
> @@ -564,7 +564,10 @@ static inline bool skb_frag_must_loop(struct page *p)
>   * &skb_shared_info. Use skb_hwtstamps() to get a pointer.
>   */
>  struct skb_shared_hwtstamps {
> -	ktime_t	hwtstamp;
> +	union {
> +		ktime_t	hwtstamp;
> +		void *phc_data;

needs kdoc update

> +	};
>  };
>  
>  /* Definitions for tx_flags in struct skb_shared_info */
> @@ -581,6 +584,9 @@ enum {
>  	/* generate hardware time stamp based on cycles if supported */
>  	SKBTX_HW_TSTAMP_USE_CYCLES = 1 << 3,
>  
> +	/* call PHC to get actual hardware time stamp */
> +	SKBTX_HW_TSTAMP_PHC = 1 << 3,
> +
>  	/* generate wifi status information (where possible) */
>  	SKBTX_WIFI_STATUS = 1 << 4,
>  
> diff --git a/net/socket.c b/net/socket.c
> index 2e932c058002..fe765d559086 100644
> --- a/net/socket.c
> +++ b/net/socket.c
> @@ -804,21 +804,17 @@ static bool skb_is_swtx_tstamp(const struct sk_buff *skb, int false_tstamp)
>  	return skb->tstamp && !false_tstamp && skb_is_err_queue(skb);
>  }
>  
> -static void put_ts_pktinfo(struct msghdr *msg, struct sk_buff *skb)
> +static void put_ts_pktinfo(struct msghdr *msg, struct sk_buff *skb,
> +			   int if_index)
>  {
>  	struct scm_ts_pktinfo ts_pktinfo;
> -	struct net_device *orig_dev;
>  
>  	if (!skb_mac_header_was_set(skb))
>  		return;
>  
>  	memset(&ts_pktinfo, 0, sizeof(ts_pktinfo));
>  
> -	rcu_read_lock();
> -	orig_dev = dev_get_by_napi_id(skb_napi_id(skb));
> -	if (orig_dev)
> -		ts_pktinfo.if_index = orig_dev->ifindex;
> -	rcu_read_unlock();
> +	ts_pktinfo.if_index = if_index;
>  
>  	ts_pktinfo.pkt_length = skb->len - skb_mac_offset(skb);
>  	put_cmsg(msg, SOL_SOCKET, SCM_TIMESTAMPING_PKTINFO,
> @@ -838,6 +834,9 @@ void __sock_recv_timestamp(struct msghdr *msg, struct sock *sk,
>  	int empty = 1, false_tstamp = 0;
>  	struct skb_shared_hwtstamps *shhwtstamps =
>  		skb_hwtstamps(skb);
> +	struct net_device *orig_dev;
> +	int if_index = 0;
> +	int phc_index = -1;
>  	ktime_t hwtstamp;
>  
>  	/* Race occurred between timestamp enabling and packet
> @@ -886,18 +885,32 @@ void __sock_recv_timestamp(struct msghdr *msg, struct sock *sk,
>  	if (shhwtstamps &&
>  	    (sk->sk_tsflags & SOF_TIMESTAMPING_RAW_HARDWARE) &&
>  	    !skb_is_swtx_tstamp(skb, false_tstamp)) {
> -		if (sk->sk_tsflags & SOF_TIMESTAMPING_BIND_PHC)
> -			hwtstamp = ptp_convert_timestamp(&shhwtstamps->hwtstamp,
> -							 sk->sk_bind_phc);
> +		rcu_read_lock();
> +		orig_dev = dev_get_by_napi_id(skb_napi_id(skb));
> +		if (orig_dev) {
> +			if_index = orig_dev->ifindex;
> +			if (skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP_PHC)
> +				phc_index = ethtool_get_phc(orig_dev);

again, this is something that can be cached, no?

> +		}
> +		rcu_read_unlock();
> +
> +		if ((skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP_PHC) &&
> +		    (phc_index != -1))
> +			hwtstamp = ptp_get_timestamp(phc_index, shhwtstamps,
> +						     sk->sk_tsflags & SOF_TIMESTAMPING_BIND_PHC);
>  		else
>  			hwtstamp = shhwtstamps->hwtstamp;
>  
> +		if (sk->sk_tsflags & SOF_TIMESTAMPING_BIND_PHC)
> +			hwtstamp = ptp_convert_timestamp(&hwtstamp,
> +							 sk->sk_bind_phc);
> +
>  		if (ktime_to_timespec64_cond(hwtstamp, tss.ts + 2)) {
>  			empty = 0;
>  
>  			if ((sk->sk_tsflags & SOF_TIMESTAMPING_OPT_PKTINFO) &&
>  			    !skb_is_err_queue(skb))
> -				put_ts_pktinfo(msg, skb);
> +				put_ts_pktinfo(msg, skb, if_index);
>  		}
>  	}
>  	if (!empty) {
> -- 
> 2.20.1
> 

Thanks,
Richard

Re: [PATCH net-next v1 5/6] ptp: Support late timestamp determination

[Gerhard Engleder]

> > diff --git a/drivers/ptp/ptp_clock.c b/drivers/ptp/ptp_clock.c
> > index 54b9f54ac0b2..b7a8cf27c349 100644
> > --- a/drivers/ptp/ptp_clock.c
> > +++ b/drivers/ptp/ptp_clock.c
> > @@ -450,6 +450,33 @@ void ptp_cancel_worker_sync(struct ptp_clock *ptp)
> >  }
> >  EXPORT_SYMBOL(ptp_cancel_worker_sync);
> >
> > +ktime_t ptp_get_timestamp(int index,
> > +                       const struct skb_shared_hwtstamps *hwtstamps,
> > +                       bool cycles)
> > +{
> > +     char name[PTP_CLOCK_NAME_LEN] = "";
> > +     struct ptp_clock *ptp;
> > +     struct device *dev;
> > +     ktime_t ts;
> > +
> > +     snprintf(name, PTP_CLOCK_NAME_LEN, "ptp%d", index);
> > +     dev = class_find_device_by_name(ptp_class, name);
>
> This seems expensive for every single Rx frame in a busy PTP network.
> Can't this be cached in the socket?

I thought that PTP packages are rare and that bloating the socket is
not welcome.
I'll try to implement some caching.

> > --- a/include/linux/skbuff.h
> > +++ b/include/linux/skbuff.h
> > @@ -564,7 +564,10 @@ static inline bool skb_frag_must_loop(struct page *p)
> >   * &skb_shared_info. Use skb_hwtstamps() to get a pointer.
> >   */
> >  struct skb_shared_hwtstamps {
> > -     ktime_t hwtstamp;
> > +     union {
> > +             ktime_t hwtstamp;
> > +             void *phc_data;
>
> needs kdoc update

Sorry, I totally forgot that. I'll fix it.

> > @@ -886,18 +885,32 @@ void __sock_recv_timestamp(struct msghdr *msg, struct sock *sk,
> >       if (shhwtstamps &&
> >           (sk->sk_tsflags & SOF_TIMESTAMPING_RAW_HARDWARE) &&
> >           !skb_is_swtx_tstamp(skb, false_tstamp)) {
> > -             if (sk->sk_tsflags & SOF_TIMESTAMPING_BIND_PHC)
> > -                     hwtstamp = ptp_convert_timestamp(&shhwtstamps->hwtstamp,
> > -                                                      sk->sk_bind_phc);
> > +             rcu_read_lock();
> > +             orig_dev = dev_get_by_napi_id(skb_napi_id(skb));
> > +             if (orig_dev) {
> > +                     if_index = orig_dev->ifindex;
> > +                     if (skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP_PHC)
> > +                             phc_index = ethtool_get_phc(orig_dev);
>
> again, this is something that can be cached, no?

I'll try to implement some caching.

Thanks for your review!

Gerhard

Re: [PATCH net-next v1 5/6] ptp: Support late timestamp determination

[Richard Cochran]

On Thu, Mar 24, 2022 at 08:52:18PM +0100, Gerhard Engleder wrote:

> I thought that PTP packages are rare and that bloating the socket is
> not welcome.

Some PTP profiles use insanely high frame rates.  like G.8275.1 with
Sync and Delay Req at 16/sec each.  times the number of clients.

Bloating the skbuff is bad, but the `sock` is not so critical in its
storage costs.

Thanks,
Richard

Re: [PATCH net-next v1 5/6] ptp: Support late timestamp determination

[Richard Cochran]

On Thu, Mar 24, 2022 at 05:04:32PM -0700, Richard Cochran wrote:
> On Thu, Mar 24, 2022 at 08:52:18PM +0100, Gerhard Engleder wrote:
> 
> > I thought that PTP packages are rare and that bloating the socket is
> > not welcome.
> 
> Some PTP profiles use insanely high frame rates.  like G.8275.1 with
> Sync and Delay Req at 16/sec each.  times the number of clients.

times the number of vclocks/Domains.

Re: [PATCH net-next v1 5/6] ptp: Support late timestamp determination

[Gerhard Engleder]

> > > > > diff --git a/drivers/ptp/ptp_clock.c b/drivers/ptp/ptp_clock.c
> > > > > index 54b9f54ac0b2..b7a8cf27c349 100644
> > > > > --- a/drivers/ptp/ptp_clock.c
> > > > > +++ b/drivers/ptp/ptp_clock.c
> > > > > @@ -450,6 +450,33 @@ void ptp_cancel_worker_sync(struct ptp_clock *ptp)
> > > > >  }
> > > > >  EXPORT_SYMBOL(ptp_cancel_worker_sync);
> > > > >
> > > > > +ktime_t ptp_get_timestamp(int index,
> > > > > +                       const struct skb_shared_hwtstamps *hwtstamps,
> > > > > +                       bool cycles)
> > > > > +{
> > > > > +     char name[PTP_CLOCK_NAME_LEN] = "";
> > > > > +     struct ptp_clock *ptp;
> > > > > +     struct device *dev;
> > > > > +     ktime_t ts;
> > > > > +
> > > > > +     snprintf(name, PTP_CLOCK_NAME_LEN, "ptp%d", index);
> > > > > +     dev = class_find_device_by_name(ptp_class, name);
> > > >
> > > > This seems expensive for every single Rx frame in a busy PTP network.
> > > > Can't this be cached in the socket?
> > > I thought that PTP packages are rare and that bloating the socket is
> > > not welcome.
> >
> > Some PTP profiles use insanely high frame rates.  like G.8275.1 with
> > Sync and Delay Req at 16/sec each.  times the number of clients.
>
> times the number of vclocks/Domains.

Getting the physical clock in __sock_recv_timestamp() is the expensive path.
The network device is already available __sock_recv_timestamp(). Timestamp
determination based on address/cookie could be done by the network device
instead of the physical clock. In my opinion, that would be a good fit, because
timestamp generation is already a task of the network device and implementation
would be faster/simpler. What do you think?

Gerhard

Re: [PATCH net-next v1 5/6] ptp: Support late timestamp determination

[Vinicius Costa Gomes]

Gerhard Engleder <gerhard@engleder-embedded.com> writes:

> If a physical clock supports a free running time called cycles, then
> timestamps shall be based on this time too. For TX it is known in
> advance before the transmission if a timestamp based on cycles is
> needed. For RX it is impossible to know which timestamp is needed before
> the packet is received and assigned to a socket.
>
> Support late timestamp determination by a physical clock. Therefore, an
> address/cookie is stored within the new phc_data field of struct
> skb_shared_hwtstamps. This address/cookie is provided to a new physical
> clock method called gettstamp(), which returns a timestamp based on the
> normal/adjustable time or based on cycles.
>
> The previously introduced flag SKBTX_HW_TSTAMP_USE_CYCLES is reused with
> an additional alias to request the late timestamp determination by the
> physical clock. That is possible, because SKBTX_HW_TSTAMP_USE_CYCLES is
> not used in the receive path.
>
> Signed-off-by: Gerhard Engleder <gerhard@engleder-embedded.com>
> ---
>  drivers/ptp/ptp_clock.c          | 27 ++++++++++++++++++++++++
>  include/linux/ptp_clock_kernel.h | 30 +++++++++++++++++++++++++++
>  include/linux/skbuff.h           |  8 +++++++-
>  net/socket.c                     | 35 ++++++++++++++++++++++----------
>  4 files changed, 88 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/ptp/ptp_clock.c b/drivers/ptp/ptp_clock.c
> index 54b9f54ac0b2..b7a8cf27c349 100644
> --- a/drivers/ptp/ptp_clock.c
> +++ b/drivers/ptp/ptp_clock.c
> @@ -450,6 +450,33 @@ void ptp_cancel_worker_sync(struct ptp_clock *ptp)
>  }
>  EXPORT_SYMBOL(ptp_cancel_worker_sync);
>  
> +ktime_t ptp_get_timestamp(int index,
> +			  const struct skb_shared_hwtstamps *hwtstamps,
> +			  bool cycles)
> +{
> +	char name[PTP_CLOCK_NAME_LEN] = "";
> +	struct ptp_clock *ptp;
> +	struct device *dev;
> +	ktime_t ts;
> +
> +	snprintf(name, PTP_CLOCK_NAME_LEN, "ptp%d", index);
> +	dev = class_find_device_by_name(ptp_class, name);
> +	if (!dev)
> +		return 0;
> +
> +	ptp = dev_get_drvdata(dev);
> +
> +	if (ptp->info->gettstamp)
> +		ts = ptp->info->gettstamp(ptp->info, hwtstamps, cycles);
> +	else
> +		ts = hwtstamps->hwtstamp;
> +
> +	put_device(dev);
> +
> +	return ts;
> +}
> +EXPORT_SYMBOL(ptp_get_timestamp);
> +
>  /* module operations */
>  
>  static void __exit ptp_exit(void)
> diff --git a/include/linux/ptp_clock_kernel.h b/include/linux/ptp_clock_kernel.h
> index cc6a7b2e267d..f4f0d8a880c6 100644
> --- a/include/linux/ptp_clock_kernel.h
> +++ b/include/linux/ptp_clock_kernel.h
> @@ -133,6 +133,16 @@ struct ptp_system_timestamp {
>   *                   parameter cts: Contains timestamp (device,system) pair,
>   *                   where system time is realtime and monotonic.
>   *
> + * @gettstamp:  Get hardware timestamp based on normal/adjustable time or free
> + *              running time. If @getcycles64 or @getcyclesx64 are supported,
> + *              then this method is required to provide timestamps based on the
> + *              free running time. This method will be called if
> + *              SKBTX_HW_TSTAMP_PHC is set by the driver.
> + *              parameter hwtstamps: skb_shared_hwtstamps structure pointer.
> + *              parameter cycles: If false, then hardware timestamp based on
> + *              normal/adjustable time is requested. If true, then hardware
> + *              timestamp based on free running time is requested.
> + *
>   * @enable:   Request driver to enable or disable an ancillary feature.
>   *            parameter request: Desired resource to enable or disable.
>   *            parameter on: Caller passes one to enable or zero to disable.
> @@ -185,6 +195,9 @@ struct ptp_clock_info {
>  			    struct ptp_system_timestamp *sts);
>  	int (*getcrosscycles)(struct ptp_clock_info *ptp,
>  			      struct system_device_crosststamp *cts);
> +	ktime_t (*gettstamp)(struct ptp_clock_info *ptp,
> +			     const struct skb_shared_hwtstamps *hwtstamps,
> +			     bool cycles);
>  	int (*enable)(struct ptp_clock_info *ptp,
>  		      struct ptp_clock_request *request, int on);
>  	int (*verify)(struct ptp_clock_info *ptp, unsigned int pin,
> @@ -364,6 +377,19 @@ static inline void ptp_cancel_worker_sync(struct ptp_clock *ptp)
>   * a loadable module.
>   */
>  
> +/**
> + * ptp_get_timestamp() - get timestamp of ptp clock
> + *
> + * @index:     phc index of ptp pclock.
> + * @hwtstamps: skb_shared_hwtstamps structure pointer.
> + * @cycles:    true for timestamp based on cycles.
> + *
> + * Returns timestamp, or 0 on error.
> + */
> +ktime_t ptp_get_timestamp(int index,
> +			  const struct skb_shared_hwtstamps *hwtstamps,
> +			  bool cycles);
> +
>  /**
>   * ptp_get_vclocks_index() - get all vclocks index on pclock, and
>   *                           caller is responsible to free memory
> @@ -386,6 +412,10 @@ int ptp_get_vclocks_index(int pclock_index, int **vclock_index);
>   */
>  ktime_t ptp_convert_timestamp(const ktime_t *hwtstamp, int vclock_index);
>  #else
> +static inline ktime_t ptp_get_timestamp(int index,
> +					const struct skb_shared_hwtstamps *hwtstamps,
> +					bool cycles);
> +{ return 0; }
>  static inline int ptp_get_vclocks_index(int pclock_index, int **vclock_index)
>  { return 0; }
>  static inline ktime_t ptp_convert_timestamp(const ktime_t *hwtstamp,
> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
> index f494ddbfc826..38929c113953 100644
> --- a/include/linux/skbuff.h
> +++ b/include/linux/skbuff.h
> @@ -564,7 +564,10 @@ static inline bool skb_frag_must_loop(struct page *p)
>   * &skb_shared_info. Use skb_hwtstamps() to get a pointer.
>   */
>  struct skb_shared_hwtstamps {
> -	ktime_t	hwtstamp;
> +	union {
> +		ktime_t	hwtstamp;
> +		void *phc_data;
> +	};
>  };
>  
>  /* Definitions for tx_flags in struct skb_shared_info */
> @@ -581,6 +584,9 @@ enum {
>  	/* generate hardware time stamp based on cycles if supported */
>  	SKBTX_HW_TSTAMP_USE_CYCLES = 1 << 3,
>  
> +	/* call PHC to get actual hardware time stamp */
> +	SKBTX_HW_TSTAMP_PHC = 1 << 3,
> +
>  	/* generate wifi status information (where possible) */
>  	SKBTX_WIFI_STATUS = 1 << 4,
>  
> diff --git a/net/socket.c b/net/socket.c
> index 2e932c058002..fe765d559086 100644
> --- a/net/socket.c
> +++ b/net/socket.c
> @@ -804,21 +804,17 @@ static bool skb_is_swtx_tstamp(const struct sk_buff *skb, int false_tstamp)
>  	return skb->tstamp && !false_tstamp && skb_is_err_queue(skb);
>  }
>  
> -static void put_ts_pktinfo(struct msghdr *msg, struct sk_buff *skb)
> +static void put_ts_pktinfo(struct msghdr *msg, struct sk_buff *skb,
> +			   int if_index)
>  {
>  	struct scm_ts_pktinfo ts_pktinfo;
> -	struct net_device *orig_dev;
>  
>  	if (!skb_mac_header_was_set(skb))
>  		return;
>  
>  	memset(&ts_pktinfo, 0, sizeof(ts_pktinfo));
>  
> -	rcu_read_lock();
> -	orig_dev = dev_get_by_napi_id(skb_napi_id(skb));
> -	if (orig_dev)
> -		ts_pktinfo.if_index = orig_dev->ifindex;
> -	rcu_read_unlock();
> +	ts_pktinfo.if_index = if_index;
>  
>  	ts_pktinfo.pkt_length = skb->len - skb_mac_offset(skb);
>  	put_cmsg(msg, SOL_SOCKET, SCM_TIMESTAMPING_PKTINFO,
> @@ -838,6 +834,9 @@ void __sock_recv_timestamp(struct msghdr *msg, struct sock *sk,
>  	int empty = 1, false_tstamp = 0;
>  	struct skb_shared_hwtstamps *shhwtstamps =
>  		skb_hwtstamps(skb);
> +	struct net_device *orig_dev;
> +	int if_index = 0;
> +	int phc_index = -1;
>  	ktime_t hwtstamp;
>  
>  	/* Race occurred between timestamp enabling and packet
> @@ -886,18 +885,32 @@ void __sock_recv_timestamp(struct msghdr *msg, struct sock *sk,
>  	if (shhwtstamps &&
>  	    (sk->sk_tsflags & SOF_TIMESTAMPING_RAW_HARDWARE) &&
>  	    !skb_is_swtx_tstamp(skb, false_tstamp)) {
> -		if (sk->sk_tsflags & SOF_TIMESTAMPING_BIND_PHC)
> -			hwtstamp = ptp_convert_timestamp(&shhwtstamps->hwtstamp,
> -							 sk->sk_bind_phc);
> +		rcu_read_lock();
> +		orig_dev = dev_get_by_napi_id(skb_napi_id(skb));
> +		if (orig_dev) {
> +			if_index = orig_dev->ifindex;
> +			if (skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP_PHC)
> +				phc_index = ethtool_get_phc(orig_dev);
> +		}
> +		rcu_read_unlock();
> +
> +		if ((skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP_PHC) &&
> +		    (phc_index != -1))
> +			hwtstamp = ptp_get_timestamp(phc_index, shhwtstamps,
> +						     sk->sk_tsflags & SOF_TIMESTAMPING_BIND_PHC);
>  		else

I think that the only part that I don't like is how ethtool_get_phc()
and ptp_get_timestamp() work together. Would it make sense for
ethtool_get_phc() to return a 'struct ptp_clock' directly? And make
ptp_get_timestamp() accept a ptp_clock?

I always get worried that by using indexes it's hard to guarantee that
we are using the "right" device (thinking when the user is creating and
destrying devices non stop).

It could be that by addressing Richard comments you will handle this
concern as well.

>  			hwtstamp = shhwtstamps->hwtstamp;
>  
> +		if (sk->sk_tsflags & SOF_TIMESTAMPING_BIND_PHC)
> +			hwtstamp = ptp_convert_timestamp(&hwtstamp,
> +							 sk->sk_bind_phc);
> +
>  		if (ktime_to_timespec64_cond(hwtstamp, tss.ts + 2)) {
>  			empty = 0;
>  
>  			if ((sk->sk_tsflags & SOF_TIMESTAMPING_OPT_PKTINFO) &&
>  			    !skb_is_err_queue(skb))
> -				put_ts_pktinfo(msg, skb);
> +				put_ts_pktinfo(msg, skb, if_index);
>  		}
>  	}
>  	if (!empty) {
> -- 
> 2.20.1
>

Cheers,
-- 
Vinicius

[PATCH net-next v1 6/6] tsnep: Add physical clock cycles support

[Gerhard Engleder]

The TSN endpoint Ethernet MAC supports a free running counter
additionally to its clock. This free running counter can be read and
hardware timestamps are supported. As the name implies, this counter
cannot be set and its frequency cannot be adjusted.

Add cycles support based on free running counter to physical clock. This
also requires hardware time stamps based on that free running counter.

Signed-off-by: Gerhard Engleder <gerhard@engleder-embedded.com>
---
 drivers/net/ethernet/engleder/tsnep_hw.h   |  9 ++++-
 drivers/net/ethernet/engleder/tsnep_main.c | 27 ++++++++-----
 drivers/net/ethernet/engleder/tsnep_ptp.c  | 44 ++++++++++++++++++++++
 3 files changed, 69 insertions(+), 11 deletions(-)

diff --git a/drivers/net/ethernet/engleder/tsnep_hw.h b/drivers/net/ethernet/engleder/tsnep_hw.h
index 71cc8577d640..916ceac3ada2 100644
--- a/drivers/net/ethernet/engleder/tsnep_hw.h
+++ b/drivers/net/ethernet/engleder/tsnep_hw.h
@@ -43,6 +43,10 @@
 #define ECM_RESET_CHANNEL 0x00000100
 #define ECM_RESET_TXRX 0x00010000
 
+/* counter */
+#define ECM_COUNTER_LOW 0x0028
+#define ECM_COUNTER_HIGH 0x002C
+
 /* control and status */
 #define ECM_STATUS 0x0080
 #define ECM_LINK_MODE_OFF 0x01000000
@@ -190,7 +194,8 @@ struct tsnep_tx_desc {
 /* tsnep TX descriptor writeback */
 struct tsnep_tx_desc_wb {
 	__le32 properties;
-	__le32 reserved1[3];
+	__le32 reserved1;
+	__le64 counter;
 	__le64 timestamp;
 	__le32 dma_delay;
 	__le32 reserved2;
@@ -221,7 +226,7 @@ struct tsnep_rx_desc_wb {
 
 /* tsnep RX inline meta */
 struct tsnep_rx_inline {
-	__le64 reserved;
+	__le64 counter;
 	__le64 timestamp;
 };
 
diff --git a/drivers/net/ethernet/engleder/tsnep_main.c b/drivers/net/ethernet/engleder/tsnep_main.c
index 904f3304727e..599776c6bd5e 100644
--- a/drivers/net/ethernet/engleder/tsnep_main.c
+++ b/drivers/net/ethernet/engleder/tsnep_main.c
@@ -441,6 +441,7 @@ static bool tsnep_tx_poll(struct tsnep_tx *tx, int napi_budget)
 	unsigned long flags;
 	int budget = 128;
 	struct tsnep_tx_entry *entry;
+	struct skb_shared_info *shinfo;
 	int count;
 
 	spin_lock_irqsave(&tx->lock, flags);
@@ -460,18 +461,26 @@ static bool tsnep_tx_poll(struct tsnep_tx *tx, int napi_budget)
 		 */
 		dma_rmb();
 
+		shinfo = skb_shinfo(entry->skb);
+
 		count = 1;
-		if (skb_shinfo(entry->skb)->nr_frags > 0)
-			count += skb_shinfo(entry->skb)->nr_frags;
+		if (shinfo->nr_frags > 0)
+			count += shinfo->nr_frags;
 
 		tsnep_tx_unmap(tx, count);
 
-		if ((skb_shinfo(entry->skb)->tx_flags & SKBTX_IN_PROGRESS) &&
+		if ((shinfo->tx_flags & SKBTX_IN_PROGRESS) &&
 		    (__le32_to_cpu(entry->desc_wb->properties) &
 		     TSNEP_DESC_EXTENDED_WRITEBACK_FLAG)) {
-			struct skb_shared_hwtstamps hwtstamps;
-			u64 timestamp =
-				__le64_to_cpu(entry->desc_wb->timestamp);
+			struct skb_shared_hwtstamps hwtstamps = {};
+			u64 timestamp;
+
+			if (shinfo->tx_flags & SKBTX_HW_TSTAMP_USE_CYCLES)
+				timestamp =
+					__le64_to_cpu(entry->desc_wb->counter);
+			else
+				timestamp =
+					__le64_to_cpu(entry->desc_wb->timestamp);
 
 			memset(&hwtstamps, 0, sizeof(hwtstamps));
 			hwtstamps.hwtstamp = ns_to_ktime(timestamp);
@@ -704,11 +713,11 @@ static int tsnep_rx_poll(struct tsnep_rx *rx, struct napi_struct *napi,
 					skb_hwtstamps(skb);
 				struct tsnep_rx_inline *rx_inline =
 					(struct tsnep_rx_inline *)skb->data;
-				u64 timestamp =
-					__le64_to_cpu(rx_inline->timestamp);
 
+				skb_shinfo(skb)->tx_flags |=
+					SKBTX_HW_TSTAMP_PHC;
 				memset(hwtstamps, 0, sizeof(*hwtstamps));
-				hwtstamps->hwtstamp = ns_to_ktime(timestamp);
+				hwtstamps->phc_data = rx_inline;
 			}
 			skb_pull(skb, TSNEP_RX_INLINE_METADATA_SIZE);
 			skb->protocol = eth_type_trans(skb,
diff --git a/drivers/net/ethernet/engleder/tsnep_ptp.c b/drivers/net/ethernet/engleder/tsnep_ptp.c
index eaad453d487e..eb66dfa98242 100644
--- a/drivers/net/ethernet/engleder/tsnep_ptp.c
+++ b/drivers/net/ethernet/engleder/tsnep_ptp.c
@@ -175,6 +175,48 @@ static int tsnep_ptp_settime64(struct ptp_clock_info *ptp,
 	return 0;
 }
 
+static int tsnep_ptp_getcyclesx64(struct ptp_clock_info *ptp,
+				  struct timespec64 *ts,
+				  struct ptp_system_timestamp *sts)
+{
+	struct tsnep_adapter *adapter = container_of(ptp, struct tsnep_adapter,
+						     ptp_clock_info);
+	u32 high_before;
+	u32 low;
+	u32 high;
+	u64 counter;
+
+	/* read high dword twice to detect overrun */
+	high = ioread32(adapter->addr + ECM_COUNTER_HIGH);
+	do {
+		ptp_read_system_prets(sts);
+		low = ioread32(adapter->addr + ECM_COUNTER_LOW);
+		ptp_read_system_postts(sts);
+		high_before = high;
+		high = ioread32(adapter->addr + ECM_COUNTER_HIGH);
+	} while (high != high_before);
+	counter = (((u64)high) << 32) | ((u64)low);
+
+	*ts = ns_to_timespec64(counter);
+
+	return 0;
+}
+
+static ktime_t tsnep_ptp_gettstamp(struct ptp_clock_info *ptp,
+				   const struct skb_shared_hwtstamps *hwtstamps,
+				   bool cycles)
+{
+	struct tsnep_rx_inline *rx_inline = hwtstamps->phc_data;
+	u64 timestamp;
+
+	if (cycles)
+		timestamp = __le64_to_cpu(rx_inline->counter);
+	else
+		timestamp = __le64_to_cpu(rx_inline->timestamp);
+
+	return ns_to_ktime(timestamp);
+}
+
 int tsnep_ptp_init(struct tsnep_adapter *adapter)
 {
 	int retval = 0;
@@ -192,6 +234,8 @@ int tsnep_ptp_init(struct tsnep_adapter *adapter)
 	adapter->ptp_clock_info.adjtime = tsnep_ptp_adjtime;
 	adapter->ptp_clock_info.gettimex64 = tsnep_ptp_gettimex64;
 	adapter->ptp_clock_info.settime64 = tsnep_ptp_settime64;
+	adapter->ptp_clock_info.getcyclesx64 = tsnep_ptp_getcyclesx64;
+	adapter->ptp_clock_info.gettstamp = tsnep_ptp_gettstamp;
 
 	spin_lock_init(&adapter->ptp_lock);
 
-- 
2.20.1

Re: [PATCH net-next v1 0/6] ptp: Support hardware clocks with additional free running time

[Vinicius Costa Gomes]

Hi,

Gerhard Engleder <gerhard@engleder-embedded.com> writes:

> ptp vclocks require a clock with free running time for the timecounter.
> Currently only a physical clock forced to free running is supported.
> If vclocks are used, then the physical clock cannot be synchronized
> anymore. The synchronized time is not available in hardware in this
> case. As a result, timed transmission with TAPRIO hardware support
> is not possible anymore.
>
> If hardware would support a free running time additionally to the
> physical clock, then the physical clock does not need to be forced to
> free running. Thus, the physical clocks can still be synchronized while
> vclocks are in use.
>
> The physical clock could be used to synchronize the time domain of the
> TSN network and trigger TAPRIO. In parallel vclocks can be used to
> synchronize other time domains.
>
> One year ago I thought for two time domains within a TSN network also
> two physical clocks are required. This would lead to new kernel
> interfaces for asking for the second clock, ... . But actually for a
> time triggered system like TSN there can be only one time domain that
> controls the system itself. All other time domains belong to other
> layers, but not to the time triggered system itself. So other time
> domains can be based on a free running counter if similar mechanisms
> like 2 step synchroisation are used.

I tried to look at this series from the point of view of the Intel i225
NIC and its 4 sets of timer registers, and thinking how adding support
for the "extra" 4 timers would fit with this proposal.

From what I could gather, the idea that would make more sense would be
exposing the other(s?) i225 timers as vclocks. That sounds neat to me,
i.e. the extra timer registers are indeed other "views" to the same
clock (the name "virtual" makes sense).

When retrieving the timestamps from packets (we can timestamp each
packet with two timers), the driver knows what timestamp (and what to do
with it) the user is interested in.

Is this what you (and others) had in mind?

If so, API-wise this series looks good to me. I will take a closer look
at the code tomorrow.

>
> Synchronisation was tested with two time domains between two directly
> connected hosts. Each host run two ptp4l instances, the first used the
> physical clock and the second used the virtual clock. I used my FPGA
> based network controller as network device. ptp4l was used in
> combination with the virtual clock support patches from Miroslav
> Lichvar.
>
> v1:
> - comlete rework based on feedback to RFC (Richard Cochran)
>
> Gerhard Engleder (6):
>   ptp: Add cycles support for virtual clocks
>   ptp: Request cycles for TX timestamp
>   ptp: Pass hwtstamp to ptp_convert_timestamp()
>   ethtool: Add kernel API for PHC index
>   ptp: Support late timestamp determination
>   tsnep: Add physical clock cycles support
>
>  drivers/net/ethernet/engleder/tsnep_hw.h   |  9 ++-
>  drivers/net/ethernet/engleder/tsnep_main.c | 27 ++++++---
>  drivers/net/ethernet/engleder/tsnep_ptp.c  | 44 ++++++++++++++
>  drivers/ptp/ptp_clock.c                    | 58 +++++++++++++++++--
>  drivers/ptp/ptp_private.h                  | 10 ++++
>  drivers/ptp/ptp_sysfs.c                    | 10 ++--
>  drivers/ptp/ptp_vclock.c                   | 18 +++---
>  include/linux/ethtool.h                    |  8 +++
>  include/linux/ptp_clock_kernel.h           | 67 ++++++++++++++++++++--
>  include/linux/skbuff.h                     | 11 +++-
>  net/core/skbuff.c                          |  2 +
>  net/ethtool/common.c                       | 13 +++++
>  net/socket.c                               | 45 +++++++++++----
>  13 files changed, 275 insertions(+), 47 deletions(-)
>
> -- 
> 2.20.1
>

-- 
Vinicius

Re: [PATCH net-next v1 0/6] ptp: Support hardware clocks with additional free running time

[Gerhard Engleder]

> Hi,
>
> Gerhard Engleder <gerhard@engleder-embedded.com> writes:
>
> > ptp vclocks require a clock with free running time for the timecounter.
> > Currently only a physical clock forced to free running is supported.
> > If vclocks are used, then the physical clock cannot be synchronized
> > anymore. The synchronized time is not available in hardware in this
> > case. As a result, timed transmission with TAPRIO hardware support
> > is not possible anymore.
> >
> > If hardware would support a free running time additionally to the
> > physical clock, then the physical clock does not need to be forced to
> > free running. Thus, the physical clocks can still be synchronized while
> > vclocks are in use.
> >
> > The physical clock could be used to synchronize the time domain of the
> > TSN network and trigger TAPRIO. In parallel vclocks can be used to
> > synchronize other time domains.
> >
> > One year ago I thought for two time domains within a TSN network also
> > two physical clocks are required. This would lead to new kernel
> > interfaces for asking for the second clock, ... . But actually for a
> > time triggered system like TSN there can be only one time domain that
> > controls the system itself. All other time domains belong to other
> > layers, but not to the time triggered system itself. So other time
> > domains can be based on a free running counter if similar mechanisms
> > like 2 step synchroisation are used.
>
> I tried to look at this series from the point of view of the Intel i225
> NIC and its 4 sets of timer registers, and thinking how adding support
> for the "extra" 4 timers would fit with this proposal.

I was hoping that there are other devices out there, which could also implement
that feature. At least in the TSN niche I expect that others have
similar requirements.

> From what I could gather, the idea that would make more sense would be
> exposing the other(s?) i225 timers as vclocks. That sounds neat to me,
> i.e. the extra timer registers are indeed other "views" to the same
> clock (the name "virtual" makes sense).

The other i225 timer would be exposed as vclock, yes. But it would be exposed
in a restricted way. Only as free running time which forms the base for the
vclocks. The actual time of the vclocks would not be available in the
extra timer
registers. So transformation of the "view" is done in software and not
in hardware.
Only one additional timer register set would be used.

> When retrieving the timestamps from packets (we can timestamp each
> packet with two timers), the driver knows what timestamp (and what to do
> with it) the user is interested in.

Yes, the driver knows it. For TX it is known in advance. For RX the
driver does not
know before reception which timestamp the user is interested in. So
the driver must
keep two timestamps (of physical clock and new free running time)
until the packet
is assigned to a socket.

As the i225 also supports two timestamps, my suggestion would be to use one
timestamp for the physical clock which would also trigger TAPRIO and the other
timestamp as base for an unlimited number of vclocks.

> Is this what you (and others) had in mind?

If one additional timer register set is used as free running time,
then I would say yes.

If you expect to export additional functions of the 4 timer register
sets with vclocks, then
I would say no. At least not with current vclock implementation, which
requires/supports
only a free running time in hardware and nothing else. But others may
know better how
vclocks could evolve in the future. These changes could be seen as a
first step, which
enhances the functionality of vclocks if additional hardware support
is available. The i225
timer register sets may allow further enhancements.

> If so, API-wise this series looks good to me. I will take a closer look
> at the code tomorrow.

Looking forward to your feedback!

Gerhard


On Fri, Mar 25, 2022 at 1:02 AM Vinicius Costa Gomes
<vinicius.gomes@intel.com> wrote:
>
> Hi,
>
> Gerhard Engleder <gerhard@engleder-embedded.com> writes:
>
> > ptp vclocks require a clock with free running time for the timecounter.
> > Currently only a physical clock forced to free running is supported.
> > If vclocks are used, then the physical clock cannot be synchronized
> > anymore. The synchronized time is not available in hardware in this
> > case. As a result, timed transmission with TAPRIO hardware support
> > is not possible anymore.
> >
> > If hardware would support a free running time additionally to the
> > physical clock, then the physical clock does not need to be forced to
> > free running. Thus, the physical clocks can still be synchronized while
> > vclocks are in use.
> >
> > The physical clock could be used to synchronize the time domain of the
> > TSN network and trigger TAPRIO. In parallel vclocks can be used to
> > synchronize other time domains.
> >
> > One year ago I thought for two time domains within a TSN network also
> > two physical clocks are required. This would lead to new kernel
> > interfaces for asking for the second clock, ... . But actually for a
> > time triggered system like TSN there can be only one time domain that
> > controls the system itself. All other time domains belong to other
> > layers, but not to the time triggered system itself. So other time
> > domains can be based on a free running counter if similar mechanisms
> > like 2 step synchroisation are used.
>
> I tried to look at this series from the point of view of the Intel i225
> NIC and its 4 sets of timer registers, and thinking how adding support
> for the "extra" 4 timers would fit with this proposal.
>
> From what I could gather, the idea that would make more sense would be
> exposing the other(s?) i225 timers as vclocks. That sounds neat to me,
> i.e. the extra timer registers are indeed other "views" to the same
> clock (the name "virtual" makes sense).
>
> When retrieving the timestamps from packets (we can timestamp each
> packet with two timers), the driver knows what timestamp (and what to do
> with it) the user is interested in.
>
> Is this what you (and others) had in mind?
>
> If so, API-wise this series looks good to me. I will take a closer look
> at the code tomorrow.
>
> >
> > Synchronisation was tested with two time domains between two directly
> > connected hosts. Each host run two ptp4l instances, the first used the
> > physical clock and the second used the virtual clock. I used my FPGA
> > based network controller as network device. ptp4l was used in
> > combination with the virtual clock support patches from Miroslav
> > Lichvar.
> >
> > v1:
> > - comlete rework based on feedback to RFC (Richard Cochran)
> >
> > Gerhard Engleder (6):
> >   ptp: Add cycles support for virtual clocks
> >   ptp: Request cycles for TX timestamp
> >   ptp: Pass hwtstamp to ptp_convert_timestamp()
> >   ethtool: Add kernel API for PHC index
> >   ptp: Support late timestamp determination
> >   tsnep: Add physical clock cycles support
> >
> >  drivers/net/ethernet/engleder/tsnep_hw.h   |  9 ++-
> >  drivers/net/ethernet/engleder/tsnep_main.c | 27 ++++++---
> >  drivers/net/ethernet/engleder/tsnep_ptp.c  | 44 ++++++++++++++
> >  drivers/ptp/ptp_clock.c                    | 58 +++++++++++++++++--
> >  drivers/ptp/ptp_private.h                  | 10 ++++
> >  drivers/ptp/ptp_sysfs.c                    | 10 ++--
> >  drivers/ptp/ptp_vclock.c                   | 18 +++---
> >  include/linux/ethtool.h                    |  8 +++
> >  include/linux/ptp_clock_kernel.h           | 67 ++++++++++++++++++++--
> >  include/linux/skbuff.h                     | 11 +++-
> >  net/core/skbuff.c                          |  2 +
> >  net/ethtool/common.c                       | 13 +++++
> >  net/socket.c                               | 45 +++++++++++----
> >  13 files changed, 275 insertions(+), 47 deletions(-)
> >
> > --
> > 2.20.1
> >
>
> --
> Vinicius