linux-kernel - Re: [PATCH net-next 6/7] net: stmmac: Fix comment about default addend calculation

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <krvdz4filnpzhdy7tjkaisa2uzeh2sjzc2krno2rns24ldka37@abay33wdcck4>
Date:   Sun, 27 Aug 2023 03:02:07 +0300
From:   Serge Semin <fancer.lancer@...il.com>
To:     Andrew Halaney <ahalaney@...hat.com>
Cc:     Alexandre Torgue <alexandre.torgue@...s.st.com>,
        Jose Abreu <joabreu@...opsys.com>,
        "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>,
        Maxime Coquelin <mcoquelin.stm32@...il.com>,
        Richard Cochran <richardcochran@...il.com>,
        netdev@...r.kernel.org, linux-stm32@...md-mailman.stormreply.com,
        linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH net-next 6/7] net: stmmac: Fix comment about default
 addend calculation

Hi Andrew

On Thu, Aug 24, 2023 at 01:32:57PM -0500, Andrew Halaney wrote:
> The comment neglects that freq_div_ratio is the ratio between
> the subsecond increment frequency and the clk_ptp_rate frequency.
> 
> Signed-off-by: Andrew Halaney <ahalaney@...hat.com>
> ---
>  drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index dfead0df6163..64185753865f 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -853,10 +853,12 @@ int stmmac_init_tstamp_counter(struct stmmac_priv *priv, u32 systime_flags)
>  	/* Store sub second increment for later use */
>  	priv->sub_second_inc = sub_second_inc;
>  

> -	/* calculate default addend value:
> -	 * formula is :
> -	 * addend = (2^32)/freq_div_ratio;
> -	 * where, freq_div_ratio = 1e9ns/sub_second_inc
> +	/* Calculate default addend so the accumulator overflows (2^32) in
> +	 * sub_second_inc (ns). The addend is added to the accumulator
> +	 * every clk_ptp cycle.
> +	 *
> +	 * addend = (2^32) / freq_div_ratio
> +	 * where, freq_div_ratio = (1e9ns / sub_second_inc) / clk_ptp_rate
>  	 */
>  	temp = div_u64(NSEC_PER_SEC, sub_second_inc);
>  	temp = temp << 32;

I am not well familiar with the way PTP works but at my naked eyes the
calculation implemented here looks a bit different than what is
described in the comment.

Basically config_sub_second_increment(clk_ptp_rate, sub_second_inc)
returns clk_ptp_rate period in nanoseconds or twice that period, or have it
scaled up on 0.465. So we have one of the next formulae:
X1 = NSEC_PER_SEC / clk_ptp_rate
X2 = 2 * NSEC_PER_SEC / clk_ptp_rate
X3 = X1 / 0.465
X4 = X2 / 0.465

Then stmmac_init_tstamp_counter() handles the retrieved period in the
next manner:
temp = div_u64(NSEC_PER_SEC, sub_second_inc);     // Convert back to frequency
temp = temp << 32;                                // multiply by 2^32
addend = div_u64(temp, priv->plat->clk_ptp_rate); // Divide by clk_ptp_rate

The code above is equivalent:

addend = ((NSEC_PER_SEC / X) * 2^32 ) / clk_ptp_rate = 
         (2^32 * NSEC_PER_SEC / X) / clk_ptp_rate = 
         2^32 / (clk_ptp_rate / (NSEC_PER_SEC / X))

AFAICS this doesn't match to what is in the comment (X = sub_second_inc).
freq_div_ratio gets to be inverted. Does it?

Substituting X to the formulae above we'll have just four possible results:
addend1 = 2^32
addend2 = 2^32 / 2
addend3 = 0.465 * 2^32
addend4 = 0.465 * 2^32 / 2

So basically clk_ptp_rate is irrelevant (neglecting all the
integer divisions rounding). Is that what implied by the implemented
algo?

Am I missing something? (it's quite possible since it's long past
midnight already.)

-Serge(y)

> 
> -- 
> 2.41.0
> 
>