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

[Salin, Samuel]

> -----Original Message-----
> From: Intel-wired-lan <intel-wired-lan-bounces@xxxxxxxxxx> On Behalf Of
> Loktionov, Aleksandr
> Sent: Friday, December 12, 2025 1:08 PM
> To: Keller, Jacob E <jacob.e.keller@xxxxxxxxx>; Kitszel, Przemyslaw
> <przemyslaw.kitszel@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
> 
> 
> 
> > -----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>
> 
> 
> 
Tested-by: Samuel Salin <Samuel.salin@xxxxxxxxx>