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

[Przemek Kitszel <przemyslaw.kitszel@intel.com>]

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@osuosl.org> On Behalf
>>> Of Mina Almasry
>>> Sent: Thursday, December 11, 2025 11:19 AM
>>> To: netdev@vger.kernel.org; linux-kernel@vger.kernel.org
>>> Cc: Mina Almasry <almasrymina@google.com>; Nguyen, Anthony L
>>> <anthony.l.nguyen@intel.com>; Kitszel, Przemyslaw
>>> <przemyslaw.kitszel@intel.com>; Andrew Lunn <andrew+netdev@lunn.ch>;
>>> David S. Miller <davem@davemloft.net>; Eric Dumazet
>>> <edumazet@google.com>; Jakub Kicinski <kuba@kernel.org>; Paolo Abeni
>>> <pabeni@redhat.com>; Richard Cochran <richardcochran@gmail.com>;
>>> Rizzo, Luigi <lrizzo@google.com>; namangulati@google.com;
>>> willemb@google.com; intel-wired-lan@lists.osuosl.org; Olech, Milena
>>> <milena.olech@intel.com>; Keller, Jacob E <jacob.e.keller@intel.com>;
>>> Shachar Raindel <shacharr@google.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@google.com
>>> Cc: namangulati@google.com
>>> Cc: willemb@google.com
>>> Cc: intel-wired-lan@lists.osuosl.org
>>> Cc: milena.olech@intel.com
>>> Cc: jacob.e.keller@intel.com
>>>
>>> Fixes: 5cb8805d2366 ("idpf: negotiate PTP capabilities and get PTP
>>> clock")
>>> Suggested-by: Shachar Raindel <shacharr@google.com>
>>> Signed-off-by: Mina Almasry <almasrymina@google.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

> 
> Reviewed-by: Jacob Keller <jacob.e.keller@intel.com>
> 
> Thanks,
> Jake