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 for Android: free password hash cracker in your pocket

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

Message-ID: <MW4PR11MB588909DAED47807C491FE7C38EAF2@MW4PR11MB5889.namprd11.prod.outlook.com>
Date: Wed, 2 Apr 2025 15:23:53 +0000
From: "Olech, Milena" <milena.olech@...el.com>
To: Jakub Kicinski <kuba@...nel.org>, "Nguyen, Anthony L"
	<anthony.l.nguyen@...el.com>
CC: "davem@...emloft.net" <davem@...emloft.net>, "pabeni@...hat.com"
	<pabeni@...hat.com>, "Dumazet, Eric" <edumazet@...gle.com>,
	"andrew+netdev@...n.ch" <andrew+netdev@...n.ch>, "netdev@...r.kernel.org"
	<netdev@...r.kernel.org>, "Kitszel, Przemyslaw"
	<przemyslaw.kitszel@...el.com>, "Kolacinski, Karol"
	<karol.kolacinski@...el.com>, "richardcochran@...il.com"
	<richardcochran@...il.com>, "Lobakin, Aleksander"
	<aleksander.lobakin@...el.com>, Willem de Bruijn <willemb@...gle.com>, "Mina
 Almasry" <almasrymina@...gle.com>, "Salin, Samuel" <samuel.salin@...el.com>
Subject: RE: [PATCH net-next 04/10] idpf: negotiate PTP capabilities and get
 PTP clock

On 03/25/2025 1:50 PM, 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.

True - my bad!

>The whole kdoc looks like complete boilerplate, but I guess 
>it's required of your internal coding style :(

About the kdoc style...I do agree that it looks like boilerplate, but
it's - to some extend - Intel style, and as Przemek mentioned, we tried
to play it safe and keep the comments...

I can try to review kdoc in this series and remove those which do not
bring any significant info. Please share your thoughts.

>
>> + */
>> +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)?

Actually we have HW support to latch both values simultaneously.

Hi and Lo are latched in idpf_ptp_enable_shtime function, so I will move
lo right after ptp_read_system_postts.

>
>> +	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.

Sure, will do.
>

Thanks,
Milena