[PATCH net-next 0/3] Improvements to SJA1105 DSA RX timestamping

[PATCH net-next 0/3] Improvements to SJA1105 DSA RX timestamping

[Vladimir Oltean]

This series makes the sja1105 DSA driver use a dedicated kernel thread
for RX timestamping, a process which is time-sensitive and otherwise a
bit fragile. This allows users to customize their system (probabil an
embedded PTP switch) fully and allocate the CPU bandwidth for the driver
to expedite the RX timestamps as quickly as possible.

While doing this conversion, add a function to the PTP core for
cancelling this kernel thread (function which I found rather strange to
be missing).

Vladimir Oltean (3):
  ptp: introduce ptp_cancel_worker_sync
  net: dsa: sja1105: Use PTP core's dedicated kernel thread for RX
    timestamping
  net: dsa: sja1105: Empty the RX timestamping queue on PTP settings
    change

 drivers/net/dsa/sja1105/sja1105_ptp.c | 36 +++++++++++++--------------
 drivers/net/dsa/sja1105/sja1105_ptp.h |  1 +
 drivers/ptp/ptp_clock.c               |  6 +++++
 include/linux/dsa/sja1105.h           |  2 --
 include/linux/ptp_clock_kernel.h      |  7 ++++++
 5 files changed, 32 insertions(+), 20 deletions(-)

-- 
2.17.1

[PATCH net-next 1/3] ptp: introduce ptp_cancel_worker_sync

[Vladimir Oltean]

In order to effectively use the PTP kernel thread for tasks such as
timestamping packets, allow the user control over stopping it, which is
needed e.g. when the timestamping queues must be drained.

Signed-off-by: Vladimir Oltean <olteanv@gmail.com>
---
 drivers/ptp/ptp_clock.c          | 6 ++++++
 include/linux/ptp_clock_kernel.h | 7 +++++++
 2 files changed, 13 insertions(+)

diff --git a/drivers/ptp/ptp_clock.c b/drivers/ptp/ptp_clock.c
index e60eab7f8a61..4f0d91a76dcb 100644
--- a/drivers/ptp/ptp_clock.c
+++ b/drivers/ptp/ptp_clock.c
@@ -371,6 +371,12 @@ int ptp_schedule_worker(struct ptp_clock *ptp, unsigned long delay)
 }
 EXPORT_SYMBOL(ptp_schedule_worker);
 
+void ptp_cancel_worker_sync(struct ptp_clock *ptp)
+{
+	kthread_cancel_delayed_work_sync(&ptp->aux_work);
+}
+EXPORT_SYMBOL(ptp_cancel_worker_sync);
+
 /* 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 93cc4f1d444a..083e32a5e456 100644
--- a/include/linux/ptp_clock_kernel.h
+++ b/include/linux/ptp_clock_kernel.h
@@ -243,6 +243,13 @@ int ptp_find_pin(struct ptp_clock *ptp,
 
 int ptp_schedule_worker(struct ptp_clock *ptp, unsigned long delay);
 
+/**
+ * ptp_cancel_worker_sync() - cancel ptp auxiliary clock
+ *
+ * @ptp:     The clock obtained from ptp_clock_register().
+ */
+void ptp_cancel_worker_sync(struct ptp_clock *ptp);
+
 #else
 static inline struct ptp_clock *ptp_clock_register(struct ptp_clock_info *info,
 						   struct device *parent)
-- 
2.17.1

[PATCH net-next 2/3] net: dsa: sja1105: Use PTP core's dedicated kernel thread for RX timestamping

[Vladimir Oltean]

And move the queue of skb's waiting for RX timestamps into the ptp_data
structure, since it isn't needed if PTP is not compiled.

Signed-off-by: Vladimir Oltean <olteanv@gmail.com>
---
 drivers/net/dsa/sja1105/sja1105_ptp.c | 34 +++++++++++++--------------
 drivers/net/dsa/sja1105/sja1105_ptp.h |  1 +
 include/linux/dsa/sja1105.h           |  2 --
 3 files changed, 17 insertions(+), 20 deletions(-)

diff --git a/drivers/net/dsa/sja1105/sja1105_ptp.c b/drivers/net/dsa/sja1105/sja1105_ptp.c
index 038c83fbd9e8..d843c6395e52 100644
--- a/drivers/net/dsa/sja1105/sja1105_ptp.c
+++ b/drivers/net/dsa/sja1105/sja1105_ptp.c
@@ -83,6 +83,7 @@ static int sja1105_init_avb_params(struct sja1105_private *priv,
 static int sja1105_change_rxtstamping(struct sja1105_private *priv,
 				      bool on)
 {
+	struct sja1105_ptp_data *ptp_data = &priv->ptp_data;
 	struct sja1105_general_params_entry *general_params;
 	struct sja1105_table *table;
 	int rc;
@@ -367,22 +368,16 @@ static int sja1105_ptpclkval_write(struct sja1105_private *priv, u64 ticks,
 				ptp_sts);
 }
 
-#define rxtstamp_to_tagger(d) \
-	container_of((d), struct sja1105_tagger_data, rxtstamp_work)
-#define tagger_to_sja1105(d) \
-	container_of((d), struct sja1105_private, tagger_data)
-
-static void sja1105_rxtstamp_work(struct work_struct *work)
+static long sja1105_rxtstamp_work(struct ptp_clock_info *ptp)
 {
-	struct sja1105_tagger_data *tagger_data = rxtstamp_to_tagger(work);
-	struct sja1105_private *priv = tagger_to_sja1105(tagger_data);
-	struct sja1105_ptp_data *ptp_data = &priv->ptp_data;
+	struct sja1105_ptp_data *ptp_data = ptp_caps_to_data(ptp);
+	struct sja1105_private *priv = ptp_data_to_sja1105(ptp_data);
 	struct dsa_switch *ds = priv->ds;
 	struct sk_buff *skb;
 
 	mutex_lock(&ptp_data->lock);
 
-	while ((skb = skb_dequeue(&tagger_data->skb_rxtstamp_queue)) != NULL) {
+	while ((skb = skb_dequeue(&ptp_data->skb_rxtstamp_queue)) != NULL) {
 		struct skb_shared_hwtstamps *shwt = skb_hwtstamps(skb);
 		u64 ticks, ts;
 		int rc;
@@ -404,6 +399,9 @@ static void sja1105_rxtstamp_work(struct work_struct *work)
 	}
 
 	mutex_unlock(&ptp_data->lock);
+
+	/* Don't restart */
+	return -1;
 }
 
 /* Called from dsa_skb_defer_rx_timestamp */
@@ -411,16 +409,16 @@ bool sja1105_port_rxtstamp(struct dsa_switch *ds, int port,
 			   struct sk_buff *skb, unsigned int type)
 {
 	struct sja1105_private *priv = ds->priv;
-	struct sja1105_tagger_data *tagger_data = &priv->tagger_data;
+	struct sja1105_ptp_data *ptp_data = &priv->ptp_data;
 
-	if (!test_bit(SJA1105_HWTS_RX_EN, &tagger_data->state))
+	if (!test_bit(SJA1105_HWTS_RX_EN, &priv->tagger_data.state))
 		return false;
 
 	/* We need to read the full PTP clock to reconstruct the Rx
 	 * timestamp. For that we need a sleepable context.
 	 */
-	skb_queue_tail(&tagger_data->skb_rxtstamp_queue, skb);
-	schedule_work(&tagger_data->rxtstamp_work);
+	skb_queue_tail(&ptp_data->skb_rxtstamp_queue, skb);
+	ptp_schedule_worker(ptp_data->clock, 0);
 	return true;
 }
 
@@ -628,11 +626,11 @@ int sja1105_ptp_clock_register(struct dsa_switch *ds)
 		.adjtime	= sja1105_ptp_adjtime,
 		.gettimex64	= sja1105_ptp_gettimex,
 		.settime64	= sja1105_ptp_settime,
+		.do_aux_work	= sja1105_rxtstamp_work,
 		.max_adj	= SJA1105_MAX_ADJ_PPB,
 	};
 
-	skb_queue_head_init(&tagger_data->skb_rxtstamp_queue);
-	INIT_WORK(&tagger_data->rxtstamp_work, sja1105_rxtstamp_work);
+	skb_queue_head_init(&ptp_data->skb_rxtstamp_queue);
 	spin_lock_init(&tagger_data->meta_lock);
 
 	ptp_data->clock = ptp_clock_register(&ptp_data->caps, ds->dev);
@@ -653,8 +651,8 @@ void sja1105_ptp_clock_unregister(struct dsa_switch *ds)
 	if (IS_ERR_OR_NULL(ptp_data->clock))
 		return;
 
-	cancel_work_sync(&priv->tagger_data.rxtstamp_work);
-	skb_queue_purge(&priv->tagger_data.skb_rxtstamp_queue);
+	ptp_cancel_worker_sync(ptp_data->clock);
+	skb_queue_purge(&ptp_data->skb_rxtstamp_queue);
 	ptp_clock_unregister(ptp_data->clock);
 	ptp_data->clock = NULL;
 }
diff --git a/drivers/net/dsa/sja1105/sja1105_ptp.h b/drivers/net/dsa/sja1105/sja1105_ptp.h
index 470f44b76318..6f4a19eec709 100644
--- a/drivers/net/dsa/sja1105/sja1105_ptp.h
+++ b/drivers/net/dsa/sja1105/sja1105_ptp.h
@@ -30,6 +30,7 @@ struct sja1105_ptp_cmd {
 };
 
 struct sja1105_ptp_data {
+	struct sk_buff_head skb_rxtstamp_queue;
 	struct ptp_clock_info caps;
 	struct ptp_clock *clock;
 	struct sja1105_ptp_cmd cmd;
diff --git a/include/linux/dsa/sja1105.h b/include/linux/dsa/sja1105.h
index 897e799dbcb9..c0b6a603ea8c 100644
--- a/include/linux/dsa/sja1105.h
+++ b/include/linux/dsa/sja1105.h
@@ -37,8 +37,6 @@
  * the structure defined in struct sja1105_private.
  */
 struct sja1105_tagger_data {
-	struct sk_buff_head skb_rxtstamp_queue;
-	struct work_struct rxtstamp_work;
 	struct sk_buff *stampable_skb;
 	/* Protects concurrent access to the meta state machine
 	 * from taggers running on multiple ports on SMP systems
-- 
2.17.1

[PATCH net-next 3/3] net: dsa: sja1105: Empty the RX timestamping queue on PTP settings change

[Vladimir Oltean]

When disabling PTP timestamping, don't reset the switch with the new
static config until all existing PTP frames have been timestamped on the
RX path or dropped. There's nothing we can do with these afterwards.

Signed-off-by: Vladimir Oltean <olteanv@gmail.com>
---
 drivers/net/dsa/sja1105/sja1105_ptp.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/net/dsa/sja1105/sja1105_ptp.c b/drivers/net/dsa/sja1105/sja1105_ptp.c
index d843c6395e52..a16628cd5f66 100644
--- a/drivers/net/dsa/sja1105/sja1105_ptp.c
+++ b/drivers/net/dsa/sja1105/sja1105_ptp.c
@@ -102,6 +102,8 @@ static int sja1105_change_rxtstamping(struct sja1105_private *priv,
 		kfree_skb(priv->tagger_data.stampable_skb);
 		priv->tagger_data.stampable_skb = NULL;
 	}
+	ptp_cancel_worker_sync(ptp_data->clock);
+	skb_queue_purge(&ptp_data->skb_rxtstamp_queue);
 
 	return sja1105_static_config_reload(priv, SJA1105_RX_HWTSTAMPING);
 }
-- 
2.17.1

Re: [PATCH net-next 2/3] net: dsa: sja1105: Use PTP core's dedicated kernel thread for RX timestamping

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 2571 bytes --]

Hi Vladimir,

I love your patch! Yet something to improve:

[auto build test ERROR on net-next/master]
[also build test ERROR on net/master linus/master v5.5-rc3 next-20191220]
[cannot apply to sparc-next/master]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Vladimir-Oltean/Improvements-to-SJA1105-DSA-RX-timestamping/20191227-104228
base:   https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git 0914d2bb11cc182039084ac3b961dc359b647468
config: sparc64-randconfig-a001-20191226 (attached as .config)
compiler: sparc64-linux-gcc (GCC) 7.5.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        GCC_VERSION=7.5.0 make.cross ARCH=sparc64 

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

All errors (new ones prefixed by >>):

   drivers/net/dsa/sja1105/sja1105_ptp.c: In function 'sja1105_change_rxtstamping':
   drivers/net/dsa/sja1105/sja1105_ptp.c:86:27: warning: unused variable 'ptp_data' [-Wunused-variable]
     struct sja1105_ptp_data *ptp_data = &priv->ptp_data;
                              ^~~~~~~~
   drivers/net/dsa/sja1105/sja1105_ptp.c: In function 'sja1105_ptp_clock_unregister':
>> drivers/net/dsa/sja1105/sja1105_ptp.c:654:2: error: implicit declaration of function 'ptp_cancel_worker_sync'; did you mean 'cancel_work_sync'? [-Werror=implicit-function-declaration]
     ptp_cancel_worker_sync(ptp_data->clock);
     ^~~~~~~~~~~~~~~~~~~~~~
     cancel_work_sync
   cc1: some warnings being treated as errors

vim +654 drivers/net/dsa/sja1105/sja1105_ptp.c

   645	
   646	void sja1105_ptp_clock_unregister(struct dsa_switch *ds)
   647	{
   648		struct sja1105_private *priv = ds->priv;
   649		struct sja1105_ptp_data *ptp_data = &priv->ptp_data;
   650	
   651		if (IS_ERR_OR_NULL(ptp_data->clock))
   652			return;
   653	
 > 654		ptp_cancel_worker_sync(ptp_data->clock);
   655		skb_queue_purge(&ptp_data->skb_rxtstamp_queue);
   656		ptp_clock_unregister(ptp_data->clock);
   657		ptp_data->clock = NULL;
   658	}
   659	

---
0-DAY kernel test infrastructure                 Open Source Technology Center
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 28446 bytes --]

Re: [PATCH net-next 2/3] net: dsa: sja1105: Use PTP core's dedicated kernel thread for RX timestamping

[Vladimir Oltean]

On Fri, 27 Dec 2019 at 07:28, kbuild test robot <lkp@intel.com> wrote:
>
> Hi Vladimir,
>
> I love your patch! Yet something to improve:
>
> [auto build test ERROR on net-next/master]
> [also build test ERROR on net/master linus/master v5.5-rc3 next-20191220]
> [cannot apply to sparc-next/master]
> [if your patch is applied to the wrong git tree, please drop us a note to help
> improve the system. BTW, we also suggest to use '--base' option to specify the
> base tree in git format-patch, please see https://stackoverflow.com/a/37406982]
>
> url:    https://github.com/0day-ci/linux/commits/Vladimir-Oltean/Improvements-to-SJA1105-DSA-RX-timestamping/20191227-104228
> base:   https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git 0914d2bb11cc182039084ac3b961dc359b647468
> config: sparc64-randconfig-a001-20191226 (attached as .config)
> compiler: sparc64-linux-gcc (GCC) 7.5.0
> reproduce:
>         wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
>         chmod +x ~/bin/make.cross
>         # save the attached .config to linux build tree
>         GCC_VERSION=7.5.0 make.cross ARCH=sparc64
>
> If you fix the issue, kindly add following tag
> Reported-by: kbuild test robot <lkp@intel.com>
>
> All errors (new ones prefixed by >>):
>
>    drivers/net/dsa/sja1105/sja1105_ptp.c: In function 'sja1105_change_rxtstamping':
>    drivers/net/dsa/sja1105/sja1105_ptp.c:86:27: warning: unused variable 'ptp_data' [-Wunused-variable]
>      struct sja1105_ptp_data *ptp_data = &priv->ptp_data;
>                               ^~~~~~~~

Oops, this should have been part of 3/3 not 2/3...

>    drivers/net/dsa/sja1105/sja1105_ptp.c: In function 'sja1105_ptp_clock_unregister':
> >> drivers/net/dsa/sja1105/sja1105_ptp.c:654:2: error: implicit declaration of function 'ptp_cancel_worker_sync'; did you mean 'cancel_work_sync'? [-Werror=implicit-function-declaration]

And the attached .config.gz says that the error here is on the case
where CONFIG_PTP_1588_CLOCK=n. Ok.

>      ptp_cancel_worker_sync(ptp_data->clock);
>      ^~~~~~~~~~~~~~~~~~~~~~
>      cancel_work_sync
>    cc1: some warnings being treated as errors
>
> vim +654 drivers/net/dsa/sja1105/sja1105_ptp.c
>
>    645
>    646  void sja1105_ptp_clock_unregister(struct dsa_switch *ds)
>    647  {
>    648          struct sja1105_private *priv = ds->priv;
>    649          struct sja1105_ptp_data *ptp_data = &priv->ptp_data;
>    650
>    651          if (IS_ERR_OR_NULL(ptp_data->clock))
>    652                  return;
>    653
>  > 654          ptp_cancel_worker_sync(ptp_data->clock);
>    655          skb_queue_purge(&ptp_data->skb_rxtstamp_queue);
>    656          ptp_clock_unregister(ptp_data->clock);
>    657          ptp_data->clock = NULL;
>    658  }
>    659
>
> ---
> 0-DAY kernel test infrastructure                 Open Source Technology Center
> https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org Intel Corporation

Will send out a v2.

Sorry,
-Vladimir