[Intel-wired-lan] [PATCH v3 5/5] net-intel: check for Tx timestamp timeouts during watchdog
Shannon Nelson
shannon.nelson at oracle.com
Sat Apr 29 17:03:18 UTC 2017
On 4/28/2017 11:44 AM, Jacob Keller wrote:
> Several drivers share similar logic for handling the Tx timestamps.
> These drivers have some parts which rely on the interrupt instead of
> only a polling work task. Although unlikely it may be possible in some
> circumstances for the PTP tx timestamp to never occur. If this happens,
> the result is that all future Tx timestamp requests will be ignored
> permanently.
>
> Fix this by adding a *ptp_tx_hang() function similar to the already
> existing *ptp_rx_hang() routine which checks to make sure that the
> timestamp hasn't taken too long. This ensures that we will eventually
> recover from this case.
>
> Signed-off-by: Jacob Keller <jacob.e.keller at intel.com>
> ---
> drivers/net/ethernet/intel/i40e/i40e.h | 2 ++
> drivers/net/ethernet/intel/i40e/i40e_main.c | 1 +
> drivers/net/ethernet/intel/i40e/i40e_ptp.c | 30 +++++++++++++++++++++++++++
> drivers/net/ethernet/intel/i40e/i40e_txrx.c | 1 +
> drivers/net/ethernet/intel/igb/igb.h | 1 +
> drivers/net/ethernet/intel/igb/igb_main.c | 1 +
> drivers/net/ethernet/intel/igb/igb_ptp.c | 29 ++++++++++++++++++++++++++
> drivers/net/ethernet/intel/ixgbe/ixgbe.h | 1 +
> drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 1 +
> drivers/net/ethernet/intel/ixgbe/ixgbe_ptp.c | 27 ++++++++++++++++++++++++
> 10 files changed, 94 insertions(+)
I'd prefer to see the patches split up between the drivers. I realize
that some commits in this series are simple patches applied to all the
drivers, but something like this is big enough that each driver should
be split out to a separate patch.
sln
>
> diff --git a/drivers/net/ethernet/intel/i40e/i40e.h b/drivers/net/ethernet/intel/i40e/i40e.h
> index f4465afe1fe1..25bf336c5f38 100644
> --- a/drivers/net/ethernet/intel/i40e/i40e.h
> +++ b/drivers/net/ethernet/intel/i40e/i40e.h
> @@ -502,6 +502,7 @@ struct i40e_pf {
> struct ptp_clock *ptp_clock;
> struct ptp_clock_info ptp_caps;
> struct sk_buff *ptp_tx_skb;
> + unsigned long ptp_tx_start;
> struct hwtstamp_config tstamp_config;
> struct mutex tmreg_lock; /* Used to protect the SYSTIME registers. */
> u64 ptp_base_adj;
> @@ -957,6 +958,7 @@ bool i40e_dcb_need_reconfig(struct i40e_pf *pf,
> struct i40e_dcbx_config *new_cfg);
> #endif /* CONFIG_I40E_DCB */
> void i40e_ptp_rx_hang(struct i40e_pf *pf);
> +void i40e_ptp_tx_hang(struct i40e_pf *pf);
> void i40e_ptp_tx_hwtstamp(struct i40e_pf *pf);
> void i40e_ptp_rx_hwtstamp(struct i40e_pf *pf, struct sk_buff *skb, u8 index);
> void i40e_ptp_set_increment(struct i40e_pf *pf);
> diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c b/drivers/net/ethernet/intel/i40e/i40e_main.c
> index c019dec988e3..e4eb97832413 100644
> --- a/drivers/net/ethernet/intel/i40e/i40e_main.c
> +++ b/drivers/net/ethernet/intel/i40e/i40e_main.c
> @@ -6373,6 +6373,7 @@ static void i40e_watchdog_subtask(struct i40e_pf *pf)
> }
>
> i40e_ptp_rx_hang(pf);
> + i40e_ptp_tx_hang(pf);
> }
>
> /**
> diff --git a/drivers/net/ethernet/intel/i40e/i40e_ptp.c b/drivers/net/ethernet/intel/i40e/i40e_ptp.c
> index 876ea169816a..cd35c4b9a8b0 100644
> --- a/drivers/net/ethernet/intel/i40e/i40e_ptp.c
> +++ b/drivers/net/ethernet/intel/i40e/i40e_ptp.c
> @@ -327,6 +327,36 @@ void i40e_ptp_rx_hang(struct i40e_pf *pf)
> pf->rx_hwtstamp_cleared += cleared;
> }
>
> +/**
> + * i40e_ptp_tx_hang - Detect error case when Tx timestamp register is hung
> + * @pf: The PF private data structure
> + *
> + * This watchdog task is run periodically to make sure that we clear the Tx
> + * timestamp logic if we don't obtain a timestamp in a reasonable amount of
> + * time. It is unexpected in the normal case but if it occurs it results in
> + * permanently prevent timestamps of future packets
> + **/
> +void i40e_ptp_tx_hang(struct i40e_pf *pf)
> +{
> + if (!(pf->flags & I40E_FLAG_PTP) || !pf->ptp_tx)
> + return;
> +
> + /* Nothing to do if we're not already waiting for a timestamp */
> + if (!test_bit(__I40E_PTP_TX_IN_PROGRESS, pf->state))
> + return;
> +
> + /* We already have a handler routine which is run when we are notified
> + * of a Tx timestamp in the hardware. If we don't get an interrupt
> + * within a second it is reasonable to assume that we never will.
> + */
> + if (time_is_before_jiffies(pf->ptp_tx_start + HZ)) {
> + dev_kfree_skb_any(pf->ptp_tx_skb);
> + pf->ptp_tx_skb = NULL;
> + clear_bit_unlock(__I40E_PTP_TX_IN_PROGRESS, pf->state);
> + pf->tx_hwtstamp_timeouts++;
> + }
> +}
> +
> /**
> * i40e_ptp_tx_hwtstamp - Utility function which returns the Tx timestamp
> * @pf: Board private structure
> diff --git a/drivers/net/ethernet/intel/i40e/i40e_txrx.c b/drivers/net/ethernet/intel/i40e/i40e_txrx.c
> index c69ee4b0cfe2..c2e9013d05eb 100644
> --- a/drivers/net/ethernet/intel/i40e/i40e_txrx.c
> +++ b/drivers/net/ethernet/intel/i40e/i40e_txrx.c
> @@ -2628,6 +2628,7 @@ static int i40e_tsyn(struct i40e_ring *tx_ring, struct sk_buff *skb,
> if (pf->ptp_tx &&
> !test_and_set_bit_lock(__I40E_PTP_TX_IN_PROGRESS, pf->state)) {
> skb_shinfo(skb)->tx_flags |= SKBTX_IN_PROGRESS;
> + pf->ptp_tx_start = jiffies;
> pf->ptp_tx_skb = skb_get(skb);
> } else {
> pf->tx_hwtstamp_skipped++;
> diff --git a/drivers/net/ethernet/intel/igb/igb.h b/drivers/net/ethernet/intel/igb/igb.h
> index be35edcf6b08..ff4d9073781a 100644
> --- a/drivers/net/ethernet/intel/igb/igb.h
> +++ b/drivers/net/ethernet/intel/igb/igb.h
> @@ -677,6 +677,7 @@ void igb_ptp_stop(struct igb_adapter *adapter);
> void igb_ptp_reset(struct igb_adapter *adapter);
> void igb_ptp_suspend(struct igb_adapter *adapter);
> void igb_ptp_rx_hang(struct igb_adapter *adapter);
> +void igb_ptp_tx_hang(struct igb_adapter *adapter);
> void igb_ptp_rx_rgtstamp(struct igb_q_vector *q_vector, struct sk_buff *skb);
> void igb_ptp_rx_pktstamp(struct igb_q_vector *q_vector, void *va,
> struct sk_buff *skb);
> diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
> index 21b455bfb4ca..eec54d9df06b 100644
> --- a/drivers/net/ethernet/intel/igb/igb_main.c
> +++ b/drivers/net/ethernet/intel/igb/igb_main.c
> @@ -4726,6 +4726,7 @@ static void igb_watchdog_task(struct work_struct *work)
>
> igb_spoof_check(adapter);
> igb_ptp_rx_hang(adapter);
> + igb_ptp_tx_hang(adapter);
>
> /* Check LVMMC register on i350/i354 only */
> if ((adapter->hw.mac.type == e1000_i350) ||
> diff --git a/drivers/net/ethernet/intel/igb/igb_ptp.c b/drivers/net/ethernet/intel/igb/igb_ptp.c
> index a2da5738bfb5..b2f67c169bd2 100644
> --- a/drivers/net/ethernet/intel/igb/igb_ptp.c
> +++ b/drivers/net/ethernet/intel/igb/igb_ptp.c
> @@ -711,6 +711,35 @@ void igb_ptp_rx_hang(struct igb_adapter *adapter)
> }
> }
>
> +/**
> + * igb_ptp_tx_hang - detect error case where Tx timestamp never finishes
> + * @adapter: private network adapter structure
> + */
> +void igb_ptp_tx_hang(struct igb_adapter *adapter)
> +{
> + bool timeout = time_is_before_jiffies(adapter->ptp_tx_start +
> + IGB_PTP_TX_TIMEOUT);
> +
> + if (!adapter->ptp_tx_skb)
> + return;
> +
> + if (!test_bit(__IGB_PTP_TX_IN_PROGRESS, &adapter->state))
> + return;
> +
> + /* If we haven't received a timestamp within the timeout, it is
> + * reasonable to assume that it will never occur, so we can unlock the
> + * timestamp bit when this occurs.
> + */
> + if (timeout) {
> + cancel_work_sync(&adapter->ptp_tx_work);
> + dev_kfree_skb_any(adapter->ptp_tx_skb);
> + adapter->ptp_tx_skb = NULL;
> + clear_bit_unlock(__IGB_PTP_TX_IN_PROGRESS, &adapter->state);
> + adapter->tx_hwtstamp_timeouts++;
> + dev_warn(&adapter->pdev->dev, "clearing Tx timestamp hang\n");
> + }
> +}
> +
> /**
> * igb_ptp_tx_hwtstamp - utility function which checks for TX time stamp
> * @adapter: Board private structure.
> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe.h b/drivers/net/ethernet/intel/ixgbe/ixgbe.h
> index eb36106218ad..dd5578756ae0 100644
> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe.h
> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe.h
> @@ -961,6 +961,7 @@ void ixgbe_ptp_suspend(struct ixgbe_adapter *adapter);
> void ixgbe_ptp_stop(struct ixgbe_adapter *adapter);
> void ixgbe_ptp_overflow_check(struct ixgbe_adapter *adapter);
> void ixgbe_ptp_rx_hang(struct ixgbe_adapter *adapter);
> +void ixgbe_ptp_tx_hang(struct ixgbe_adapter *adapter);
> void ixgbe_ptp_rx_pktstamp(struct ixgbe_q_vector *, struct sk_buff *);
> void ixgbe_ptp_rx_rgtstamp(struct ixgbe_q_vector *, struct sk_buff *skb);
> static inline void ixgbe_ptp_rx_hwtstamp(struct ixgbe_ring *rx_ring,
> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> index c61459fcc21e..f3199c73faed 100644
> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> @@ -7645,6 +7645,7 @@ static void ixgbe_service_task(struct work_struct *work)
> if (test_bit(__IXGBE_PTP_RUNNING, &adapter->state)) {
> ixgbe_ptp_overflow_check(adapter);
> ixgbe_ptp_rx_hang(adapter);
> + ixgbe_ptp_tx_hang(adapter);
> }
>
> ixgbe_service_event_complete(adapter);
> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_ptp.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_ptp.c
> index 9270e6f4fcff..24f4eaf37727 100644
> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_ptp.c
> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_ptp.c
> @@ -662,6 +662,33 @@ static void ixgbe_ptp_clear_tx_timestamp(struct ixgbe_adapter *adapter)
> clear_bit_unlock(__IXGBE_PTP_TX_IN_PROGRESS, &adapter->state);
> }
>
> +/**
> + * ixgbe_ptp_tx_hang - detect error case where Tx timestamp never finishes
> + * @adapter: private network adapter structure
> + */
> +void ixgbe_ptp_tx_hang(struct ixgbe_adapter *adapter)
> +{
> + bool timeout = time_is_before_jiffies(adapter->ptp_tx_start +
> + IXGBE_PTP_TX_TIMEOUT);
> +
> + if (!adapter->ptp_tx_skb)
> + return;
> +
> + if (!test_bit(__IXGBE_PTP_TX_IN_PROGRESS, &adapter->state))
> + return;
> +
> + /* If we haven't received a timestamp within the timeout, it is
> + * reasonable to assume that it will never occur, so we can unlock the
> + * timestamp bit when this occurs.
> + */
> + if (timeout) {
> + cancel_work_sync(&adapter->ptp_tx_work);
> + ixgbe_ptp_clear_tx_timestamp(adapter);
> + adapter->tx_hwtstamp_timeouts++;
> + e_warn(drv, "clearing Tx timestamp hang\n");
> + }
> +}
> +
> /**
> * ixgbe_ptp_tx_hwtstamp - utility function which checks for TX time stamp
> * @adapter: the private adapter struct
>
More information about the Intel-wired-lan
mailing list