netdev - RE: [PATCH net-next 01/10] idpf: add initial PTP support

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: <MW4PR11MB58891AED0B67637F23AD9C3C8EAF2@MW4PR11MB5889.namprd11.prod.outlook.com>
Date: Wed, 2 Apr 2025 13:16:26 +0000
From: "Olech, Milena" <milena.olech@...el.com>
To: "Keller, Jacob E" <jacob.e.keller@...el.com>, 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>, Vadim Fedorenko <vadim.fedorenko@...ux.dev>,
	Willem de Bruijn <willemb@...gle.com>, Mina Almasry <almasrymina@...gle.com>,
	"Salin, Samuel" <samuel.salin@...el.com>
Subject: RE: [PATCH net-next 01/10] idpf: add initial PTP support

On 03/25/2025 10:06 PM, Jacob Keller wrote:

>On 3/25/2025 5:44 AM, Jakub Kicinski wrote:
>> On Tue, 18 Mar 2025 09:13:16 -0700 Tony Nguyen wrote:
>>> From: Milena Olech <milena.olech@...el.com>
>>>
>>> PTP feature is supported if the VIRTCHNL2_CAP_PTP is negotiated during the
>>> capabilities recognition. Initial PTP support includes PTP initialization
>>> and registration of the clock.
>>>
>>> Reviewed-by: Alexander Lobakin <aleksander.lobakin@...el.com>
>>> Reviewed-by: Vadim Fedorenko <vadim.fedorenko@...ux.dev>
>>> Reviewed-by: Willem de Bruijn <willemb@...gle.com>
>>> Tested-by: Mina Almasry <almasrymina@...gle.com>
>>> Signed-off-by: Milena Olech <milena.olech@...el.com>
>>> Tested-by: Samuel Salin <Samuel.salin@...el.com>
>>> Signed-off-by: Tony Nguyen <anthony.l.nguyen@...el.com>
>> 
>> Would be great to see a review tag from Jake on these :(
>>
>
>I saw these go by on IWL, and did give some comments. However, I didn't
>have the time to do a proper review so I didn't tag them.
>
>I'm happy to look these over and review them before the submission next
>cycle :)
>

First of all, sorry for delayed response - I was OOO :)

+ Thanks Jake!

>>> +#if IS_ENABLED(CONFIG_PTP_1588_CLOCK)
>>> +int idpf_ptp_init(struct idpf_adapter *adapter);
>>> +void idpf_ptp_release(struct idpf_adapter *adapter);
>>> +#else /* CONFIG_PTP_1588_CLOCK */
>>> +static inline int idpf_ptp_init(struct idpf_adapter *adapter)
>>> +{
>>> +	return 0;
>>> +}
>>> +
>>> +static inline void idpf_ptp_release(struct idpf_adapter *adapter) { }
>>> +#endif /* CONFIG_PTP_1588_CLOCK */
>>> +#endif /* _IDPF_PTP_H */
>> 
>> You add an unusual number of ifdefs for CONFIG_PTP_1588_CLOCK.
>> Is this really necessary? What breaks if 1588 is not enabled?
>> 
>
>This style of converting the init and release to no-op is fairly common
>in Intel drivers. I don't know about other places, but I think the
>init/release is good since it just makes us disable the PTP
>functionality when not supported.
>
>We should make sure to try and limit these checks to the idpf_ptp.h
>header file in one place, and make everything transparently disable if
>the kernel lacks the PTP support :)
>

Ofc I've checked IDPF behavior when PTP support is disabled.

About the number of CONFIG_PTP_1588_CLOCK in the code - apart of ptp.h,
it's used in idpf_txrx.c and idpf_virtchnl.c, in both places it's about
defining static function. Let me know if you prefer to have it in
idpf_ptp.h.

Regards,
Milena