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

[Jacob Keller <jacob.e.keller@intel.com>]

[-- Attachment #1.1: Type: text/plain, Size: 3222 bytes --]



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.

I think the patch is fine-as-is.

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

Thanks,
Jake

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 236 bytes --]