diff mbox series

net: stmmac: Fix signed/unsigned wreckage

Message ID 87mtm578cs.ffs@tglx (mailing list archive)
State Accepted
Commit 3751c3d34cd5a750c86d1c8eaf217d8faf7f9325
Delegated to: Netdev Maintainers
Headers show
Series net: stmmac: Fix signed/unsigned wreckage | expand

Checks

Context Check Description
netdev/fixes_present success Fixes tag not required for -next series
netdev/subject_prefix warning Target tree name not specified in the subject
netdev/cover_letter success Single patches do not need cover letters
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 warning 4 maintainers not CCed: kuba@kernel.org linux-stm32@st-md-mailman.stormreply.com mcoquelin.stm32@gmail.com linux-arm-kernel@lists.infradead.org
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/verify_fixes success Fixes tag looks correct
netdev/build_allmodconfig_warn success Errors and warnings before: 0 this patch: 0
netdev/checkpatch warning WARNING: 'accidentaly' may be misspelled - perhaps 'accidentally'?
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline fail Was 0 now: 1
netdev/tree_selection success Guessing tree name failed - patch did not apply

Commit Message

Thomas Gleixner Nov. 15, 2021, 3:21 p.m. UTC
The recent addition of timestamp correction to compensate the CDC error
introduced a subtle signed/unsigned bug in stmmac_get_tx_hwtstamp() while
it managed for some obscure reason to avoid that in stmmac_get_rx_hwtstamp().

The issue is:

    s64 adjust = 0;
    u64 ns;

    adjust += -(2 * (NSEC_PER_SEC / priv->plat->clk_ptp_rate));
    ns += adjust;

works by chance on 64bit, but falls apart on 32bit because the compiler
knows that adjust fits into 32bit and then treats the addition as a u64 +
u32 resulting in an off by ~2 seconds failure.

The RX variant uses an u64 for adjust and does the adjustment via

    ns -= adjust;

because consistency is obviously overrated.

Get rid of the pointless zero initialized adjust variable and do:

	ns -= (2 * NSEC_PER_SEC) / priv->plat->clk_ptp_rate;

which is obviously correct and spares the adjust obfuscation. Aside of that
it yields a more accurate result because the multiplication takes place
before the integer divide truncation and not afterwards.

Stick the calculation into an inline so it can't be accidentaly
disimproved. Return an u32 from that inline as the result is guaranteed
to fit which lets the compiler optimize the substraction.

Fixes: 3600be5f58c1 ("net: stmmac: add timestamp correction to rid CDC sync error")
Reported-by: Benedikt Spranger <b.spranger@linutronix.de>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Tested-by: Benedikt Spranger <b.spranger@linutronix.de>
Cc: stable@vger.kernel.org
---
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c |   23 +++++++++-------------
 1 file changed, 10 insertions(+), 13 deletions(-)

Comments

Kurt Kanzenbach Nov. 16, 2021, 7:11 a.m. UTC | #1
+ BL

On Mon Nov 15 2021, Thomas Gleixner wrote:
> The recent addition of timestamp correction to compensate the CDC error
> introduced a subtle signed/unsigned bug in stmmac_get_tx_hwtstamp() while
> it managed for some obscure reason to avoid that in stmmac_get_rx_hwtstamp().
>
> The issue is:
>
>     s64 adjust = 0;
>     u64 ns;
>
>     adjust += -(2 * (NSEC_PER_SEC / priv->plat->clk_ptp_rate));
>     ns += adjust;
>
> works by chance on 64bit, but falls apart on 32bit because the compiler
> knows that adjust fits into 32bit and then treats the addition as a u64 +
> u32 resulting in an off by ~2 seconds failure.
>
> The RX variant uses an u64 for adjust and does the adjustment via
>
>     ns -= adjust;
>
> because consistency is obviously overrated.
>
> Get rid of the pointless zero initialized adjust variable and do:
>
> 	ns -= (2 * NSEC_PER_SEC) / priv->plat->clk_ptp_rate;
>
> which is obviously correct and spares the adjust obfuscation. Aside of that
> it yields a more accurate result because the multiplication takes place
> before the integer divide truncation and not afterwards.
>
> Stick the calculation into an inline so it can't be accidentaly
> disimproved. Return an u32 from that inline as the result is guaranteed
> to fit which lets the compiler optimize the substraction.
>
> Fixes: 3600be5f58c1 ("net: stmmac: add timestamp correction to rid CDC sync error")
> Reported-by: Benedikt Spranger <b.spranger@linutronix.de>
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> Tested-by: Benedikt Spranger <b.spranger@linutronix.de>
> Cc: stable@vger.kernel.org

Thanks!

Tested-by: Kurt Kanzenbach <kurt@linutronix.de> # Intel EHL
David Laight Nov. 16, 2021, 1:06 p.m. UTC | #2
From: Thomas Gleixner
> Sent: 15 November 2021 15:21
> 
> The recent addition of timestamp correction to compensate the CDC error
> introduced a subtle signed/unsigned bug in stmmac_get_tx_hwtstamp() while
> it managed for some obscure reason to avoid that in stmmac_get_rx_hwtstamp().
> 
> The issue is:
> 
>     s64 adjust = 0;
>     u64 ns;
> 
>     adjust += -(2 * (NSEC_PER_SEC / priv->plat->clk_ptp_rate));
>     ns += adjust;
> 
> works by chance on 64bit, but falls apart on 32bit because the compiler
> knows that adjust fits into 32bit and then treats the addition as a u64 +
> u32 resulting in an off by ~2 seconds failure.

The problem is earlier.
NSEC_PER_SEC and clk_ptp_rate are almost certainly 32bit and unsigned.
So you have:
	adjust = (s64)((u64)adjust + (u32)-(2 * (NSEC_PER_SEC/...)));

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
Wong Vee Khee Nov. 17, 2021, 3:57 a.m. UTC | #3
On Mon, Nov 15, 2021 at 04:21:23PM +0100, Thomas Gleixner wrote:
> The recent addition of timestamp correction to compensate the CDC error
> introduced a subtle signed/unsigned bug in stmmac_get_tx_hwtstamp() while
> it managed for some obscure reason to avoid that in stmmac_get_rx_hwtstamp().
> 
> The issue is:
> 
>     s64 adjust = 0;
>     u64 ns;
> 
>     adjust += -(2 * (NSEC_PER_SEC / priv->plat->clk_ptp_rate));
>     ns += adjust;
> 
> works by chance on 64bit, but falls apart on 32bit because the compiler
> knows that adjust fits into 32bit and then treats the addition as a u64 +
> u32 resulting in an off by ~2 seconds failure.
> 
> The RX variant uses an u64 for adjust and does the adjustment via
> 
>     ns -= adjust;
> 
> because consistency is obviously overrated.
> 
> Get rid of the pointless zero initialized adjust variable and do:
> 
> 	ns -= (2 * NSEC_PER_SEC) / priv->plat->clk_ptp_rate;
> 
> which is obviously correct and spares the adjust obfuscation. Aside of that
> it yields a more accurate result because the multiplication takes place
> before the integer divide truncation and not afterwards.
> 
> Stick the calculation into an inline so it can't be accidentaly
> disimproved. Return an u32 from that inline as the result is guaranteed
> to fit which lets the compiler optimize the substraction.
> 
> Fixes: 3600be5f58c1 ("net: stmmac: add timestamp correction to rid CDC sync error")
> Reported-by: Benedikt Spranger <b.spranger@linutronix.de>
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> Tested-by: Benedikt Spranger <b.spranger@linutronix.de>
> Cc: stable@vger.kernel.org
> Tested-by: Kurt Kanzenbach <kurt@linutronix.de> # Intel EHL

Thanks for the fix!

Tested-by: Wong Vee Khee <vee.khee.wong@linux.intel.com> # Intel ADL
patchwork-bot+netdevbpf@kernel.org Nov. 17, 2021, 4 a.m. UTC | #4
Hello:

This patch was applied to netdev/net.git (master)
by Jakub Kicinski <kuba@kernel.org>:

On Mon, 15 Nov 2021 16:21:23 +0100 you wrote:
> The recent addition of timestamp correction to compensate the CDC error
> introduced a subtle signed/unsigned bug in stmmac_get_tx_hwtstamp() while
> it managed for some obscure reason to avoid that in stmmac_get_rx_hwtstamp().
> 
> The issue is:
> 
>     s64 adjust = 0;
>     u64 ns;
> 
> [...]

Here is the summary with links:
  - net: stmmac: Fix signed/unsigned wreckage
    https://git.kernel.org/netdev/net/c/3751c3d34cd5

You are awesome, thank you!
diff mbox series

Patch

--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -511,6 +511,14 @@  bool stmmac_eee_init(struct stmmac_priv
 	return true;
 }
 
+static inline u32 stmmac_cdc_adjust(struct stmmac_priv *priv)
+{
+	/* Correct the clk domain crossing(CDC) error */
+	if (priv->plat->has_gmac4 && priv->plat->clk_ptp_rate)
+		return (2 * NSEC_PER_SEC) / priv->plat->clk_ptp_rate;
+	return 0;
+}
+
 /* stmmac_get_tx_hwtstamp - get HW TX timestamps
  * @priv: driver private structure
  * @p : descriptor pointer
@@ -524,7 +532,6 @@  static void stmmac_get_tx_hwtstamp(struc
 {
 	struct skb_shared_hwtstamps shhwtstamp;
 	bool found = false;
-	s64 adjust = 0;
 	u64 ns = 0;
 
 	if (!priv->hwts_tx_en)
@@ -543,12 +550,7 @@  static void stmmac_get_tx_hwtstamp(struc
 	}
 
 	if (found) {
-		/* Correct the clk domain crossing(CDC) error */
-		if (priv->plat->has_gmac4 && priv->plat->clk_ptp_rate) {
-			adjust += -(2 * (NSEC_PER_SEC /
-					 priv->plat->clk_ptp_rate));
-			ns += adjust;
-		}
+		ns -= stmmac_cdc_adjust(priv);
 
 		memset(&shhwtstamp, 0, sizeof(struct skb_shared_hwtstamps));
 		shhwtstamp.hwtstamp = ns_to_ktime(ns);
@@ -573,7 +575,6 @@  static void stmmac_get_rx_hwtstamp(struc
 {
 	struct skb_shared_hwtstamps *shhwtstamp = NULL;
 	struct dma_desc *desc = p;
-	u64 adjust = 0;
 	u64 ns = 0;
 
 	if (!priv->hwts_rx_en)
@@ -586,11 +587,7 @@  static void stmmac_get_rx_hwtstamp(struc
 	if (stmmac_get_rx_timestamp_status(priv, p, np, priv->adv_ts)) {
 		stmmac_get_timestamp(priv, desc, priv->adv_ts, &ns);
 
-		/* Correct the clk domain crossing(CDC) error */
-		if (priv->plat->has_gmac4 && priv->plat->clk_ptp_rate) {
-			adjust += 2 * (NSEC_PER_SEC / priv->plat->clk_ptp_rate);
-			ns -= adjust;
-		}
+		ns -= stmmac_cdc_adjust(priv);
 
 		netdev_dbg(priv->dev, "get valid RX hw timestamp %llu\n", ns);
 		shhwtstamp = skb_hwtstamps(skb);