netdev - Re: [PATCH net-next 7/9] mlxsw: spectrum_ptp: Add implementation for physical hardware clock operations

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]

Date:   Wed, 5 Jun 2019 19:28:38 +0000
From:   Shalom Toledo <shalomt@...lanox.com>
To:     Richard Cochran <richardcochran@...il.com>
CC:     Ido Schimmel <idosch@...sch.org>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "davem@...emloft.net" <davem@...emloft.net>,
        Jiri Pirko <jiri@...lanox.com>,
        Petr Machata <petrm@...lanox.com>, mlxsw <mlxsw@...lanox.com>,
        Ido Schimmel <idosch@...lanox.com>
Subject: Re: [PATCH net-next 7/9] mlxsw: spectrum_ptp: Add implementation for
 physical hardware clock operations

On 05/06/2019 20:40, Richard Cochran wrote:
> On Wed, Jun 05, 2019 at 11:44:21AM +0000, Shalom Toledo wrote:
>> On 04/06/2019 17:28, Richard Cochran wrote:
>>> Now I see why you did this.  Nice try.
>>
>> I didn't try anything.
>>
>> The reason is that the hardware units is in ppb and not in scaled_ppm(or ppm),
>> so I just converted to ppb in order to set the hardware.
> 
> Oh, I thought you were adapting code for the deprecated .adjfreq method.
>  
>> But I got your point, I will change my calculation to use scaled_ppm (to get a
>> more finer resolution) and not ppb, and convert to ppb just before setting the
>> hardware. Is that make sense?
> 
> So the HW actually accepts ppb adjustment values?  Fine.

So I leave the code as is.

> 
> But I don't understand this:
> 
>>>> +	if (ppb < 0) {
>>>> +		neg_adj = 1;
>>>> +		ppb = -ppb;
>>>> +	}
>>>> +
>>>> +	adj = clock->nominal_c_mult;
>>>> +	adj *= ppb;
>>>> +	diff = div_u64(adj, NSEC_PER_SEC);
>>>> +
>>>> +	spin_lock(&clock->lock);
>>>> +	timecounter_read(&clock->tc);
>>>> +	clock->cycles.mult = neg_adj ? clock->nominal_c_mult - diff :
>>>> +				       clock->nominal_c_mult + diff;
>>>> +	spin_unlock(&clock->lock);
> 
> You have a SW time counter here

Yep.

> 
>>>> +	return mlxsw_sp1_ptp_update_phc_adjfreq(clock, neg_adj ? -ppb : ppb);
> 
> and a hardware method here?

Yep.

> 
> Why not choose one or the other?

You are right, but there is a reason behind it of course.

The hardware has a free running counter that the SW can only read. This is
used when we do gettimex op. The rest ops, settime, adjfine and adjtime, we do
only in SW, since we can't control the HW free running counter.

Now, you are asking yourself, why the driver also update the HW? what does it
mean?

So, there is an HW machine which responsible for adding UTC timestamp on
R-SPAN mirror packets and there is no connection to the HW free running
counter. The SW can and should control this HW machine, from setting the UTC
time when doing settime for example. Or adjusting the frequency when doing
adjfine.

I hope it is more clear now.

> 
> Thanks,
> Richard
>