netdev - Re: [PATCH net-next 04/10] idpf: negotiate PTP capabilities and get PTP clock

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: <6d7512f4-19b7-40b9-aeff-cae39c47423b@intel.com>
Date: Tue, 25 Mar 2025 14:07:54 -0700
From: Jacob Keller <jacob.e.keller@...el.com>
To: Jakub Kicinski <kuba@...nel.org>, Tony Nguyen <anthony.l.nguyen@...el.com>
CC: <davem@...emloft.net>, <pabeni@...hat.com>, <edumazet@...gle.com>,
	<andrew+netdev@...n.ch>, <netdev@...r.kernel.org>, Milena Olech
	<milena.olech@...el.com>, <przemyslaw.kitszel@...el.com>,
	<karol.kolacinski@...el.com>, <richardcochran@...il.com>, Alexander Lobakin
	<aleksander.lobakin@...el.com>, Willem de Bruijn <willemb@...gle.com>, "Mina
 Almasry" <almasrymina@...gle.com>, Samuel Salin <Samuel.salin@...el.com>
Subject: Re: [PATCH net-next 04/10] idpf: negotiate PTP capabilities and get
 PTP clock



On 3/25/2025 5:49 AM, Jakub Kicinski wrote:
> On Tue, 18 Mar 2025 09:13:19 -0700 Tony Nguyen wrote:
>> +/**
>> + * idpf_ptp_read_src_clk_reg_direct - Read directly the main timer value
>> + * @adapter: Driver specific private structure
>> + * @sts: Optional parameter for holding a pair of system timestamps from
>> + *	 the system clock. Will be ignored when NULL is given.
>> + *
>> + * Return: the device clock time on success, -errno otherwise.
> 
> I don't see no -errno in this function.
> The whole kdoc looks like complete boilerplate, but I guess 
> it's required of your internal coding style :(
> 
>> + */
>> +static u64 idpf_ptp_read_src_clk_reg_direct(struct idpf_adapter *adapter,
>> +					    struct ptp_system_timestamp *sts)
>> +{
>> +	struct idpf_ptp *ptp = adapter->ptp;
>> +	u32 hi, lo;
>> +
>> +	/* Read the system timestamp pre PHC read */
>> +	ptp_read_system_prets(sts);
>> +
>> +	idpf_ptp_enable_shtime(adapter);
>> +	lo = readl(ptp->dev_clk_regs.dev_clk_ns_l);
>> +
>> +	/* Read the system timestamp post PHC read */
>> +	ptp_read_system_postts(sts);
>> +
>> +	hi = readl(ptp->dev_clk_regs.dev_clk_ns_h);
> 
> So hi is latched when lo is read? Or the timer may wrap between 
> the reads? Can reads happen in parallel (re-latching hi)?
> 

Yep, without knowledge of the idpf particulars, I would have expected
this to be written like ice where we read multiple times and compare.
Unless something special is done in hardware to latch.. But even on old
hardware which did claim to latch, it usually was broken so we had to do
something like that regardless.

>> +	return ((u64)hi << 32) | lo;
>> +}
> 
>> +#if IS_ENABLED(CONFIG_X86)
>> +	system->cycles = ns_time_sys;
>> +	system->cs_id = CSID_X86_ART;
>> +#endif /* CONFIG_X86 */
>> +
>> +	return 0;
> 
> Please split the cross-stamping into separate patches.
> 

+1. There is space in the series to make that easy and it will make it
much easier to review.