Re: [Intel-wired-lan] [PATCH 2/9] ice: introduce PTP state machine

[Przemek Kitszel <przemyslaw.kitszel@intel.com>]

On 8/7/23 12:36, Karol Kolacinski wrote:
> Add PTP state machine so that the driver can correctly identify PTP
> state around resets.
> When the driver got information about ungraceful reset, PTP was not
> prepared for reset and it returned error. When this situation occurs,
> prepare PTP before rebuilding its structures.
> 
> Signed-off-by: Karol Kolacinski <karol.kolacinski@intel.com>
> ---
>   drivers/net/ethernet/intel/ice/ice.h         |   1 -
>   drivers/net/ethernet/intel/ice/ice_ethtool.c |   2 +-
>   drivers/net/ethernet/intel/ice/ice_ptp.c     | 131 +++++++++++++------
>   drivers/net/ethernet/intel/ice/ice_ptp.h     |  10 ++
>   4 files changed, 99 insertions(+), 45 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/ice/ice.h b/drivers/net/ethernet/intel/ice/ice.h
> index 34be1cb1e28f..86f6f94da535 100644
> --- a/drivers/net/ethernet/intel/ice/ice.h
> +++ b/drivers/net/ethernet/intel/ice/ice.h
> @@ -490,7 +490,6 @@ enum ice_pf_flags {
>   	ICE_FLAG_DCB_ENA,
>   	ICE_FLAG_FD_ENA,
>   	ICE_FLAG_PTP_SUPPORTED,		/* PTP is supported by NVM */
> -	ICE_FLAG_PTP,			/* PTP is enabled by software */
>   	ICE_FLAG_ADV_FEATURES,
>   	ICE_FLAG_TC_MQPRIO,		/* support for Multi queue TC */
>   	ICE_FLAG_CLS_FLOWER,
> diff --git a/drivers/net/ethernet/intel/ice/ice_ethtool.c b/drivers/net/ethernet/intel/ice/ice_ethtool.c
> index d3cb08e66dcb..7d57ecf48da0 100644
> --- a/drivers/net/ethernet/intel/ice/ice_ethtool.c
> +++ b/drivers/net/ethernet/intel/ice/ice_ethtool.c
> @@ -3275,7 +3275,7 @@ ice_get_ts_info(struct net_device *dev, struct ethtool_ts_info *info)
>   	struct ice_pf *pf = ice_netdev_to_pf(dev);
>   
>   	/* only report timestamping if PTP is enabled */
> -	if (!test_bit(ICE_FLAG_PTP, pf->flags))
> +	if (!test_bit(ICE_FLAG_PTP_SUPPORTED, pf->flags))
>   		return ethtool_op_get_ts_info(dev, info);
>   
>   	info->so_timestamping = SOF_TIMESTAMPING_TX_SOFTWARE |
> diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c b/drivers/net/ethernet/intel/ice/ice_ptp.c
> index 0669ca905c46..a6ea90b9461e 100644
> --- a/drivers/net/ethernet/intel/ice/ice_ptp.c
> +++ b/drivers/net/ethernet/intel/ice/ice_ptp.c
> @@ -255,6 +255,31 @@ ice_verify_pin_e810t(struct ptp_clock_info *info, unsigned int pin,
>   	return ice_ptp_set_sma_e810t(info, pin, func);
>   }
>   
> +/**
> + * ice_ptp_state_str - Convert PTP state to readable string
> + * @state: PTP state to convert
> + *
> + * Returns: the human readable string representation of the provided PTP
> + * state, used for printing error messages.
> + */
> +static const char *ice_ptp_state_str(enum ice_ptp_state state)
> +{
> +	switch (state) {
> +	case ICE_PTP_UNINIT:
> +		return "UNINITIALIZED";
> +	case ICE_PTP_INITIALIZING:
> +		return "INITIALIZING";
> +	case ICE_PTP_READY:
> +		return "READY";
> +	case ICE_PTP_RESETTING:
> +		return "RESETTING";
> +	case ICE_PTP_ERROR:
> +		return "ERROR";
> +	}
> +
> +	return "UNKNOWN";
> +}
> +
>   /**
>    * ice_ptp_configure_tx_tstamp - Enable or disable Tx timestamp interrupt
>    * @pf: The PF pointer to search in
> @@ -1285,7 +1310,7 @@ void ice_ptp_link_change(struct ice_pf *pf, u8 port, bool linkup)
>   	struct ice_ptp_port *ptp_port;
>   	struct ice_hw *hw = &pf->hw;
>   
> -	if (!test_bit(ICE_FLAG_PTP, pf->flags))
> +	if (pf->ptp.state != ICE_PTP_READY)

test_bit() is atomic API, but "just reading/using variable" is rather not.
Please extend commit message to say something about why transition  here 
(here=whole commit) is safe.

>   		return;
>   
>   	if (WARN_ON_ONCE(port >= ICE_NUM_EXTERNAL_PORTS))
> @@ -2020,7 +2045,7 @@ int ice_ptp_get_ts_config(struct ice_pf *pf, struct ifreq *ifr)
>   {
>   	struct hwtstamp_config *config;
>   
> -	if (!test_bit(ICE_FLAG_PTP, pf->flags))
> +	if (pf->ptp.state != ICE_PTP_READY)
>   		return -EIO;
>   
>   	config = &pf->ptp.tstamp_config;
> @@ -2087,7 +2112,7 @@ int ice_ptp_set_ts_config(struct ice_pf *pf, struct ifreq *ifr)
>   	struct hwtstamp_config config;
>   	int err;
>   
> -	if (!test_bit(ICE_FLAG_PTP, pf->flags))
> +	if (pf->ptp.state != ICE_PTP_READY)
>   		return -EAGAIN;
>   
>   	if (copy_from_user(&config, ifr->ifr_data, sizeof(config)))
> @@ -2422,7 +2447,7 @@ static void ice_ptp_periodic_work(struct kthread_work *work)
>   	struct ice_pf *pf = container_of(ptp, struct ice_pf, ptp);
>   	int err;
>   
> -	if (!test_bit(ICE_FLAG_PTP, pf->flags))
> +	if (pf->ptp.state != ICE_PTP_READY)
>   		return;
>   
>   	err = ice_ptp_update_cached_phctime(pf);
> @@ -2432,6 +2457,42 @@ static void ice_ptp_periodic_work(struct kthread_work *work)
>   				   msecs_to_jiffies(err ? 10 : 500));
>   }
>   
> +/**
> + * ice_ptp_prepare_for_reset - Prepare PTP for reset
> + * @pf: Board private structure
> + */
> +void ice_ptp_prepare_for_reset(struct ice_pf *pf)
> +{
> +	struct ice_ptp *ptp = &pf->ptp;
> +	u8 src_tmr;
> +
> +	if (ptp->state == ICE_PTP_RESETTING)
> +		return;
> +
> +	ptp->state = ICE_PTP_RESETTING;
> +
> +	/* Disable timestamping for both Tx and Rx */
> +	ice_ptp_cfg_timestamp(pf, false);
> +
> +	kthread_cancel_delayed_work_sync(&ptp->work);
> +
> +	if (test_bit(ICE_PFR_REQ, pf->state))
> +		return;
> +
> +	ice_ptp_release_tx_tracker(pf, &pf->ptp.port.tx);
> +
> +	/* Disable periodic outputs */
> +	ice_ptp_disable_all_clkout(pf);
> +
> +	src_tmr = ice_get_ptp_src_clock_index(&pf->hw);
> +
> +	/* Disable source clock */
> +	wr32(&pf->hw, GLTSYN_ENA(src_tmr), (u32)~GLTSYN_ENA_TSYN_ENA_M);
> +
> +	/* Acquire PHC and system timer to restore after reset */
> +	ptp->reset_time = ktime_get_real_ns();
> +}
> +
>   /**
>    * ice_ptp_reset - Initialize PTP hardware clock support after reset
>    * @pf: Board private structure
> @@ -2444,6 +2505,16 @@ void ice_ptp_reset(struct ice_pf *pf)
>   	int err, itr = 1;
>   	u64 time_diff;
>   
> +	if (ptp->state != ICE_PTP_RESETTING) {
> +		if (ptp->state == ICE_PTP_READY) {
> +			ice_ptp_prepare_for_reset(pf);
> +		} else {
> +			err = -EINVAL;
> +			dev_err(ice_pf_to_dev(pf), "PTP was not initialized\n");
> +			goto err;
> +		}
> +	}
> +
>   	if (test_bit(ICE_PFR_REQ, pf->state))
>   		goto pfr;
>   
> @@ -2510,7 +2581,7 @@ void ice_ptp_reset(struct ice_pf *pf)
>   	if (err)
>   		goto err;
>   
> -	set_bit(ICE_FLAG_PTP, pf->flags);
> +	ptp->state = ICE_PTP_READY;
>   
>   	/* Start periodic work going */
>   	kthread_queue_delayed_work(ptp->kworker, &ptp->work, 0);
> @@ -2519,6 +2590,7 @@ void ice_ptp_reset(struct ice_pf *pf)
>   	return;
>   
>   err:
> +	ptp->state = ICE_PTP_ERROR;
>   	dev_err(ice_pf_to_dev(pf), "PTP reset failed %d\n", err);
>   }
>   
> @@ -2725,39 +2797,6 @@ int ice_ptp_clock_index(struct ice_pf *pf)
>   	return clock ? ptp_clock_index(clock) : -1;
>   }
>   
> -/**
> - * ice_ptp_prepare_for_reset - Prepare PTP for reset
> - * @pf: Board private structure
> - */
> -void ice_ptp_prepare_for_reset(struct ice_pf *pf)
> -{
> -	struct ice_ptp *ptp = &pf->ptp;
> -	u8 src_tmr;
> -
> -	clear_bit(ICE_FLAG_PTP, pf->flags);
> -
> -	/* Disable timestamping for both Tx and Rx */
> -	ice_ptp_cfg_timestamp(pf, false);
> -
> -	kthread_cancel_delayed_work_sync(&ptp->work);
> -
> -	if (test_bit(ICE_PFR_REQ, pf->state))
> -		return;
> -
> -	ice_ptp_release_tx_tracker(pf, &pf->ptp.port.tx);
> -
> -	/* Disable periodic outputs */
> -	ice_ptp_disable_all_clkout(pf);
> -
> -	src_tmr = ice_get_ptp_src_clock_index(&pf->hw);
> -
> -	/* Disable source clock */
> -	wr32(&pf->hw, GLTSYN_ENA(src_tmr), (u32)~GLTSYN_ENA_TSYN_ENA_M);
> -
> -	/* Acquire PHC and system timer to restore after reset */
> -	ptp->reset_time = ktime_get_real_ns();
> -}
> -
>   /**
>    * ice_ptp_init_owner - Initialize PTP_1588_CLOCK device
>    * @pf: Board private structure
> @@ -3011,6 +3050,8 @@ void ice_ptp_init(struct ice_pf *pf)
>   	struct ice_hw *hw = &pf->hw;
>   	int err;
>   
> +	ptp->state = ICE_PTP_INITIALIZING;
> +
>   	ice_ptp_init_phy_model(hw);
>   
>   	ice_ptp_init_tx_interrupt_mode(pf);
> @@ -3032,7 +3073,6 @@ void ice_ptp_init(struct ice_pf *pf)
>   	/* Start the PHY timestamping block */
>   	ice_ptp_reset_phy_timestamping(pf);
>   
> -	set_bit(ICE_FLAG_PTP, pf->flags);
>   	err = ice_ptp_init_work(pf, ptp);
>   	if (err)
>   		goto err;
> @@ -3041,6 +3081,7 @@ void ice_ptp_init(struct ice_pf *pf)
>   	if (err)
>   		goto err;
>   
> +	ptp->state = ICE_PTP_READY;
>   	dev_info(ice_pf_to_dev(pf), "PTP init successful\n");
>   	return;
>   
> @@ -3050,7 +3091,7 @@ void ice_ptp_init(struct ice_pf *pf)
>   		ptp_clock_unregister(ptp->clock);
>   		pf->ptp.clock = NULL;
>   	}
> -	clear_bit(ICE_FLAG_PTP, pf->flags);
> +	ptp->state = ICE_PTP_ERROR;
>   	dev_err(ice_pf_to_dev(pf), "PTP failed %d\n", err);
>   }
>   
> @@ -3063,9 +3104,15 @@ void ice_ptp_init(struct ice_pf *pf)
>    */
>   void ice_ptp_release(struct ice_pf *pf)
>   {
> -	if (!test_bit(ICE_FLAG_PTP, pf->flags))
> +	if (pf->ptp.state == ICE_PTP_UNINIT)
>   		return;
>   
> +	if (pf->ptp.state != ICE_PTP_READY)
> +		dev_warn(ice_pf_to_dev(pf), "PTP state machine is %s, tearing down PTP anyways\n",
> +			 ice_ptp_state_str(pf->ptp.state));
> +
> +	pf->ptp.state = ICE_PTP_UNINIT;
> +
>   	/* Disable timestamping for both Tx and Rx */
>   	ice_ptp_cfg_timestamp(pf, false);
>   
> @@ -3073,8 +3120,6 @@ void ice_ptp_release(struct ice_pf *pf)
>   
>   	ice_ptp_release_tx_tracker(pf, &pf->ptp.port.tx);
>   
> -	clear_bit(ICE_FLAG_PTP, pf->flags);
> -
>   	kthread_cancel_delayed_work_sync(&pf->ptp.work);
>   
>   	ice_ptp_port_phy_stop(&pf->ptp.port);
> diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.h b/drivers/net/ethernet/intel/ice/ice_ptp.h
> index 8f6f94392756..674a0abe3cdd 100644
> --- a/drivers/net/ethernet/intel/ice/ice_ptp.h
> +++ b/drivers/net/ethernet/intel/ice/ice_ptp.h
> @@ -201,8 +201,17 @@ struct ice_ptp_port_owner {
>   
>   #define GLTSYN_TGT_H_IDX_MAX		4
>   
> +enum ice_ptp_state {
> +	ICE_PTP_UNINIT = 0,
> +	ICE_PTP_INITIALIZING,
> +	ICE_PTP_READY,
> +	ICE_PTP_RESETTING,
> +	ICE_PTP_ERROR,
> +};
> +
>   /**
>    * struct ice_ptp - data used for integrating with CONFIG_PTP_1588_CLOCK
> + * @state: current state of PTP state machine
>    * @tx_interrupt_mode: the TX interrupt mode for the PTP clock
>    * @port: data for the PHY port initialization procedure
>    * @ports_owner: data for the auxiliary driver owner
> @@ -225,6 +234,7 @@ struct ice_ptp_port_owner {
>    * @late_cached_phc_updates: number of times cached PHC update is late
>    */
>   struct ice_ptp {
> +	enum ice_ptp_state state;
>   	enum ice_ptp_tx_interrupt tx_interrupt_mode;
>   	struct ice_ptp_port port;
>   	struct ice_ptp_port_owner ports_owner;

_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan