| Message ID | 1394818481-7652-3-git-send-email-kubakici@wp.pl |
|---|---|
| State | Awaiting Upstream, archived |
| Delegated to: | David Miller |
| Headers | show |
On Fri, 2014-03-14 at 18:34 +0100, Jakub Kicinski wrote: > Hardware may fail to report time stamp e.g.: > - when hardware time stamping is not enabled > - when time stamp is requested shortly after ifup trivia: > diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c b/drivers/net/ethernet/intel/e1000e/netdev.c [] > @@ -1155,6 +1155,12 @@ static void e1000e_tx_hwtstamp_work(struct work_struct *work) > skb_tstamp_tx(adapter->tx_hwtstamp_skb, &shhwtstamps); > dev_kfree_skb_any(adapter->tx_hwtstamp_skb); > adapter->tx_hwtstamp_skb = NULL; > + } else if (time_after(jiffies, adapter->tx_hwtstamp_start > + + adapter->tx_timeout_factor * HZ)) { > + dev_kfree_skb_any(adapter->tx_hwtstamp_skb); > + adapter->tx_hwtstamp_skb = NULL; > + adapter->tx_hwtstamp_timeouts++; > + e_warn("clearing Tx timestamp hang"); Missing \n termination -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, 14 Mar 2014 10:48:37 -0700, Joe Perches wrote: > On Fri, 2014-03-14 at 18:34 +0100, Jakub Kicinski wrote: > > Hardware may fail to report time stamp e.g.: > > - when hardware time stamping is not enabled > > - when time stamp is requested shortly after ifup > > trivia: > > > diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c b/drivers/net/ethernet/intel/e1000e/netdev.c > [] > > @@ -1155,6 +1155,12 @@ static void e1000e_tx_hwtstamp_work(struct work_struct *work) > > skb_tstamp_tx(adapter->tx_hwtstamp_skb, &shhwtstamps); > > dev_kfree_skb_any(adapter->tx_hwtstamp_skb); > > adapter->tx_hwtstamp_skb = NULL; > > + } else if (time_after(jiffies, adapter->tx_hwtstamp_start > > + + adapter->tx_timeout_factor * HZ)) { > > + dev_kfree_skb_any(adapter->tx_hwtstamp_skb); > > + adapter->tx_hwtstamp_skb = NULL; > > + adapter->tx_hwtstamp_timeouts++; > > + e_warn("clearing Tx timestamp hang"); > > Missing \n termination I copied that line from ixgbe and assumed that the magic macro adds termination... I see that igb and i40e also lack the \n on similar messages. Can I fix them all in a single follow-up patch? Thanks for catching this. -- kuba -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, 2014-03-14 at 19:02 +0100, Jakub Kiciński wrote: > On Fri, 14 Mar 2014 10:48:37 -0700, Joe Perches wrote: > > On Fri, 2014-03-14 at 18:34 +0100, Jakub Kicinski wrote: > > > Hardware may fail to report time stamp e.g.: > > > - when hardware time stamping is not enabled > > > - when time stamp is requested shortly after ifup > > > > trivia: > > > > > diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c b/drivers/net/ethernet/intel/e1000e/netdev.c > > [] > > > @@ -1155,6 +1155,12 @@ static void e1000e_tx_hwtstamp_work(struct work_struct *work) > > > skb_tstamp_tx(adapter->tx_hwtstamp_skb, &shhwtstamps); > > > dev_kfree_skb_any(adapter->tx_hwtstamp_skb); > > > adapter->tx_hwtstamp_skb = NULL; > > > + } else if (time_after(jiffies, adapter->tx_hwtstamp_start > > > + + adapter->tx_timeout_factor * HZ)) { > > > + dev_kfree_skb_any(adapter->tx_hwtstamp_skb); > > > + adapter->tx_hwtstamp_skb = NULL; > > > + adapter->tx_hwtstamp_timeouts++; > > > + e_warn("clearing Tx timestamp hang"); > > > > Missing \n termination > > I copied that line from ixgbe and assumed that the magic macro adds > termination... > > I see that igb and i40e also lack the \n on similar messages. There's a bunch of them. > Can I fix > them all in a single follow-up patch? There are also a bunch of _dbg messages that lack that \n and those seem to be function tracing uses that should just be deleted. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, 14 Mar 2014 11:44:15 -0700, Joe Perches wrote: > On Fri, 2014-03-14 at 19:02 +0100, Jakub Kiciński wrote: > > On Fri, 14 Mar 2014 10:48:37 -0700, Joe Perches wrote: > > > On Fri, 2014-03-14 at 18:34 +0100, Jakub Kicinski wrote: > > > > Hardware may fail to report time stamp e.g.: > > > > - when hardware time stamping is not enabled > > > > - when time stamp is requested shortly after ifup > > > > > > trivia: > > > > > > > diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c b/drivers/net/ethernet/intel/e1000e/netdev.c > > > [] > > > > @@ -1155,6 +1155,12 @@ static void e1000e_tx_hwtstamp_work(struct work_struct *work) > > > > skb_tstamp_tx(adapter->tx_hwtstamp_skb, &shhwtstamps); > > > > dev_kfree_skb_any(adapter->tx_hwtstamp_skb); > > > > adapter->tx_hwtstamp_skb = NULL; > > > > + } else if (time_after(jiffies, adapter->tx_hwtstamp_start > > > > + + adapter->tx_timeout_factor * HZ)) { > > > > + dev_kfree_skb_any(adapter->tx_hwtstamp_skb); > > > > + adapter->tx_hwtstamp_skb = NULL; > > > > + adapter->tx_hwtstamp_timeouts++; > > > > + e_warn("clearing Tx timestamp hang"); > > > > > > Missing \n termination > > > > I copied that line from ixgbe and assumed that the magic macro adds > > termination... > > > > I see that igb and i40e also lack the \n on similar messages. > > There's a bunch of them. > > > Can I fix > > them all in a single follow-up patch? > > There are also a bunch of _dbg messages that lack that > \n and those seem to be function tracing uses that should > just be deleted. Good point! I'll take care of them too, thanks! -- kuba -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, 2014-03-14 at 18:34 +0100, Jakub Kicinski wrote: > Hardware may fail to report time stamp e.g.: > - when hardware time stamping is not enabled > - when time stamp is requested shortly after ifup > > Timeout time stamp reading work to prevent it from > scheduling itself indefinitely. Report timeout events > via system log and device stats. > > Signed-off-by: Jakub Kicinski <kubakici@wp.pl> > --- > drivers/net/ethernet/intel/e1000e/e1000.h | 2 ++ > drivers/net/ethernet/intel/e1000e/ethtool.c | 1 + > drivers/net/ethernet/intel/e1000e/netdev.c | 7 +++++++ > 3 files changed, 10 insertions(+) Thanks Jakub, I will add this patch to my queue.
diff --git a/drivers/net/ethernet/intel/e1000e/e1000.h b/drivers/net/ethernet/intel/e1000e/e1000.h index 5325e3e..1471c54 100644 --- a/drivers/net/ethernet/intel/e1000e/e1000.h +++ b/drivers/net/ethernet/intel/e1000e/e1000.h @@ -262,6 +262,7 @@ struct e1000_adapter { u32 tx_head_addr; u32 tx_fifo_size; u32 tx_dma_failed; + u32 tx_hwtstamp_timeouts; /* Rx */ bool (*clean_rx) (struct e1000_ring *ring, int *work_done, @@ -334,6 +335,7 @@ struct e1000_adapter { struct hwtstamp_config hwtstamp_config; struct delayed_work systim_overflow_work; struct sk_buff *tx_hwtstamp_skb; + unsigned long tx_hwtstamp_start; struct work_struct tx_hwtstamp_work; spinlock_t systim_lock; /* protects SYSTIML/H regsters */ struct cyclecounter cc; diff --git a/drivers/net/ethernet/intel/e1000e/ethtool.c b/drivers/net/ethernet/intel/e1000e/ethtool.c index 3c2898d..cad250b 100644 --- a/drivers/net/ethernet/intel/e1000e/ethtool.c +++ b/drivers/net/ethernet/intel/e1000e/ethtool.c @@ -104,6 +104,7 @@ static const struct e1000_stats e1000_gstrings_stats[] = { E1000_STAT("rx_hwtstamp_cleared", rx_hwtstamp_cleared), E1000_STAT("uncorr_ecc_errors", uncorr_errors), E1000_STAT("corr_ecc_errors", corr_errors), + E1000_STAT("tx_hwtstamp_timeouts", tx_hwtstamp_timeouts), }; #define E1000_GLOBAL_STATS_LEN ARRAY_SIZE(e1000_gstrings_stats) diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c b/drivers/net/ethernet/intel/e1000e/netdev.c index 3f044e7..ccdc977 100644 --- a/drivers/net/ethernet/intel/e1000e/netdev.c +++ b/drivers/net/ethernet/intel/e1000e/netdev.c @@ -1155,6 +1155,12 @@ static void e1000e_tx_hwtstamp_work(struct work_struct *work) skb_tstamp_tx(adapter->tx_hwtstamp_skb, &shhwtstamps); dev_kfree_skb_any(adapter->tx_hwtstamp_skb); adapter->tx_hwtstamp_skb = NULL; + } else if (time_after(jiffies, adapter->tx_hwtstamp_start + + adapter->tx_timeout_factor * HZ)) { + dev_kfree_skb_any(adapter->tx_hwtstamp_skb); + adapter->tx_hwtstamp_skb = NULL; + adapter->tx_hwtstamp_timeouts++; + e_warn("clearing Tx timestamp hang"); } else { /* reschedule to check later */ schedule_work(&adapter->tx_hwtstamp_work); @@ -5545,6 +5551,7 @@ static netdev_tx_t e1000_xmit_frame(struct sk_buff *skb, skb_shinfo(skb)->tx_flags |= SKBTX_IN_PROGRESS; tx_flags |= E1000_TX_FLAGS_HWTSTAMP; adapter->tx_hwtstamp_skb = skb_get(skb); + adapter->tx_hwtstamp_start = jiffies; schedule_work(&adapter->tx_hwtstamp_work); } else { skb_tx_timestamp(skb);
Hardware may fail to report time stamp e.g.: - when hardware time stamping is not enabled - when time stamp is requested shortly after ifup Timeout time stamp reading work to prevent it from scheduling itself indefinitely. Report timeout events via system log and device stats. Signed-off-by: Jakub Kicinski <kubakici@wp.pl> --- drivers/net/ethernet/intel/e1000e/e1000.h | 2 ++ drivers/net/ethernet/intel/e1000e/ethtool.c | 1 + drivers/net/ethernet/intel/e1000e/netdev.c | 7 +++++++ 3 files changed, 10 insertions(+)