Re: [PATCH] net: stmmac: Fix signed/unsigned wreckage

[Wong Vee Khee <vee.khee.wong@linux.intel.com>]

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