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: <IA0PR11MB838091A67C0AE3598BFCDF8D86BE2@IA0PR11MB8380.namprd11.prod.outlook.com>
Date: Mon, 5 Aug 2024 16:21:39 +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: 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.

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