[PATCH v3 0/3] net: ethernet: ti: cpts: fix tx timestamping timeout

[PATCH v3 0/3] net: ethernet: ti: cpts: fix tx timestamping timeout

[Grygorii Strashko]

Hi

With the low Ethernet connection speed cpdma notification about packet
processing can be received before CPTS TX timestamp event, which is set
when packet actually left CPSW while cpdma notification is sent when packet
pushed in CPSW fifo. As result, when connection is slow and CPU is fast
enough TX timestamping is not working properly.
Issue was discovered using timestamping tool on am57x boards with Ethernet link
speed forced to 100M and on am335x-evm with Ethernet link speed forced to 10M.

Patch3 - This series fixes it by introducing TX SKB queue to store PTP SKBs for
which Ethernet Transmit Event hasn't been received yet and then re-check this
queue with new Ethernet Transmit Events by scheduling CPTS overflow
work more often until TX SKB queue is not empty.

Patch 1,2 - As CPTS overflow work is time critical task it important to ensure
that its scheduling is not delayed. Unfortunately, There could be significant
delay in CPTS work schedule under high system load and on -RT which could cause
CPTS misbehavior due to internal counter overflow and there is no way to tune
CPTS overflow work execution policy and priority manually. The kthread_worker
can be used instead of workqueues, as it creates separate named kthread for
each worker and its its execution policy and priority can be configured
using chrt tool. Instead of modifying CPTS driver itself it was proposed to
it was proposed to add PTP auxiliary worker to the PHC subsystem [1], so
other drivers can benefit from this feature also.

[1] https://www.spinics.net/lists/netdev/msg445392.html

changes in v3:
- patch 1: added proper error handling in ptp_clock_register. 
  minor comments applied.

changes in v2:
- added PTP auxiliary worker to the PHC subsystem

Links
v2: https://www.spinics.net/lists/netdev/msg445859.html
v1: https://www.spinics.net/lists/netdev/msg445387.html

Grygorii Strashko (3):
  ptp: introduce ptp auxiliary worker
  net: ethernet: ti: cpts: convert to use ptp auxiliary worker
  net: ethernet: ti: cpts: fix tx timestamping timeout

 drivers/net/ethernet/ti/cpts.c   | 111 +++++++++++++++++++++++++++++++++------
 drivers/net/ethernet/ti/cpts.h   |   2 +-
 drivers/ptp/ptp_clock.c          |  41 +++++++++++++++
 drivers/ptp/ptp_private.h        |   3 ++
 include/linux/ptp_clock_kernel.h |  20 +++++++
 5 files changed, 160 insertions(+), 17 deletions(-)

-- 
2.10.1

[PATCH v3 1/3] ptp: introduce ptp auxiliary worker

[Grygorii Strashko]

Many PTP drivers required to perform some asynchronous or periodic work,
like periodically handling PHC counter overflow or handle delayed timestamp
for RX/TX network packets. In most of the cases, such work is implemented
using workqueues. Unfortunately, Kernel workqueues might introduce
significant delay in work scheduling under high system load and on -RT,
which could cause misbehavior of PTP drivers due to internal counter
overflow, for example, and there is no way to tune its execution policy and
priority manually.

Hence, The kthread_worker can be used instead of workqueues, as it creates
separate named kthread for each worker and its its execution policy and
priority can be configured using chrt tool.

This problem was reported for two drivers TI CPSW CPTS and dp83640, so
instead of modifying each of these driver it was proposed to add PTP
auxiliary worker to the PHC subsystem [1].

The patch adds PTP auxiliary worker in PHC subsystem using kthread_worker
and kthread_delayed_work and introduces two new PHC subsystem APIs:

- long (*do_aux_work)(struct ptp_clock_info *ptp) callback in
ptp_clock_info structure, which driver should assign if it require to
perform asynchronous or periodic work. Driver should return the delay of
the PTP next auxiliary work scheduling time (>=0) or negative value in case
further scheduling is not required.

- int ptp_schedule_worker(struct ptp_clock *ptp, unsigned long delay) which
allows schedule PTP auxiliary work.

The name of kthread_worker thread corresponds PTP PHC device name "ptp%d".

[1] https://www.spinics.net/lists/netdev/msg445392.html
Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
---
 drivers/ptp/ptp_clock.c          | 41 ++++++++++++++++++++++++++++++++++++++++
 drivers/ptp/ptp_private.h        |  3 +++
 include/linux/ptp_clock_kernel.h | 20 ++++++++++++++++++++
 3 files changed, 64 insertions(+)

diff --git a/drivers/ptp/ptp_clock.c b/drivers/ptp/ptp_clock.c
index b774357..899433c 100644
--- a/drivers/ptp/ptp_clock.c
+++ b/drivers/ptp/ptp_clock.c
@@ -28,6 +28,7 @@
 #include <linux/slab.h>
 #include <linux/syscalls.h>
 #include <linux/uaccess.h>
+#include <uapi/linux/sched/types.h>
 
 #include "ptp_private.h"
 
@@ -184,6 +185,19 @@ static void delete_ptp_clock(struct posix_clock *pc)
 	kfree(ptp);
 }
 
+static void ptp_aux_kworker(struct kthread_work *work)
+{
+	struct ptp_clock *ptp = container_of(work, struct ptp_clock,
+					     aux_work.work);
+	struct ptp_clock_info *info = ptp->info;
+	long delay;
+
+	delay = info->do_aux_work(info);
+
+	if (delay >= 0)
+		kthread_queue_delayed_work(ptp->kworker, &ptp->aux_work, delay);
+}
+
 /* public interface */
 
 struct ptp_clock *ptp_clock_register(struct ptp_clock_info *info,
@@ -217,6 +231,19 @@ struct ptp_clock *ptp_clock_register(struct ptp_clock_info *info,
 	mutex_init(&ptp->pincfg_mux);
 	init_waitqueue_head(&ptp->tsev_wq);
 
+	if (ptp->info->do_aux_work) {
+		char *worker_name = kasprintf(GFP_KERNEL, "ptp%d", ptp->index);
+
+		kthread_init_delayed_work(&ptp->aux_work, ptp_aux_kworker);
+		ptp->kworker = kthread_create_worker(0, worker_name ?
+						     worker_name : info->name);
+		if (IS_ERR(ptp->kworker)) {
+			err = PTR_ERR(ptp->kworker);
+			pr_err("failed to create ptp aux_worker %d\n", err);
+			goto kworker_err;
+		}
+	}
+
 	err = ptp_populate_pin_groups(ptp);
 	if (err)
 		goto no_pin_groups;
@@ -259,6 +286,9 @@ struct ptp_clock *ptp_clock_register(struct ptp_clock_info *info,
 no_device:
 	ptp_cleanup_pin_groups(ptp);
 no_pin_groups:
+	if (ptp->kworker)
+		kthread_destroy_worker(ptp->kworker);
+kworker_err:
 	mutex_destroy(&ptp->tsevq_mux);
 	mutex_destroy(&ptp->pincfg_mux);
 	ida_simple_remove(&ptp_clocks_map, index);
@@ -274,6 +304,11 @@ int ptp_clock_unregister(struct ptp_clock *ptp)
 	ptp->defunct = 1;
 	wake_up_interruptible(&ptp->tsev_wq);
 
+	if (ptp->kworker) {
+		kthread_cancel_delayed_work_sync(&ptp->aux_work);
+		kthread_destroy_worker(ptp->kworker);
+	}
+
 	/* Release the clock's resources. */
 	if (ptp->pps_source)
 		pps_unregister_source(ptp->pps_source);
@@ -339,6 +374,12 @@ int ptp_find_pin(struct ptp_clock *ptp,
 }
 EXPORT_SYMBOL(ptp_find_pin);
 
+int ptp_schedule_worker(struct ptp_clock *ptp, unsigned long delay)
+{
+	return kthread_mod_delayed_work(ptp->kworker, &ptp->aux_work, delay);
+}
+EXPORT_SYMBOL(ptp_schedule_worker);
+
 /* module operations */
 
 static void __exit ptp_exit(void)
diff --git a/drivers/ptp/ptp_private.h b/drivers/ptp/ptp_private.h
index d958889..b86f1bf 100644
--- a/drivers/ptp/ptp_private.h
+++ b/drivers/ptp/ptp_private.h
@@ -22,6 +22,7 @@
 
 #include <linux/cdev.h>
 #include <linux/device.h>
+#include <linux/kthread.h>
 #include <linux/mutex.h>
 #include <linux/posix-clock.h>
 #include <linux/ptp_clock.h>
@@ -56,6 +57,8 @@ struct ptp_clock {
 	struct attribute_group pin_attr_group;
 	/* 1st entry is a pointer to the real group, 2nd is NULL terminator */
 	const struct attribute_group *pin_attr_groups[2];
+	struct kthread_worker *kworker;
+	struct kthread_delayed_work aux_work;
 };
 
 /*
diff --git a/include/linux/ptp_clock_kernel.h b/include/linux/ptp_clock_kernel.h
index a026bfd..51349d1 100644
--- a/include/linux/ptp_clock_kernel.h
+++ b/include/linux/ptp_clock_kernel.h
@@ -99,6 +99,11 @@ struct system_device_crosststamp;
  *            parameter func: the desired function to use.
  *            parameter chan: the function channel index to use.
  *
+ * @do_work:  Request driver to perform auxiliary (periodic) operations
+ *	      Driver should return delay of the next auxiliary work scheduling
+ *	      time (>=0) or negative value in case further scheduling
+ *	      is not required.
+ *
  * Drivers should embed their ptp_clock_info within a private
  * structure, obtaining a reference to it using container_of().
  *
@@ -126,6 +131,7 @@ struct ptp_clock_info {
 		      struct ptp_clock_request *request, int on);
 	int (*verify)(struct ptp_clock_info *ptp, unsigned int pin,
 		      enum ptp_pin_function func, unsigned int chan);
+	long (*do_aux_work)(struct ptp_clock_info *ptp);
 };
 
 struct ptp_clock;
@@ -211,6 +217,16 @@ extern int ptp_clock_index(struct ptp_clock *ptp);
 int ptp_find_pin(struct ptp_clock *ptp,
 		 enum ptp_pin_function func, unsigned int chan);
 
+/**
+ * ptp_schedule_worker() - schedule ptp auxiliary work
+ *
+ * @ptp:    The clock obtained from ptp_clock_register().
+ * @delay:  number of jiffies to wait before queuing
+ *          See kthread_queue_delayed_work() for more info.
+ */
+
+int ptp_schedule_worker(struct ptp_clock *ptp, unsigned long delay);
+
 #else
 static inline struct ptp_clock *ptp_clock_register(struct ptp_clock_info *info,
 						   struct device *parent)
@@ -225,6 +241,10 @@ static inline int ptp_clock_index(struct ptp_clock *ptp)
 static inline int ptp_find_pin(struct ptp_clock *ptp,
 			       enum ptp_pin_function func, unsigned int chan)
 { return -1; }
+static inline int ptp_schedule_worker(struct ptp_clock *ptp,
+				      unsigned long delay)
+{ return -EOPNOTSUPP; }
+
 #endif
 
 #endif
-- 
2.10.1

[PATCH v4 2/3] net: ethernet: ti: cpts: convert to use ptp auxiliary worker

[Grygorii Strashko]

There could be significant delay in CPTS work schedule under high system
load and on -RT which could cause CPTS misbehavior due to internal counter
overflow. Usage of own kthread_worker allows to avoid such kind of issues
and makes it possible to tune priority of CPTS kthread_worker thread on -RT.

Hence, the CPTS driver is converted to use PTP auxiliary worker as PHC
subsystem implements such functionality in a generic way now.

Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
---
 drivers/net/ethernet/ti/cpts.c | 27 +++++++++++++--------------
 drivers/net/ethernet/ti/cpts.h |  1 -
 2 files changed, 13 insertions(+), 15 deletions(-)

diff --git a/drivers/net/ethernet/ti/cpts.c b/drivers/net/ethernet/ti/cpts.c
index 32279d2..3ed438e 100644
--- a/drivers/net/ethernet/ti/cpts.c
+++ b/drivers/net/ethernet/ti/cpts.c
@@ -224,6 +224,17 @@ static int cpts_ptp_enable(struct ptp_clock_info *ptp,
 	return -EOPNOTSUPP;
 }
 
+static long cpts_overflow_check(struct ptp_clock_info *ptp)
+{
+	struct cpts *cpts = container_of(ptp, struct cpts, info);
+	unsigned long delay = cpts->ov_check_period;
+	struct timespec64 ts;
+
+	cpts_ptp_gettime(&cpts->info, &ts);
+	pr_debug("cpts overflow check at %lld.%09lu\n", ts.tv_sec, ts.tv_nsec);
+	return (long)delay;
+}
+
 static struct ptp_clock_info cpts_info = {
 	.owner		= THIS_MODULE,
 	.name		= "CTPS timer",
@@ -236,18 +247,9 @@ static struct ptp_clock_info cpts_info = {
 	.gettime64	= cpts_ptp_gettime,
 	.settime64	= cpts_ptp_settime,
 	.enable		= cpts_ptp_enable,
+	.do_aux_work	= cpts_overflow_check,
 };
 
-static void cpts_overflow_check(struct work_struct *work)
-{
-	struct timespec64 ts;
-	struct cpts *cpts = container_of(work, struct cpts, overflow_work.work);
-
-	cpts_ptp_gettime(&cpts->info, &ts);
-	pr_debug("cpts overflow check at %lld.%09lu\n", ts.tv_sec, ts.tv_nsec);
-	schedule_delayed_work(&cpts->overflow_work, cpts->ov_check_period);
-}
-
 static int cpts_match(struct sk_buff *skb, unsigned int ptp_class,
 		      u16 ts_seqid, u8 ts_msgtype)
 {
@@ -378,7 +380,7 @@ int cpts_register(struct cpts *cpts)
 	}
 	cpts->phc_index = ptp_clock_index(cpts->clock);
 
-	schedule_delayed_work(&cpts->overflow_work, cpts->ov_check_period);
+	ptp_schedule_worker(cpts->clock, cpts->ov_check_period);
 	return 0;
 
 err_ptp:
@@ -392,8 +394,6 @@ void cpts_unregister(struct cpts *cpts)
 	if (WARN_ON(!cpts->clock))
 		return;
 
-	cancel_delayed_work_sync(&cpts->overflow_work);
-
 	ptp_clock_unregister(cpts->clock);
 	cpts->clock = NULL;
 
@@ -476,7 +476,6 @@ struct cpts *cpts_create(struct device *dev, void __iomem *regs,
 	cpts->dev = dev;
 	cpts->reg = (struct cpsw_cpts __iomem *)regs;
 	spin_lock_init(&cpts->lock);
-	INIT_DELAYED_WORK(&cpts->overflow_work, cpts_overflow_check);
 
 	ret = cpts_of_parse(cpts, node);
 	if (ret)
diff --git a/drivers/net/ethernet/ti/cpts.h b/drivers/net/ethernet/ti/cpts.h
index 01ea82b..586edd9 100644
--- a/drivers/net/ethernet/ti/cpts.h
+++ b/drivers/net/ethernet/ti/cpts.h
@@ -119,7 +119,6 @@ struct cpts {
 	u32 cc_mult; /* for the nominal frequency */
 	struct cyclecounter cc;
 	struct timecounter tc;
-	struct delayed_work overflow_work;
 	int phc_index;
 	struct clk *refclk;
 	struct list_head events;
-- 
2.10.1

[PATCH v3 3/3] net: ethernet: ti: cpts: fix tx timestamping timeout

[Grygorii Strashko]

With the low speed Ethernet connection CPDMA notification about packet
processing can be received before CPTS TX timestamp event, which is set
when packet actually left CPSW while cpdma notification is sent when packet
pushed in CPSW fifo.  As result, when connection is slow and CPU is fast
enough TX timestamping is not working properly.

Fix it, by introducing TX SKB queue to store PTP SKBs for which Ethernet
Transmit Event hasn't been received yet and then re-check this queue
with new Ethernet Transmit Events by scheduling CPTS overflow
work more often (every 1 jiffies) until TX SKB queue is not empty.

Side effect of this change is:
 - User space tools require to take into account possible delay in TX
timestamp processing (for example ptp4l works with tx_timestamp_timeout=400
under net traffic and tx_timestamp_timeout=25 in idle).

Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
---
 drivers/net/ethernet/ti/cpts.c | 86 ++++++++++++++++++++++++++++++++++++++++--
 drivers/net/ethernet/ti/cpts.h |  1 +
 2 files changed, 84 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/ti/cpts.c b/drivers/net/ethernet/ti/cpts.c
index 3ed438e..c2121d2 100644
--- a/drivers/net/ethernet/ti/cpts.c
+++ b/drivers/net/ethernet/ti/cpts.c
@@ -31,9 +31,18 @@
 
 #include "cpts.h"
 
+#define CPTS_SKB_TX_WORK_TIMEOUT 1 /* jiffies */
+
+struct cpts_skb_cb_data {
+	unsigned long tmo;
+};
+
 #define cpts_read32(c, r)	readl_relaxed(&c->reg->r)
 #define cpts_write32(c, v, r)	writel_relaxed(v, &c->reg->r)
 
+static int cpts_match(struct sk_buff *skb, unsigned int ptp_class,
+		      u16 ts_seqid, u8 ts_msgtype);
+
 static int event_expired(struct cpts_event *event)
 {
 	return time_after(jiffies, event->tmo);
@@ -77,6 +86,47 @@ static int cpts_purge_events(struct cpts *cpts)
 	return removed ? 0 : -1;
 }
 
+static bool cpts_match_tx_ts(struct cpts *cpts, struct cpts_event *event)
+{
+	struct sk_buff *skb, *tmp;
+	u16 seqid;
+	u8 mtype;
+	bool found = false;
+
+	mtype = (event->high >> MESSAGE_TYPE_SHIFT) & MESSAGE_TYPE_MASK;
+	seqid = (event->high >> SEQUENCE_ID_SHIFT) & SEQUENCE_ID_MASK;
+
+	/* no need to grab txq.lock as access is always done under cpts->lock */
+	skb_queue_walk_safe(&cpts->txq, skb, tmp) {
+		struct skb_shared_hwtstamps ssh;
+		unsigned int class = ptp_classify_raw(skb);
+		struct cpts_skb_cb_data *skb_cb =
+					(struct cpts_skb_cb_data *)skb->cb;
+
+		if (cpts_match(skb, class, seqid, mtype)) {
+			u64 ns = timecounter_cyc2time(&cpts->tc, event->low);
+
+			memset(&ssh, 0, sizeof(ssh));
+			ssh.hwtstamp = ns_to_ktime(ns);
+			skb_tstamp_tx(skb, &ssh);
+			found = true;
+			__skb_unlink(skb, &cpts->txq);
+			dev_consume_skb_any(skb);
+			dev_dbg(cpts->dev, "match tx timestamp mtype %u seqid %04x\n",
+				mtype, seqid);
+		} else if (time_after(jiffies, skb_cb->tmo)) {
+			/* timeout any expired skbs over 1s */
+			dev_dbg(cpts->dev,
+				"expiring tx timestamp mtype %u seqid %04x\n",
+				mtype, seqid);
+			__skb_unlink(skb, &cpts->txq);
+			dev_consume_skb_any(skb);
+		}
+	}
+
+	return found;
+}
+
 /*
  * Returns zero if matching event type was found.
  */
@@ -101,9 +151,15 @@ static int cpts_fifo_read(struct cpts *cpts, int match)
 		event->low = lo;
 		type = event_type(event);
 		switch (type) {
+		case CPTS_EV_TX:
+			if (cpts_match_tx_ts(cpts, event)) {
+				/* if the new event matches an existing skb,
+				 * then don't queue it
+				 */
+				break;
+			}
 		case CPTS_EV_PUSH:
 		case CPTS_EV_RX:
-		case CPTS_EV_TX:
 			list_del_init(&event->list);
 			list_add_tail(&event->list, &cpts->events);
 			break;
@@ -229,8 +285,15 @@ static long cpts_overflow_check(struct ptp_clock_info *ptp)
 	struct cpts *cpts = container_of(ptp, struct cpts, info);
 	unsigned long delay = cpts->ov_check_period;
 	struct timespec64 ts;
+	unsigned long flags;
+
+	spin_lock_irqsave(&cpts->lock, flags);
+	ts = ns_to_timespec64(timecounter_read(&cpts->tc));
+
+	if (!skb_queue_empty(&cpts->txq))
+		delay = CPTS_SKB_TX_WORK_TIMEOUT;
+	spin_unlock_irqrestore(&cpts->lock, flags);
 
-	cpts_ptp_gettime(&cpts->info, &ts);
 	pr_debug("cpts overflow check at %lld.%09lu\n", ts.tv_sec, ts.tv_nsec);
 	return (long)delay;
 }
@@ -301,7 +364,7 @@ static u64 cpts_find_ts(struct cpts *cpts, struct sk_buff *skb, int ev_type)
 		return 0;
 
 	spin_lock_irqsave(&cpts->lock, flags);
-	cpts_fifo_read(cpts, CPTS_EV_PUSH);
+	cpts_fifo_read(cpts, -1);
 	list_for_each_safe(this, next, &cpts->events) {
 		event = list_entry(this, struct cpts_event, list);
 		if (event_expired(event)) {
@@ -319,6 +382,19 @@ static u64 cpts_find_ts(struct cpts *cpts, struct sk_buff *skb, int ev_type)
 			break;
 		}
 	}
+
+	if (ev_type == CPTS_EV_TX && !ns) {
+		struct cpts_skb_cb_data *skb_cb =
+				(struct cpts_skb_cb_data *)skb->cb;
+		/* Not found, add frame to queue for processing later.
+		 * The periodic FIFO check will handle this.
+		 */
+		skb_get(skb);
+		/* get the timestamp for timeouts */
+		skb_cb->tmo = jiffies + msecs_to_jiffies(100);
+		__skb_queue_tail(&cpts->txq, skb);
+		ptp_schedule_worker(cpts->clock, 0);
+	}
 	spin_unlock_irqrestore(&cpts->lock, flags);
 
 	return ns;
@@ -360,6 +436,7 @@ int cpts_register(struct cpts *cpts)
 {
 	int err, i;
 
+	skb_queue_head_init(&cpts->txq);
 	INIT_LIST_HEAD(&cpts->events);
 	INIT_LIST_HEAD(&cpts->pool);
 	for (i = 0; i < CPTS_MAX_EVENTS; i++)
@@ -400,6 +477,9 @@ void cpts_unregister(struct cpts *cpts)
 	cpts_write32(cpts, 0, int_enable);
 	cpts_write32(cpts, 0, control);
 
+	/* Drop all packet */
+	skb_queue_purge(&cpts->txq);
+
 	clk_disable(cpts->refclk);
 }
 EXPORT_SYMBOL_GPL(cpts_unregister);
diff --git a/drivers/net/ethernet/ti/cpts.h b/drivers/net/ethernet/ti/cpts.h
index 586edd9..73d73fa 100644
--- a/drivers/net/ethernet/ti/cpts.h
+++ b/drivers/net/ethernet/ti/cpts.h
@@ -125,6 +125,7 @@ struct cpts {
 	struct list_head pool;
 	struct cpts_event pool_data[CPTS_MAX_EVENTS];
 	unsigned long ov_check_period;
+	struct sk_buff_head txq;
 };
 
 void cpts_rx_timestamp(struct cpts *cpts, struct sk_buff *skb);
-- 
2.10.1

Re: [PATCH v3 1/3] ptp: introduce ptp auxiliary worker

[Richard Cochran]

On Wed, Jul 26, 2017 at 05:11:36PM -0500, Grygorii Strashko wrote:
> @@ -217,6 +231,19 @@ struct ptp_clock *ptp_clock_register(struct ptp_clock_info *info,
>  	mutex_init(&ptp->pincfg_mux);
>  	init_waitqueue_head(&ptp->tsev_wq);
>  
> +	if (ptp->info->do_aux_work) {
> +		char *worker_name = kasprintf(GFP_KERNEL, "ptp%d", ptp->index);

This string is allocated but never freed.

> +		kthread_init_delayed_work(&ptp->aux_work, ptp_aux_kworker);
> +		ptp->kworker = kthread_create_worker(0, worker_name ?
> +						     worker_name : info->name);
> +		if (IS_ERR(ptp->kworker)) {
> +			err = PTR_ERR(ptp->kworker);
> +			pr_err("failed to create ptp aux_worker %d\n", err);
> +			goto kworker_err;
> +		}
> +	}
> +

Thanks,
Richard

Re: [PATCH v3 1/3] ptp: introduce ptp auxiliary worker

[Grygorii Strashko]

On 07/27/2017 03:08 PM, Richard Cochran wrote:
> On Wed, Jul 26, 2017 at 05:11:36PM -0500, Grygorii Strashko wrote:
>> @@ -217,6 +231,19 @@ struct ptp_clock *ptp_clock_register(struct ptp_clock_info *info,
>>   	mutex_init(&ptp->pincfg_mux);
>>   	init_waitqueue_head(&ptp->tsev_wq);
>>   
>> +	if (ptp->info->do_aux_work) {
>> +		char *worker_name = kasprintf(GFP_KERNEL, "ptp%d", ptp->index);
> 
> This string is allocated but never freed.
> 
>> +		kthread_init_delayed_work(&ptp->aux_work, ptp_aux_kworker);
>> +		ptp->kworker = kthread_create_worker(0, worker_name ?
>> +						     worker_name : info->name);

Ops. Right need to add kfree(worker_name) here.


>> +		if (IS_ERR(ptp->kworker)) {
>> +			err = PTR_ERR(ptp->kworker);
>> +			pr_err("failed to create ptp aux_worker %d\n", err);
>> +			goto kworker_err;
>> +		}
>> +	}
>> +
> 
> Thanks,
> Richard
> 

-- 
regards,
-grygorii

Re: [PATCH v3 3/3] net: ethernet: ti: cpts: fix tx timestamping timeout

[Ivan Khoronzhuk]

On Wed, Jul 26, 2017 at 05:11:38PM -0500, Grygorii Strashko wrote:
> With the low speed Ethernet connection CPDMA notification about packet
> processing can be received before CPTS TX timestamp event, which is set
> when packet actually left CPSW while cpdma notification is sent when packet
> pushed in CPSW fifo.  As result, when connection is slow and CPU is fast
> enough TX timestamping is not working properly.
> 
> Fix it, by introducing TX SKB queue to store PTP SKBs for which Ethernet
> Transmit Event hasn't been received yet and then re-check this queue
> with new Ethernet Transmit Events by scheduling CPTS overflow
> work more often (every 1 jiffies) until TX SKB queue is not empty.
> 
> Side effect of this change is:
>  - User space tools require to take into account possible delay in TX
> timestamp processing (for example ptp4l works with tx_timestamp_timeout=400
> under net traffic and tx_timestamp_timeout=25 in idle).
> 
> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>


> ---
>  drivers/net/ethernet/ti/cpts.c | 86 ++++++++++++++++++++++++++++++++++++++++--
>  drivers/net/ethernet/ti/cpts.h |  1 +
>  2 files changed, 84 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/ethernet/ti/cpts.c b/drivers/net/ethernet/ti/cpts.c
> index 3ed438e..c2121d2 100644
> --- a/drivers/net/ethernet/ti/cpts.c
> +++ b/drivers/net/ethernet/ti/cpts.c
> @@ -31,9 +31,18 @@
>  
>  #include "cpts.h"
>  
> +#define CPTS_SKB_TX_WORK_TIMEOUT 1 /* jiffies */
> +
> +struct cpts_skb_cb_data {
> +	unsigned long tmo;
> +};
> +
>  #define cpts_read32(c, r)	readl_relaxed(&c->reg->r)
>  #define cpts_write32(c, v, r)	writel_relaxed(v, &c->reg->r)
>  
> +static int cpts_match(struct sk_buff *skb, unsigned int ptp_class,
> +		      u16 ts_seqid, u8 ts_msgtype);
> +
>  static int event_expired(struct cpts_event *event)
>  {
>  	return time_after(jiffies, event->tmo);
> @@ -77,6 +86,47 @@ static int cpts_purge_events(struct cpts *cpts)
>  	return removed ? 0 : -1;
>  }
>  
> +static bool cpts_match_tx_ts(struct cpts *cpts, struct cpts_event *event)
> +{
> +	struct sk_buff *skb, *tmp;
> +	u16 seqid;
> +	u8 mtype;
> +	bool found = false;
> +
> +	mtype = (event->high >> MESSAGE_TYPE_SHIFT) & MESSAGE_TYPE_MASK;
> +	seqid = (event->high >> SEQUENCE_ID_SHIFT) & SEQUENCE_ID_MASK;
> +
> +	/* no need to grab txq.lock as access is always done under cpts->lock */
> +	skb_queue_walk_safe(&cpts->txq, skb, tmp) {
> +		struct skb_shared_hwtstamps ssh;
> +		unsigned int class = ptp_classify_raw(skb);
> +		struct cpts_skb_cb_data *skb_cb =
> +					(struct cpts_skb_cb_data *)skb->cb;
> +
> +		if (cpts_match(skb, class, seqid, mtype)) {
> +			u64 ns = timecounter_cyc2time(&cpts->tc, event->low);
> +
> +			memset(&ssh, 0, sizeof(ssh));
> +			ssh.hwtstamp = ns_to_ktime(ns);
> +			skb_tstamp_tx(skb, &ssh);
> +			found = true;
> +			__skb_unlink(skb, &cpts->txq);
> +			dev_consume_skb_any(skb);
> +			dev_dbg(cpts->dev, "match tx timestamp mtype %u seqid %04x\n",
> +				mtype, seqid);
> +		} else if (time_after(jiffies, skb_cb->tmo)) {
> +			/* timeout any expired skbs over 1s */
> +			dev_dbg(cpts->dev,
> +				"expiring tx timestamp mtype %u seqid %04x\n",
> +				mtype, seqid);
> +			__skb_unlink(skb, &cpts->txq);
> +			dev_consume_skb_any(skb);
> +		}
> +	}
> +
> +	return found;
> +}
> +
>  /*
>   * Returns zero if matching event type was found.
>   */
> @@ -101,9 +151,15 @@ static int cpts_fifo_read(struct cpts *cpts, int match)
>  		event->low = lo;
>  		type = event_type(event);
>  		switch (type) {
> +		case CPTS_EV_TX:
> +			if (cpts_match_tx_ts(cpts, event)) {
> +				/* if the new event matches an existing skb,
> +				 * then don't queue it
> +				 */
> +				break;
> +			}
>  		case CPTS_EV_PUSH:
>  		case CPTS_EV_RX:
> -		case CPTS_EV_TX:
>  			list_del_init(&event->list);
>  			list_add_tail(&event->list, &cpts->events);
>  			break;
> @@ -229,8 +285,15 @@ static long cpts_overflow_check(struct ptp_clock_info *ptp)
>  	struct cpts *cpts = container_of(ptp, struct cpts, info);
>  	unsigned long delay = cpts->ov_check_period;
>  	struct timespec64 ts;
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&cpts->lock, flags);
> +	ts = ns_to_timespec64(timecounter_read(&cpts->tc));
> +
> +	if (!skb_queue_empty(&cpts->txq))
> +		delay = CPTS_SKB_TX_WORK_TIMEOUT;
> +	spin_unlock_irqrestore(&cpts->lock, flags);
>  
> -	cpts_ptp_gettime(&cpts->info, &ts);
>  	pr_debug("cpts overflow check at %lld.%09lu\n", ts.tv_sec, ts.tv_nsec);
>  	return (long)delay;
>  }
> @@ -301,7 +364,7 @@ static u64 cpts_find_ts(struct cpts *cpts, struct sk_buff *skb, int ev_type)
>  		return 0;
>  
>  	spin_lock_irqsave(&cpts->lock, flags);
> -	cpts_fifo_read(cpts, CPTS_EV_PUSH);
> +	cpts_fifo_read(cpts, -1);
Seems it should be explained or send as separate patch before this one.
Not sure, but smth like "Don't stop read fifo if PUSH event is found",
but how it can happen that push event is present w/o TS_PUSH request..,
anyway, if there is some valuable reason for that - should be explained.

>  	list_for_each_safe(this, next, &cpts->events) {
>  		event = list_entry(this, struct cpts_event, list);
>  		if (event_expired(event)) {
> @@ -319,6 +382,19 @@ static u64 cpts_find_ts(struct cpts *cpts, struct sk_buff *skb, int ev_type)
>  			break;
>  		}
>  	}
> +
> +	if (ev_type == CPTS_EV_TX && !ns) {
> +		struct cpts_skb_cb_data *skb_cb =
> +				(struct cpts_skb_cb_data *)skb->cb;
> +		/* Not found, add frame to queue for processing later.
> +		 * The periodic FIFO check will handle this.
> +		 */
> +		skb_get(skb);
> +		/* get the timestamp for timeouts */
> +		skb_cb->tmo = jiffies + msecs_to_jiffies(100);
> +		__skb_queue_tail(&cpts->txq, skb);
> +		ptp_schedule_worker(cpts->clock, 0);
> +	}
As I see no need in separate irq handler for cpts events after this.
Is that caused by h/w limitations on keystone2?

And what about rx path introduced in RFC, is it not needed any more or are
you going to send additional series?

>  	spin_unlock_irqrestore(&cpts->lock, flags);
>  
>  	return ns;
> @@ -360,6 +436,7 @@ int cpts_register(struct cpts *cpts)
>  {
>  	int err, i;
>  
> +	skb_queue_head_init(&cpts->txq);
>  	INIT_LIST_HEAD(&cpts->events);
>  	INIT_LIST_HEAD(&cpts->pool);
>  	for (i = 0; i < CPTS_MAX_EVENTS; i++)
> @@ -400,6 +477,9 @@ void cpts_unregister(struct cpts *cpts)
>  	cpts_write32(cpts, 0, int_enable);
>  	cpts_write32(cpts, 0, control);
>  
> +	/* Drop all packet */
> +	skb_queue_purge(&cpts->txq);
> +
>  	clk_disable(cpts->refclk);
>  }
>  EXPORT_SYMBOL_GPL(cpts_unregister);
> diff --git a/drivers/net/ethernet/ti/cpts.h b/drivers/net/ethernet/ti/cpts.h
> index 586edd9..73d73fa 100644
> --- a/drivers/net/ethernet/ti/cpts.h
> +++ b/drivers/net/ethernet/ti/cpts.h
> @@ -125,6 +125,7 @@ struct cpts {
>  	struct list_head pool;
>  	struct cpts_event pool_data[CPTS_MAX_EVENTS];
>  	unsigned long ov_check_period;
> +	struct sk_buff_head txq;
>  };
>  
>  void cpts_rx_timestamp(struct cpts *cpts, struct sk_buff *skb);
> -- 
> 2.10.1
> 

Re: [PATCH v3 3/3] net: ethernet: ti: cpts: fix tx timestamping timeout

[Grygorii Strashko]

On 07/28/2017 12:02 PM, Ivan Khoronzhuk wrote:
> On Wed, Jul 26, 2017 at 05:11:38PM -0500, Grygorii Strashko wrote:
>> With the low speed Ethernet connection CPDMA notification about packet
>> processing can be received before CPTS TX timestamp event, which is set
>> when packet actually left CPSW while cpdma notification is sent when packet
>> pushed in CPSW fifo.  As result, when connection is slow and CPU is fast
>> enough TX timestamping is not working properly.
>>
>> Fix it, by introducing TX SKB queue to store PTP SKBs for which Ethernet
>> Transmit Event hasn't been received yet and then re-check this queue
>> with new Ethernet Transmit Events by scheduling CPTS overflow
>> work more often (every 1 jiffies) until TX SKB queue is not empty.
>>
>> Side effect of this change is:
>>   - User space tools require to take into account possible delay in TX
>> timestamp processing (for example ptp4l works with tx_timestamp_timeout=400
>> under net traffic and tx_timestamp_timeout=25 in idle).
>>
>> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
> 
> 
>> ---
>>   drivers/net/ethernet/ti/cpts.c | 86 ++++++++++++++++++++++++++++++++++++++++--
>>   drivers/net/ethernet/ti/cpts.h |  1 +
>>   2 files changed, 84 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/ti/cpts.c b/drivers/net/ethernet/ti/cpts.c
>> index 3ed438e..c2121d2 100644
>> --- a/drivers/net/ethernet/ti/cpts.c
>> +++ b/drivers/net/ethernet/ti/cpts.c
>> @@ -31,9 +31,18 @@
>>   
>>   #include "cpts.h"
>>   
>> +#define CPTS_SKB_TX_WORK_TIMEOUT 1 /* jiffies */
>> +
>> +struct cpts_skb_cb_data {
>> +	unsigned long tmo;
>> +};
>> +
>>   #define cpts_read32(c, r)	readl_relaxed(&c->reg->r)
>>   #define cpts_write32(c, v, r)	writel_relaxed(v, &c->reg->r)
>>   
>> +static int cpts_match(struct sk_buff *skb, unsigned int ptp_class,
>> +		      u16 ts_seqid, u8 ts_msgtype);
>> +
>>   static int event_expired(struct cpts_event *event)
>>   {
>>   	return time_after(jiffies, event->tmo);
>> @@ -77,6 +86,47 @@ static int cpts_purge_events(struct cpts *cpts)
>>   	return removed ? 0 : -1;
>>   }
>>   
>> +static bool cpts_match_tx_ts(struct cpts *cpts, struct cpts_event *event)
>> +{
>> +	struct sk_buff *skb, *tmp;
>> +	u16 seqid;
>> +	u8 mtype;
>> +	bool found = false;
>> +
>> +	mtype = (event->high >> MESSAGE_TYPE_SHIFT) & MESSAGE_TYPE_MASK;
>> +	seqid = (event->high >> SEQUENCE_ID_SHIFT) & SEQUENCE_ID_MASK;
>> +
>> +	/* no need to grab txq.lock as access is always done under cpts->lock */
>> +	skb_queue_walk_safe(&cpts->txq, skb, tmp) {
>> +		struct skb_shared_hwtstamps ssh;
>> +		unsigned int class = ptp_classify_raw(skb);
>> +		struct cpts_skb_cb_data *skb_cb =
>> +					(struct cpts_skb_cb_data *)skb->cb;
>> +
>> +		if (cpts_match(skb, class, seqid, mtype)) {
>> +			u64 ns = timecounter_cyc2time(&cpts->tc, event->low);
>> +
>> +			memset(&ssh, 0, sizeof(ssh));
>> +			ssh.hwtstamp = ns_to_ktime(ns);
>> +			skb_tstamp_tx(skb, &ssh);
>> +			found = true;
>> +			__skb_unlink(skb, &cpts->txq);
>> +			dev_consume_skb_any(skb);
>> +			dev_dbg(cpts->dev, "match tx timestamp mtype %u seqid %04x\n",
>> +				mtype, seqid);
>> +		} else if (time_after(jiffies, skb_cb->tmo)) {
>> +			/* timeout any expired skbs over 1s */
>> +			dev_dbg(cpts->dev,
>> +				"expiring tx timestamp mtype %u seqid %04x\n",
>> +				mtype, seqid);
>> +			__skb_unlink(skb, &cpts->txq);
>> +			dev_consume_skb_any(skb);
>> +		}
>> +	}
>> +
>> +	return found;
>> +}
>> +
>>   /*
>>    * Returns zero if matching event type was found.
>>    */
>> @@ -101,9 +151,15 @@ static int cpts_fifo_read(struct cpts *cpts, int match)
>>   		event->low = lo;
>>   		type = event_type(event);
>>   		switch (type) {
>> +		case CPTS_EV_TX:
>> +			if (cpts_match_tx_ts(cpts, event)) {
>> +				/* if the new event matches an existing skb,
>> +				 * then don't queue it
>> +				 */
>> +				break;
>> +			}
>>   		case CPTS_EV_PUSH:
>>   		case CPTS_EV_RX:
>> -		case CPTS_EV_TX:
>>   			list_del_init(&event->list);
>>   			list_add_tail(&event->list, &cpts->events);
>>   			break;
>> @@ -229,8 +285,15 @@ static long cpts_overflow_check(struct ptp_clock_info *ptp)
>>   	struct cpts *cpts = container_of(ptp, struct cpts, info);
>>   	unsigned long delay = cpts->ov_check_period;
>>   	struct timespec64 ts;
>> +	unsigned long flags;
>> +
>> +	spin_lock_irqsave(&cpts->lock, flags);
>> +	ts = ns_to_timespec64(timecounter_read(&cpts->tc));
>> +
>> +	if (!skb_queue_empty(&cpts->txq))
>> +		delay = CPTS_SKB_TX_WORK_TIMEOUT;
>> +	spin_unlock_irqrestore(&cpts->lock, flags);
>>   
>> -	cpts_ptp_gettime(&cpts->info, &ts);
>>   	pr_debug("cpts overflow check at %lld.%09lu\n", ts.tv_sec, ts.tv_nsec);
>>   	return (long)delay;
>>   }
>> @@ -301,7 +364,7 @@ static u64 cpts_find_ts(struct cpts *cpts, struct sk_buff *skb, int ev_type)
>>   		return 0;
>>   
>>   	spin_lock_irqsave(&cpts->lock, flags);
>> -	cpts_fifo_read(cpts, CPTS_EV_PUSH);
>> +	cpts_fifo_read(cpts, -1);
> Seems it should be explained or send as separate patch before this one.
> Not sure, but smth like "Don't stop read fifo if PUSH event is found",
> but how it can happen that push event is present w/o TS_PUSH request..,
> anyway, if there is some valuable reason for that - should be explained.

ok. I'll drop it from this series.

> 
>>   	list_for_each_safe(this, next, &cpts->events) {
>>   		event = list_entry(this, struct cpts_event, list);
>>   		if (event_expired(event)) {
>> @@ -319,6 +382,19 @@ static u64 cpts_find_ts(struct cpts *cpts, struct sk_buff *skb, int ev_type)
>>   			break;
>>   		}
>>   	}
>> +
>> +	if (ev_type == CPTS_EV_TX && !ns) {
>> +		struct cpts_skb_cb_data *skb_cb =
>> +				(struct cpts_skb_cb_data *)skb->cb;
>> +		/* Not found, add frame to queue for processing later.
>> +		 * The periodic FIFO check will handle this.
>> +		 */
>> +		skb_get(skb);
>> +		/* get the timestamp for timeouts */
>> +		skb_cb->tmo = jiffies + msecs_to_jiffies(100);
>> +		__skb_queue_tail(&cpts->txq, skb);
>> +		ptp_schedule_worker(cpts->clock, 0);
>> +	}
> As I see no need in separate irq handler for cpts events after this.
> Is that caused by h/w limitations on keystone2?

It's still required for proper support of HW_TS_PUSH events.
This fixes current issue which blocks CPTS usage.

> 
> And what about rx path introduced in RFC, is it not needed any more or are
> you going to send additional series?

And I'm still trying to work on enabling CPTS IRQs and if it will be 
enabled - the RX path will also need to be modified.



-- 
regards,
-grygorii