diff mbox series

[net,v2,2/2] net: micrel: Fix set/get PHC time for lan8814

Message ID 20240118085916.1204354-3-horatiu.vultur@microchip.com (mailing list archive)
State Deferred
Delegated to: Netdev Maintainers
Headers show
Series net: micrel: Fixes for PHC for lan8814 | expand

Checks

Context Check Description
netdev/series_format success Posting correctly formatted
netdev/tree_selection success Clearly marked for net
netdev/ynl success SINGLE THREAD; Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag present in non-next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 1080 this patch: 1080
netdev/cc_maintainers success CCed 0 of 0 maintainers
netdev/build_clang success Errors and warnings before: 1095 this patch: 1095
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success Fixes tag looks correct
netdev/build_allmodconfig_warn success Errors and warnings before: 1095 this patch: 1095
netdev/checkpatch warning WARNING: line length of 83 exceeds 80 columns WARNING: line length of 84 exceeds 80 columns
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0
netdev/contest success net-next-2024-01-18--12-00 (tests: 403)

Commit Message

Horatiu Vultur Jan. 18, 2024, 8:59 a.m. UTC
When setting or getting PHC time, the higher bits of the second time (>32
bits) they were ignored. Meaning that setting some time in the future like
year 2150, it was failing to set this.

The issue can be reproduced like this:

 # phc_ctl /dev/ptp1 set 10000000000
 phc_ctl[118.619]: set clock time to 4294967295.000000000 or Sun Feb  7 06:28:15 2106

 # phc_ctl /dev/ptp1 get
 phc_ctl[120.858]: clock time is 1.238620924 or Thu Jan  1 00:00:01 1970

Fixes: ece19502834d ("net: phy: micrel: 1588 support for LAN8814 phy")
Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>
Reviewed-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
Reviewed-by: Divya Koppera <divya.koppera@microchip.com>
---
 drivers/net/phy/micrel.c | 61 +++++++++++++++++++---------------------
 1 file changed, 29 insertions(+), 32 deletions(-)

Comments

Andrew Lunn Jan. 18, 2024, 1:52 p.m. UTC | #1
On Thu, Jan 18, 2024 at 09:59:16AM +0100, Horatiu Vultur wrote:
> When setting or getting PHC time, the higher bits of the second time (>32
> bits) they were ignored. Meaning that setting some time in the future like
> year 2150, it was failing to set this.

Stable rules say:

It must either fix a real bug that bothers people...

Do we have users of this device in 2150?

Maybe submit this for net-next?

      Andrew
Horatiu Vultur Jan. 18, 2024, 3:23 p.m. UTC | #2
The 01/18/2024 14:52, Andrew Lunn wrote:

Hi Andrew,

> 
> On Thu, Jan 18, 2024 at 09:59:16AM +0100, Horatiu Vultur wrote:
> > When setting or getting PHC time, the higher bits of the second time (>32
> > bits) they were ignored. Meaning that setting some time in the future like
> > year 2150, it was failing to set this.
> 
> Stable rules say:
> 
> It must either fix a real bug that bothers people...
> 
> Do we have users of this device in 2150?

No, and it is not like a customer raise this issue because they have
some special cases. It was me who discover it by accident and I
found it annoying not being able to do something that is that simple.
I mean, I just want to set the time and then be able to read it back.

> 
> Maybe submit this for net-next?

Anyway, I don't have strong feelings about this, if it goes to net or
net-next, I just want to fix this at some point :)

> 
>       Andrew
Andrew Lunn Jan. 18, 2024, 5 p.m. UTC | #3
> > Maybe submit this for net-next?
> 
> Anyway, I don't have strong feelings about this, if it goes to net or
> net-next, I just want to fix this at some point :)

Please submit to net-next. I think the ML bot which picks out patches
to backport is likely to see the work Fix in the subject and decided
to backport it anyway. But its not our problem if the bot breaks the
stable rules.

Is there any danger of regressions? Could the higher word actually
have a value for some reason today, which is being ignored. Would this
change then jump the time forward?

       Andrew
Horatiu Vultur Jan. 19, 2024, 8:21 a.m. UTC | #4
The 01/18/2024 18:00, Andrew Lunn wrote:
> 
> > > Maybe submit this for net-next?
> >
> > Anyway, I don't have strong feelings about this, if it goes to net or
> > net-next, I just want to fix this at some point :)
> 
> Please submit to net-next. I think the ML bot which picks out patches
> to backport is likely to see the work Fix in the subject and decided
> to backport it anyway. But its not our problem if the bot breaks the
> stable rules.

Yes, I will do that.

> 
> Is there any danger of regressions? Could the higher word actually
> have a value for some reason today, which is being ignored. Would this
> change then jump the time forward?

I am not seeing any danger of regressions.
By default the higher word has a value of 0, and doesn't have any
special functionality to jump forward or backwards. It just contains
upper 16 bits of the second [47:32].
I have seen only on register PTP_LTC_STEP_ADJ_HI that the most significant
bit can signify if to jump forward or backwards.

> 
>        Andrew
Andrew Lunn Jan. 19, 2024, 1:07 p.m. UTC | #5
On Thu, Jan 18, 2024 at 09:59:16AM +0100, Horatiu Vultur wrote:
> When setting or getting PHC time, the higher bits of the second time (>32
> bits) they were ignored. Meaning that setting some time in the future like
> year 2150, it was failing to set this.
> 
> The issue can be reproduced like this:
> 
>  # phc_ctl /dev/ptp1 set 10000000000
>  phc_ctl[118.619]: set clock time to 4294967295.000000000 or Sun Feb  7 06:28:15 2106
> 
>  # phc_ctl /dev/ptp1 get
>  phc_ctl[120.858]: clock time is 1.238620924 or Thu Jan  1 00:00:01 1970
> 
> Fixes: ece19502834d ("net: phy: micrel: 1588 support for LAN8814 phy")
> Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>
> Reviewed-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
> Reviewed-by: Divya Koppera <divya.koppera@microchip.com>

When submitted to net-next:

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew
diff mbox series

Patch

diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c
index 43520ac0f4e00..0ceba62d55c08 100644
--- a/drivers/net/phy/micrel.c
+++ b/drivers/net/phy/micrel.c
@@ -154,11 +154,13 @@ 
 #define PTP_CMD_CTL_PTP_LTC_STEP_SEC_		BIT(5)
 #define PTP_CMD_CTL_PTP_LTC_STEP_NSEC_		BIT(6)
 
+#define PTP_CLOCK_SET_SEC_HI			0x0205
 #define PTP_CLOCK_SET_SEC_MID			0x0206
 #define PTP_CLOCK_SET_SEC_LO			0x0207
 #define PTP_CLOCK_SET_NS_HI			0x0208
 #define PTP_CLOCK_SET_NS_LO			0x0209
 
+#define PTP_CLOCK_READ_SEC_HI			0x0229
 #define PTP_CLOCK_READ_SEC_MID			0x022A
 #define PTP_CLOCK_READ_SEC_LO			0x022B
 #define PTP_CLOCK_READ_NS_HI			0x022C
@@ -2592,35 +2594,31 @@  static bool lan8814_rxtstamp(struct mii_timestamper *mii_ts, struct sk_buff *skb
 }
 
 static void lan8814_ptp_clock_set(struct phy_device *phydev,
-				  u32 seconds, u32 nano_seconds)
+				  time64_t sec, u32 nsec)
 {
-	u32 sec_low, sec_high, nsec_low, nsec_high;
-
-	sec_low = seconds & 0xffff;
-	sec_high = (seconds >> 16) & 0xffff;
-	nsec_low = nano_seconds & 0xffff;
-	nsec_high = (nano_seconds >> 16) & 0x3fff;
-
-	lanphy_write_page_reg(phydev, 4, PTP_CLOCK_SET_SEC_LO, sec_low);
-	lanphy_write_page_reg(phydev, 4, PTP_CLOCK_SET_SEC_MID, sec_high);
-	lanphy_write_page_reg(phydev, 4, PTP_CLOCK_SET_NS_LO, nsec_low);
-	lanphy_write_page_reg(phydev, 4, PTP_CLOCK_SET_NS_HI, nsec_high);
+	lanphy_write_page_reg(phydev, 4, PTP_CLOCK_SET_SEC_LO, lower_16_bits(sec));
+	lanphy_write_page_reg(phydev, 4, PTP_CLOCK_SET_SEC_MID, upper_16_bits(sec));
+	lanphy_write_page_reg(phydev, 4, PTP_CLOCK_SET_SEC_HI, upper_32_bits(sec));
+	lanphy_write_page_reg(phydev, 4, PTP_CLOCK_SET_NS_LO, lower_16_bits(nsec));
+	lanphy_write_page_reg(phydev, 4, PTP_CLOCK_SET_NS_HI, upper_16_bits(nsec));
 
 	lanphy_write_page_reg(phydev, 4, PTP_CMD_CTL, PTP_CMD_CTL_PTP_CLOCK_LOAD_);
 }
 
 static void lan8814_ptp_clock_get(struct phy_device *phydev,
-				  u32 *seconds, u32 *nano_seconds)
+				  time64_t *sec, u32 *nsec)
 {
 	lanphy_write_page_reg(phydev, 4, PTP_CMD_CTL, PTP_CMD_CTL_PTP_CLOCK_READ_);
 
-	*seconds = lanphy_read_page_reg(phydev, 4, PTP_CLOCK_READ_SEC_MID);
-	*seconds = (*seconds << 16) |
-		   lanphy_read_page_reg(phydev, 4, PTP_CLOCK_READ_SEC_LO);
+	*sec = lanphy_read_page_reg(phydev, 4, PTP_CLOCK_READ_SEC_HI);
+	*sec <<= 16;
+	*sec |= lanphy_read_page_reg(phydev, 4, PTP_CLOCK_READ_SEC_MID);
+	*sec <<= 16;
+	*sec |= lanphy_read_page_reg(phydev, 4, PTP_CLOCK_READ_SEC_LO);
 
-	*nano_seconds = lanphy_read_page_reg(phydev, 4, PTP_CLOCK_READ_NS_HI);
-	*nano_seconds = ((*nano_seconds & 0x3fff) << 16) |
-			lanphy_read_page_reg(phydev, 4, PTP_CLOCK_READ_NS_LO);
+	*nsec = lanphy_read_page_reg(phydev, 4, PTP_CLOCK_READ_NS_HI);
+	*nsec <<= 16;
+	*nsec |= lanphy_read_page_reg(phydev, 4, PTP_CLOCK_READ_NS_LO);
 }
 
 static int lan8814_ptpci_gettime64(struct ptp_clock_info *ptpci,
@@ -2630,7 +2628,7 @@  static int lan8814_ptpci_gettime64(struct ptp_clock_info *ptpci,
 							  ptp_clock_info);
 	struct phy_device *phydev = shared->phydev;
 	u32 nano_seconds;
-	u32 seconds;
+	time64_t seconds;
 
 	mutex_lock(&shared->shared_lock);
 	lan8814_ptp_clock_get(phydev, &seconds, &nano_seconds);
@@ -2660,38 +2658,37 @@  static void lan8814_ptp_clock_step(struct phy_device *phydev,
 {
 	u32 nano_seconds_step;
 	u64 abs_time_step_ns;
-	u32 unsigned_seconds;
+	time64_t set_seconds;
 	u32 nano_seconds;
 	u32 remainder;
 	s32 seconds;
 
 	if (time_step_ns >  15000000000LL) {
 		/* convert to clock set */
-		lan8814_ptp_clock_get(phydev, &unsigned_seconds, &nano_seconds);
-		unsigned_seconds += div_u64_rem(time_step_ns, 1000000000LL,
-						&remainder);
+		lan8814_ptp_clock_get(phydev, &set_seconds, &nano_seconds);
+		set_seconds += div_u64_rem(time_step_ns, 1000000000LL,
+					   &remainder);
 		nano_seconds += remainder;
 		if (nano_seconds >= 1000000000) {
-			unsigned_seconds++;
+			set_seconds++;
 			nano_seconds -= 1000000000;
 		}
-		lan8814_ptp_clock_set(phydev, unsigned_seconds, nano_seconds);
+		lan8814_ptp_clock_set(phydev, set_seconds, nano_seconds);
 		return;
 	} else if (time_step_ns < -15000000000LL) {
 		/* convert to clock set */
 		time_step_ns = -time_step_ns;
 
-		lan8814_ptp_clock_get(phydev, &unsigned_seconds, &nano_seconds);
-		unsigned_seconds -= div_u64_rem(time_step_ns, 1000000000LL,
-						&remainder);
+		lan8814_ptp_clock_get(phydev, &set_seconds, &nano_seconds);
+		set_seconds -= div_u64_rem(time_step_ns, 1000000000LL,
+					   &remainder);
 		nano_seconds_step = remainder;
 		if (nano_seconds < nano_seconds_step) {
-			unsigned_seconds--;
+			set_seconds--;
 			nano_seconds += 1000000000;
 		}
 		nano_seconds -= nano_seconds_step;
-		lan8814_ptp_clock_set(phydev, unsigned_seconds,
-				      nano_seconds);
+		lan8814_ptp_clock_set(phydev, set_seconds, nano_seconds);
 		return;
 	}