[net-next 0/7] Intel Wired LAN Driver Updates

[net-next 0/7] Intel Wired LAN Driver Updates

[Aaron Brown]

This series contains add Live Error Recovery (LER) support to the ixgbe
driver. This support also improves behavior in Thunderbolt environments.
This involves checking all register reads for a value of all ones and when
that is seen, to read the status register, which should never properly
return all ones, to confirm whether the received value was correct. When
this detects a removal, the hw_addr field is cleared to indicate the
removal.  This then blocks subsequent access to the device registers.

All register access macros have been changed to static inline functions
and all register accesses now use them.

The __IXGBE_DOWN bit is no longer overloaded to also mean that device
removal has been initiated. Now the bit can be used to protect ixgbe_down
from multiple entry via test_and_set_bit. A needed smp_mb__before_clear_bit
was also added.

Mark Rustad (7):
  1/7 ixbge: Protect ixgbe_down with __IXGBE_DOWN bit
  2/7 ixgbe: Indicate removal state explicitly
  3/7 ixgbe: Use static inlines instead of macros
  4/7 ixgbe: Make ethtool register test use accessors
  5/7 ixgbe: Check register reads for adapter removal
  6/7 ixgbe: Check for adapter removal on register writes
  7/7 ixgbe: Additional adapter removal checks

 drivers/net/ethernet/intel/ixgbe/ixgbe.h         |   7 ++
 drivers/net/ethernet/intel/ixgbe/ixgbe_common.h  |  55 +++++++++---
 drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c | 110 +++++++++++++----------
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c    |  74 ++++++++++++---
 drivers/net/ethernet/intel/ixgbe/ixgbe_mbx.c     |   3 +-
 drivers/net/ethernet/intel/ixgbe/ixgbe_phy.c     |   2 +-
 6 files changed, 180 insertions(+), 71 deletions(-)

-- 
1.8.5.GIT

[net-next 1/7] ixbge: Protect ixgbe_down with __IXGBE_DOWN bit

[Aaron Brown]

From: Mark Rustad <mark.d.rustad@intel.com>

The ixgbe_down function can now prevent multiple executions by
doing test_and_set_bit on __IXGBE_DOWN. This did not work before
introduction of the __IXGBE_REMOVED bit, because of overloading
of __IXGBE_DOWN. Also add smp_mb__before_clear_bit call before
clearing the __IXGBE_DOWN bit.

Signed-off-by: Mark Rustad <mark.d.rustad@intel.com>
Tested-by: Phil Schmitt <phillip.j.schmitt@intel.com>
Signed-off-by: Aaron Brown <aaron.f.brown@intel.com>
---
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
index cc06854..90a0b4a 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
@@ -4572,6 +4572,7 @@ static void ixgbe_up_complete(struct ixgbe_adapter *adapter)
 	if (hw->mac.ops.enable_tx_laser)
 		hw->mac.ops.enable_tx_laser(hw);
 
+	smp_mb__before_clear_bit();
 	clear_bit(__IXGBE_DOWN, &adapter->state);
 	ixgbe_napi_enable_all(adapter);
 
@@ -4783,7 +4784,8 @@ void ixgbe_down(struct ixgbe_adapter *adapter)
 	int i;
 
 	/* signal that we are down to the interrupt handler */
-	set_bit(__IXGBE_DOWN, &adapter->state);
+	if (test_and_set_bit(__IXGBE_DOWN, &adapter->state))
+		return; /* do nothing if already down */
 
 	/* disable receives */
 	rxctrl = IXGBE_READ_REG(hw, IXGBE_RXCTRL);
-- 
1.8.5.GIT

[net-next 2/7] ixgbe: Indicate removal state explicitly

[Aaron Brown]

From: Mark Rustad <mark.d.rustad@intel.com>

Add a bit, __IXGBE_REMOVE, to indicate that the module is being
removed. The __IXGBE_DOWN bit had been overloaded for this purpose,
but that leads to trouble. A few places now check both __IXGBE_DOWN
and __IXGBE_REMOVE. Notably, setting either bit will prevent service
task execution.

Signed-off-by: Mark Rustad <mark.d.rustad@intel.com>
Tested-by: Phil Schmitt <phillip.j.schmitt@intel.com>
Signed-off-by: Aaron Brown <aaron.f.brown@intel.com>
---
 drivers/net/ethernet/intel/ixgbe/ixgbe.h      |  1 +
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 12 ++++++++----
 2 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe.h b/drivers/net/ethernet/intel/ixgbe/ixgbe.h
index 49531cd..8da263a 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe.h
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe.h
@@ -798,6 +798,7 @@ enum ixgbe_state_t {
 	__IXGBE_TESTING,
 	__IXGBE_RESETTING,
 	__IXGBE_DOWN,
+	__IXGBE_REMOVE,
 	__IXGBE_SERVICE_SCHED,
 	__IXGBE_IN_SFP_INIT,
 	__IXGBE_PTP_RUNNING,
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
index 90a0b4a..923b0fa 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
@@ -278,6 +278,7 @@ static void ixgbe_check_minimum_link(struct ixgbe_adapter *adapter,
 static void ixgbe_service_event_schedule(struct ixgbe_adapter *adapter)
 {
 	if (!test_bit(__IXGBE_DOWN, &adapter->state) &&
+	    !test_bit(__IXGBE_REMOVE, &adapter->state) &&
 	    !test_and_set_bit(__IXGBE_SERVICE_SCHED, &adapter->state))
 		schedule_work(&adapter->service_task);
 }
@@ -5876,8 +5877,9 @@ static void ixgbe_check_hang_subtask(struct ixgbe_adapter *adapter)
 	u64 eics = 0;
 	int i;
 
-	/* If we're down or resetting, just bail */
+	/* If we're down, removing or resetting, just bail */
 	if (test_bit(__IXGBE_DOWN, &adapter->state) ||
+	    test_bit(__IXGBE_REMOVE, &adapter->state) ||
 	    test_bit(__IXGBE_RESETTING, &adapter->state))
 		return;
 
@@ -6124,8 +6126,9 @@ static void ixgbe_spoof_check(struct ixgbe_adapter *adapter)
  **/
 static void ixgbe_watchdog_subtask(struct ixgbe_adapter *adapter)
 {
-	/* if interface is down do nothing */
+	/* if interface is down, removing or resetting, do nothing */
 	if (test_bit(__IXGBE_DOWN, &adapter->state) ||
+	    test_bit(__IXGBE_REMOVE, &adapter->state) ||
 	    test_bit(__IXGBE_RESETTING, &adapter->state))
 		return;
 
@@ -6343,8 +6346,9 @@ static void ixgbe_reset_subtask(struct ixgbe_adapter *adapter)
 
 	adapter->flags2 &= ~IXGBE_FLAG2_RESET_REQUESTED;
 
-	/* If we're already down or resetting, just bail */
+	/* If we're already down, removing or resetting, just bail */
 	if (test_bit(__IXGBE_DOWN, &adapter->state) ||
+	    test_bit(__IXGBE_REMOVE, &adapter->state) ||
 	    test_bit(__IXGBE_RESETTING, &adapter->state))
 		return;
 
@@ -8219,7 +8223,7 @@ static void ixgbe_remove(struct pci_dev *pdev)
 
 	ixgbe_dbg_adapter_exit(adapter);
 
-	set_bit(__IXGBE_DOWN, &adapter->state);
+	set_bit(__IXGBE_REMOVE, &adapter->state);
 	cancel_work_sync(&adapter->service_task);
 
 
-- 
1.8.5.GIT

Re: [net-next 2/7] ixgbe: Indicate removal state explicitly

[Scott Feldman]

On Jan 7, 2014, at 11:40 PM, Aaron Brown <aaron.f.brown@intel.com> wrote:

> From: Mark Rustad <mark.d.rustad@intel.com>
> 
> Add a bit, __IXGBE_REMOVE, to indicate that the module is being
> removed. The __IXGBE_DOWN bit had been overloaded for this purpose,
> but that leads to trouble. A few places now check both __IXGBE_DOWN
> and __IXGBE_REMOVE. Notably, setting either bit will prevent service
> task execution.
> 
> Signed-off-by: Mark Rustad <mark.d.rustad@intel.com>
> Tested-by: Phil Schmitt <phillip.j.schmitt@intel.com>
> Signed-off-by: Aaron Brown <aaron.f.brown@intel.com>
> ---
> drivers/net/ethernet/intel/ixgbe/ixgbe.h      |  1 +
> drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 12 ++++++++----
> 2 files changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe.h b/drivers/net/ethernet/intel/ixgbe/ixgbe.h
> index 49531cd..8da263a 100644
> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe.h
> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe.h
> @@ -798,6 +798,7 @@ enum ixgbe_state_t {
> 	__IXGBE_TESTING,
> 	__IXGBE_RESETTING,
> 	__IXGBE_DOWN,
> +	__IXGBE_REMOVE,

__IXGBE_REMOVING?  More consistent with _TESTING, _RESETTING, etc.

Re: [net-next 2/7] ixgbe: Indicate removal state explicitly

[Rustad, Mark D]

[-- Attachment #1: Type: text/plain, Size: 2029 bytes --]

On Jan 8, 2014, at 12:35 AM, Scott Feldman <sfeldma@gmail.com> wrote:

> On Jan 7, 2014, at 11:40 PM, Aaron Brown <aaron.f.brown@intel.com> wrote:
> 
>> From: Mark Rustad <mark.d.rustad@intel.com>
>> 
>> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_common.h b/drivers/net/ethernet/intel/ixgbe/ixgbe_common.h
>> index 5e157ac..480c5c1 100644
>> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_common.h
>> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_common.h
>> @@ -124,6 +124,11 @@ s32 ixgbe_reset_pipeline_82599(struct ixgbe_hw *hw);
>> s32 ixgbe_get_thermal_sensor_data_generic(struct ixgbe_hw *hw);
>> s32 ixgbe_init_thermal_sensor_thresh_generic(struct ixgbe_hw *hw);
>> 
>> +#define IXGBE_FAILED_READ_REG 0xffffffffU
>> +
>> +void ixgbe_check_remove(struct ixgbe_hw *hw, u32 reg);
>> +#define IXGBE_REMOVED(a) unlikely(!(a))
> 
> IXGBE_REMOVED seems pretty closely tied to hw->hw_addr, but the macro turns any input into !input.  Maybe an inline that takes a *hw?

An earlier version had two different inputs, but not any more. I will change it to take a hw parameter. Good suggestion.

<snip>

>> @@ -2970,7 +2999,7 @@ void ixgbe_configure_tx_ring(struct ixgbe_adapter *adapter,
>> 			ring->count * sizeof(union ixgbe_adv_tx_desc));
>> 	IXGBE_WRITE_REG(hw, IXGBE_TDH(reg_idx), 0);
>> 	IXGBE_WRITE_REG(hw, IXGBE_TDT(reg_idx), 0);
>> -	ring->tail = hw->hw_addr + IXGBE_TDT(reg_idx);
>> +	ring->tail = adapter->io_addr + IXGBE_TDT(reg_idx);
> 
> How is this related to the commit msg about register reads?  Seems like two patches.

No, it is theoretically possible for hw_addr to be NULL now if a removal was somehow detected, so using the new io_addr that will never be NULL is safer. The possibility of hw_addr being NULL is introduced by these patches, so this change should be a part of these patches.

<snip>

Sorry for the duplicate Scott. You may have noticed that I failed to copy everyone else as I should have.

-- 
Mark Rustad, Networking Division, Intel Corporation


[-- Attachment #2: Message signed with OpenPGP using GPGMail --]
[-- Type: application/pgp-signature, Size: 841 bytes --]

Re: [net-next 2/7] ixgbe: Indicate removal state explicitly

[Rustad, Mark D]

[-- Attachment #1: Type: text/plain, Size: 1915 bytes --]

On Jan 9, 2014, at 9:27 AM, Rustad, Mark D <mark.d.rustad@intel.com> wrote:

> On Jan 8, 2014, at 12:35 AM, Scott Feldman <sfeldma@gmail.com> wrote:
> 
>> On Jan 7, 2014, at 11:40 PM, Aaron Brown <aaron.f.brown@intel.com> wrote:
>> 
>>> From: Mark Rustad <mark.d.rustad@intel.com>
>>> 
>>> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_common.h b/drivers/net/ethernet/intel/ixgbe/ixgbe_common.h
>>> index 5e157ac..480c5c1 100644
>>> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_common.h
>>> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_common.h
>>> @@ -124,6 +124,11 @@ s32 ixgbe_reset_pipeline_82599(struct ixgbe_hw *hw);
>>> s32 ixgbe_get_thermal_sensor_data_generic(struct ixgbe_hw *hw);
>>> s32 ixgbe_init_thermal_sensor_thresh_generic(struct ixgbe_hw *hw);
>>> 
>>> +#define IXGBE_FAILED_READ_REG 0xffffffffU
>>> +
>>> +void ixgbe_check_remove(struct ixgbe_hw *hw, u32 reg);
>>> +#define IXGBE_REMOVED(a) unlikely(!(a))
>> 
>> IXGBE_REMOVED seems pretty closely tied to hw->hw_addr, but the macro turns any input into !input.  Maybe an inline that takes a *hw?
> 
> An earlier version had two different inputs, but not any more. I will change it to take a hw parameter. Good suggestion.

Actually, I responded too quickly. Although my comment on there previously having been two different inputs was true, that isn't the real reason for this form. The reason is that the check needs to be done on the value retrieved by ACCESS_ONCE - the address that will be used for the access. If the parameter were hw, then a separate dereference could occur, which is definitely not wanted.

One could argue that the macro should just be eliminated and the expansion put everywhere, but I feel that the code is much clearer with the macro. Unless there is another suggestion, I think this should stay as it is in this series.

-- 
Mark Rustad, Networking Division, Intel Corporation


[-- Attachment #2: Message signed with OpenPGP using GPGMail --]
[-- Type: application/pgp-signature, Size: 841 bytes --]

Re: [net-next 2/7] ixgbe: Indicate removal state explicitly

[Rustad, Mark D]

[-- Attachment #1: Type: text/plain, Size: 1461 bytes --]

On Jan 8, 2014, at 12:37 AM, Scott Feldman <sfeldma@gmail.com> wrote:

> 
> On Jan 7, 2014, at 11:40 PM, Aaron Brown <aaron.f.brown@intel.com> wrote:
> 
>> From: Mark Rustad <mark.d.rustad@intel.com>
>> 
>> Add a bit, __IXGBE_REMOVE, to indicate that the module is being
>> removed. The __IXGBE_DOWN bit had been overloaded for this purpose,
>> but that leads to trouble. A few places now check both __IXGBE_DOWN
>> and __IXGBE_REMOVE. Notably, setting either bit will prevent service
>> task execution.
>> 
>> Signed-off-by: Mark Rustad <mark.d.rustad@intel.com>
>> Tested-by: Phil Schmitt <phillip.j.schmitt@intel.com>
>> Signed-off-by: Aaron Brown <aaron.f.brown@intel.com>
>> ---
>> drivers/net/ethernet/intel/ixgbe/ixgbe.h      |  1 +
>> drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 12 ++++++++----
>> 2 files changed, 9 insertions(+), 4 deletions(-)
>> 
>> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe.h b/drivers/net/ethernet/intel/ixgbe/ixgbe.h
>> index 49531cd..8da263a 100644
>> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe.h
>> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe.h
>> @@ -798,6 +798,7 @@ enum ixgbe_state_t {
>> 	__IXGBE_TESTING,
>> 	__IXGBE_RESETTING,
>> 	__IXGBE_DOWN,
>> +	__IXGBE_REMOVE,
> 
> __IXGBE_REMOVING?  More consistent with _TESTING, _RESETTING, etc.

And more correct in actual meaning as well. Another good suggestion.

-- 
Mark Rustad, Networking Division, Intel Corporation


[-- Attachment #2: Message signed with OpenPGP using GPGMail --]
[-- Type: application/pgp-signature, Size: 841 bytes --]

[net-next 3/7] ixgbe: Use static inlines instead of macros

[Aaron Brown]

From: Mark Rustad <mark.d.rustad@intel.com>

Kernel coding standard prefers static inline functions instead
of macros, so use them for register accessors. This is to prepare
for adding LER, Live Error Recovery, checks to those accessors.

Signed-off-by: Mark Rustad <mark.d.rustad@intel.com>
Tested-by: Phil Schmitt <phillip.j.schmitt@intel.com>
Signed-off-by: Aaron Brown <aaron.f.brown@intel.com>
---
 drivers/net/ethernet/intel/ixgbe/ixgbe.h        |  5 +++++
 drivers/net/ethernet/intel/ixgbe/ixgbe_common.h | 30 +++++++++++++++++--------
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c   |  4 ++--
 3 files changed, 28 insertions(+), 11 deletions(-)

diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe.h b/drivers/net/ethernet/intel/ixgbe/ixgbe.h
index 8da263a..199cf74 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe.h
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe.h
@@ -585,6 +585,11 @@ static inline u16 ixgbe_desc_unused(struct ixgbe_ring *ring)
 	return ((ntc > ntu) ? 0 : ring->count) + ntc - ntu - 1;
 }
 
+static inline void ixgbe_write_tail(struct ixgbe_ring *ring, u32 value)
+{
+	writel(value, ring->tail);
+}
+
 #define IXGBE_RX_DESC(R, i)	    \
 	(&(((union ixgbe_adv_rx_desc *)((R)->desc))[i]))
 #define IXGBE_TX_DESC(R, i)	    \
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_common.h b/drivers/net/ethernet/intel/ixgbe/ixgbe_common.h
index d259dc7..5e157ac 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_common.h
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_common.h
@@ -124,22 +124,34 @@ s32 ixgbe_reset_pipeline_82599(struct ixgbe_hw *hw);
 s32 ixgbe_get_thermal_sensor_data_generic(struct ixgbe_hw *hw);
 s32 ixgbe_init_thermal_sensor_thresh_generic(struct ixgbe_hw *hw);
 
-#define IXGBE_WRITE_REG(a, reg, value) writel((value), ((a)->hw_addr + (reg)))
+static inline void IXGBE_WRITE_REG(struct ixgbe_hw *hw, u32 reg, u32 value)
+{
+	writel(value, hw->hw_addr + reg);
+}
 
 #ifndef writeq
-#define writeq(val, addr) writel((u32) (val), addr); \
-    writel((u32) (val >> 32), (addr + 4));
+static inline void writeq(u64 val, void __iomem *addr)
+{
+	writel((u32)val, addr);
+	writel((u32)(val >> 32), addr + 4);
+}
 #endif
 
-#define IXGBE_WRITE_REG64(a, reg, value) writeq((value), ((a)->hw_addr + (reg)))
+static inline void IXGBE_WRITE_REG64(struct ixgbe_hw *hw, u32 reg, u64 value)
+{
+	writeq(value, hw->hw_addr + reg);
+}
 
-#define IXGBE_READ_REG(a, reg) readl((a)->hw_addr + (reg))
+static inline u32 IXGBE_READ_REG(struct ixgbe_hw *hw, u32 reg)
+{
+	return readl(hw->hw_addr + reg);
+}
 
-#define IXGBE_WRITE_REG_ARRAY(a, reg, offset, value) (\
-    writel((value), ((a)->hw_addr + (reg) + ((offset) << 2))))
+#define IXGBE_WRITE_REG_ARRAY(a, reg, offset, value) \
+		IXGBE_WRITE_REG((a), (reg) + ((offset) << 2), (value))
 
-#define IXGBE_READ_REG_ARRAY(a, reg, offset) (\
-    readl((a)->hw_addr + (reg) + ((offset) << 2)))
+#define IXGBE_READ_REG_ARRAY(a, reg, offset) \
+		IXGBE_READ_REG((a), (reg) + ((offset) << 2))
 
 #define IXGBE_WRITE_FLUSH(a) IXGBE_READ_REG(a, IXGBE_STATUS)
 
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
index 923b0fa..4d71277 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
@@ -1315,7 +1315,7 @@ static inline void ixgbe_release_rx_desc(struct ixgbe_ring *rx_ring, u32 val)
 	 * such as IA-64).
 	 */
 	wmb();
-	writel(val, rx_ring->tail);
+	ixgbe_write_tail(rx_ring, val);
 }
 
 static bool ixgbe_alloc_mapped_page(struct ixgbe_ring *rx_ring,
@@ -6699,7 +6699,7 @@ static void ixgbe_tx_map(struct ixgbe_ring *tx_ring,
 	tx_ring->next_to_use = i;
 
 	/* notify HW of packet */
-	writel(i, tx_ring->tail);
+	ixgbe_write_tail(tx_ring, i);
 
 	return;
 dma_error:
-- 
1.8.5.GIT

Re: [net-next 3/7] ixgbe: Use static inlines instead of macros

[Scott Feldman]

On Jan 7, 2014, at 11:40 PM, Aaron Brown <aaron.f.brown@intel.com> wrote:

> From: Mark Rustad <mark.d.rustad@intel.com>
> 
> -#define IXGBE_WRITE_REG(a, reg, value) writel((value), ((a)->hw_addr + (reg)))
> +static inline void IXGBE_WRITE_REG(struct ixgbe_hw *hw, u32 reg, u32 value)

Bummer, now you have a all-caps func name.

Re: [net-next 3/7] ixgbe: Use static inlines instead of macros

[Rustad, Mark D]

[-- Attachment #1: Type: text/plain, Size: 864 bytes --]

On Jan 8, 2014, at 12:47 AM, Scott Feldman <sfeldma@cumulusnetworks.com> wrote:

> 
> On Jan 7, 2014, at 11:40 PM, Aaron Brown <aaron.f.brown@intel.com> wrote:
> 
>> From: Mark Rustad <mark.d.rustad@intel.com>
>> 
>> -#define IXGBE_WRITE_REG(a, reg, value) writel((value), ((a)->hw_addr + (reg)))
>> +static inline void IXGBE_WRITE_REG(struct ixgbe_hw *hw, u32 reg, u32 value)
> 
> Bummer, now you have a all-caps func name.

Agreed, but this is actually a fairly common condition among drivers that used to use macros. It isn't perfect, but at least it is moving in the right direction. I'd rather leave the case change for a later patch series that does only that or has some reason to touch all of the register access sites.

At least the new accessor I introduced is lower case. :-)

-- 
Mark Rustad, Networking Division, Intel Corporation


[-- Attachment #2: Message signed with OpenPGP using GPGMail --]
[-- Type: application/pgp-signature, Size: 841 bytes --]

Re: [net-next 3/7] ixgbe: Use static inlines instead of macros

[David Miller]

From: "Rustad, Mark D" <mark.d.rustad@intel.com>
Date: Thu, 9 Jan 2014 17:34:18 +0000

> On Jan 8, 2014, at 12:47 AM, Scott Feldman <sfeldma@cumulusnetworks.com> wrote:
> 
>> 
>> On Jan 7, 2014, at 11:40 PM, Aaron Brown <aaron.f.brown@intel.com> wrote:
>> 
>>> From: Mark Rustad <mark.d.rustad@intel.com>
>>> 
>>> -#define IXGBE_WRITE_REG(a, reg, value) writel((value), ((a)->hw_addr + (reg)))
>>> +static inline void IXGBE_WRITE_REG(struct ixgbe_hw *hw, u32 reg, u32 value)
>> 
>> Bummer, now you have a all-caps func name.
> 
> Agreed, but this is actually a fairly common condition among drivers that used to use macros. It isn't perfect, but at least it is moving in the right direction. I'd rather leave the case change for a later patch series that does only that or has some reason to touch all of the register access sites.
> 
> At least the new accessor I introduced is lower case. :-)

Please address this feedback, all caps function names are really not
appropriate.

Re: [net-next 3/7] ixgbe: Use static inlines instead of macros

[Rustad, Mark D]

[-- Attachment #1: Type: text/plain, Size: 1984 bytes --]

On Jan 9, 2014, at 11:39 AM, David Miller <davem@davemloft.net> wrote:

> From: "Rustad, Mark D" <mark.d.rustad@intel.com>
> Date: Thu, 9 Jan 2014 17:34:18 +0000
> 
>> On Jan 8, 2014, at 12:47 AM, Scott Feldman <sfeldma@cumulusnetworks.com> wrote:
>> 
>>> 
>>> On Jan 7, 2014, at 11:40 PM, Aaron Brown <aaron.f.brown@intel.com> wrote:
>>> 
>>>> From: Mark Rustad <mark.d.rustad@intel.com>
>>>> 
>>>> -#define IXGBE_WRITE_REG(a, reg, value) writel((value), ((a)->hw_addr + (reg)))
>>>> +static inline void IXGBE_WRITE_REG(struct ixgbe_hw *hw, u32 reg, u32 value)
>>> 
>>> Bummer, now you have a all-caps func name.
>> 
>> Agreed, but this is actually a fairly common condition among drivers that used to use macros. It isn't perfect, but at least it is moving in the right direction. I'd rather leave the case change for a later patch series that does only that or has some reason to touch all of the register access sites.
>> 
>> At least the new accessor I introduced is lower case. :-)
> 
> Please address this feedback, all caps function names are really not
> appropriate.

I really don't think it is a good idea to do that as part of this patch series. It makes this patch series a pretty solid barrier to any other patches going into this driver because it would change the name of all the register accessors.

This makes me want to deal with that as a separate issue, since there would be no functional reason to drop such a patch and it can be planned into a workflow.

Obviously I could do it here, but I *really* think it is procedurally a really bad idea to change the case as part of a functional change. I thought I was doing a favor my at least making them inlines, but prehaps not.

Anyone want to take on changing the upper case static inlines in mcf8390, 7990, benet, ns83820, s2io, vxge, iwlwifi, ath9k, wil6210, mwifiex, and rtlwifi? And those are just under drivers/net.

-- 
Mark Rustad, Networking Division, Intel Corporation


[-- Attachment #2: Message signed with OpenPGP using GPGMail --]
[-- Type: application/pgp-signature, Size: 841 bytes --]

Re: [net-next 3/7] ixgbe: Use static inlines instead of macros

[David Miller]

From: "Rustad, Mark D" <mark.d.rustad@intel.com>
Date: Thu, 9 Jan 2014 20:14:51 +0000

> Obviously I could do it here, but I *really* think it is
> procedurally a really bad idea to change the case as part of a
> functional change. I thought I was doing a favor my at least making
> them inlines, but prehaps not.

It is never a good idea to allow functions to have all caps
names and vice versa.  Please don't use "difficulty" as a reason
to violate this.

Doing things right is sometimes hard, I'm sorry to inform you :)

> Anyone want to take on changing the upper case static inlines in
> mcf8390, 7990, benet, ns83820, s2io, vxge, iwlwifi, ath9k, wil6210,
> mwifiex, and rtlwifi? And those are just under drivers/net.

Sorry the "crap exists elsewhere, therefore I can make crap too"
argument never holds any water.

Just because crap exists elsewhere, doesn't mean you should duplicate it.

Re: [net-next 3/7] ixgbe: Use static inlines instead of macros

[Rustad, Mark D]

[-- Attachment #1: Type: text/plain, Size: 2210 bytes --]

On Jan 9, 2014, at 12:19 PM, David Miller <davem@davemloft.net> wrote:

> From: "Rustad, Mark D" <mark.d.rustad@intel.com>
> Date: Thu, 9 Jan 2014 20:14:51 +0000

I'm sorry you dropped my entire procedural argument for not combining the changing of the case with the implementation of the static inlines. That really was and is important! I'm not opposed to changing the case. My concern is for procedurally when and how it gets done.

>> Obviously I could do it here, but I *really* think it is
>> procedurally a really bad idea to change the case as part of a
>> functional change. I thought I was doing a favor my at least making
>> them inlines, but prehaps not.
> 
> It is never a good idea to allow functions to have all caps
> names and vice versa.  Please don't use "difficulty" as a reason
> to violate this.
> 
> Doing things right is sometimes hard, I'm sorry to inform you :)

It is just that sometimes things take multiple steps. This was just one step. I'm sorry that I may have not made that clear enough. Doing the work is absolutely not the problem, managing the process so everyone can continue working is a concern.

>> Anyone want to take on changing the upper case static inlines in
>> mcf8390, 7990, benet, ns83820, s2io, vxge, iwlwifi, ath9k, wil6210,
>> mwifiex, and rtlwifi? And those are just under drivers/net.
> 
> Sorry the "crap exists elsewhere, therefore I can make crap too"
> argument never holds any water.
> 
> Just because crap exists elsewhere, doesn't mean you should duplicate it.

I'm sorry that the presence misled me into thinking that it was acceptable at least in a transitional phase.

I think I have found a way to address the barrier issue. I will add upper case macros that call the lower case inlines, so for a time both forms may exist, and then have a patch that will update the call sites, and then a patch to remove the macros.

Is that acceptable?

If only one person were working in this area and everything were serial, it would be much easier to manage this kind of change. When that is not the case, it is better for things to happen in separate steps.

-- 
Mark Rustad, Networking Division, Intel Corporation


[-- Attachment #2: Message signed with OpenPGP using GPGMail --]
[-- Type: application/pgp-signature, Size: 841 bytes --]

Re: [net-next 3/7] ixgbe: Use static inlines instead of macros

[David Miller]

From: "Rustad, Mark D" <mark.d.rustad@intel.com>
Date: Thu, 9 Jan 2014 20:46:33 +0000

> I think I have found a way to address the barrier issue. I will add
> upper case macros that call the lower case inlines, so for a time
> both forms may exist, and then have a patch that will update the
> call sites, and then a patch to remove the macros.
> 
> Is that acceptable?

Yes.

[net-next 4/7] ixgbe: Make ethtool register test use accessors

[Aaron Brown]

From: Mark Rustad <mark.d.rustad@intel.com>

Make the ethtool register test use the normal register accessor
functions. Also eliminate macros used for calling register test
functions to make error exits clearer. Use boolean values for
boolean returns instead of 0 and 1.

Signed-off-by: Mark Rustad <mark.d.rustad@intel.com>
Tested-by: Phil Schmitt <phillip.j.schmitt@intel.com>
Signed-off-by: Aaron Brown <aaron.f.brown@intel.com>
---
 drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c | 88 +++++++++++-------------
 1 file changed, 42 insertions(+), 46 deletions(-)

diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c
index 4e7c9b0..412cb1a 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c
@@ -1343,54 +1343,41 @@ static bool reg_pattern_test(struct ixgbe_adapter *adapter, u64 *data, int reg,
 		0x5A5A5A5A, 0xA5A5A5A5, 0x00000000, 0xFFFFFFFF};
 
 	for (pat = 0; pat < ARRAY_SIZE(test_pattern); pat++) {
-		before = readl(adapter->hw.hw_addr + reg);
-		writel((test_pattern[pat] & write),
-		       (adapter->hw.hw_addr + reg));
-		val = readl(adapter->hw.hw_addr + reg);
+		before = IXGBE_READ_REG(&adapter->hw, reg);
+		IXGBE_WRITE_REG(&adapter->hw, reg, test_pattern[pat] & write);
+		val = IXGBE_READ_REG(&adapter->hw, reg);
 		if (val != (test_pattern[pat] & write & mask)) {
 			e_err(drv, "pattern test reg %04X failed: got "
 			      "0x%08X expected 0x%08X\n",
 			      reg, val, (test_pattern[pat] & write & mask));
 			*data = reg;
-			writel(before, adapter->hw.hw_addr + reg);
-			return 1;
+			IXGBE_WRITE_REG(&adapter->hw, reg, before);
+			return true;
 		}
-		writel(before, adapter->hw.hw_addr + reg);
+		IXGBE_WRITE_REG(&adapter->hw, reg, before);
 	}
-	return 0;
+	return false;
 }
 
 static bool reg_set_and_check(struct ixgbe_adapter *adapter, u64 *data, int reg,
 			      u32 mask, u32 write)
 {
 	u32 val, before;
-	before = readl(adapter->hw.hw_addr + reg);
-	writel((write & mask), (adapter->hw.hw_addr + reg));
-	val = readl(adapter->hw.hw_addr + reg);
+
+	before = IXGBE_READ_REG(&adapter->hw, reg);
+	IXGBE_WRITE_REG(&adapter->hw, reg, write & mask);
+	val = IXGBE_READ_REG(&adapter->hw, reg);
 	if ((write & mask) != (val & mask)) {
 		e_err(drv, "set/check reg %04X test failed: got 0x%08X "
 		      "expected 0x%08X\n", reg, (val & mask), (write & mask));
 		*data = reg;
-		writel(before, (adapter->hw.hw_addr + reg));
-		return 1;
+		IXGBE_WRITE_REG(&adapter->hw, reg, before);
+		return true;
 	}
-	writel(before, (adapter->hw.hw_addr + reg));
-	return 0;
+	IXGBE_WRITE_REG(&adapter->hw, reg, before);
+	return false;
 }
 
-#define REG_PATTERN_TEST(reg, mask, write)				      \
-	do {								      \
-		if (reg_pattern_test(adapter, data, reg, mask, write))	      \
-			return 1;					      \
-	} while (0)							      \
-
-
-#define REG_SET_AND_CHECK(reg, mask, write)				      \
-	do {								      \
-		if (reg_set_and_check(adapter, data, reg, mask, write))	      \
-			return 1;					      \
-	} while (0)							      \
-
 static int ixgbe_reg_test(struct ixgbe_adapter *adapter, u64 *data)
 {
 	const struct ixgbe_reg_test *test;
@@ -1438,38 +1425,47 @@ static int ixgbe_reg_test(struct ixgbe_adapter *adapter, u64 *data)
 	 */
 	while (test->reg) {
 		for (i = 0; i < test->array_len; i++) {
+			bool b = false;
+
 			switch (test->test_type) {
 			case PATTERN_TEST:
-				REG_PATTERN_TEST(test->reg + (i * 0x40),
-						 test->mask,
-						 test->write);
+				b = reg_pattern_test(adapter, data,
+						     test->reg + (i * 0x40),
+						     test->mask,
+						     test->write);
 				break;
 			case SET_READ_TEST:
-				REG_SET_AND_CHECK(test->reg + (i * 0x40),
-						  test->mask,
-						  test->write);
+				b = reg_set_and_check(adapter, data,
+						      test->reg + (i * 0x40),
+						      test->mask,
+						      test->write);
 				break;
 			case WRITE_NO_TEST:
-				writel(test->write,
-				       (adapter->hw.hw_addr + test->reg)
-				       + (i * 0x40));
+				IXGBE_WRITE_REG(&adapter->hw,
+						test->reg + (i * 0x40),
+						test->write);
 				break;
 			case TABLE32_TEST:
-				REG_PATTERN_TEST(test->reg + (i * 4),
-						 test->mask,
-						 test->write);
+				b = reg_pattern_test(adapter, data,
+						     test->reg + (i * 4),
+						     test->mask,
+						     test->write);
 				break;
 			case TABLE64_TEST_LO:
-				REG_PATTERN_TEST(test->reg + (i * 8),
-						 test->mask,
-						 test->write);
+				b = reg_pattern_test(adapter, data,
+						     test->reg + (i * 8),
+						     test->mask,
+						     test->write);
 				break;
 			case TABLE64_TEST_HI:
-				REG_PATTERN_TEST((test->reg + 4) + (i * 8),
-						 test->mask,
-						 test->write);
+				b = reg_pattern_test(adapter, data,
+						     (test->reg + 4) + (i * 8),
+						     test->mask,
+						     test->write);
 				break;
 			}
+			if (b)
+				return 1;
 		}
 		test++;
 	}
-- 
1.8.5.GIT

[net-next 5/7] ixgbe: Check register reads for adapter removal

[Aaron Brown]

From: Mark Rustad <mark.d.rustad@intel.com>

Check all register reads for adapter removal by checking the status
register after any register read that returns 0xFFFFFFFF. Since the
status register will never return 0xFFFFFFFF unless the adapter is
removed, such a value from a status register read confirms the
removal.

Signed-off-by: Mark Rustad <mark.d.rustad@intel.com>
Tested-by: Phil Schmitt <phillip.j.schmitt@intel.com>
Signed-off-by: Aaron Brown <aaron.f.brown@intel.com>
---
 drivers/net/ethernet/intel/ixgbe/ixgbe.h        |  1 +
 drivers/net/ethernet/intel/ixgbe/ixgbe_common.h | 15 +++++++++-
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c   | 38 ++++++++++++++++++++++---
 3 files changed, 49 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe.h b/drivers/net/ethernet/intel/ixgbe/ixgbe.h
index 199cf74..002c9e2 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe.h
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe.h
@@ -747,6 +747,7 @@ struct ixgbe_adapter {
 #ifdef IXGBE_FCOE
 	struct ixgbe_fcoe fcoe;
 #endif /* IXGBE_FCOE */
+	u8 __iomem *io_addr; /* Mainly for iounmap use */
 	u32 wol;
 
 	u16 bd_number;
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_common.h b/drivers/net/ethernet/intel/ixgbe/ixgbe_common.h
index 5e157ac..480c5c1 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_common.h
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_common.h
@@ -124,6 +124,11 @@ s32 ixgbe_reset_pipeline_82599(struct ixgbe_hw *hw);
 s32 ixgbe_get_thermal_sensor_data_generic(struct ixgbe_hw *hw);
 s32 ixgbe_init_thermal_sensor_thresh_generic(struct ixgbe_hw *hw);
 
+#define IXGBE_FAILED_READ_REG 0xffffffffU
+
+void ixgbe_check_remove(struct ixgbe_hw *hw, u32 reg);
+#define IXGBE_REMOVED(a) unlikely(!(a))
+
 static inline void IXGBE_WRITE_REG(struct ixgbe_hw *hw, u32 reg, u32 value)
 {
 	writel(value, hw->hw_addr + reg);
@@ -144,7 +149,15 @@ static inline void IXGBE_WRITE_REG64(struct ixgbe_hw *hw, u32 reg, u64 value)
 
 static inline u32 IXGBE_READ_REG(struct ixgbe_hw *hw, u32 reg)
 {
-	return readl(hw->hw_addr + reg);
+	u8 __iomem *reg_addr = ACCESS_ONCE(hw->hw_addr);
+	u32 value;
+
+	if (IXGBE_REMOVED(reg_addr))
+		return IXGBE_FAILED_READ_REG;
+	value = readl(reg_addr + reg);
+	if (unlikely(value == IXGBE_FAILED_READ_REG))
+		ixgbe_check_remove(hw, reg);
+	return value;
 }
 
 #define IXGBE_WRITE_REG_ARRAY(a, reg, offset, value) \
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
index 4d71277..896a8b0 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
@@ -283,6 +283,35 @@ static void ixgbe_service_event_schedule(struct ixgbe_adapter *adapter)
 		schedule_work(&adapter->service_task);
 }
 
+static void ixgbe_remove_adapter(struct ixgbe_hw *hw)
+{
+	struct ixgbe_adapter *adapter = hw->back;
+
+	if (!hw->hw_addr)
+		return;
+	hw->hw_addr = NULL;
+	e_dev_err("Adapter removed\n");
+}
+
+void ixgbe_check_remove(struct ixgbe_hw *hw, u32 reg)
+{
+	u32 value;
+
+	/* The following check not only optimizes a bit by not
+	 * performing a read on the status register when the
+	 * register just read was a status register read that
+	 * returned IXGBE_FAILED_READ_REG. It also blocks any
+	 * potential recursion.
+	 */
+	if (reg == IXGBE_STATUS) {
+		ixgbe_remove_adapter(hw);
+		return;
+	}
+	value = IXGBE_READ_REG(hw, IXGBE_STATUS);
+	if (value == IXGBE_FAILED_READ_REG)
+		ixgbe_remove_adapter(hw);
+}
+
 static void ixgbe_service_event_complete(struct ixgbe_adapter *adapter)
 {
 	BUG_ON(!test_bit(__IXGBE_SERVICE_SCHED, &adapter->state));
@@ -2970,7 +2999,7 @@ void ixgbe_configure_tx_ring(struct ixgbe_adapter *adapter,
 			ring->count * sizeof(union ixgbe_adv_tx_desc));
 	IXGBE_WRITE_REG(hw, IXGBE_TDH(reg_idx), 0);
 	IXGBE_WRITE_REG(hw, IXGBE_TDT(reg_idx), 0);
-	ring->tail = hw->hw_addr + IXGBE_TDT(reg_idx);
+	ring->tail = adapter->io_addr + IXGBE_TDT(reg_idx);
 
 	/*
 	 * set WTHRESH to encourage burst writeback, it should not be set
@@ -3373,7 +3402,7 @@ void ixgbe_configure_rx_ring(struct ixgbe_adapter *adapter,
 			ring->count * sizeof(union ixgbe_adv_rx_desc));
 	IXGBE_WRITE_REG(hw, IXGBE_RDH(reg_idx), 0);
 	IXGBE_WRITE_REG(hw, IXGBE_RDT(reg_idx), 0);
-	ring->tail = hw->hw_addr + IXGBE_RDT(reg_idx);
+	ring->tail = adapter->io_addr + IXGBE_RDT(reg_idx);
 
 	ixgbe_configure_srrctl(adapter, ring);
 	ixgbe_configure_rscctl(adapter, ring);
@@ -7887,6 +7916,7 @@ static int ixgbe_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 
 	hw->hw_addr = ioremap(pci_resource_start(pdev, 0),
 			      pci_resource_len(pdev, 0));
+	adapter->io_addr = hw->hw_addr;
 	if (!hw->hw_addr) {
 		err = -EIO;
 		goto err_ioremap;
@@ -8195,7 +8225,7 @@ err_register:
 err_sw_init:
 	ixgbe_disable_sriov(adapter);
 	adapter->flags2 &= ~IXGBE_FLAG2_SEARCH_FOR_SFP;
-	iounmap(hw->hw_addr);
+	iounmap(adapter->io_addr);
 err_ioremap:
 	free_netdev(netdev);
 err_alloc_etherdev:
@@ -8262,7 +8292,7 @@ static void ixgbe_remove(struct pci_dev *pdev)
 	kfree(adapter->ixgbe_ieee_ets);
 
 #endif
-	iounmap(adapter->hw.hw_addr);
+	iounmap(adapter->io_addr);
 	pci_release_selected_regions(pdev, pci_select_bars(pdev,
 				     IORESOURCE_MEM));
 
-- 
1.8.5.GIT

Re: [net-next 5/7] ixgbe: Check register reads for adapter removal

[Scott Feldman]

On Jan 7, 2014, at 11:40 PM, Aaron Brown <aaron.f.brown@intel.com> wrote:

> From: Mark Rustad <mark.d.rustad@intel.com>
> 
> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_common.h b/drivers/net/ethernet/intel/ixgbe/ixgbe_common.h
> index 5e157ac..480c5c1 100644
> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_common.h
> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_common.h
> @@ -124,6 +124,11 @@ s32 ixgbe_reset_pipeline_82599(struct ixgbe_hw *hw);
> s32 ixgbe_get_thermal_sensor_data_generic(struct ixgbe_hw *hw);
> s32 ixgbe_init_thermal_sensor_thresh_generic(struct ixgbe_hw *hw);
> 
> +#define IXGBE_FAILED_READ_REG 0xffffffffU
> +
> +void ixgbe_check_remove(struct ixgbe_hw *hw, u32 reg);
> +#define IXGBE_REMOVED(a) unlikely(!(a))

IXGBE_REMOVED seems pretty closely tied to hw->hw_addr, but the macro turns any input into !input.  Maybe an inline that takes a *hw?

> +
> static inline void IXGBE_WRITE_REG(struct ixgbe_hw *hw, u32 reg, u32 value)
> {
> 	writel(value, hw->hw_addr + reg);
> @@ -144,7 +149,15 @@ static inline void IXGBE_WRITE_REG64(struct ixgbe_hw *hw, u32 reg, u64 value)
> 
> static inline u32 IXGBE_READ_REG(struct ixgbe_hw *hw, u32 reg)
> {
> -	return readl(hw->hw_addr + reg);
> +	u8 __iomem *reg_addr = ACCESS_ONCE(hw->hw_addr);
> +	u32 value;
> +
> +	if (IXGBE_REMOVED(reg_addr))
> +		return IXGBE_FAILED_READ_REG;
> +	value = readl(reg_addr + reg);
> +	if (unlikely(value == IXGBE_FAILED_READ_REG))
> +		ixgbe_check_remove(hw, reg);
> +	return value;
> }
> 
> #define IXGBE_WRITE_REG_ARRAY(a, reg, offset, value) \
> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> index 4d71277..896a8b0 100644
> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> @@ -283,6 +283,35 @@ static void ixgbe_service_event_schedule(struct ixgbe_adapter *adapter)
> 		schedule_work(&adapter->service_task);
> }
> 
> +static void ixgbe_remove_adapter(struct ixgbe_hw *hw)
> +{
> +	struct ixgbe_adapter *adapter = hw->back;
> +
> +	if (!hw->hw_addr)
> +		return;
> +	hw->hw_addr = NULL;
> +	e_dev_err("Adapter removed\n");
> +}
> +
> +void ixgbe_check_remove(struct ixgbe_hw *hw, u32 reg)
> +{
> +	u32 value;
> +
> +	/* The following check not only optimizes a bit by not
> +	 * performing a read on the status register when the
> +	 * register just read was a status register read that
> +	 * returned IXGBE_FAILED_READ_REG. It also blocks any
> +	 * potential recursion.
> +	 */
> +	if (reg == IXGBE_STATUS) {
> +		ixgbe_remove_adapter(hw);
> +		return;
> +	}
> +	value = IXGBE_READ_REG(hw, IXGBE_STATUS);
> +	if (value == IXGBE_FAILED_READ_REG)
> +		ixgbe_remove_adapter(hw);
> +}
> +
> static void ixgbe_service_event_complete(struct ixgbe_adapter *adapter)
> {
> 	BUG_ON(!test_bit(__IXGBE_SERVICE_SCHED, &adapter->state));
> @@ -2970,7 +2999,7 @@ void ixgbe_configure_tx_ring(struct ixgbe_adapter *adapter,
> 			ring->count * sizeof(union ixgbe_adv_tx_desc));
> 	IXGBE_WRITE_REG(hw, IXGBE_TDH(reg_idx), 0);
> 	IXGBE_WRITE_REG(hw, IXGBE_TDT(reg_idx), 0);
> -	ring->tail = hw->hw_addr + IXGBE_TDT(reg_idx);
> +	ring->tail = adapter->io_addr + IXGBE_TDT(reg_idx);

How is this related to the commit msg about register reads?  Seems like two patches.

> 
> 	/*
> 	 * set WTHRESH to encourage burst writeback, it should not be set
> @@ -3373,7 +3402,7 @@ void ixgbe_configure_rx_ring(struct ixgbe_adapter *adapter,
> 			ring->count * sizeof(union ixgbe_adv_rx_desc));
> 	IXGBE_WRITE_REG(hw, IXGBE_RDH(reg_idx), 0);
> 	IXGBE_WRITE_REG(hw, IXGBE_RDT(reg_idx), 0);
> -	ring->tail = hw->hw_addr + IXGBE_RDT(reg_idx);
> +	ring->tail = adapter->io_addr + IXGBE_RDT(reg_idx);
> 
> 	ixgbe_configure_srrctl(adapter, ring);
> 	ixgbe_configure_rscctl(adapter, ring);
> @@ -7887,6 +7916,7 @@ static int ixgbe_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
> 
> 	hw->hw_addr = ioremap(pci_resource_start(pdev, 0),
> 			      pci_resource_len(pdev, 0));
> +	adapter->io_addr = hw->hw_addr;
> 	if (!hw->hw_addr) {
> 		err = -EIO;
> 		goto err_ioremap;
> @@ -8195,7 +8225,7 @@ err_register:
> err_sw_init:
> 	ixgbe_disable_sriov(adapter);
> 	adapter->flags2 &= ~IXGBE_FLAG2_SEARCH_FOR_SFP;
> -	iounmap(hw->hw_addr);
> +	iounmap(adapter->io_addr);
> err_ioremap:
> 	free_netdev(netdev);
> err_alloc_etherdev:
> @@ -8262,7 +8292,7 @@ static void ixgbe_remove(struct pci_dev *pdev)
> 	kfree(adapter->ixgbe_ieee_ets);
> 
> #endif
> -	iounmap(adapter->hw.hw_addr);
> +	iounmap(adapter->io_addr);
> 	pci_release_selected_regions(pdev, pci_select_bars(pdev,
> 				     IORESOURCE_MEM));
> 
> -- 
> 1.8.5.GIT
> 
> --
> 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

[net-next 6/7] ixgbe: Check for adapter removal on register writes

[Aaron Brown]

From: Mark Rustad <mark.d.rustad@intel.com>

Prevent writes to an adapter that has been detected as removed
by a previous failing read. This also fixes some include file
ordering confusion that this patch revealed.

Signed-off-by: Mark Rustad <mark.d.rustad@intel.com>
Tested-by: Phil Schmitt <phillip.j.schmitt@intel.com>
Signed-off-by: Aaron Brown <aaron.f.brown@intel.com>
---
 drivers/net/ethernet/intel/ixgbe/ixgbe_common.h | 12 ++++++++++--
 drivers/net/ethernet/intel/ixgbe/ixgbe_mbx.c    |  3 +--
 drivers/net/ethernet/intel/ixgbe/ixgbe_phy.c    |  2 +-
 3 files changed, 12 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_common.h b/drivers/net/ethernet/intel/ixgbe/ixgbe_common.h
index 480c5c1..5320586 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_common.h
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_common.h
@@ -131,7 +131,11 @@ void ixgbe_check_remove(struct ixgbe_hw *hw, u32 reg);
 
 static inline void IXGBE_WRITE_REG(struct ixgbe_hw *hw, u32 reg, u32 value)
 {
-	writel(value, hw->hw_addr + reg);
+	u8 __iomem *reg_addr = ACCESS_ONCE(hw->hw_addr);
+
+	if (IXGBE_REMOVED(reg_addr))
+		return;
+	writel(value, reg_addr + reg);
 }
 
 #ifndef writeq
@@ -144,7 +148,11 @@ static inline void writeq(u64 val, void __iomem *addr)
 
 static inline void IXGBE_WRITE_REG64(struct ixgbe_hw *hw, u32 reg, u64 value)
 {
-	writeq(value, hw->hw_addr + reg);
+	u8 __iomem *reg_addr = ACCESS_ONCE(hw->hw_addr);
+
+	if (IXGBE_REMOVED(reg_addr))
+		return;
+	writeq(value, reg_addr + reg);
 }
 
 static inline u32 IXGBE_READ_REG(struct ixgbe_hw *hw, u32 reg)
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_mbx.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_mbx.c
index d4a64e6..cc3101a 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_mbx.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_mbx.c
@@ -27,8 +27,7 @@
 
 #include <linux/pci.h>
 #include <linux/delay.h>
-#include "ixgbe_type.h"
-#include "ixgbe_common.h"
+#include "ixgbe.h"
 #include "ixgbe_mbx.h"
 
 /**
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_phy.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_phy.c
index 39217e5..132557c 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_phy.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_phy.c
@@ -29,7 +29,7 @@
 #include <linux/delay.h>
 #include <linux/sched.h>
 
-#include "ixgbe_common.h"
+#include "ixgbe.h"
 #include "ixgbe_phy.h"
 
 static void ixgbe_i2c_start(struct ixgbe_hw *hw);
-- 
1.8.5.GIT

[net-next 7/7] ixgbe: Additional adapter removal checks

[Aaron Brown]

From: Mark Rustad <mark.d.rustad@intel.com>

Additional checks are needed for a detected removal not to cause
problems. Some involve simply avoiding a lot of stuff that can't
do anything good, and also cases where the phony return value can
cause problems. In addition, down the adapter when the removal is
sensed.

Signed-off-by: Mark Rustad <mark.d.rustad@intel.com>
Tested-by: Phil Schmitt <phillip.j.schmitt@intel.com>
Signed-off-by: Aaron Brown <aaron.f.brown@intel.com>
---
 drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c | 22 ++++++++++++++++++++++
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c    | 16 ++++++++++++++++
 2 files changed, 38 insertions(+)

diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c
index 412cb1a..0899b1d 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c
@@ -1342,6 +1342,10 @@ static bool reg_pattern_test(struct ixgbe_adapter *adapter, u64 *data, int reg,
 	static const u32 test_pattern[] = {
 		0x5A5A5A5A, 0xA5A5A5A5, 0x00000000, 0xFFFFFFFF};
 
+	if (IXGBE_REMOVED(adapter->hw.hw_addr)) {
+		*data = 1;
+		return 1;
+	}
 	for (pat = 0; pat < ARRAY_SIZE(test_pattern); pat++) {
 		before = IXGBE_READ_REG(&adapter->hw, reg);
 		IXGBE_WRITE_REG(&adapter->hw, reg, test_pattern[pat] & write);
@@ -1364,6 +1368,10 @@ static bool reg_set_and_check(struct ixgbe_adapter *adapter, u64 *data, int reg,
 {
 	u32 val, before;
 
+	if (IXGBE_REMOVED(adapter->hw.hw_addr)) {
+		*data = 1;
+		return 1;
+	}
 	before = IXGBE_READ_REG(&adapter->hw, reg);
 	IXGBE_WRITE_REG(&adapter->hw, reg, write & mask);
 	val = IXGBE_READ_REG(&adapter->hw, reg);
@@ -1384,6 +1392,11 @@ static int ixgbe_reg_test(struct ixgbe_adapter *adapter, u64 *data)
 	u32 value, before, after;
 	u32 i, toggle;
 
+	if (IXGBE_REMOVED(adapter->hw.hw_addr)) {
+		e_err(drv, "Adapter removed - register test blocked\n");
+		*data = 1;
+		return 1;
+	}
 	switch (adapter->hw.mac.type) {
 	case ixgbe_mac_82598EB:
 		toggle = 0x7FFFF3FF;
@@ -1950,6 +1963,15 @@ static void ixgbe_diag_test(struct net_device *netdev,
 	struct ixgbe_adapter *adapter = netdev_priv(netdev);
 	bool if_running = netif_running(netdev);
 
+	if (IXGBE_REMOVED(adapter->hw.hw_addr)) {
+		e_err(hw, "Adapter removed - test blocked\n");
+		data[0] = 1;
+		data[1] = 1;
+		data[2] = 1;
+		data[3] = 1;
+		eth_test->flags |= ETH_TEST_FL_FAILED;
+		return;
+	}
 	set_bit(__IXGBE_TESTING, &adapter->state);
 	if (eth_test->flags == ETH_TEST_FL_OFFLINE) {
 		struct ixgbe_hw *hw = &adapter->hw;
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
index 896a8b0..364df2c 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
@@ -291,6 +291,7 @@ static void ixgbe_remove_adapter(struct ixgbe_hw *hw)
 		return;
 	hw->hw_addr = NULL;
 	e_dev_err("Adapter removed\n");
+	ixgbe_service_event_schedule(adapter);
 }
 
 void ixgbe_check_remove(struct ixgbe_hw *hw, u32 reg)
@@ -3338,6 +3339,8 @@ static void ixgbe_rx_desc_queue_enable(struct ixgbe_adapter *adapter,
 	u32 rxdctl;
 	u8 reg_idx = ring->reg_idx;
 
+	if (IXGBE_REMOVED(hw->hw_addr))
+		return;
 	/* RXDCTL.EN will return 0 on 82598 if link is down, so skip it */
 	if (hw->mac.type == ixgbe_mac_82598EB &&
 	    !(IXGBE_READ_REG(hw, IXGBE_LINKS) & IXGBE_LINKS_UP))
@@ -3362,6 +3365,8 @@ void ixgbe_disable_rx_queue(struct ixgbe_adapter *adapter,
 	u32 rxdctl;
 	u8 reg_idx = ring->reg_idx;
 
+	if (IXGBE_REMOVED(hw->hw_addr))
+		return;
 	rxdctl = IXGBE_READ_REG(hw, IXGBE_RXDCTL(reg_idx));
 	rxdctl &= ~IXGBE_RXDCTL_ENABLE;
 
@@ -4687,6 +4692,8 @@ void ixgbe_reset(struct ixgbe_adapter *adapter)
 	struct ixgbe_hw *hw = &adapter->hw;
 	int err;
 
+	if (IXGBE_REMOVED(hw->hw_addr))
+		return;
 	/* lock SFP init bit to prevent race conditions with the watchdog */
 	while (test_and_set_bit(__IXGBE_IN_SFP_INIT, &adapter->state))
 		usleep_range(1000, 2000);
@@ -6397,6 +6404,15 @@ static void ixgbe_service_task(struct work_struct *work)
 	struct ixgbe_adapter *adapter = container_of(work,
 						     struct ixgbe_adapter,
 						     service_task);
+	if (IXGBE_REMOVED(adapter->hw.hw_addr)) {
+		if (!test_bit(__IXGBE_DOWN, &adapter->state)) {
+			rtnl_lock();
+			ixgbe_down(adapter);
+			rtnl_unlock();
+		}
+		ixgbe_service_event_complete(adapter);
+		return;
+	}
 	ixgbe_reset_subtask(adapter);
 	ixgbe_sfp_detection_subtask(adapter);
 	ixgbe_sfp_link_config_subtask(adapter);
-- 
1.8.5.GIT