| 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
| ||
|
Message-ID: <IA3PR11MB89862D9C81A1411B201CDBA3E5AEA@IA3PR11MB8986.namprd11.prod.outlook.com>
Date: Fri, 12 Dec 2025 21:08:10 +0000
From: "Loktionov, Aleksandr" <aleksandr.loktionov@...el.com>
To: "Keller, Jacob E" <jacob.e.keller@...el.com>, "Kitszel, Przemyslaw"
<przemyslaw.kitszel@...el.com>, Mina Almasry <almasrymina@...gle.com>
CC: "Nguyen, Anthony L" <anthony.l.nguyen@...el.com>, Andrew Lunn
<andrew+netdev@...n.ch>, "David S. Miller" <davem@...emloft.net>,
"netdev@...r.kernel.org" <netdev@...r.kernel.org>, Eric Dumazet
<edumazet@...gle.com>, "linux-kernel@...r.kernel.org"
<linux-kernel@...r.kernel.org>, Jakub Kicinski <kuba@...nel.org>, Paolo Abeni
<pabeni@...hat.com>, Richard Cochran <richardcochran@...il.com>, "Rizzo,
Luigi" <lrizzo@...gle.com>, "namangulati@...gle.com"
<namangulati@...gle.com>, "willemb@...gle.com" <willemb@...gle.com>,
"intel-wired-lan@...ts.osuosl.org" <intel-wired-lan@...ts.osuosl.org>,
"Olech, Milena" <milena.olech@...el.com>, Shachar Raindel
<shacharr@...gle.com>
Subject: RE: [Intel-wired-lan] [PATCH net v1] idpf: read lower clock bits
inside the time sandwich
> -----Original Message-----
> From: Keller, Jacob E <jacob.e.keller@...el.com>
> Sent: Friday, December 12, 2025 8:43 PM
> To: Kitszel, Przemyslaw <przemyslaw.kitszel@...el.com>; Loktionov,
> Aleksandr <aleksandr.loktionov@...el.com>; Mina Almasry
> <almasrymina@...gle.com>
> Cc: Nguyen, Anthony L <anthony.l.nguyen@...el.com>; Andrew Lunn
> <andrew+netdev@...n.ch>; David S. Miller <davem@...emloft.net>;
> netdev@...r.kernel.org; Eric Dumazet <edumazet@...gle.com>; linux-
> kernel@...r.kernel.org; Jakub Kicinski <kuba@...nel.org>; Paolo Abeni
> <pabeni@...hat.com>; Richard Cochran <richardcochran@...il.com>;
> Rizzo, Luigi <lrizzo@...gle.com>; namangulati@...gle.com;
> willemb@...gle.com; intel-wired-lan@...ts.osuosl.org; Olech, Milena
> <milena.olech@...el.com>; Shachar Raindel <shacharr@...gle.com>
> Subject: Re: [Intel-wired-lan] [PATCH net v1] idpf: read lower clock
> bits inside the time sandwich
>
>
>
> On 12/11/2025 11:57 PM, Przemek Kitszel wrote:
> > On 12/11/25 23:06, Jacob Keller wrote:
> >>
> >>
> >> On 12/11/2025 2:37 AM, Loktionov, Aleksandr wrote:
> >>>
> >>>
> >>>> -----Original Message-----
> >>>> From: Intel-wired-lan <intel-wired-lan-bounces@...osl.org> On
> >>>> Behalf Of Mina Almasry
> >>>> Sent: Thursday, December 11, 2025 11:19 AM
> >>>> To: netdev@...r.kernel.org; linux-kernel@...r.kernel.org
> >>>> Cc: Mina Almasry <almasrymina@...gle.com>; Nguyen, Anthony L
> >>>> <anthony.l.nguyen@...el.com>; Kitszel, Przemyslaw
> >>>> <przemyslaw.kitszel@...el.com>; Andrew Lunn
> >>>> <andrew+netdev@...n.ch>; David S. Miller <davem@...emloft.net>;
> >>>> Eric Dumazet <edumazet@...gle.com>; Jakub Kicinski
> >>>> <kuba@...nel.org>; Paolo Abeni <pabeni@...hat.com>; Richard
> Cochran
> >>>> <richardcochran@...il.com>; Rizzo, Luigi <lrizzo@...gle.com>;
> >>>> namangulati@...gle.com; willemb@...gle.com;
> >>>> intel-wired-lan@...ts.osuosl.org; Olech, Milena
> >>>> <milena.olech@...el.com>; Keller, Jacob E
> >>>> <jacob.e.keller@...el.com>; Shachar Raindel <shacharr@...gle.com>
> >>>> Subject: [Intel-wired-lan] [PATCH net v1] idpf: read lower clock
> >>>> bits inside the time sandwich
> >>>>
> >>>> PCIe reads need to be done inside the time sandwich because PCIe
> >>>> writes may get buffered in the PCIe fabric and posted to the
> device
> >>>> after the _postts completes. Doing the PCIe read inside the time
> >>>> sandwich guarantees that the write gets flushed before the
> _postts
> >>>> timestamp is taken.
> >>>>
> >>>> Cc: lrizzo@...gle.com
> >>>> Cc: namangulati@...gle.com
> >>>> Cc: willemb@...gle.com
> >>>> Cc: intel-wired-lan@...ts.osuosl.org
> >>>> Cc: milena.olech@...el.com
> >>>> Cc: jacob.e.keller@...el.com
> >>>>
> >>>> Fixes: 5cb8805d2366 ("idpf: negotiate PTP capabilities and get
> PTP
> >>>> clock")
> >>>> Suggested-by: Shachar Raindel <shacharr@...gle.com>
> >>>> Signed-off-by: Mina Almasry <almasrymina@...gle.com>
> >>>> ---
> >>>> drivers/net/ethernet/intel/idpf/idpf_ptp.c | 2 +-
> >>>> 1 file changed, 1 insertion(+), 1 deletion(-)
> >>>>
> >>>> diff --git a/drivers/net/ethernet/intel/idpf/idpf_ptp.c
> >>>> b/drivers/net/ethernet/intel/idpf/idpf_ptp.c
> >>>> index 3e1052d070cf..0a8b50350b86 100644
> >>>> --- a/drivers/net/ethernet/intel/idpf/idpf_ptp.c
> >>>> +++ b/drivers/net/ethernet/intel/idpf/idpf_ptp.c
> >>>> @@ -108,11 +108,11 @@ static u64
> >>>> idpf_ptp_read_src_clk_reg_direct(struct idpf_adapter *adapter,
> >>>> ptp_read_system_prets(sts);
> >>>>
> >>>> idpf_ptp_enable_shtime(adapter);
> >>>> + lo = readl(ptp->dev_clk_regs.dev_clk_ns_l);
> >>> The high 32 bits (hi) are still read outside the time sandwich
> >>> (after ptp_read_system_postts()), which defeats the stated purpose
> of ensuring PCIe write flush before timestamp capture.
> >>> /* I think he "time sandwich" is defined by the region between
> ptp_read_system_prets(sts) and ptp_read_system_postts(sts) */ Isn't
> it?
> >>>
> >>>
> >>
> >> Any read will cause writes to flush, so we don't need to move both
> >> registers.
> >>
> >> The point here is that we write to the shadow register to snapshot
> >> time, and it won't guarantee to be flushed to the device until a
> >> read. By moving a single read in side the time sandwhich, we ensure
> >> that its actually complete before the time snapshot is taken. We
> >> don't need to wait for both registers because of the snapshot
> behavior.
> >
> > very nice explanation Jake, thank you
> >
> > I don't know if it should be considered "basic common knowledge", or
> > warrants an entry in commit message/code comment For sure we don't
> > want anyone not knowing that to touch the code, so barrier to entry
> is
> > also a good thing ;)
> >
> >>
> >> I think the patch is fine-as-is.
> >
> > given the scope of the function, agree
> >
> Reading both registers would take longer, and would delay post
> timestamp, which would lower the accuracy of the clock comparison
> mechanisms that use the pre+post timestamps. We *must* read one of the
> values because we need to ensure the PHC timestamp is snapshot between
> pre+post, but we should do as little work as necessary, so only
> reading
> the low register is the most optimal.
>
> This could be put into the commit message, but at least to me as a
> domain expert the original commit message was sufficient, so I'm not
> sure that I'm a good judge for what is necessary for others to
> understand the logic.
Thank you for the clarification!
Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@...el.com>
Powered by blists - more mailing lists