netdev - Re: [Intel-wired-lan] [PATCH v3 iwl-next 4/4] ice: combine cross timestamp functions for E82x and E830

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: <e54793ef-f81c-447a-8cdd-bed97df65f2e@intel.com>
Date: Wed, 7 Aug 2024 15:54:37 +0200
From: Alexander Lobakin <aleksander.lobakin@...el.com>
To: "Kolacinski, Karol" <karol.kolacinski@...el.com>
CC: "intel-wired-lan@...ts.osuosl.org" <intel-wired-lan@...ts.osuosl.org>,
	"Keller, Jacob E" <jacob.e.keller@...el.com>, "netdev@...r.kernel.org"
	<netdev@...r.kernel.org>, "Nguyen, Anthony L" <anthony.l.nguyen@...el.com>,
	"Kitszel, Przemyslaw" <przemyslaw.kitszel@...el.com>
Subject: Re: [Intel-wired-lan] [PATCH v3 iwl-next 4/4] ice: combine cross
 timestamp functions for E82x and E830

From: Kolacinski, Karol <karol.kolacinski@...el.com>
Date: Mon, 5 Aug 2024 18:21:39 +0200

> From: Aleksander Lobakin <aleksander.lobakin@...el.com>
> Date: Fri, 26 Jul 2024 15:37 +0200
>>> +/**
>>> + * ice_ptp_set_funcs_e830 - Set specialized functions for E830 support
>>> + * @pf: Board private structure
>>> + *
>>> + * Assign functions to the PTP capabiltiies structure for E830 devices.
>>> + * Functions which operate across all device families should be set directly
>>> + * in ice_ptp_set_caps. Only add functions here which are distinct for E830
>>> + * devices.
>>> + */
>>> +static void ice_ptp_set_funcs_e830(struct ice_pf *pf)
>>> +{
>>> +#ifdef CONFIG_ICE_HWTS
>>> +     if (pcie_ptm_enabled(pf->pdev) &&
>>> +         boot_cpu_has(X86_FEATURE_ART) &&
>>> +         boot_cpu_has(X86_FEATURE_TSC_KNOWN_FREQ))
>>> +             pf->ptp.info.getcrosststamp = ice_ptp_getcrosststamp;
>>> +#endif /* CONFIG_ICE_HWTS */
>>
>> I've seen this pattern in several drivers already. I really feel like it
>> needs a generic wrapper.
>> I mean, there should be something like
>>
>> arch/x86/include/asm/ptm.h (not sure about the name)
>>
>> where you put let's say
>>
>> static inline bool arch_has_ptm(struct pci_device *pdev)
>>
>> Same for
>>
>> include/asm-generic/ptm.h
>>
>> there it will be `return false`.
>>
>> In include/asm-generic/Kbuild, you add
>>
>> mandatory-y += ptm.h.
>>
>> Then, include/linux/ptm.h should include <asm/ptm.h> and in your driver
>> sources, you include <linux/ptm.h> and check
>>
>>         if (arch_has_ptm(pdev))
>>                 pf->ptp.info.getcrosststamp = ice_ptp_getcrosststamp;
>>
>> It's just a draft, adjust accordingly.
>>
>> Checking for x86 features in the driver doesn't look good. Especially
>> when you add ARM64 or whatever support in future.
> 
> For PTM, we check only pcie_ptm_enabled(). PTM is a PCIE feature
> supported regardless of arch.
> 
> The two other checks are for the x86 Always Running Timer (ART) and x86
> TimeStamp Counter (TSC) features. Those are not tied to PTM, but are
> necessary for crosstimestamping on devices supported by ice driver.

Ah okay, it's not tied.
So, instead of asm/ptm.h, it should be named somehow else :D

But this X86_FEATURE_ART + X86_FEATURE_TSC_KNOWN_FREQ check really
should be abstracted to something like arch_has_crosststamp() or
arch_has_tstamp(), dunno. Maybe to the already existing asm/timex.h?
Then, implementing this for ARM64 would be easier, as instead of adding
more ifdefs and checks you'd just implement arch_has_tstamp() in its
timex.h (I've seen Milena'd been playing with PTP on ARM).
At least that's how I see it. But if it's fine for the maintainers to
have arch-specific ifdefs and the same code pattern in several drivers,
I'm fine, too :D

> 
> I guess I can remove checks from E82X since all of those are SoC, so
> HW always supports this.
> 
> For E830, I see no other way, than to check the ART feature. This is
> what the HW latches in its registers.
> I think we could drop TSC_KNOWN_FREQ check since there's new logic
> around get_device_system_crosststamp() and cycles conversion.
> 
> Thanks,
> Karol

Thanks,
Olek