Re: [Intel-wired-lan] [PATCH net-next 2/2] ice: Remove gettime HW semaphore

[Tony Nguyen <anthony.l.nguyen@intel.com>]

On 10/3/2022 2:55 AM, Karol Kolacinski wrote:
> Reading the time should not block other accesses to the PTP hardware.
> There isn't a significant risk of reading bad values while another
> thread is modifying the clock. Removing the hardware lock around the
> gettime allows multiple application threads to read the clock time with
> less contention.
> 
> Signed-off-by: Karol Kolacinski <karol.kolacinski@intel.com>
> ---

Vinicius had a comment/concern on this. Adding him to this and bringing 
the comment over.

"
I think the problem is less about concurrent writes/reads than 
concurrent reads: the fact that the registers are latched when the 
"lower" register is read, makes me worried that there's a (very narrow) 
window during rollover in which the "losing" read (of multiple threads 
doing reads) can return a wrong value.

I could be missing something.
"

Thanks,
Tony

>   drivers/net/ethernet/intel/ice/ice_ptp.c | 31 +++---------------------
>   1 file changed, 3 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c b/drivers/net/ethernet/intel/ice/ice_ptp.c
> index 9d1afeca9624..1f481d455134 100644
> --- a/drivers/net/ethernet/intel/ice/ice_ptp.c
> +++ b/drivers/net/ethernet/intel/ice/ice_ptp.c
> @@ -979,26 +979,6 @@ static void ice_ptp_reset_cached_phctime(struct ice_pf *pf)
>   	ice_ptp_flush_tx_tracker(pf, &pf->ptp.port.tx);
>   }
>   
> -/**
> - * ice_ptp_read_time - Read the time from the device
> - * @pf: Board private structure
> - * @ts: timespec structure to hold the current time value
> - * @sts: Optional parameter for holding a pair of system timestamps from
> - *       the system clock. Will be ignored if NULL is given.
> - *
> - * This function reads the source clock registers and stores them in a timespec.
> - * However, since the registers are 64 bits of nanoseconds, we must convert the
> - * result to a timespec before we can return.
> - */
> -static void
> -ice_ptp_read_time(struct ice_pf *pf, struct timespec64 *ts,
> -		  struct ptp_system_timestamp *sts)
> -{
> -	u64 time_ns = ice_ptp_read_src_clk_reg(pf, sts);
> -
> -	*ts = ns_to_timespec64(time_ns);
> -}
> -
>   /**
>    * ice_ptp_write_init - Set PHC time to provided value
>    * @pf: Board private structure
> @@ -1835,15 +1815,10 @@ ice_ptp_gettimex64(struct ptp_clock_info *info, struct timespec64 *ts,
>   		   struct ptp_system_timestamp *sts)
>   {
>   	struct ice_pf *pf = ptp_info_to_pf(info);
> -	struct ice_hw *hw = &pf->hw;
> +	u64 time_ns;
>   
> -	if (!ice_ptp_lock(hw)) {
> -		dev_err(ice_pf_to_dev(pf), "PTP failed to get time\n");
> -		return -EBUSY;
> -	}
> -
> -	ice_ptp_read_time(pf, ts, sts);
> -	ice_ptp_unlock(hw);
> +	time_ns = ice_ptp_read_src_clk_reg(pf, sts);
> +	*ts = ns_to_timespec64(time_ns);
>   
>   	return 0;
>   }
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan