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: <IA0PR11MB8380B6EC30AC39EAAC1F480986B82@IA0PR11MB8380.namprd11.prod.outlook.com>
Date: Wed, 7 Aug 2024 14:26:29 +0000
From: "Kolacinski, Karol" <karol.kolacinski@...el.com>
To: "Lobakin, Aleksander" <aleksander.lobakin@...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: Aleksander Lobakin <aleksander.lobakin@...el.com>
Date: Wed, 07 Aug 2024 15:54 +0200
>>>> +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

Technically, neither ART nor TSC are directly related to the PTP cross
timestamp. It's just the implementation on Intel NICs, where those
NICs use x86 ART to crosstimestamp.

For cross timestamp on ARM, it's also HW specific and depends on which
timer the HW uses for timestamping. I'm not really sure what's the HW
protocol in this case and if e.g. E830 can latch other timers than
x86 ART in its ART_TIME registers.

get_device_system_crosststamp() supports multiple clock sources defined
in enum clocksource_ids. Maybe instead of checking ART flag, the driver
could get clocksources and if CSID_X86_ART is available, it would assign
the pointer to crosststamp function, but I'm not convinced.

Thanks,
Karol