diff mbox series

[net-next,v6,04/13] net: dsa: microchip: ptp: manipulating absolute time using ptp hw clock

Message ID 20230102050459.31023-5-arun.ramadoss@microchip.com (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series net: dsa: microchip: add PTP support for KSZ9563/KSZ8563 and LAN937x | expand

Checks

Context Check Description
netdev/tree_selection success Clearly marked for net-next
netdev/fixes_present success Fixes tag not required for -next series
netdev/subject_prefix success Link
netdev/cover_letter success Series has a cover letter
netdev/patch_count success Link
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 0 this patch: 0
netdev/cc_maintainers success CCed 11 of 11 maintainers
netdev/build_clang success Errors and warnings before: 0 this patch: 0
netdev/module_param success Was 0 now: 0
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 0 this patch: 0
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 114 lines checked
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Arun Ramadoss Jan. 2, 2023, 5:04 a.m. UTC 
From: Christian Eggers <ceggers@arri.de>

This patch is used for reconstructing the absolute time from the 32bit
hardware time stamping value. The do_aux ioctl is used for reading the
ptp hardware clock and store it to global variable.
The timestamped value in tail tag during rx and register during tx are
32 bit value (2 bit seconds and 30 bit nanoseconds). The time taken to
read entire ptp clock will be time consuming. In order to speed up, the
software clock is maintained. This clock time will be added to 32 bit
timestamp to get the absolute time stamp.

Signed-off-by: Christian Eggers <ceggers@arri.de>
Co-developed-by: Arun Ramadoss <arun.ramadoss@microchip.com>
Signed-off-by: Arun Ramadoss <arun.ramadoss@microchip.com>
---
v1 -> v2
- Used ksz_ptp_gettime instead of _ksz_ptp_gettime in do_aux_work()
- Removed the spin_lock_bh in the ksz_ptp_start_clock()

RFC v1
- This patch is based on Christian Eggers Initial hardware timestamping
support
---
 drivers/net/dsa/microchip/ksz_ptp.c | 52 ++++++++++++++++++++++++++++-
 drivers/net/dsa/microchip/ksz_ptp.h |  3 ++
 2 files changed, 54 insertions(+), 1 deletion(-)

Comments

Vladimir Oltean Jan. 3, 2023, 5:15 p.m. UTC | #1 
On Mon, Jan 02, 2023 at 10:34:50AM +0530, Arun Ramadoss wrote:
> From: Christian Eggers <ceggers@arri.de>
> 
> This patch is used for reconstructing the absolute time from the 32bit
> hardware time stamping value. The do_aux ioctl is used for reading the
> ptp hardware clock and store it to global variable.
> The timestamped value in tail tag during rx and register during tx are
> 32 bit value (2 bit seconds and 30 bit nanoseconds). The time taken to
> read entire ptp clock will be time consuming. In order to speed up, the
> software clock is maintained. This clock time will be added to 32 bit
> timestamp to get the absolute time stamp.
> 
> Signed-off-by: Christian Eggers <ceggers@arri.de>
> Co-developed-by: Arun Ramadoss <arun.ramadoss@microchip.com>
> Signed-off-by: Arun Ramadoss <arun.ramadoss@microchip.com>
> ---
> v1 -> v2
> - Used ksz_ptp_gettime instead of _ksz_ptp_gettime in do_aux_work()
> - Removed the spin_lock_bh in the ksz_ptp_start_clock()
> 
> RFC v1
> - This patch is based on Christian Eggers Initial hardware timestamping
> support
> ---
>  drivers/net/dsa/microchip/ksz_ptp.c | 52 ++++++++++++++++++++++++++++-
>  drivers/net/dsa/microchip/ksz_ptp.h |  3 ++
>  2 files changed, 54 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/dsa/microchip/ksz_ptp.c b/drivers/net/dsa/microchip/ksz_ptp.c
> index 8be03095e061..16f172c1f5c2 100644
> --- a/drivers/net/dsa/microchip/ksz_ptp.c
> +++ b/drivers/net/dsa/microchip/ksz_ptp.c
> @@ -28,9 +28,11 @@
>  static int ksz_ptp_enable_mode(struct ksz_device *dev)
>  {
>  	struct ksz_tagger_data *tagger_data = ksz_tagger_data(dev->ds);
> +	struct ksz_ptp_data *ptp_data = &dev->ptp_data;
>  	struct ksz_port *prt;
>  	struct dsa_port *dp;
>  	bool tag_en = false;
> +	int ret;
>  
>  	dsa_switch_for_each_user_port(dp, dev->ds) {
>  		prt = &dev->ports[dp->index];
> @@ -40,6 +42,14 @@ static int ksz_ptp_enable_mode(struct ksz_device *dev)
>  		}
>  	}
>  
> +	if (tag_en) {
> +		ret = ptp_schedule_worker(ptp_data->clock, 0);
> +		if (ret)
> +			return ret;
> +	} else {
> +		ptp_cancel_worker_sync(ptp_data->clock);
> +	}
> +
>  	tagger_data->hwtstamp_set_state(dev->ds, tag_en);
>  
>  	return ksz_rmw16(dev, REG_PTP_MSG_CONF1, PTP_ENABLE,
> @@ -221,6 +231,12 @@ static int ksz_ptp_settime(struct ptp_clock_info *ptp,
>  		goto unlock;
>  
>  	ret = ksz_rmw16(dev, REG_PTP_CLK_CTRL, PTP_LOAD_TIME, PTP_LOAD_TIME);
> +	if (ret)
> +		goto unlock;
> +
> +	spin_lock_bh(&ptp_data->clock_lock);
> +	ptp_data->clock_time = *ts;
> +	spin_unlock_bh(&ptp_data->clock_lock);
>  
>  unlock:
>  	mutex_unlock(&ptp_data->lock);
> @@ -271,6 +287,7 @@ static int ksz_ptp_adjtime(struct ptp_clock_info *ptp, s64 delta)
>  {
>  	struct ksz_ptp_data *ptp_data = ptp_caps_to_data(ptp);
>  	struct ksz_device *dev = ptp_data_to_ksz_dev(ptp_data);
> +	struct timespec64 delta64 = ns_to_timespec64(delta);
>  	s32 sec, nsec;
>  	u16 data16;
>  	int ret;
> @@ -303,15 +320,46 @@ static int ksz_ptp_adjtime(struct ptp_clock_info *ptp, s64 delta)
>  		data16 |= PTP_STEP_DIR;
>  
>  	ret = ksz_write16(dev, REG_PTP_CLK_CTRL, data16);
> +	if (ret)
> +		goto unlock;
> +
> +	spin_lock_bh(&ptp_data->clock_lock);
> +	ptp_data->clock_time = timespec64_add(ptp_data->clock_time, delta64);
> +	spin_unlock_bh(&ptp_data->clock_lock);
>  
>  unlock:
>  	mutex_unlock(&ptp_data->lock);
>  	return ret;
>  }
>  
> +/*  Function is pointer to the do_aux_work in the ptp_clock capability */
> +static long ksz_ptp_do_aux_work(struct ptp_clock_info *ptp)
> +{
> +	struct ksz_ptp_data *ptp_data = ptp_caps_to_data(ptp);
> +	struct timespec64 ts;
> +
> +	ksz_ptp_gettime(ptp, &ts);
> +
> +	spin_lock_bh(&ptp_data->clock_lock);
> +	ptp_data->clock_time = ts;
> +	spin_unlock_bh(&ptp_data->clock_lock);
> +
> +	return HZ;  /* reschedule in 1 second */
> +}

This races with both ksz_ptp_adjtime() and ksz_ptp_settime() because here,
ptp_data->clock_time is not written under mutex_unlock(&ptp_data->lock).

So the following can happen:

CPU 0                                         |   CPU 1
                                              |
ksz_ptp_do_aux_work()                         |
-> ksz_ptp_gettime(ptp, &ts);                 |
   -> mutex_lock(&ptp_data->lock);            |
   -> mutex_unlock(&ptp_data->lock);          |
                                              |   ksz_ptp_adjtime()
                                              |   -> mutex_lock(&ptp_data->lock);
                                              |   -> spin_lock_bh(&ptp_data->clock_lock);
                                              |   -> updates ptp_data->clock_time
                                              |   -> spin_unlock_bh(&ptp_data->clock_lock);
                                              |   -> mutex_unlock(&ptp_data->lock);
   -> spin_lock_bh(&ptp_data->clock_lock);    |
   -> overwites ptp_data->clock_time          |
   -> spin_unlock_bh(&ptp_data->clock_lock);  |


So at the end, ptp_data->clock_time will contain a time prior to the
ksz_ptp_adjtime() call, which can drift over time. This can lead to the
30 us phase offset that Christian has been complaining about privately
to you, me and Richard.

You see, the entire ksz_ptp_do_aux_work() operation needs to take place
under the mutex, to block user space from modifying the clock.

Neither yourself nor Christian wanted to get rid of this apparent
shortcut (to cache ptp_data->clock_time instead of reading it when
needed, and making ksz_port_rxtstamp() ask for skb deferral so that we
have sleepable context to access SPI/I2C).

As a result (because we can't make ksz_tstamp_reconstruct() block user
space, so we need to make the cached value take into account user space
modifications too), we now have this more complicated alternative which
also contains subtle bugs.

> +
>  static int ksz_ptp_start_clock(struct ksz_device *dev)
>  {
> -	return ksz_rmw16(dev, REG_PTP_CLK_CTRL, PTP_CLK_ENABLE, PTP_CLK_ENABLE);
> +	struct ksz_ptp_data *ptp_data = &dev->ptp_data;
> +	int ret;
> +
> +	ret = ksz_rmw16(dev, REG_PTP_CLK_CTRL, PTP_CLK_ENABLE, PTP_CLK_ENABLE);
> +	if (ret)
> +		return ret;
> +
> +	ptp_data->clock_time.tv_sec = 0;
> +	ptp_data->clock_time.tv_nsec = 0;
> +
> +	return 0;
>  }
>  
>  int ksz_ptp_clock_register(struct dsa_switch *ds)
> @@ -322,6 +370,7 @@ int ksz_ptp_clock_register(struct dsa_switch *ds)
>  
>  	ptp_data = &dev->ptp_data;
>  	mutex_init(&ptp_data->lock);
> +	spin_lock_init(&ptp_data->clock_lock);
>  
>  	ptp_data->caps.owner		= THIS_MODULE;
>  	snprintf(ptp_data->caps.name, 16, "Microchip Clock");
> @@ -330,6 +379,7 @@ int ksz_ptp_clock_register(struct dsa_switch *ds)
>  	ptp_data->caps.settime64	= ksz_ptp_settime;
>  	ptp_data->caps.adjfine		= ksz_ptp_adjfine;
>  	ptp_data->caps.adjtime		= ksz_ptp_adjtime;
> +	ptp_data->caps.do_aux_work	= ksz_ptp_do_aux_work;
>  
>  	ret = ksz_ptp_start_clock(dev);
>  	if (ret)
> diff --git a/drivers/net/dsa/microchip/ksz_ptp.h b/drivers/net/dsa/microchip/ksz_ptp.h
> index 7bb3fde2dd14..2c29a0b604bb 100644
> --- a/drivers/net/dsa/microchip/ksz_ptp.h
> +++ b/drivers/net/dsa/microchip/ksz_ptp.h
> @@ -17,6 +17,9 @@ struct ksz_ptp_data {
>  	struct ptp_clock *clock;
>  	/* Serializes all operations on the PTP hardware clock */
>  	struct mutex lock;
> +	/* lock for accessing the clock_time */
> +	spinlock_t clock_lock;
> +	struct timespec64 clock_time;
>  };
>  
>  int ksz_ptp_clock_register(struct dsa_switch *ds);
> -- 
> 2.36.1
>
diff mbox series

Patch

diff --git a/drivers/net/dsa/microchip/ksz_ptp.c b/drivers/net/dsa/microchip/ksz_ptp.c
index 8be03095e061..16f172c1f5c2 100644
--- a/drivers/net/dsa/microchip/ksz_ptp.c
+++ b/drivers/net/dsa/microchip/ksz_ptp.c
@@ -28,9 +28,11 @@ 
 static int ksz_ptp_enable_mode(struct ksz_device *dev)
 {
 	struct ksz_tagger_data *tagger_data = ksz_tagger_data(dev->ds);
+	struct ksz_ptp_data *ptp_data = &dev->ptp_data;
 	struct ksz_port *prt;
 	struct dsa_port *dp;
 	bool tag_en = false;
+	int ret;
 
 	dsa_switch_for_each_user_port(dp, dev->ds) {
 		prt = &dev->ports[dp->index];
@@ -40,6 +42,14 @@  static int ksz_ptp_enable_mode(struct ksz_device *dev)
 		}
 	}
 
+	if (tag_en) {
+		ret = ptp_schedule_worker(ptp_data->clock, 0);
+		if (ret)
+			return ret;
+	} else {
+		ptp_cancel_worker_sync(ptp_data->clock);
+	}
+
 	tagger_data->hwtstamp_set_state(dev->ds, tag_en);
 
 	return ksz_rmw16(dev, REG_PTP_MSG_CONF1, PTP_ENABLE,
@@ -221,6 +231,12 @@  static int ksz_ptp_settime(struct ptp_clock_info *ptp,
 		goto unlock;
 
 	ret = ksz_rmw16(dev, REG_PTP_CLK_CTRL, PTP_LOAD_TIME, PTP_LOAD_TIME);
+	if (ret)
+		goto unlock;
+
+	spin_lock_bh(&ptp_data->clock_lock);
+	ptp_data->clock_time = *ts;
+	spin_unlock_bh(&ptp_data->clock_lock);
 
 unlock:
 	mutex_unlock(&ptp_data->lock);
@@ -271,6 +287,7 @@  static int ksz_ptp_adjtime(struct ptp_clock_info *ptp, s64 delta)
 {
 	struct ksz_ptp_data *ptp_data = ptp_caps_to_data(ptp);
 	struct ksz_device *dev = ptp_data_to_ksz_dev(ptp_data);
+	struct timespec64 delta64 = ns_to_timespec64(delta);
 	s32 sec, nsec;
 	u16 data16;
 	int ret;
@@ -303,15 +320,46 @@  static int ksz_ptp_adjtime(struct ptp_clock_info *ptp, s64 delta)
 		data16 |= PTP_STEP_DIR;
 
 	ret = ksz_write16(dev, REG_PTP_CLK_CTRL, data16);
+	if (ret)
+		goto unlock;
+
+	spin_lock_bh(&ptp_data->clock_lock);
+	ptp_data->clock_time = timespec64_add(ptp_data->clock_time, delta64);
+	spin_unlock_bh(&ptp_data->clock_lock);
 
 unlock:
 	mutex_unlock(&ptp_data->lock);
 	return ret;
 }
 
+/*  Function is pointer to the do_aux_work in the ptp_clock capability */
+static long ksz_ptp_do_aux_work(struct ptp_clock_info *ptp)
+{
+	struct ksz_ptp_data *ptp_data = ptp_caps_to_data(ptp);
+	struct timespec64 ts;
+
+	ksz_ptp_gettime(ptp, &ts);
+
+	spin_lock_bh(&ptp_data->clock_lock);
+	ptp_data->clock_time = ts;
+	spin_unlock_bh(&ptp_data->clock_lock);
+
+	return HZ;  /* reschedule in 1 second */
+}
+
 static int ksz_ptp_start_clock(struct ksz_device *dev)
 {
-	return ksz_rmw16(dev, REG_PTP_CLK_CTRL, PTP_CLK_ENABLE, PTP_CLK_ENABLE);
+	struct ksz_ptp_data *ptp_data = &dev->ptp_data;
+	int ret;
+
+	ret = ksz_rmw16(dev, REG_PTP_CLK_CTRL, PTP_CLK_ENABLE, PTP_CLK_ENABLE);
+	if (ret)
+		return ret;
+
+	ptp_data->clock_time.tv_sec = 0;
+	ptp_data->clock_time.tv_nsec = 0;
+
+	return 0;
 }
 
 int ksz_ptp_clock_register(struct dsa_switch *ds)
@@ -322,6 +370,7 @@  int ksz_ptp_clock_register(struct dsa_switch *ds)
 
 	ptp_data = &dev->ptp_data;
 	mutex_init(&ptp_data->lock);
+	spin_lock_init(&ptp_data->clock_lock);
 
 	ptp_data->caps.owner		= THIS_MODULE;
 	snprintf(ptp_data->caps.name, 16, "Microchip Clock");
@@ -330,6 +379,7 @@  int ksz_ptp_clock_register(struct dsa_switch *ds)
 	ptp_data->caps.settime64	= ksz_ptp_settime;
 	ptp_data->caps.adjfine		= ksz_ptp_adjfine;
 	ptp_data->caps.adjtime		= ksz_ptp_adjtime;
+	ptp_data->caps.do_aux_work	= ksz_ptp_do_aux_work;
 
 	ret = ksz_ptp_start_clock(dev);
 	if (ret)
diff --git a/drivers/net/dsa/microchip/ksz_ptp.h b/drivers/net/dsa/microchip/ksz_ptp.h
index 7bb3fde2dd14..2c29a0b604bb 100644
--- a/drivers/net/dsa/microchip/ksz_ptp.h
+++ b/drivers/net/dsa/microchip/ksz_ptp.h
@@ -17,6 +17,9 @@  struct ksz_ptp_data {
 	struct ptp_clock *clock;
 	/* Serializes all operations on the PTP hardware clock */
 	struct mutex lock;
+	/* lock for accessing the clock_time */
+	spinlock_t clock_lock;
+	struct timespec64 clock_time;
 };
 
 int ksz_ptp_clock_register(struct dsa_switch *ds);