> -----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]>
