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

[Kurt Kanzenbach <kurt@linutronix.de>]

[-- Attachment #1: Type: text/plain, Size: 1667 bytes --]


+ 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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 861 bytes --]