netdev - Re: [PATCH 2/2] net: stmmac: Fix sub-second increment

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] [day] [month] [year] [list]

Message-ID: <697c2998-9faa-4e25-ebc4-4a983adb2fcf@orolia.com>
Date:   Tue, 14 Apr 2020 15:33:15 +0200
From:   Julien Beraud <julien.beraud@...lia.com>
To:     Jose Abreu <Jose.Abreu@...opsys.com>,
        Giuseppe Cavallaro <peppe.cavallaro@...com>,
        Alexandre Torgue <alexandre.torgue@...com>,
        "David S . Miller" <davem@...emloft.net>
Cc:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: Re: [PATCH 2/2] net: stmmac: Fix sub-second increment



On 14/04/2020 13:14, Jose Abreu wrote:
> From: Julien Beraud <julien.beraud@...lia.com>
> Date: Apr/14/2020, 10:46:49 (UTC+00:00)
> 
>> The numbers I have in the documentation say that the minimum clock
>> frequency for PTP is determined by "3 * PTP clock period + 4 * GMII/MII
>> clock period <= Minimum gap between two SFDs". The example values say
>> 5MHz for 1000-Mbps Full Duplex. Is this documentation incorrect ?
> 
> I meant for fine update method (which is the one currently used), the
> clock frequency must be higher than the desired accuracy (which is
> 50MHz).
For the fine update method, this patch makes it possible to adjust the 
accuracy (sub seconds increment) of the ptp clock to the ptp_ref_clk's 
frequency, making it possible to work with frequencies down to the 
minimal value to the maximum value of the ptp_ref_clk freq. It also 
allow to set a value that is inferior to 20ns, thus improving the 
accuracy for frequencies higher than 100MHz.

> 
>> Apart from that, the existing logic doesn't work. The calculation is off
>> by a factor 2, making the ptp clock increment twice slower as it should,
>> at least on socfpga platform but I expect that it is the same on other
>> platforms. Please check commit 19d857c, which has kind of been reverted
>> since for more explanation on the sub-seconds + addend calculation.
>> Also, it artificially sets the increment to a value while we should
>> allow it to be as small as posible for higher frequencies, in order to
>> gain accuracy in timestamping.
> 
> I'm sorry but I don't see any "off by factor of 2". I also don't
> understand this:
>   "the accumulator can only overflow once every 2 additions"
> 
> Can you please provide more details ?
Sorry, my explanation was incorrect. The current calculation is not off 
by a factor 2, it is compensated later by a division. It is just that in 
the case of a 50MHz clock, it will lead to a value of 0 in the addend 
register, so it will not work either like it is described in commit 19d857c.

About "the accumulator can only overflow once every 2 additions", please 
see commit 19d857c.

> 
> BTW, I applied your patch and I saw no difference at all in my setup
> except for path delay increasing a little bit.
Ok, my bad, the current calculation does work but it limits the the 
accuracy in case of a clock frequency higher than 100MHz, and doesn't 
work for frequencies inferior or equal to 50MHz. What is your 
ptp_ref_clk frequency? If it is 100MHz, then our 2 calculations give the 
same result.

To summarize, I think I can rephrase the commit message to make it 
easier to understand, but this patch gives the ability to solve 2 issues 
with the current code:
-> The impossibility to use a clock freq inferior or equal to 50MHz
-> The limitation of the sub-second-increment that limits the accuracy 
of the timestamping for frequencies higher than 100MHz.