diff mbox series

[v5,iwl-next,5/6] ice: factor out ice_ptp_rebuild_owner()

Message ID 20240108124717.1845481-6-karol.kolacinski@intel.com (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series ice: fix timestamping in reset process | expand

Checks

Context Check Description
netdev/series_format warning Target tree name not specified in the subject
netdev/tree_selection success Guessed tree name to be net-next
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 1081 this patch: 1081
netdev/cc_maintainers success CCed 0 of 0 maintainers
netdev/build_clang success Errors and warnings before: 1108 this patch: 1108
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
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: 1108 this patch: 1108
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 140 lines checked
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Karol Kolacinski Jan. 8, 2024, 12:47 p.m. UTC 
From: Jacob Keller <jacob.e.keller@intel.com>

The ice_ptp_reset() function uses a goto to skip past clock owner
operations if performing a PF reset or if the device is not the clock
owner. This is a bit confusing. Factor this out into
ice_ptp_rebuild_owner() instead.

The ice_ptp_reset() function is called by ice_rebuild() to restore PTP
functionality after a device reset. Follow the convention set by the
ice_main.c file and rename this function to ice_ptp_rebuild(), in the
same way that we have ice_prepare_for_reset() and
ice_ptp_prepare_for_reset().

Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
Signed-off-by: Karol Kolacinski <karol.kolacinski@intel.com>
Reviewed-by: Jacob Keller <jacob.e.keller@intel.com>
---
 drivers/net/ethernet/intel/ice/ice_main.c |  2 +-
 drivers/net/ethernet/intel/ice/ice_ptp.c  | 66 ++++++++++++++---------
 drivers/net/ethernet/intel/ice/ice_ptp.h  |  6 +--
 3 files changed, 44 insertions(+), 30 deletions(-)

Comments

Pucha, HimasekharX Reddy Jan. 12, 2024, 5:33 p.m. UTC | #1 
> -----Original Message-----
> From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf Of Karol Kolacinski
> Sent: Monday, January 8, 2024 6:17 PM
> To: intel-wired-lan@lists.osuosl.org
> Cc: Keller, Jacob E <jacob.e.keller@intel.com>; netdev@vger.kernel.org; Kolacinski, Karol <karol.kolacinski@intel.com>; Nguyen, Anthony L <anthony.l.nguyen@intel.com>; Brandeburg, Jesse <jesse.brandeburg@intel.com>
> Subject: [Intel-wired-lan] [PATCH v5 iwl-next 5/6] ice: factor out ice_ptp_rebuild_owner()
>
> From: Jacob Keller <jacob.e.keller@intel.com>
>
> The ice_ptp_reset() function uses a goto to skip past clock owner
> operations if performing a PF reset or if the device is not the clock
> owner. This is a bit confusing. Factor this out into
> ice_ptp_rebuild_owner() instead.
>
> The ice_ptp_reset() function is called by ice_rebuild() to restore PTP
> functionality after a device reset. Follow the convention set by the
> ice_main.c file and rename this function to ice_ptp_rebuild(), in the
> same way that we have ice_prepare_for_reset() and
> ice_ptp_prepare_for_reset().
>
> Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
> Signed-off-by: Karol Kolacinski <karol.kolacinski@intel.com>
> Reviewed-by: Jacob Keller <jacob.e.keller@intel.com>
> ---
>  drivers/net/ethernet/intel/ice/ice_main.c |  2 +-
>  drivers/net/ethernet/intel/ice/ice_ptp.c  | 66 ++++++++++++++---------
>  drivers/net/ethernet/intel/ice/ice_ptp.h  |  6 +--
>  3 files changed, 44 insertions(+), 30 deletions(-)
>

Tested-by: Pucha Himasekhar Reddy <himasekharx.reddy.pucha@intel.com> (A Contingent worker at Intel)
Simon Horman Jan. 15, 2024, 10:39 a.m. UTC | #2 
On Mon, Jan 08, 2024 at 01:47:16PM +0100, Karol Kolacinski wrote:
> From: Jacob Keller <jacob.e.keller@intel.com>
> 
> The ice_ptp_reset() function uses a goto to skip past clock owner
> operations if performing a PF reset or if the device is not the clock
> owner. This is a bit confusing. Factor this out into
> ice_ptp_rebuild_owner() instead.
> 
> The ice_ptp_reset() function is called by ice_rebuild() to restore PTP
> functionality after a device reset. Follow the convention set by the
> ice_main.c file and rename this function to ice_ptp_rebuild(), in the
> same way that we have ice_prepare_for_reset() and
> ice_ptp_prepare_for_reset().

nit: This feels more like two changes than one,
     which I might have put into two patches.

> Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
> Signed-off-by: Karol Kolacinski <karol.kolacinski@intel.com>
> Reviewed-by: Jacob Keller <jacob.e.keller@intel.com>

...

> diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c b/drivers/net/ethernet/intel/ice/ice_ptp.c
> index fe2d8389627b..8a589f853e96 100644
> --- a/drivers/net/ethernet/intel/ice/ice_ptp.c
> +++ b/drivers/net/ethernet/intel/ice/ice_ptp.c
> @@ -2665,11 +2665,13 @@ void ice_ptp_prepare_for_reset(struct ice_pf *pf, enum ice_reset_req reset_type)
>  }
>  
>  /**
> - * ice_ptp_reset - Initialize PTP hardware clock support after reset
> + * ice_ptp_rebuild_owner - Initialize PTP clock owner after reset
>   * @pf: Board private structure
> - * @reset_type: the reset type being performed
> + *
> + * Companion function for ice_ptp_rebuild() which handles tasks that only the
> + * PTP clock owner instance should perform.
>   */
> -void ice_ptp_reset(struct ice_pf *pf, enum ice_reset_req reset_type)
> +static int ice_ptp_rebuild_owner(struct ice_pf *pf)
>  {
>  	struct ice_ptp *ptp = &pf->ptp;
>  	struct ice_hw *hw = &pf->hw;
> @@ -2677,34 +2679,21 @@ void ice_ptp_reset(struct ice_pf *pf, enum ice_reset_req reset_type)
>  	u64 time_diff;
>  	int err;
>  
> -	if (ptp->state != ICE_PTP_RESETTING) {
> -		if (ptp->state == ICE_PTP_READY) {
> -			ice_ptp_prepare_for_reset(pf, reset_type);
> -		} else {
> -			err = -EINVAL;
> -			dev_err(ice_pf_to_dev(pf), "PTP was not initialized\n");
> -			goto err;
> -		}
> -	}
> -
> -	if (reset_type == ICE_RESET_PFR || !ice_pf_src_tmr_owned(pf))
> -		goto pfr;
> -
>  	err = ice_ptp_init_phc(hw);
>  	if (err)
> -		goto err;
> +		return err;
>  
>  	/* Acquire the global hardware lock */
>  	if (!ice_ptp_lock(hw)) {
>  		err = -EBUSY;
> -		goto err;
> +		return err;
>  	}
>  
>  	/* Write the increment time value to PHY and LAN */
>  	err = ice_ptp_write_incval(hw, ice_base_incval(pf));
>  	if (err) {
>  		ice_ptp_unlock(hw);
> -		goto err;
> +		return err;
>  	}
>  
>  	/* Write the initial Time value to PHY and LAN using the cached PHC
> @@ -2720,7 +2709,7 @@ void ice_ptp_reset(struct ice_pf *pf, enum ice_reset_req reset_type)
>  	err = ice_ptp_write_init(pf, &ts);
>  	if (err) {
>  		ice_ptp_unlock(hw);
> -		goto err;
> +		return err;
>  	}
>  
>  	/* Release the global hardware lock */
> @@ -2729,11 +2718,41 @@ void ice_ptp_reset(struct ice_pf *pf, enum ice_reset_req reset_type)
>  	if (!ice_is_e810(hw)) {
>  		/* Enable quad interrupts */
>  		err = ice_ptp_cfg_phy_interrupt(pf, true, 1);
> +		if (err)
> +			return err;
> +
> +		ice_ptp_restart_all_phy(pf);

The conditions for calling ice_ptp_restart_all_phy() seem to have
changed (though perhaps in practice they are the same).
And the ordering of this operation relative to the following code has
changed:

	/* Init Tx structures */
	if (ice_is_e810(&pf->hw)) {
		err = ice_ptp_init_tx_e810(pf, &ptp->port.tx);
	} else {
		kthread_init_delayed_work(&ptp->port.ov_work,
					  ice_ptp_wait_for_offsets);
		err = ice_ptp_init_tx_e82x(pf, &ptp->port.tx,
					   ptp->port.port_num);
	}

	ptp->state = ICE_PTP_READY;

Is this intentional?

I do see that the above code is removed in the following patch,
and replaced by a call to ice_ptp_flush_all_tx_tracker()
in ice_ptp_rebuild_owner(). But perhaps this patch
should move this code block code to that location?

> +	}
> +
> +	return 0;
> +}
> +
> +/**
> + * ice_ptp_rebuild - Initialize PTP hardware clock support after reset
> + * @pf: Board private structure
> + * @reset_type: the reset type being performed
> + */
> +void ice_ptp_rebuild(struct ice_pf *pf, enum ice_reset_req reset_type)
> +{
> +	struct ice_ptp *ptp = &pf->ptp;
> +	int err;
> +
> +	if (ptp->state != ICE_PTP_RESETTING) {
> +		if (ptp->state == ICE_PTP_READY) {
> +			ice_ptp_prepare_for_reset(pf, reset_type);
> +		} else {
> +			err = -EINVAL;
> +			dev_err(ice_pf_to_dev(pf), "PTP was not initialized\n");
> +			goto err;
> +		}
> +	}
> +
> +	if (ice_pf_src_tmr_owned(pf) && reset_type != ICE_RESET_PFR) {
> +		err = ice_ptp_rebuild_owner(pf);
>  		if (err)
>  			goto err;
>  	}
>  
> -pfr:
>  	/* Init Tx structures */
>  	if (ice_is_e810(&pf->hw)) {
>  		err = ice_ptp_init_tx_e810(pf, &ptp->port.tx);
> @@ -2748,11 +2767,6 @@ void ice_ptp_reset(struct ice_pf *pf, enum ice_reset_req reset_type)
>  
>  	ptp->state = ICE_PTP_READY;
>  
> -	/* Restart the PHY timestamping block */
> -	if (!test_bit(ICE_PFR_REQ, pf->state) &&
> -	    ice_pf_src_tmr_owned(pf))
> -		ice_ptp_restart_all_phy(pf);
> -
>  	/* Start periodic work going */
>  	kthread_queue_delayed_work(ptp->kworker, &ptp->work, 0);
>  

...
Karol Kolacinski Jan. 18, 2024, 5:36 p.m. UTC | #3 
On Mon, Jan 15, 2024 at 11:39AM +0100, Simon Horman wrote:
> > @@ -2729,11 +2718,41 @@ void ice_ptp_reset(struct ice_pf *pf, enum ice_reset_req reset_type)
> >        if (!ice_is_e810(hw)) {
> >                /* Enable quad interrupts */
> >                err = ice_ptp_cfg_phy_interrupt(pf, true, 1);
> > +             if (err)
> > +                     return err;
> > +
> > +             ice_ptp_restart_all_phy(pf);
> 
> The conditions for calling ice_ptp_restart_all_phy() seem to have
> changed (though perhaps in practice they are the same).
> And the ordering of this operation relative to the following code has
> changed:
> 
>         /* Init Tx structures */
>         if (ice_is_e810(&pf->hw)) {
>                 err = ice_ptp_init_tx_e810(pf, &ptp->port.tx);
>         } else {
>                 kthread_init_delayed_work(&ptp->port.ov_work,
>                                           ice_ptp_wait_for_offsets);
>                 err = ice_ptp_init_tx_e82x(pf, &ptp->port.tx,
>                                            ptp->port.port_num);
>         }
> 
>         ptp->state = ICE_PTP_READY;
> 
> Is this intentional?
> 
> I do see that the above code is removed in the following patch,
> and replaced by a call to ice_ptp_flush_all_tx_tracker()
> in ice_ptp_rebuild_owner(). But perhaps this patch
> should move this code block code to that location?

ice_ptp_restart_all_phy() was previously done for clock owner on
non-PF reset. This patch adds only a check if the device is not E810,
because only E82X devices need PHY restart.

ice_ptp_init_tx_<dev> location does not matter as long as it's done
before setting ICE_PTP_READY.

Thanks,
Karol
diff mbox series

Patch

diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ethernet/intel/ice/ice_main.c
index a14e8734cc27..1fa3f40743f5 100644
--- a/drivers/net/ethernet/intel/ice/ice_main.c
+++ b/drivers/net/ethernet/intel/ice/ice_main.c
@@ -7554,7 +7554,7 @@  static void ice_rebuild(struct ice_pf *pf, enum ice_reset_req reset_type)
 	 * fail.
 	 */
 	if (test_bit(ICE_FLAG_PTP_SUPPORTED, pf->flags))
-		ice_ptp_reset(pf, reset_type);
+		ice_ptp_rebuild(pf, reset_type);
 
 	if (ice_is_feature_supported(pf, ICE_F_GNSS))
 		ice_gnss_init(pf);
diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c b/drivers/net/ethernet/intel/ice/ice_ptp.c
index fe2d8389627b..8a589f853e96 100644
--- a/drivers/net/ethernet/intel/ice/ice_ptp.c
+++ b/drivers/net/ethernet/intel/ice/ice_ptp.c
@@ -2665,11 +2665,13 @@  void ice_ptp_prepare_for_reset(struct ice_pf *pf, enum ice_reset_req reset_type)
 }
 
 /**
- * ice_ptp_reset - Initialize PTP hardware clock support after reset
+ * ice_ptp_rebuild_owner - Initialize PTP clock owner after reset
  * @pf: Board private structure
- * @reset_type: the reset type being performed
+ *
+ * Companion function for ice_ptp_rebuild() which handles tasks that only the
+ * PTP clock owner instance should perform.
  */
-void ice_ptp_reset(struct ice_pf *pf, enum ice_reset_req reset_type)
+static int ice_ptp_rebuild_owner(struct ice_pf *pf)
 {
 	struct ice_ptp *ptp = &pf->ptp;
 	struct ice_hw *hw = &pf->hw;
@@ -2677,34 +2679,21 @@  void ice_ptp_reset(struct ice_pf *pf, enum ice_reset_req reset_type)
 	u64 time_diff;
 	int err;
 
-	if (ptp->state != ICE_PTP_RESETTING) {
-		if (ptp->state == ICE_PTP_READY) {
-			ice_ptp_prepare_for_reset(pf, reset_type);
-		} else {
-			err = -EINVAL;
-			dev_err(ice_pf_to_dev(pf), "PTP was not initialized\n");
-			goto err;
-		}
-	}
-
-	if (reset_type == ICE_RESET_PFR || !ice_pf_src_tmr_owned(pf))
-		goto pfr;
-
 	err = ice_ptp_init_phc(hw);
 	if (err)
-		goto err;
+		return err;
 
 	/* Acquire the global hardware lock */
 	if (!ice_ptp_lock(hw)) {
 		err = -EBUSY;
-		goto err;
+		return err;
 	}
 
 	/* Write the increment time value to PHY and LAN */
 	err = ice_ptp_write_incval(hw, ice_base_incval(pf));
 	if (err) {
 		ice_ptp_unlock(hw);
-		goto err;
+		return err;
 	}
 
 	/* Write the initial Time value to PHY and LAN using the cached PHC
@@ -2720,7 +2709,7 @@  void ice_ptp_reset(struct ice_pf *pf, enum ice_reset_req reset_type)
 	err = ice_ptp_write_init(pf, &ts);
 	if (err) {
 		ice_ptp_unlock(hw);
-		goto err;
+		return err;
 	}
 
 	/* Release the global hardware lock */
@@ -2729,11 +2718,41 @@  void ice_ptp_reset(struct ice_pf *pf, enum ice_reset_req reset_type)
 	if (!ice_is_e810(hw)) {
 		/* Enable quad interrupts */
 		err = ice_ptp_cfg_phy_interrupt(pf, true, 1);
+		if (err)
+			return err;
+
+		ice_ptp_restart_all_phy(pf);
+	}
+
+	return 0;
+}
+
+/**
+ * ice_ptp_rebuild - Initialize PTP hardware clock support after reset
+ * @pf: Board private structure
+ * @reset_type: the reset type being performed
+ */
+void ice_ptp_rebuild(struct ice_pf *pf, enum ice_reset_req reset_type)
+{
+	struct ice_ptp *ptp = &pf->ptp;
+	int err;
+
+	if (ptp->state != ICE_PTP_RESETTING) {
+		if (ptp->state == ICE_PTP_READY) {
+			ice_ptp_prepare_for_reset(pf, reset_type);
+		} else {
+			err = -EINVAL;
+			dev_err(ice_pf_to_dev(pf), "PTP was not initialized\n");
+			goto err;
+		}
+	}
+
+	if (ice_pf_src_tmr_owned(pf) && reset_type != ICE_RESET_PFR) {
+		err = ice_ptp_rebuild_owner(pf);
 		if (err)
 			goto err;
 	}
 
-pfr:
 	/* Init Tx structures */
 	if (ice_is_e810(&pf->hw)) {
 		err = ice_ptp_init_tx_e810(pf, &ptp->port.tx);
@@ -2748,11 +2767,6 @@  void ice_ptp_reset(struct ice_pf *pf, enum ice_reset_req reset_type)
 
 	ptp->state = ICE_PTP_READY;
 
-	/* Restart the PHY timestamping block */
-	if (!test_bit(ICE_PFR_REQ, pf->state) &&
-	    ice_pf_src_tmr_owned(pf))
-		ice_ptp_restart_all_phy(pf);
-
 	/* Start periodic work going */
 	kthread_queue_delayed_work(ptp->kworker, &ptp->work, 0);
 
diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.h b/drivers/net/ethernet/intel/ice/ice_ptp.h
index aa7a5588d11d..3af20025043a 100644
--- a/drivers/net/ethernet/intel/ice/ice_ptp.h
+++ b/drivers/net/ethernet/intel/ice/ice_ptp.h
@@ -316,7 +316,7 @@  enum ice_tx_tstamp_work ice_ptp_process_ts(struct ice_pf *pf);
 
 u64 ice_ptp_get_rx_hwts(const union ice_32b_rx_flex_desc *rx_desc,
 			const struct ice_pkt_ctx *pkt_ctx);
-void ice_ptp_reset(struct ice_pf *pf, enum ice_reset_req reset_type);
+void ice_ptp_rebuild(struct ice_pf *pf, enum ice_reset_req reset_type);
 void ice_ptp_prepare_for_reset(struct ice_pf *pf,
 			       enum ice_reset_req reset_type);
 void ice_ptp_init(struct ice_pf *pf);
@@ -358,8 +358,8 @@  ice_ptp_get_rx_hwts(const union ice_32b_rx_flex_desc *rx_desc,
 	return 0;
 }
 
-static inline void ice_ptp_reset(struct ice_pf *pf,
-				 enum ice_reset_req reset_type)
+static inline void ice_ptp_rebuild(struct ice_pf *pf,
+				   enum ice_reset_req reset_type)
 {
 }