RE: [Intel-wired-lan] [PATCH net v1] idpf: read lower clock bits inside the time sandwich

[Loktionov, Aleksandr]

> -----Original Message-----
> From: Keller, Jacob E <jacob.e.keller@xxxxxxxxx>
> Sent: Friday, December 12, 2025 8:43 PM
> To: Kitszel, Przemyslaw <przemyslaw.kitszel@xxxxxxxxx>; Loktionov,
> Aleksandr <aleksandr.loktionov@xxxxxxxxx>; Mina Almasry
> <almasrymina@xxxxxxxxxx>
> Cc: Nguyen, Anthony L <anthony.l.nguyen@xxxxxxxxx>; Andrew Lunn
> <andrew+netdev@xxxxxxx>; David S. Miller <davem@xxxxxxxxxxxxx>;
> netdev@xxxxxxxxxxxxxxx; Eric Dumazet <edumazet@xxxxxxxxxx>; linux-
> kernel@xxxxxxxxxxxxxxx; Jakub Kicinski <kuba@xxxxxxxxxx>; Paolo Abeni
> <pabeni@xxxxxxxxxx>; Richard Cochran <richardcochran@xxxxxxxxx>;
> Rizzo, Luigi <lrizzo@xxxxxxxxxx>; namangulati@xxxxxxxxxx;
> willemb@xxxxxxxxxx; intel-wired-lan@xxxxxxxxxxxxxxxx; Olech, Milena
> <milena.olech@xxxxxxxxx>; Shachar Raindel <shacharr@xxxxxxxxxx>
> 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@xxxxxxxxxx> On
> >>>> Behalf Of Mina Almasry
> >>>> Sent: Thursday, December 11, 2025 11:19 AM
> >>>> To: netdev@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx
> >>>> Cc: Mina Almasry <almasrymina@xxxxxxxxxx>; Nguyen, Anthony L
> >>>> <anthony.l.nguyen@xxxxxxxxx>; Kitszel, Przemyslaw
> >>>> <przemyslaw.kitszel@xxxxxxxxx>; Andrew Lunn
> >>>> <andrew+netdev@xxxxxxx>; David S. Miller <davem@xxxxxxxxxxxxx>;
> >>>> Eric Dumazet <edumazet@xxxxxxxxxx>; Jakub Kicinski
> >>>> <kuba@xxxxxxxxxx>; Paolo Abeni <pabeni@xxxxxxxxxx>; Richard
> Cochran
> >>>> <richardcochran@xxxxxxxxx>; Rizzo, Luigi <lrizzo@xxxxxxxxxx>;
> >>>> namangulati@xxxxxxxxxx; willemb@xxxxxxxxxx;
> >>>> intel-wired-lan@xxxxxxxxxxxxxxxx; Olech, Milena
> >>>> <milena.olech@xxxxxxxxx>; Keller, Jacob E
> >>>> <jacob.e.keller@xxxxxxxxx>; Shachar Raindel <shacharr@xxxxxxxxxx>
> >>>> 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@xxxxxxxxxx
> >>>> Cc: namangulati@xxxxxxxxxx
> >>>> Cc: willemb@xxxxxxxxxx
> >>>> Cc: intel-wired-lan@xxxxxxxxxxxxxxxx
> >>>> Cc: milena.olech@xxxxxxxxx
> >>>> Cc: jacob.e.keller@xxxxxxxxx
> >>>>
> >>>> Fixes: 5cb8805d2366 ("idpf: negotiate PTP capabilities and get
> PTP
> >>>> clock")
> >>>> Suggested-by: Shachar Raindel <shacharr@xxxxxxxxxx>
> >>>> Signed-off-by: Mina Almasry <almasrymina@xxxxxxxxxx>
> >>>> ---
> >>>>   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@xxxxxxxxx>