i40e: use the safe hash table iterator when deleting mac filters

-----Original Message-----
From: Intel-wired-lan [mailto:intel-wired-lan-bounces at osuosl.org] On Behalf Of Alice Michael
Sent: Thursday, September 7, 2017 5:06 AM
To: Michael, Alice <[email protected]>; intel-wired-
lan at lists.osuosl.org
Subject: [Intel-wired-lan] [next PATCH S80-V3 02/11] i40evf: fix mac filter removal timing issue

From: Alan Brady <[email protected]>

Due to the asynchronous nature in which mac filters are added and deleted, there exists a bug in which filters are erroneously removed if removed then added again quickly.

The events are as such:
- filter marked for removal
- same filter is re-added before watchdog that cleans up filters
- we skip re-adding the filter because we have it already in the list
- watchdog filter cleanup kicks off and filter is removed

So when we were re-adding the same filter, it didn't actually get added because it already existed in the list, but was marked for removal and had yet to actually be removed.

This patch fixes the issue by making sure that when adding a filter, if we find it already existing in our list, make sure it is not marked to be removed.

Signed-off-by Alan Brady <[email protected]>

---
 drivers/net/ethernet/intel/i40evf/i40evf_main.c | 2 ++
 1 file changed, 2 insertions(+)

-----Original Message-----
From: Intel-wired-lan [mailto:intel-wired-lan-bounces at osuosl.org] On Behalf Of Alice Michael
Sent: Thursday, September 7, 2017 5:06 AM
To: Michael, Alice <[email protected]>; intel-wired- lan at lists.osuosl.org
Subject: [Intel-wired-lan] [next PATCH S80-V3 03/11] i40e/i40evf: fix incorrect default ITR values on driver load

From: Jacob Keller <[email protected]>

The ITR register expects to be programmed in units of 2 microseconds. Because of this, all of the drivers I40E_ITR_* constants are in terms of this 2 microsecond register.

Unfortunately, the rx_itr_default value is expected to be programmed in microseconds.

Effectively the driver defaults to an ITR value of half the expected value (in terms of minimum microseconds between interrupts).

Fix this by changing the default values to be calculated using ITR_REG_TO_USEC macro which indicates that we're converting from the register units into microseconds.

Signed-off-by Jacob Keller <[email protected]>

---
 drivers/net/ethernet/intel/i40e/i40e_main.c     | 4 ++--
 drivers/net/ethernet/intel/i40e/i40e_txrx.h     | 6 ++++--
 drivers/net/ethernet/intel/i40evf/i40e_txrx.h   | 6 ++++--
 drivers/net/ethernet/intel/i40evf/i40evf_main.c | 4 ++--
 4 files changed, 12 insertions(+), 8 deletions(-)

-----Original Message-----
From: Intel-wired-lan [mailto:intel-wired-lan-bounces at osuosl.org] On Behalf Of Alice Michael
Sent: Thursday, September 7, 2017 5:06 AM
To: Michael, Alice <[email protected]>; intel-wired-
lan at lists.osuosl.org
Subject: [Intel-wired-lan] [next PATCH S80-V3 04/11] i40e/i40evf: always set the CLEARPBA flag when re-enabling interrupts

From: Jacob Keller <[email protected]>

In the past we changed driver behavior to not clear the PBA when re- enabling interrupts. This change was motivated by the flawed belief that clearing the PBA would cause a lost interrupt if a receive interrupt occurred while interrupts were disabled.

According to empirical testing this isn't the case. Additionally, the data sheet specifically says that we should set the CLEARPBA bit when re-enabling interrupts in a polling setup.

This reverts commit 40d72a509862 ("i40e/i40evf: don't lose interrupts")

Signed-off-by Jacob Keller <[email protected]>

---
 drivers/net/ethernet/intel/i40e/i40e.h             |  5 +----
 drivers/net/ethernet/intel/i40e/i40e_main.c        | 11 +++++------
 drivers/net/ethernet/intel/i40e/i40e_txrx.c        |  6 ++----
 drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c |  2 +-
 drivers/net/ethernet/intel/i40evf/i40e_txrx.c      |  4 +---
 5 files changed, 10 insertions(+), 18 deletions(-)

Quoted 22 lines

-----Original Message-----
From: Intel-wired-lan [mailto:intel-wired-lan-bounces at osuosl.org] On Behalf Of Alice Michael
Sent: Thursday, September 7, 2017 5:06 AM
To: Michael, Alice <[email protected]>; intel-wired-
lan at lists.osuosl.org
Subject: [Intel-wired-lan] [next PATCH S80-V3 04/11] i40e/i40evf: always set the CLEARPBA flag when re-enabling interrupts

From: Jacob Keller <[email protected]>

In the past we changed driver behavior to not clear the PBA when re- enabling interrupts. This change was motivated by the flawed belief that clearing the PBA would cause a lost interrupt if a receive interrupt occurred while interrupts were disabled.

According to empirical testing this isn't the case. Additionally, the data sheet specifically says that we should set the CLEARPBA bit when re-enabling interrupts in a polling setup.

This reverts commit 40d72a509862 ("i40e/i40evf: don't lose interrupts")

Quoted 8 lines

Signed-off-by Jacob Keller <[email protected]>

---
 drivers/net/ethernet/intel/i40e/i40e.h             |  5 +----
 drivers/net/ethernet/intel/i40e/i40e_main.c        | 11 +++++------
 drivers/net/ethernet/intel/i40e/i40e_txrx.c        |  6 ++----
 drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c |  2 +-
 drivers/net/ethernet/intel/i40evf/i40e_txrx.c      |  4 +---
 5 files changed, 10 insertions(+), 18 deletions(-)

Tested-by Andrew Bowers <[email protected]>

-----Original Message-----
From: Duyck, Alexander H
Sent: Thursday, October 05, 2017 11:32 AM
To: Keller, Jacob E <[email protected]>
Cc: Bowers, AndrewX <[email protected]>; intel-wired-
lan at lists.osuosl.org
Subject: Re: [Intel-wired-lan] [next PATCH S80-V3 04/11] i40e/i40evf: always set the CLEARPBA flag when re-enabling interrupts

Quoted 31 lines · AndrewX

Quoted 22 lines

-----Original Message-----
From: Intel-wired-lan [mailto:intel-wired-lan-bounces at osuosl.org] On Behalf Of Alice Michael
Sent: Thursday, September 7, 2017 5:06 AM
To: Michael, Alice <[email protected]>; intel-wired-
lan at lists.osuosl.org
Subject: [Intel-wired-lan] [next PATCH S80-V3 04/11] i40e/i40evf: always set the CLEARPBA flag when re-enabling interrupts

From: Jacob Keller <[email protected]>

In the past we changed driver behavior to not clear the PBA when re- enabling interrupts. This change was motivated by the flawed belief that clearing the PBA would cause a lost interrupt if a receive interrupt occurred while interrupts were disabled.

According to empirical testing this isn't the case. Additionally, the data sheet specifically says that we should set the CLEARPBA bit when re-enabling interrupts in a polling setup.

This reverts commit 40d72a509862 ("i40e/i40evf: don't lose interrupts")

Quoted 8 lines

Signed-off-by Jacob Keller <[email protected]>

---
 drivers/net/ethernet/intel/i40e/i40e.h             |  5 +----
 drivers/net/ethernet/intel/i40e/i40e_main.c        | 11 +++++------
 drivers/net/ethernet/intel/i40e/i40e_txrx.c        |  6 ++----
 drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c |  2 +-
 drivers/net/ethernet/intel/i40evf/i40e_txrx.c      |  4 +---
 5 files changed, 10 insertions(+), 18 deletions(-)

Quoted 2 lines

Tested-by Andrew Bowers <[email protected]>

This patch just got pointed out to me in a hallway conversation.

I am pretty sure this _will_ cause you to lose interrupts. Specifically you shouldn't be clearing the PBA bit at the end of the polling routine unless you know you are going to poll again.

The PBA bit should only be cleared if:
1. You are at the start of your clean-up routine and want to clear it in case any additional work has come in since the original vector fired. (currently handled via an auto-clear function for MSI-X)
2. You are at the end of your polling routine and you know you are going to be polling again.

The original patch was more aggressive than it needed to be. For example you could probably go ahead and clear the PBA in the i40e_intr()q interrupt routine itself since all it is doing is scheduling the polling routine which will run after the interrupt routine has completed. You shouldn't be clearing the PBA at the end of NAPI poll unless you know you are not going to be exiting polling.

I hope this helps to clarify things.

- Alex

Quoted 7 lines

The original patch was more aggressive than it needed to be. For example you could probably go ahead and clear the PBA in the i40e_intr()q interrupt routine itself since all it is doing is scheduling the polling routine which will run after the interrupt routine has completed. You shouldn't be clearing the PBA at the end of NAPI poll unless you know you are not going to be exiting polling.

Quoted 3 lines

I hope this helps to clarify things.

- Alex

Ok so only clear it if we're continuing to poll. I'll cook up a patch for that.

-----Original Message-----
From: Brandeburg, Jesse
Sent: Thursday, October 05, 2017 4:01 PM
To: Keller, Jacob E <[email protected]>
Cc: Duyck, Alexander H <[email protected]>; intel-wired- lan at lists.osuosl.org; Brandeburg, Jesse <[email protected]>
Subject: Re: [Intel-wired-lan] [next PATCH S80-V3 04/11] i40e/i40evf: always set the CLEARPBA flag when re-enabling interrupts

Quoted 7 lines · Jacob E

Quoted 7 lines

The original patch was more aggressive than it needed to be. For example you could probably go ahead and clear the PBA in the i40e_intr()q interrupt routine itself since all it is doing is scheduling the polling routine which will run after the interrupt routine has completed. You shouldn't be clearing the PBA at the end of NAPI poll unless you know you are not going to be exiting polling.

Quoted 5 lines

Quoted 3 lines

I hope this helps to clarify things.

- Alex

Ok so only clear it if we're continuing to poll. I'll cook up a patch for that.

Hi Jake, we may just need to cook up a better test for this instead of generating further patches. I believe based on updated understanding of the hardware functionality that this patch is correct, but it is difficult to prove and we should probably spend more time doing the actual proof.

At least this patch follow the actual directions in the hardware manual, so I think we should stick with it.

Jesse

-----Original Message-----
From: Intel-wired-lan [mailto:intel-wired-lan-bounces at osuosl.org] On Behalf Of Alice Michael
Sent: Thursday, September 7, 2017 5:06 AM
To: Michael, Alice <[email protected]>; intel-wired-
lan at lists.osuosl.org
Subject: [Intel-wired-lan] [next PATCH S80-V3 05/11] i40e: reduce lrxqthresh from 2 to 1

From: Jacob Keller <[email protected]>

The lrxq thresh value tells hardware to immediately interrupt when there are fewer than N*64 packets left in the ring.

Counter intuitively, empirical testing has shown that decreasing this value from 2 to 1, and thus changing from an immediate interrupt at fewer than 128 descriptors down to 64 descriptors causes a small increase in the maximum total packets per second we can receive. This increase occurs even when we're polling with interrupts masked, as the hardware must still handle interrupts internally even if we've disabled them in software.

Also reduce the value for any VFs we allocate.

Signed-off-by Jacob Keller <[email protected]>

---
 drivers/net/ethernet/intel/i40e/i40e_main.c        | 2 +-
 drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

-----Original Message-----
From: Intel-wired-lan [mailto:intel-wired-lan-bounces at osuosl.org] On Behalf Of Alice Michael
Sent: Thursday, September 7, 2017 5:06 AM
To: Michael, Alice <[email protected]>; intel-wired-
lan at lists.osuosl.org
Subject: [Intel-wired-lan] [next PATCH S80-V3 06/11] i40e/i40evf: bump tail only in multiples of 8

From: Jacob Keller <[email protected]>

Hardware only fetches descriptors on cachelines of 8, essentially ignoring the lower 3 bits of the tail register. Thus, it is pointless to bump tail by an unaligned access as the hardware will ignore some of the new descriptors we allocated. Thus, it's ideal if we can ensure tail writes are always aligned to 8.

At first, it seems like we'd already do this, since we allocate descriptors in batches which are a multiple of 8. Since we'd always increment by a multiple of 8, it seems like the value should always be aligned.

However, this ignores allocation failures. If we fail to allocate a buffer, our tail register will become unaligned. Once it has become unaligned it will essentially be stuck unaligned until a buffer allocation happens to fail at the exact amount necessary to re-align it.

We can do better, by simply rounding down the number of buffers we're about to allocate (cleaned_count) such that "next_to_clean
+ cleaned_count" is rounded to the nearest multiple of 8.

We do this by calculating how far off that value is and subtracting it from the cleaned_count. This essentially defers allocation of buffers if they're going to be ignored by hardware anyways, and re-aligns our next_to_use and tail values after a failure to allocate a descriptor.

This calculation ensures that we always align the tail writes in a way the hardware expects and don't unnecessarily allocate buffers which won't be fetched immediately.

Signed-off-by Jacob Keller <[email protected]>

---
 drivers/net/ethernet/intel/i40e/i40e_txrx.c   | 9 +++++++++
 drivers/net/ethernet/intel/i40evf/i40e_txrx.c | 9 +++++++++
 2 files changed, 18 insertions(+)

-----Original Message-----
From: Intel-wired-lan [mailto:intel-wired-lan-bounces at osuosl.org] On Behalf Of Alice Michael
Sent: Thursday, September 7, 2017 5:06 AM
To: Michael, Alice <[email protected]>; intel-wired- lan at lists.osuosl.org
Subject: [Intel-wired-lan] [next PATCH S80-V3 07/11] i40e/i40evf: bundle more descriptors when allocating buffers

From: Jacob Keller <[email protected]>

Double the number of descriptors we'll bundle into one tail bump when receiving. Empirical testing has shown that we reduce CPU utilization and don't appear to reduce throughput or packet rate. 32 seems to be the sweet spot, as it's half the default polling budget, so we'd essentially reduce from 4 tail writes when polling down to 2. Increasing this up to
64 appears to have negative impacts as it may become possible that we don't bump the tail each time we get polled, which could cause a long delay between returning descriptors to the hardware.

Signed-off-by Jacob Keller <[email protected]>

---
 drivers/net/ethernet/intel/i40e/i40e_txrx.h   | 2 +-
 drivers/net/ethernet/intel/i40evf/i40e_txrx.h | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

-----Original Message-----
From: Intel-wired-lan [mailto:intel-wired-lan-bounces at osuosl.org] On Behalf Of Alice Michael
Sent: Thursday, September 7, 2017 5:06 AM
To: Michael, Alice <[email protected]>; intel-wired- lan at lists.osuosl.org
Subject: [Intel-wired-lan] [next PATCH S80-V3 08/11] i40e: allow XPS with QoS enabled

From: Jacob Keller <[email protected]>

Recently, the kernel gained support for enabling XPS and QoS at the same time. Thus, we no longer need to worry about the number of traffic classes when enabling XPS.

Signed-off-by Jacob Keller <[email protected]>

---
 drivers/net/ethernet/intel/i40e/i40e_main.c | 17 ++++++-----------
 1 file changed, 6 insertions(+), 11 deletions(-)

-----Original Message-----
From: Intel-wired-lan [mailto:intel-wired-lan-bounces at osuosl.org] On Behalf Of Alice Michael
Sent: Thursday, September 7, 2017 5:06 AM
To: Michael, Alice <[email protected]>; intel-wired- lan at lists.osuosl.org
Subject: [Intel-wired-lan] [next PATCH S80-V3 09/11] i40e: add check for return from find_first_bit call

From: Lihong Yang <[email protected]>

The find_first_bit function will return the size passed to search if the first set bit is not found. This patch adds the check in case that happens as the return value would be used as the index in an array and that would have caused the out-of-bounds access.

Detected by CoverityScan, CID 1295969 Out-of-bounds access

Signed-off-by Lihong Yang <[email protected]>

---
 drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c | 4 ++++
 1 file changed, 4 insertions(+)

-----Original Message-----
From: Intel-wired-lan [mailto:intel-wired-lan-bounces at osuosl.org] On Behalf Of Alice Michael
Sent: Thursday, September 7, 2017 5:06 AM
To: Michael, Alice <[email protected]>; intel-wired- lan at lists.osuosl.org
Subject: [Intel-wired-lan] [next PATCH S80-V3 10/11] i40e: Retry AQC GetPhyAbilities to overcome I2CRead hangs

From: Jayaprakash Shanmugam <[email protected]>

- When the I2C is busy, the PHY reads are delayed. The firmware will return EGAIN in these cases with an expectation that the SW will trigger the reads again
- This patch retries the operation for a maximum period of 500ms

Signed-off-by: Jayaprakash Shanmugam
<[email protected]>

---
 drivers/net/ethernet/intel/i40e/i40e_common.c | 42
++++++++++++++++++---------
 drivers/net/ethernet/intel/i40e/i40e_type.h   |  3 ++
 drivers/net/ethernet/intel/i40evf/i40e_type.h |  3 ++
 3 files changed, 35 insertions(+), 13 deletions(-)

-----Original Message-----
From: Intel-wired-lan [mailto:intel-wired-lan-bounces at osuosl.org] On Behalf Of Alice Michael
Sent: Thursday, September 7, 2017 5:06 AM
To: Michael, Alice <[email protected]>; intel-wired-
lan at lists.osuosl.org
Subject: [Intel-wired-lan] [next PATCH S80-V3 11/11] i40e: use a local variable instead of calculating multiple times

From: Lihong Yang <[email protected]>

The computed result of I40E_MAX_VSI_QP * I40E_VIRTCHNL_SUPPORTED_QTYPES is used more than three times in function i40e_config_irq_link_list.
Simply declare a local variable to store it to improve readability.

Signed-off-by Lihong Yang <[email protected]>

---
 drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c | 20 +++++++-------------
 1 file changed, 7 insertions(+), 13 deletions(-)

-----Original Message-----
From: Intel-wired-lan [mailto:intel-wired-lan-bounces at osuosl.org] On Behalf Of Alice Michael
Sent: Thursday, September 7, 2017 5:06 AM
To: Michael, Alice <[email protected]>; intel-wired-
lan at lists.osuosl.org
Subject: [Intel-wired-lan] [next PATCH S80-V3 01/11] i40e: use the safe hash table iterator when deleting mac filters

From: Lihong Yang <[email protected]>

This patch replaces hash_for_each function with hash_for_each_safe when calling __i40e_del_filter. The hash_for_each_safe function is the right one to use when iterating over a hash table to safely remove a hash entry. Otherwise, incorrect values may be read from freed memory.

Detected by CoverityScan, CID 1402048 Read from pointer after free

Signed-off-by Lihong Yang <[email protected]>

---
 drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)