| Message ID | 20240108124717.1845481-2-karol.kolacinski@intel.com |
|---|---|
| State | Superseded |
| Delegated to: | Anthony Nguyen |
| Headers | show |
| Series | ice: fix timestamping in reset process | expand |
> -----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 1/6] ice: introduce PTP state machine > > 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: 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> > --- > V3 -> V4: removed merge conflict leftovers > V2 -> V3: fixed Tx timestamps missing by moving ICE_PTP_READY before > ice_ptp_init_work() > > 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 | 112 +++++++++++-------- > drivers/net/ethernet/intel/ice/ice_ptp.h | 10 ++ > 4 files changed, 76 insertions(+), 49 deletions(-) > Tested-by: Pucha Himasekhar Reddy <himasekharx.reddy.pucha@intel.com> (A Contingent worker at Intel)
On Mon, Jan 08, 2024 at 01:47:12PM +0100, Karol Kolacinski wrote: Should there be a "From: Jacob" line here to match the Signed-off-by below? > 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: 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> Hi Karol and Jacob, FWIIW, The combination of both a Signed-off-by and Reviewed-by tag from Jacob seems a little odd to me. If he authored the patch then I would have gone with the following (along with the From line mentioned above): Signed-off-by: Jacob Keller <jacob.e.keller@intel.com> Signed-off-by: Karol Kolacinski <karol.kolacinski@intel.com> Otherwise, if he reviewed the patch I would have gone with: Reviewed-by: Jacob Keller <jacob.e.keller@intel.com> Signed-off-by: Karol Kolacinski <karol.kolacinski@intel.com> ... > diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c b/drivers/net/ethernet/intel/ice/ice_ptp.c ... > @@ -2640,6 +2676,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; > + } > + } nit: perhaps this following is slightly nicer? (completely untested!) if (ptp->state == ICE_PTP_READY) { ice_ptp_prepare_for_reset(pf); } else if (ptp->state != ICE_PTP_RESETTING) { err = -EINVAL; dev_err(ice_pf_to_dev(pf), "PTP was not initialized\n"); goto err; } > + > if (test_bit(ICE_PFR_REQ, pf->state) || > !ice_pf_src_tmr_owned(pf)) > goto pfr; ...
> -----Original Message----- > From: Simon Horman <horms@kernel.org> > Sent: Monday, January 15, 2024 2:33 AM > To: Kolacinski, Karol <karol.kolacinski@intel.com> > Cc: intel-wired-lan@lists.osuosl.org; netdev@vger.kernel.org; Nguyen, Anthony L > <anthony.l.nguyen@intel.com>; Brandeburg, Jesse > <jesse.brandeburg@intel.com>; Keller, Jacob E <jacob.e.keller@intel.com> > Subject: Re: [PATCH v5 iwl-next 1/6] ice: introduce PTP state machine > > On Mon, Jan 08, 2024 at 01:47:12PM +0100, Karol Kolacinski wrote: > > Should there be a "From: Jacob" line here to > match the Signed-off-by below? > > > 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: 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> > > Hi Karol and Jacob, > > FWIIW, The combination of both a Signed-off-by and Reviewed-by tag from > Jacob seems a little odd to me. If he authored the patch then I would have > gone with the following (along with the From line mentioned above): > > Signed-off-by: Jacob Keller <jacob.e.keller@intel.com> > Signed-off-by: Karol Kolacinski <karol.kolacinski@intel.com> > > Otherwise, if he reviewed the patch I would have gone with: > > Reviewed-by: Jacob Keller <jacob.e.keller@intel.com> > Signed-off-by: Karol Kolacinski <karol.kolacinski@intel.com> > It's a bit odd, because I authored the initial code and patches some time ago, and Karol has been working to rebase and re-organize the code, so in some sense he authored part of this. I think a Co-authored would be suitable here. Additionally, I reviewed the result before it was published here. Thanks, Jake > ... > > > diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c > b/drivers/net/ethernet/intel/ice/ice_ptp.c > > ... > > > @@ -2640,6 +2676,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; > > + } > > + } > > nit: perhaps this following is slightly nicer? > (completely untested!) > > if (ptp->state == ICE_PTP_READY) { > ice_ptp_prepare_for_reset(pf); > } else if (ptp->state != ICE_PTP_RESETTING) { > err = -EINVAL; > dev_err(ice_pf_to_dev(pf), "PTP was not initialized\n"); > goto err; > } > > > + > > if (test_bit(ICE_PFR_REQ, pf->state) || > > !ice_pf_src_tmr_owned(pf)) > > goto pfr; > > ...
On Wed, Jan 17, 2024 at 10:07:52PM +0000, Keller, Jacob E wrote: > > > > -----Original Message----- > > From: Simon Horman <horms@kernel.org> > > Sent: Monday, January 15, 2024 2:33 AM > > To: Kolacinski, Karol <karol.kolacinski@intel.com> > > Cc: intel-wired-lan@lists.osuosl.org; netdev@vger.kernel.org; Nguyen, Anthony L > > <anthony.l.nguyen@intel.com>; Brandeburg, Jesse > > <jesse.brandeburg@intel.com>; Keller, Jacob E <jacob.e.keller@intel.com> > > Subject: Re: [PATCH v5 iwl-next 1/6] ice: introduce PTP state machine > > > > On Mon, Jan 08, 2024 at 01:47:12PM +0100, Karol Kolacinski wrote: > > > > Should there be a "From: Jacob" line here to > > match the Signed-off-by below? > > > > > 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: 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> > > > > Hi Karol and Jacob, > > > > FWIIW, The combination of both a Signed-off-by and Reviewed-by tag from > > Jacob seems a little odd to me. If he authored the patch then I would have > > gone with the following (along with the From line mentioned above): > > > > Signed-off-by: Jacob Keller <jacob.e.keller@intel.com> > > Signed-off-by: Karol Kolacinski <karol.kolacinski@intel.com> > > > > Otherwise, if he reviewed the patch I would have gone with: > > > > Reviewed-by: Jacob Keller <jacob.e.keller@intel.com> > > Signed-off-by: Karol Kolacinski <karol.kolacinski@intel.com> > > > > It's a bit odd, because I authored the initial code and patches some time ago, and Karol has been working to rebase and re-organize the code, so in some sense he authored part of this. I think a Co-authored would be suitable here. Additionally, I reviewed the result before it was published here. Understood. I agree Co-authored might be useful here.
diff --git a/drivers/net/ethernet/intel/ice/ice.h b/drivers/net/ethernet/intel/ice/ice.h index e841f6c4f1c4..a4ba60e17d0b 100644 --- a/drivers/net/ethernet/intel/ice/ice.h +++ b/drivers/net/ethernet/intel/ice/ice.h @@ -493,7 +493,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 f25e43881df2..3cc364a4d682 100644 --- a/drivers/net/ethernet/intel/ice/ice_ethtool.c +++ b/drivers/net/ethernet/intel/ice/ice_ethtool.c @@ -3361,7 +3361,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 (pf->ptp.state != ICE_PTP_READY) 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 3b6605c8585e..d7de65f8dd53 100644 --- a/drivers/net/ethernet/intel/ice/ice_ptp.c +++ b/drivers/net/ethernet/intel/ice/ice_ptp.c @@ -1430,7 +1430,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) return; if (WARN_ON_ONCE(port >= ICE_NUM_EXTERNAL_PORTS)) @@ -2162,7 +2162,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; @@ -2232,7 +2232,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))) @@ -2616,7 +2616,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); @@ -2628,6 +2628,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_READY) + return; + + ptp->state = ICE_PTP_RESETTING; + + /* Disable timestamping for both Tx and Rx */ + ice_ptp_disable_timestamp_mode(pf); + + 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 @@ -2640,6 +2676,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) || !ice_pf_src_tmr_owned(pf)) goto pfr; @@ -2700,7 +2746,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; /* Restart the PHY timestamping block */ if (!test_bit(ICE_PFR_REQ, pf->state) && @@ -2714,6 +2760,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); } @@ -2922,39 +2969,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_disable_timestamp_mode(pf); - - 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 @@ -3195,6 +3209,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); @@ -3219,12 +3235,13 @@ void ice_ptp_init(struct ice_pf *pf) /* Configure initial Tx interrupt settings */ ice_ptp_cfg_tx_interrupt(pf); - set_bit(ICE_FLAG_PTP, pf->flags); - err = ice_ptp_init_work(pf, ptp); + err = ice_ptp_create_auxbus_device(pf); if (err) goto err; - err = ice_ptp_create_auxbus_device(pf); + ptp->state = ICE_PTP_READY; + + err = ice_ptp_init_work(pf, ptp); if (err) goto err; @@ -3237,7 +3254,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); } @@ -3250,9 +3267,11 @@ 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_READY) return; + pf->ptp.state = ICE_PTP_UNINIT; + /* Disable timestamping for both Tx and Rx */ ice_ptp_disable_timestamp_mode(pf); @@ -3260,8 +3279,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); @@ -3271,6 +3288,9 @@ void ice_ptp_release(struct ice_pf *pf) pf->ptp.kworker = NULL; } + if (ice_pf_src_tmr_owned(pf)) + ice_ptp_unregister_auxbus_driver(pf); + if (!pf->ptp.clock) return; @@ -3280,7 +3300,5 @@ void ice_ptp_release(struct ice_pf *pf) ptp_clock_unregister(pf->ptp.clock); pf->ptp.clock = NULL; - ice_ptp_unregister_auxbus_driver(pf); - dev_info(ice_pf_to_dev(pf), "Removed PTP clock\n"); } diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.h b/drivers/net/ethernet/intel/ice/ice_ptp.h index 087dd32d8762..2457380142e1 100644 --- a/drivers/net/ethernet/intel/ice/ice_ptp.h +++ b/drivers/net/ethernet/intel/ice/ice_ptp.h @@ -203,8 +203,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 @@ -227,6 +236,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;