> -----Original Message-----
> From: Intel-wired-lan <[email protected]> On Behalf Of
> Loktionov, Aleksandr
> Sent: Friday, December 12, 2025 1:08 PM
> To: Keller, Jacob E <[email protected]>; Kitszel, Przemyslaw
> <[email protected]>; Mina Almasry <[email protected]>
> Cc: Nguyen, Anthony L <[email protected]>; Andrew Lunn
> <[email protected]>; David S. Miller <[email protected]>;
> [email protected]; Eric Dumazet <[email protected]>; linux-
> [email protected]; Jakub Kicinski <[email protected]>; Paolo Abeni
> <[email protected]>; Richard Cochran <[email protected]>; Rizzo,
> Luigi <[email protected]>; [email protected]; [email protected];
> [email protected]; Olech, Milena <[email protected]>;
> Shachar Raindel <[email protected]>
> Subject: Re: [Intel-wired-lan] [PATCH net v1] idpf: read lower clock bits
> inside
> the time sandwich
>
>
>
> > -----Original Message-----
> > From: Keller, Jacob E <[email protected]>
> > Sent: Friday, December 12, 2025 8:43 PM
> > To: Kitszel, Przemyslaw <[email protected]>; Loktionov,
> > Aleksandr <[email protected]>; Mina Almasry
> > <[email protected]>
> > Cc: Nguyen, Anthony L <[email protected]>; Andrew Lunn
> > <[email protected]>; David S. Miller <[email protected]>;
> > [email protected]; Eric Dumazet <[email protected]>; linux-
> > [email protected]; Jakub Kicinski <[email protected]>; Paolo Abeni
> > <[email protected]>; Richard Cochran <[email protected]>;
> > Rizzo, Luigi <[email protected]>; [email protected];
> > [email protected]; [email protected]; Olech, Milena
> > <[email protected]>; Shachar Raindel <[email protected]>
> > 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 <[email protected]> On
> > >>>> Behalf Of Mina Almasry
> > >>>> Sent: Thursday, December 11, 2025 11:19 AM
> > >>>> To: [email protected]; [email protected]
> > >>>> Cc: Mina Almasry <[email protected]>; Nguyen, Anthony L
> > >>>> <[email protected]>; Kitszel, Przemyslaw
> > >>>> <[email protected]>; Andrew Lunn
> > >>>> <[email protected]>; David S. Miller <[email protected]>;
> > >>>> Eric Dumazet <[email protected]>; Jakub Kicinski
> > >>>> <[email protected]>; Paolo Abeni <[email protected]>; Richard
> > Cochran
> > >>>> <[email protected]>; Rizzo, Luigi <[email protected]>;
> > >>>> [email protected]; [email protected];
> > >>>> [email protected]; Olech, Milena
> > >>>> <[email protected]>; Keller, Jacob E
> > >>>> <[email protected]>; Shachar Raindel <[email protected]>
> > >>>> 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: [email protected]
> > >>>> Cc: [email protected]
> > >>>> Cc: [email protected]
> > >>>> Cc: [email protected]
> > >>>> Cc: [email protected]
> > >>>> Cc: [email protected]
> > >>>>
> > >>>> Fixes: 5cb8805d2366 ("idpf: negotiate PTP capabilities and get
> > PTP
> > >>>> clock")
> > >>>> Suggested-by: Shachar Raindel <[email protected]>
> > >>>> Signed-off-by: Mina Almasry <[email protected]>
> > >>>> ---
> > >>>> 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 <[email protected]>
>
>
>
Tested-by: Samuel Salin <[email protected]>
