diff mbox series

[v3,net-next,2/4] net: phy: nxp-c45-tja11xx: express timestamp wraparound interval in terms of TS_SEC_MASK

Message ID 20210614134441.497008-3-olteanv@gmail.com (mailing list archive)
State Accepted
Commit 661fef5698bc44c9cc4844140ce055e69d57e1b7
Delegated to: Netdev Maintainers
Headers show
Series Fixes and improvements to TJA1103 PHY driver | expand

Checks

Context Check Description
netdev/cover_letter success Link
netdev/fixes_present success Link
netdev/patch_count success Link
netdev/tree_selection success Clearly marked for net-next
netdev/subject_prefix success Link
netdev/cc_maintainers success CCed 7 of 7 maintainers
netdev/source_inline success Was 0 now: 0
netdev/verify_signedoff success Link
netdev/module_param success Was 0 now: 0
netdev/build_32bit success Errors and warnings before: 0 this patch: 0
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/verify_fixes success Link
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 8 lines checked
netdev/build_allmodconfig_warn success Errors and warnings before: 0 this patch: 0
netdev/header_inline success Link

Commit Message

Vladimir Oltean June 14, 2021, 1:44 p.m. UTC 
From: Vladimir Oltean <vladimir.oltean@nxp.com>

nxp_c45_reconstruct_ts() takes a partial hardware timestamp in @hwts,
with 2 bits of the 'seconds' portion, and a full PTP time in @ts.

It patches in the lower bits of @hwts into @ts, and to ensure that the
reconstructed timestamp is correct, it checks whether the lower 2 bits
of @hwts are not in fact higher than the lower 2 bits of @ts. This is
not logically possible because, according to the calling convention, @ts
was collected later in time than @hwts, but due to two's complement
arithmetic it can actually happen, because the current PTP time might
have wrapped around between when @hwts was collected and when @ts was,
yielding the lower 2 bits of @ts smaller than those of @hwts.

To correct for that situation which is expected to happen under normal
conditions, the driver subtracts exactly one wraparound interval from
the reconstructed timestamp, since the upper bits of that need to
correspond to what the upper bits of @hwts were, not to what the upper
bits of @ts were.

Readers might be confused because the driver denotes the amount of bits
that the partial hardware timestamp has to offer as TS_SEC_MASK
(timestamp mask for seconds). But it subtracts a seemingly unrelated
BIT(2), which is in fact more subtle: if the hardware timestamp provides
2 bits of partial 'seconds' timestamp, then the wraparound interval is
2^2 == BIT(2).

But nonetheless, it is better to express the wraparound interval in
terms of a definition we already have, so replace BIT(2) with
1 + GENMASK(1, 0) which produces the same result but is clearer.

Suggested-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
Cc: Richard Cochran <richardcochran@gmail.com>
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
v2->v3: Patch is new

 drivers/net/phy/nxp-c45-tja11xx.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Russell King (Oracle) June 14, 2021, 2:14 p.m. UTC | #1 
On Mon, Jun 14, 2021 at 04:44:39PM +0300, Vladimir Oltean wrote:
> From: Vladimir Oltean <vladimir.oltean@nxp.com>
> 
> nxp_c45_reconstruct_ts() takes a partial hardware timestamp in @hwts,
> with 2 bits of the 'seconds' portion, and a full PTP time in @ts.
> 
> It patches in the lower bits of @hwts into @ts, and to ensure that the
> reconstructed timestamp is correct, it checks whether the lower 2 bits
> of @hwts are not in fact higher than the lower 2 bits of @ts. This is
> not logically possible because, according to the calling convention, @ts
> was collected later in time than @hwts, but due to two's complement
> arithmetic it can actually happen, because the current PTP time might
> have wrapped around between when @hwts was collected and when @ts was,
> yielding the lower 2 bits of @ts smaller than those of @hwts.
> 
> To correct for that situation which is expected to happen under normal
> conditions, the driver subtracts exactly one wraparound interval from
> the reconstructed timestamp, since the upper bits of that need to
> correspond to what the upper bits of @hwts were, not to what the upper
> bits of @ts were.
> 
> Readers might be confused because the driver denotes the amount of bits
> that the partial hardware timestamp has to offer as TS_SEC_MASK
> (timestamp mask for seconds). But it subtracts a seemingly unrelated
> BIT(2), which is in fact more subtle: if the hardware timestamp provides
> 2 bits of partial 'seconds' timestamp, then the wraparound interval is
> 2^2 == BIT(2).
> 
> But nonetheless, it is better to express the wraparound interval in
> terms of a definition we already have, so replace BIT(2) with
> 1 + GENMASK(1, 0) which produces the same result but is clearer.
> 
> Suggested-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
> Cc: Richard Cochran <richardcochran@gmail.com>
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>

Thanks!

Reviewed-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
diff mbox series

Patch

diff --git a/drivers/net/phy/nxp-c45-tja11xx.c b/drivers/net/phy/nxp-c45-tja11xx.c
index 902fe1aa7782..afdcd6772b1d 100644
--- a/drivers/net/phy/nxp-c45-tja11xx.c
+++ b/drivers/net/phy/nxp-c45-tja11xx.c
@@ -325,7 +325,7 @@  static void nxp_c45_reconstruct_ts(struct timespec64 *ts,
 {
 	ts->tv_nsec = hwts->nsec;
 	if ((ts->tv_sec & TS_SEC_MASK) < (hwts->sec & TS_SEC_MASK))
-		ts->tv_sec -= BIT(2);
+		ts->tv_sec -= TS_SEC_MASK + 1;
 	ts->tv_sec &= ~TS_SEC_MASK;
 	ts->tv_sec |= hwts->sec & TS_SEC_MASK;
 }