diff mbox series

[net-next,v2,2/7] ice: Remove gettime HW semaphore

Message ID 20221122221047.3095231-3-anthony.l.nguyen@intel.com (mailing list archive)
State Changes Requested
Delegated to: Netdev Maintainers
Headers show
Series Intel Wired LAN Driver Updates 2022-11-22 (ice) | expand

Checks

Context Check Description
netdev/tree_selection success Clearly marked for net-next
netdev/fixes_present success Fixes tag not required for -next series
netdev/subject_prefix success Link
netdev/cover_letter success Series has a cover letter
netdev/patch_count success Link
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 0 this patch: 0
netdev/cc_maintainers success CCed 6 of 6 maintainers
netdev/build_clang success Errors and warnings before: 0 this patch: 0
netdev/module_param success Was 0 now: 0
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 0 this patch: 0
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 44 lines checked
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Tony Nguyen Nov. 22, 2022, 10:10 p.m. UTC 
From: Karol Kolacinski <karol.kolacinski@intel.com>

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>
Tested-by: Gurucharan G <gurucharanx.g@intel.com> (A Contingent worker at Intel)
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
---
 drivers/net/ethernet/intel/ice/ice_ptp.c | 31 +++---------------------
 1 file changed, 3 insertions(+), 28 deletions(-)

Comments

Richard Cochran Nov. 22, 2022, 10:36 p.m. UTC | #1 
On Tue, Nov 22, 2022 at 02:10:42PM -0800, Tony Nguyen wrote:
> From: Karol Kolacinski <karol.kolacinski@intel.com>
> 
> 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.

NAK

Correctness comes before performance.

Thanks,
Richard


> 
> Signed-off-by: Karol Kolacinski <karol.kolacinski@intel.com>
> Tested-by: Gurucharan G <gurucharanx.g@intel.com> (A Contingent worker at Intel)
> Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
> ---
>  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 5cf198a33e26..d1bab1876249 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
> @@ -1789,15 +1769,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;
>  }
> -- 
> 2.35.1
>
Tony Nguyen Nov. 23, 2022, 6:42 p.m. UTC | #2 
On 11/22/2022 2:36 PM, Richard Cochran wrote:
> On Tue, Nov 22, 2022 at 02:10:42PM -0800, Tony Nguyen wrote:
>> From: Karol Kolacinski <karol.kolacinski@intel.com>
>>
>> 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.
> 
> NAK
> 
> Correctness comes before performance.

Will drop this patch from the series.

Thanks,
Tony
diff mbox series

Patch

diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c b/drivers/net/ethernet/intel/ice/ice_ptp.c
index 5cf198a33e26..d1bab1876249 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
@@ -1789,15 +1769,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;
 }