From: Jeff Kirsher <hidden> Date: 2017-06-06 08:58:14
This series contains updates and fixes to e1000e and igb.
Matwey V Kornilov fixes an issue where igb_get_phy_id_82575() relies on
the fact that page 0 is already selected, but this is not the case after
igb_read_phy_reg_gs40g()/igb_write_phy_reg_gs40g() were removed in a
previous commit. This leads to initialization failure and some devices
not working. To fix the issue, explicitly select page 0 before first
access to PHY registers.
Arnd Bergmann modifies the driver to avoid a "defined but not used"
warning by removing #ifdefs and using __maybe_unused annotation instead
for new power management functions.
Jake provides most of the changes in the series, all around PTP and
timestamp fixes/updates. Resolved several race conditions based on
the hardware can only handle one transmit timestamp at a time, so
fix the locking logic, as well as create a statistic for "skipped"
timestamps to help administrators identify issues.
Benjamin Poirier provides 2 changes, first to igb to remove the
second argument to igb_update_stats() since it always passes the
same two arguments. So instead of having to pass the second argument,
just update the function to the necessary information from the adapter
structure. Second modifies the e1000e_get_stats64() call to
dev_get_stats() to avoid ethtool garbage being reported.
Konstantin Khlebnikov modifies e1000e to use disable_hardirq(), instead
of disable_irq() for MSIx vectors in e1000_netpoll().
The following are changes since commit 9d15e5cc8cb68326f7f89b76e04c834fe8e1a2db:
mdio: mux: fix an incorrect less than zero error check using a u32
and are available in the git repository at:
git://git.kernel.org/pub/scm/linux/kernel/git/jkirsher/next-queue 1GbE
Arnd Bergmann (1):
igb: mark PM functions as __maybe_unused
Benjamin Poirier (2):
igb: Remove useless argument
e1000e: Don't return uninitialized stats
Jacob Keller (6):
e1000e: fix race condition around skb_tstamp_tx()
igb: fix race condition with PTP_TX_IN_PROGRESS bits
igb: avoid permanent lock of *_PTP_TX_IN_PROGRESS
e1000e: add statistic indicating number of skipped Tx timestamps
igb: add statistic indicating number of skipped Tx timestamps
igb: check for Tx timestamp timeouts during watchdog
Konstantin Khlebnikov (1):
e1000e: use disable_hardirq() also for MSIX vectors in e1000_netpoll()
Matwey V Kornilov (1):
igb: Explicitly select page 0 at initialization
drivers/net/ethernet/intel/e1000e/e1000.h | 1 +
drivers/net/ethernet/intel/e1000e/ethtool.c | 3 +-
drivers/net/ethernet/intel/e1000e/netdev.c | 39 ++++++++++++--------
drivers/net/ethernet/intel/igb/e1000_82575.c | 1 +
drivers/net/ethernet/intel/igb/igb.h | 4 ++-
drivers/net/ethernet/intel/igb/igb_ethtool.c | 3 +-
drivers/net/ethernet/intel/igb/igb_main.c | 54 ++++++++++++++++------------
drivers/net/ethernet/intel/igb/igb_ptp.c | 41 +++++++++++++++++++--
8 files changed, 103 insertions(+), 43 deletions(-)
--
2.12.2
From: Jeff Kirsher <hidden> Date: 2017-06-06 08:58:15
From: Matwey V Kornilov <redacted>
The functions igb_read_phy_reg_gs40g/igb_write_phy_reg_gs40g (which were
removed in 2a3cdea) explicitly selected the required page at every phy_reg
access. Currently, igb_get_phy_id_82575 relays on the fact that page 0 is
already selected. The assumption is not fulfilled for my Lex 3I380CW
motherboard with integrated dual i211 based gigabit ethernet. This leads to igb
initialization failure and network interfaces are not working:
igb: Intel(R) Gigabit Ethernet Network Driver - version 5.4.0-k
igb: Copyright (c) 2007-2014 Intel Corporation.
igb: probe of 0000:01:00.0 failed with error -2
igb: probe of 0000:02:00.0 failed with error -2
In order to fix it, we explicitly select page 0 before first access to phy
registers.
See also: https://bugzilla.suse.com/show_bug.cgi?id=1009911
See also: http://www.lex.com.tw/products/pdf/3I380A&3I380CW.pdf
Fixes: 2a3cdea ("igb: Remove GS40G specific defines/functions")
Cc: <redacted> # 4.5+
Signed-off-by: Matwey V Kornilov <redacted>
Tested-by: Aaron Brown <redacted>
Signed-off-by: Jeff Kirsher <redacted>
---
drivers/net/ethernet/intel/igb/e1000_82575.c | 1 +
1 file changed, 1 insertion(+)
From: Jeff Kirsher <hidden> Date: 2017-06-06 08:58:15
From: Arnd Bergmann <arnd@arndb.de>
The new wake function is only used by the suspend/resume handlers that
are defined in inside of an #ifdef, which can cause this harmless
warning:
drivers/net/ethernet/intel/igb/igb_main.c:7988:13: warning: 'igb_deliver_wake_packet' defined but not used [-Wunused-function]
Removing the #ifdef, instead using a __maybe_unused annotation
simplifies the code and avoids the warning.
Fixes: b90fa8763560 ("igb: Enable reading of wake up packet")
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Tested-by: Aaron Brown <redacted>
Signed-off-by: Jeff Kirsher <redacted>
---
drivers/net/ethernet/intel/igb/igb_main.c | 18 +++++-------------
1 file changed, 5 insertions(+), 13 deletions(-)
From: Jeff Kirsher <hidden> Date: 2017-06-06 08:58:16
From: Jacob Keller <jacob.e.keller@intel.com>
The igb driver uses a state bit lock to avoid handling more than one Tx
timestamp request at once. This is required because hardware is limited
to a single set of registers for Tx timestamps.
The state bit lock is not properly cleaned up during
igb_xmit_frame_ring() if the transmit fails such as due to DMA or TSO
failure. In some hardware this results in blocking timestamps until the
service task times out. In other hardware this results in a permanent
lock of the timestamp bit because we never receive an interrupt
indicating the timestamp occurred, since indeed the packet was never
transmitted.
Fix this by checking for DMA and TSO errors in igb_xmit_frame_ring() and
properly cleaning up after ourselves when these occur.
Reported-by: Reported-by: David Mirabito <redacted>
Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
Tested-by: Aaron Brown <redacted>
Signed-off-by: Jeff Kirsher <redacted>
---
drivers/net/ethernet/intel/igb/igb_main.c | 23 ++++++++++++++++++-----
1 file changed, 18 insertions(+), 5 deletions(-)
From: Jeff Kirsher <hidden> Date: 2017-06-06 08:58:16
From: Jacob Keller <jacob.e.keller@intel.com>
The e1000e driver and related hardware has a limitation on Tx PTP
packets which requires we limit to timestamping a single packet at once.
We do this by verifying that we never request a new Tx timestamp while
we still have a tx_hwtstamp_skb pointer.
Unfortunately the driver suffers from a race condition around this. The
tx_hwtstamp_skb pointer is not set to NULL until after skb_tstamp_tx()
is called. This function notifies the stack and applications of a new
timestamp. Even a well behaved application that only sends a new request
when the first one is finished might be woken up and possibly send
a packet before we can free the timestamp in the driver again. The
result is that we needlessly ignore some Tx timestamp requests in this
corner case.
Fix this by assigning the tx_hwtstamp_skb pointer prior to calling
skb_tstamp_tx() and use a temporary pointer to hold the timestamped skb
until that function finishes. This ensures that the application is not
woken up until the driver is ready to begin timestamping a new packet.
This ensures that well behaved applications do not accidentally race
with condition to skip Tx timestamps. Obviously an application which
sends multiple Tx timestamp requests at once will still only timestamp
one packet at a time. Unfortunately there is nothing we can do about
this.
Reported-by: David Mirabito <redacted>
Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
Tested-by: Aaron Brown <redacted>
Signed-off-by: Jeff Kirsher <redacted>
---
drivers/net/ethernet/intel/e1000e/netdev.c | 10 ++++++++--
1 file changed, 8 insertions(+), 2 deletions(-)
@@ -1191,9 +1192,14 @@ static void e1000e_tx_hwtstamp_work(struct work_struct *work)e1000e_systim_to_hwtstamp(adapter,&shhwtstamps,txstmp);-skb_tstamp_tx(adapter->tx_hwtstamp_skb,&shhwtstamps);-dev_kfree_skb_any(adapter->tx_hwtstamp_skb);+/* Clear the global tx_hwtstamp_skb pointer and force writes+*priortonotifyingthestackofaTxtimestamp.+*/adapter->tx_hwtstamp_skb=NULL;+wmb();/* force write prior to skb_tstamp_tx */++skb_tstamp_tx(skb,&shhwtstamps);+dev_kfree_skb_any(skb);}elseif(time_after(jiffies,adapter->tx_hwtstamp_start+adapter->tx_timeout_factor*HZ)){dev_kfree_skb_any(adapter->tx_hwtstamp_skb);
From: Jeff Kirsher <hidden> Date: 2017-06-06 08:58:16
From: Jacob Keller <jacob.e.keller@intel.com>
Hardware related to the igb driver has a limitation of only handling one
Tx timestamp at a time. Thus, the driver uses a state bit lock to
enforce that only one timestamp request is honored at a time.
Unfortunately this suffers from a simple race condition. The bit lock is
not cleared until after skb_tstamp_tx() is called notifying the stack of
a new Tx timestamp. Even a well behaved application which sends only one
timestamp request at once and waits for a response might wake up and
send a new packet before the bit lock is cleared. This results in
needlessly dropping some Tx timestamp requests.
We can fix this by unlocking the state bit as soon as we read the
Timestamp register, as this is the first point at which it is safe to
unlock.
To avoid issues with the skb pointer, we'll use a copy of the pointer
and set the global variable in the driver structure to NULL first. This
ensures that the next timestamp request does not modify our local copy
of the skb pointer.
This ensures that well behaved applications do not accidentally race
with the unlock bit. Obviously an application which sends multiple Tx
timestamp requests at once will still only timestamp one packet at
a time. Unfortunately there is nothing we can do about this.
Reported-by: David Mirabito <redacted>
Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
Tested-by: Aaron Brown <redacted>
Signed-off-by: Jeff Kirsher <redacted>
---
drivers/net/ethernet/intel/igb/igb_ptp.c | 12 ++++++++++--
1 file changed, 10 insertions(+), 2 deletions(-)
@@ -748,10 +749,17 @@ static void igb_ptp_tx_hwtstamp(struct igb_adapter *adapter)shhwtstamps.hwtstamp=ktime_add_ns(shhwtstamps.hwtstamp,adjust);-skb_tstamp_tx(adapter->ptp_tx_skb,&shhwtstamps);-dev_kfree_skb_any(adapter->ptp_tx_skb);+/* Clear the lock early before calling skb_tstamp_tx so that+*applicationsarenotwokenupbeforethelockbitisclear.Weuse+*acopyoftheskbpointertoensureotherthreadscan'tchangeit+*whilewe'renotifyingthestack.+*/adapter->ptp_tx_skb=NULL;clear_bit_unlock(__IGB_PTP_TX_IN_PROGRESS,&adapter->state);++/* Notify the stack and free the skb after we've unlocked */+skb_tstamp_tx(skb,&shhwtstamps);+dev_kfree_skb_any(skb);}/**
From: Jeff Kirsher <hidden> Date: 2017-06-06 08:58:16
From: Jacob Keller <jacob.e.keller@intel.com>
The e1000e driver can only handle one Tx timestamp request at a time.
This means it is possible for an application timestamp request to be
ignored.
There is no easy way for an administrator to determine if this occurred.
Add a new statistic which tracks this, tx_hwtstamp_skipped.
Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
Tested-by: Aaron Brown <redacted>
Signed-off-by: Jeff Kirsher <redacted>
---
drivers/net/ethernet/intel/e1000e/e1000.h | 1 +
drivers/net/ethernet/intel/e1000e/ethtool.c | 1 +
drivers/net/ethernet/intel/e1000e/netdev.c | 17 ++++++++++-------
3 files changed, 12 insertions(+), 7 deletions(-)
From: Jeff Kirsher <hidden> Date: 2017-06-06 08:58:16
From: Jacob Keller <jacob.e.keller@intel.com>
The igb driver can only handle one Tx timestamp request at a time.
This means it is possible for an application timestamp request to be
ignored.
There is no easy way for an administrator to determine if this occurred.
Add a new statistic which tracks this, tx_hwtstamp_skipped.
Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
Tested-by: Aaron Brown <redacted>
Signed-off-by: Jeff Kirsher <redacted>
---
drivers/net/ethernet/intel/igb/igb.h | 1 +
drivers/net/ethernet/intel/igb/igb_ethtool.c | 1 +
drivers/net/ethernet/intel/igb/igb_main.c | 2 ++
3 files changed, 4 insertions(+)
From: Jeff Kirsher <hidden> Date: 2017-06-06 08:58:17
From: Konstantin Khlebnikov <redacted>
Replace disable_irq() which waits for threaded irq handlers with
disable_hardirq() which waits only for hardirq part.
Fixes: 311191297125 ("e1000: use disable_hardirq() for e1000_netpoll()")
Signed-off-by: Konstantin Khlebnikov <redacted>
Acked-by: Cong Wang <redacted>
Tested-by: Aaron Brown <redacted>
Signed-off-by: Jeff Kirsher <redacted>
---
drivers/net/ethernet/intel/e1000e/netdev.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
From: Jeff Kirsher <hidden> Date: 2017-06-06 08:58:17
From: Jacob Keller <jacob.e.keller@intel.com>
The igb driver has logic to handle only one Tx timestamp at a time,
using a state bit lock to avoid multiple requests at once.
It may be possible, if incredibly unlikely, that a Tx timestamp event is
requested but never completes. Since we use an interrupt scheme to
determine when the Tx timestamp occurred we would never clear the state
bit in this case.
Add an igb_ptp_tx_hang() function similar to the already existing
igb_ptp_rx_hang() function. This function runs in the watchdog routine
and makes sure we eventually recover from this case instead of
permanently disabling Tx timestamps.
Note: there is no currently known way to cause this without hacking the
driver code to force it.
Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
Tested-by: Aaron Brown <redacted>
Signed-off-by: Jeff Kirsher <redacted>
---
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 +++++++++++++++++++++++++++++
3 files changed, 31 insertions(+)
@@ -712,6 +712,35 @@ void igb_ptp_rx_hang(struct igb_adapter *adapter)}/**+*igb_ptp_tx_hang-detecterrorcasewhereTxtimestampneverfinishes+*@adapter:privatenetworkadapterstructure+*/+voidigb_ptp_tx_hang(structigb_adapter*adapter)+{+booltimeout=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+*reasonabletoassumethatitwillneveroccur,sowecanunlockthe+*timestampbitwhenthisoccurs.+*/+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-utilityfunctionwhichchecksforTXtimestamp*@adapter:Boardprivatestructure.*
From: Jeff Kirsher <hidden> Date: 2017-06-06 08:58:17
From: Benjamin Poirier <redacted>
Given that all callers of igb_update_stats() pass the same two arguments:
(adapter, &adapter->stats64), the second argument can be removed.
Signed-off-by: Benjamin Poirier <redacted>
Tested-by: Aaron Brown <redacted>
Signed-off-by: Jeff Kirsher <redacted>
---
drivers/net/ethernet/intel/igb/igb.h | 2 +-
drivers/net/ethernet/intel/igb/igb_ethtool.c | 2 +-
drivers/net/ethernet/intel/igb/igb_main.c | 10 +++++-----
3 files changed, 7 insertions(+), 7 deletions(-)
@@ -1818,7 +1818,7 @@ void igb_down(struct igb_adapter *adapter)/* record the stats before reset*/spin_lock(&adapter->stats64_lock);-igb_update_stats(adapter,&adapter->stats64);+igb_update_stats(adapter);spin_unlock(&adapter->stats64_lock);adapter->link_speed=0;
From: Jeff Kirsher <hidden> Date: 2017-06-06 08:58:17
From: Benjamin Poirier <redacted>
Some statistics passed to ethtool are garbage because e1000e_get_stats64()
doesn't write them, for example: tx_heartbeat_errors. This leaks kernel
memory to userspace and confuses users.
Do like ixgbe and use dev_get_stats() which first zeroes out
rtnl_link_stats64.
Fixes: 5944701df90d ("net: remove useless memset's in drivers get_stats64")
Reported-by: Stefan Priebe <redacted>
Signed-off-by: Benjamin Poirier <redacted>
Tested-by: Aaron Brown <redacted>
Signed-off-by: Jeff Kirsher <redacted>
---
drivers/net/ethernet/intel/e1000e/ethtool.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)