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);
 
 	/* Read the system timestamp post PHC read */
 	ptp_read_system_postts(sts);
 
-	lo = readl(ptp->dev_clk_regs.dev_clk_ns_l);
 	hi = readl(ptp->dev_clk_regs.dev_clk_ns_h);
 
 	spin_unlock(&ptp->read_dev_clk_lock);

base-commit: 885bebac9909994050bbbeed0829c727e42bd1b7
-- 
2.52.0.223.gf5cc29aaa4-goog

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


> 
>  	/* Read the system timestamp post PHC read */
>  	ptp_read_system_postts(sts);
> 
> -	lo = readl(ptp->dev_clk_regs.dev_clk_ns_l);
>  	hi = readl(ptp->dev_clk_regs.dev_clk_ns_h);
> 
>  	spin_unlock(&ptp->read_dev_clk_lock);
> 
> base-commit: 885bebac9909994050bbbeed0829c727e42bd1b7
> --
> 2.52.0.223.gf5cc29aaa4-goog

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

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

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@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
> 
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.

> -----Original Message-----
> From: Keller, Jacob E <jacob.e.keller@intel.com>
> Sent: Friday, December 12, 2025 8:43 PM
> To: Kitszel, Przemyslaw <przemyslaw.kitszel@intel.com>; Loktionov,
> Aleksandr <aleksandr.loktionov@intel.com>; Mina Almasry
> <almasrymina@google.com>
> Cc: Nguyen, Anthony L <anthony.l.nguyen@intel.com>; Andrew Lunn
> <andrew+netdev@lunn.ch>; David S. Miller <davem@davemloft.net>;
> netdev@vger.kernel.org; Eric Dumazet <edumazet@google.com>; linux-
> kernel@vger.kernel.org; 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>; Shachar Raindel <shacharr@google.com>
> 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@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
> >
> 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@intel.com>

> -----Original Message-----
> From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf Of
> Loktionov, Aleksandr
> Sent: Friday, December 12, 2025 1:08 PM
> To: Keller, Jacob E <jacob.e.keller@intel.com>; Kitszel, Przemyslaw
> <przemyslaw.kitszel@intel.com>; Mina Almasry <almasrymina@google.com>
> Cc: Nguyen, Anthony L <anthony.l.nguyen@intel.com>; Andrew Lunn
> <andrew+netdev@lunn.ch>; David S. Miller <davem@davemloft.net>;
> netdev@vger.kernel.org; Eric Dumazet <edumazet@google.com>; linux-
> kernel@vger.kernel.org; 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>;
> Shachar Raindel <shacharr@google.com>
> 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@intel.com>
> > Sent: Friday, December 12, 2025 8:43 PM
> > To: Kitszel, Przemyslaw <przemyslaw.kitszel@intel.com>; Loktionov,
> > Aleksandr <aleksandr.loktionov@intel.com>; Mina Almasry
> > <almasrymina@google.com>
> > Cc: Nguyen, Anthony L <anthony.l.nguyen@intel.com>; Andrew Lunn
> > <andrew+netdev@lunn.ch>; David S. Miller <davem@davemloft.net>;
> > netdev@vger.kernel.org; Eric Dumazet <edumazet@google.com>; linux-
> > kernel@vger.kernel.org; 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>; Shachar Raindel <shacharr@google.com>
> > 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@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
> > >
> > 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@intel.com>
> 
> 
> 
Tested-by: Samuel Salin <Samuel.salin@intel.com>