[PATCH net-next 0/4][pull request] 40GbE Intel Wired LAN Driver Updates 2021-10-25

This series contains updates to i40e, ice, igb, and ixgbevf drivers.

Caleb Sander adds cond_resched() call to yield CPU, if needed, for long
delayed admin queue calls for i40e.

Yang Li simplifies return statements of bool values for i40e and ice.

Jan Kundrát corrects problems with I2C bit-banging for igb.

Colin Ian King removes unneeded variable initialization for ixgbevf.

The following are changes since commit 3fb59a5de5cbb04de76915d9f5bff01d16aa1fc4:
  net/tls: getsockopt supports complete algorithm list
and are available in the git repository at:
  git://git.kernel.org/pub/scm/linux/kernel/git/tnguy/next-queue 40GbE

Caleb Sander (1):
  i40e: avoid spin loop in i40e_asq_send_command()

Colin Ian King (1):
  net: ixgbevf: Remove redundant initialization of variable ret_val

Jan Kundrát (1):
  igb: unbreak I2C bit-banging on i350

Yang Li (1):
  intel: Simplify bool conversion

 drivers/net/ethernet/intel/i40e/i40e_adminq.c |  6 ++---
 drivers/net/ethernet/intel/i40e/i40e_xsk.c    |  2 +-
 drivers/net/ethernet/intel/ice/ice_xsk.c      |  2 +-
 drivers/net/ethernet/intel/igb/igb_main.c     | 23 ++++++++++++-------
 drivers/net/ethernet/intel/ixgbevf/vf.c       |  2 +-
 5 files changed, 21 insertions(+), 14 deletions(-)

-- 
2.31.1

From: Caleb Sander <redacted>

Previously, the kernel could spend up to 250 ms waiting for a command to
be submitted to an admin queue. This function is also called in a loop,
e.g., in i40e_get_module_eeprom() (through i40e_aq_get_phy_register()),
so the time spent in the kernel may be even higher. We observed
scheduling delays of over 2 seconds in production,
with stacktraces pointing to this code as the culprit.

Add a call to cond_resched() so the loop can yield the CPU.
Also compute the total time using the jiffies counter
instead of assuming udelay() waits the precise time interval requested.

Signed-off-by: Caleb Sander <redacted>
Reviewed-by: Joern Engel <redacted>
Tested-by: Tony Brelinski <redacted>
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
---
 drivers/net/ethernet/intel/i40e/i40e_adminq.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/intel/i40e/i40e_adminq.c b/drivers/net/ethernet/intel/i40e/i40e_adminq.c
index 593912b17609..f1fcb867a999 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_adminq.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_adminq.c

@@ -902,7 +902,7 @@ i40e_status i40e_asq_send_command(struct i40e_hw *hw,
 	 * we need to wait for desc write back
 	 */
 	if (!details->async && !details->postpone) {
-		u32 total_delay = 0;
+		unsigned long timeout_end = jiffies + usecs_to_jiffies(hw->aq.asq_cmd_timeout);
 
 		do {
 			/* AQ designers suggest use of head for better

@@ -910,9 +910,9 @@ i40e_status i40e_asq_send_command(struct i40e_hw *hw,
 			 */
 			if (i40e_asq_done(hw))
 				break;
+			cond_resched();
 			udelay(50);
-			total_delay += 50;
-		} while (total_delay < hw->aq.asq_cmd_timeout);
+		} while (time_before(jiffies, timeout_end));
 	}
 
 	/* if ready, copy the desc back to temp */

-- 
2.31.1

From: Yang Li <redacted>

Fix the following coccicheck warning:
./drivers/net/ethernet/intel/i40e/i40e_xsk.c:229:35-40: WARNING:
conversion to bool not needed here
./drivers/net/ethernet/intel/ice/ice_xsk.c:399:35-40: WARNING:
conversion to bool not needed here

Reported-by: Abaci Robot <redacted>
Signed-off-by: Yang Li <redacted>
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
---
 drivers/net/ethernet/intel/i40e/i40e_xsk.c | 2 +-
 drivers/net/ethernet/intel/ice/ice_xsk.c   | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/intel/i40e/i40e_xsk.c b/drivers/net/ethernet/intel/i40e/i40e_xsk.c
index 6f85879ba993..ea06e957393e 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_xsk.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_xsk.c

@@ -226,7 +226,7 @@ bool i40e_alloc_rx_buffers_zc(struct i40e_ring *rx_ring, u16 count)
 	rx_desc->wb.qword1.status_error_len = 0;
 	i40e_release_rx_desc(rx_ring, ntu);
 
-	return count == nb_buffs ? true : false;
+	return count == nb_buffs;
 }
 
 /**

diff --git a/drivers/net/ethernet/intel/ice/ice_xsk.c b/drivers/net/ethernet/intel/ice/ice_xsk.c
index d9dfcfc2c6f9..ff55cb415b11 100644
--- a/drivers/net/ethernet/intel/ice/ice_xsk.c
+++ b/drivers/net/ethernet/intel/ice/ice_xsk.c

@@ -399,7 +399,7 @@ bool ice_alloc_rx_bufs_zc(struct ice_rx_ring *rx_ring, u16 count)
 	rx_desc->wb.status_error0 = 0;
 	ice_release_rx_desc(rx_ring, ntu);
 
-	return count == nb_buffs ? true : false;
+	return count == nb_buffs;
 }
 
 /**

-- 
2.31.1

From: Colin Ian King <redacted>

The variable ret_val is being initialized with a value that is never
read, it is being updated later on. The assignment is redundant and
can be removed.

Addresses-Coverity: ("Unused value")
Signed-off-by: Colin Ian King <redacted>
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
---
 drivers/net/ethernet/intel/ixgbevf/vf.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/intel/ixgbevf/vf.c b/drivers/net/ethernet/intel/ixgbevf/vf.c
index 5fc347abab3c..d459f5c8e98f 100644
--- a/drivers/net/ethernet/intel/ixgbevf/vf.c
+++ b/drivers/net/ethernet/intel/ixgbevf/vf.c

@@ -66,9 +66,9 @@ static s32 ixgbevf_reset_hw_vf(struct ixgbe_hw *hw)
 {
 	struct ixgbe_mbx_info *mbx = &hw->mbx;
 	u32 timeout = IXGBE_VF_INIT_TIMEOUT;
-	s32 ret_val = IXGBE_ERR_INVALID_MAC_ADDR;
 	u32 msgbuf[IXGBE_VF_PERMADDR_MSG_LEN];
 	u8 *addr = (u8 *)(&msgbuf[1]);
+	s32 ret_val;
 
 	/* Call adapter stop to disable tx/rx and clear interrupts */
 	hw->mac.ops.stop_adapter(hw);

-- 
2.31.1

From: Jan Kundrát <redacted>

The driver tried to use Linux' native software I2C bus master
(i2c-algo-bits) for exporting the I2C interface that talks to the SFP
cage(s) towards userspace. As-is, however, the physical SCL/SDA pins
were not moving at all, staying at logical 1 all the time.

The main culprit was the I2CPARAMS register where igb was not setting
the I2CBB_EN bit. That meant that all the careful signal bit-banging was
actually not being propagated to the chip pads (I verified this with a
scope).

The bit-banging was not correct either, because I2C is supposed to be an
open-collector bus, and the code was driving both lines via a totem
pole. The code was also trying to do operations which did not make any
sense with the i2c-algo-bits, namely manipulating both SDA and SCL from
igb_set_i2c_data (which is only supposed to set SDA). I'm not sure if
that was meant as an optimization, or was just flat out wrong, but given
that the i2c-algo-bits is set up to work with a totally dumb GPIO-ish
implementation underneath, there's no need for this code to be smart.

The open-drain vs. totem-pole is fixed by the usual trick where the
logical zero is implemented via regular output mode and outputting a
logical 0, and the logical high is implemented via the IO pad configured
as an input (thus floating), and letting the mandatory pull-up resistors
do the rest. Anything else is actually wrong on I2C where all devices
are supposed to have open-drain connection to the bus.

The missing I2CBB_EN is set (along with a safe initial value of the
GPIOs) just before registering this software I2C bus.

The chip datasheet mentions HW-implemented I2C transactions (SFP EEPROM
reads and writes) as well, but I'm not touching these for simplicity.

Tested on a LR-Link LRES2203PF-2SFP (which is an almost-miniPCIe form
factor card, a cable, and a module with two SFP cages). There was one
casualty, an old broken SFP we had laying around, which was used to
solder some thin wires as a DIY I2C breakout. Thanks for your service.
With this patch in place, I can `i2cdump -y 3 0x51 c` and read back data
which make sense. Yay.

Signed-off-by: Jan Kundrát <redacted>
See-also: https://www.spinics.net/lists/netdev/msg490554.html
Reviewed-by: Jesse Brandeburg <redacted>
Tested-by: Tony Brelinski <redacted>
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
---
 drivers/net/ethernet/intel/igb/igb_main.c | 23 +++++++++++++++--------
 1 file changed, 15 insertions(+), 8 deletions(-)

diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
index e67a71c3f141..836be0d3b291 100644
--- a/drivers/net/ethernet/intel/igb/igb_main.c
+++ b/drivers/net/ethernet/intel/igb/igb_main.c

@@ -577,16 +577,15 @@ static void igb_set_i2c_data(void *data, int state)
 	struct e1000_hw *hw = &adapter->hw;
 	s32 i2cctl = rd32(E1000_I2CPARAMS);
 
-	if (state)
-		i2cctl |= E1000_I2C_DATA_OUT;
-	else
+	if (state) {
+		i2cctl |= E1000_I2C_DATA_OUT | E1000_I2C_DATA_OE_N;
+	} else {
+		i2cctl &= ~E1000_I2C_DATA_OE_N;
 		i2cctl &= ~E1000_I2C_DATA_OUT;
+	}
 
-	i2cctl &= ~E1000_I2C_DATA_OE_N;
-	i2cctl |= E1000_I2C_CLK_OE_N;
 	wr32(E1000_I2CPARAMS, i2cctl);
 	wrfl();
-
 }
 
 /**

@@ -603,8 +602,7 @@ static void igb_set_i2c_clk(void *data, int state)
 	s32 i2cctl = rd32(E1000_I2CPARAMS);
 
 	if (state) {
-		i2cctl |= E1000_I2C_CLK_OUT;
-		i2cctl &= ~E1000_I2C_CLK_OE_N;
+		i2cctl |= E1000_I2C_CLK_OUT | E1000_I2C_CLK_OE_N;
 	} else {
 		i2cctl &= ~E1000_I2C_CLK_OUT;
 		i2cctl &= ~E1000_I2C_CLK_OE_N;

@@ -3116,12 +3114,21 @@ static void igb_init_mas(struct igb_adapter *adapter)
  **/
 static s32 igb_init_i2c(struct igb_adapter *adapter)
 {
+	struct e1000_hw *hw = &adapter->hw;
 	s32 status = 0;
+	s32 i2cctl;
 
 	/* I2C interface supported on i350 devices */
 	if (adapter->hw.mac.type != e1000_i350)
 		return 0;
 
+	i2cctl = rd32(E1000_I2CPARAMS);
+	i2cctl |= E1000_I2CBB_EN
+		| E1000_I2C_CLK_OUT | E1000_I2C_CLK_OE_N
+		| E1000_I2C_DATA_OUT | E1000_I2C_DATA_OE_N;
+	wr32(E1000_I2CPARAMS, i2cctl);
+	wrfl();
+
 	/* Initialize the i2c bus which is controlled by the registers.
 	 * This bus will use the i2c_algo_bit structure that implements
 	 * the protocol through toggling of the 4 bits in the register.

-- 
2.31.1

On Mon, 2021-10-25 at 10:55 -0700, Tony Nguyen wrote:

This series contains updates to i40e, ice, igb, and ixgbevf drivers.

Caleb Sander adds cond_resched() call to yield CPU, if needed, for
long
delayed admin queue calls for i40e.

Yang Li simplifies return statements of bool values for i40e and ice.

Jan Kundrát corrects problems with I2C bit-banging for igb.

Colin Ian King removes unneeded variable initialization for ixgbevf.

Hi Dave, Jakub,

I'm seeing this in Patchworks as accepted [1], but I'm not seeing the
patches on the tree. Should I resend this pull request?

Thanks,
Tony

[1]
https://patchwork.kernel.org/project/netdevbpf/list/?series=569843&stat
e=*

On Wed, 27 Oct 2021 15:51:58 +0000 Nguyen, Anthony L wrote:

On Mon, 2021-10-25 at 10:55 -0700, Tony Nguyen wrote:

quoted

This series contains updates to i40e, ice, igb, and ixgbevf drivers.

Caleb Sander adds cond_resched() call to yield CPU, if needed, for
long
delayed admin queue calls for i40e.

Yang Li simplifies return statements of bool values for i40e and ice.

Jan Kundrát corrects problems with I2C bit-banging for igb.

Colin Ian King removes unneeded variable initialization for ixgbevf.  

I'm seeing this in Patchworks as accepted [1], but I'm not seeing the
patches on the tree. Should I resend this pull request?

Sorry about that, let me take a look.

On Mon, 25 Oct 2021 10:55:05 -0700 Tony Nguyen wrote:

+			cond_resched();
 			udelay(50);

Why not switch to usleep_range() if we can sleep here?

On Mon, 25 Oct 2021 10:55:07 -0700 Tony Nguyen wrote:

From: Jan Kundrát <redacted>

The driver tried to use Linux' native software I2C bus master
(i2c-algo-bits) for exporting the I2C interface that talks to the SFP
cage(s) towards userspace. As-is, however, the physical SCL/SDA pins
were not moving at all, staying at logical 1 all the time.

So targeting net-next because this never worked?

On Wed, Oct 27, 2021 at 09:01:03AM -0700, Jakub Kicinski wrote:

On Mon, 25 Oct 2021 10:55:05 -0700 Tony Nguyen wrote:

quoted

+			cond_resched();
 			udelay(50);

Why not switch to usleep_range() if we can sleep here?

Looking at usleep_range() vs. udelay(), I wonder if there is still a
hidden reason to prefer udelay().  Basically, if you typically want
short delays like the 50µs above, going to sleep will often result in
much longer delays, 1ms or higher.  I can easily see situations where
multiple calls to udelay(50) are fine while multiple calls to
usleep_range() will cause timeouts.

Is that a known problem and do we have good heuristics when to prefer
one over the other?

Jörn

--
Audacity augments courage; hesitation, fear.
-- Publilius Syrus

On Thu, 28 Oct 2021 04:49:03 -0700 Jörn Engel wrote:

On Wed, Oct 27, 2021 at 09:01:03AM -0700, Jakub Kicinski wrote:

quoted

On Mon, 25 Oct 2021 10:55:05 -0700 Tony Nguyen wrote:  

quoted

+			cond_resched();
 			udelay(50);  

Why not switch to usleep_range() if we can sleep here?  

Looking at usleep_range() vs. udelay(), I wonder if there is still a
hidden reason to prefer udelay().  Basically, if you typically want
short delays like the 50µs above, going to sleep will often result in
much longer delays, 1ms or higher.

Right, if you call sleep you always yield so another process can kick
in and consume its scheduler slice before it lets you back in.

How much does the FW typically take to respond? If we really care about
latency 50us already sounds like a pretty coarse granularity.

Also if cond_resched() fired doing the delay is probably a waste of
time:

	if (!cond_resched())
		usleep/udelay

I can easily see situations where multiple calls to udelay(50) are
fine while multiple calls to usleep_range() will cause timeouts.

The status of the command is re-checked after the loop, sleeping too
long should not cause timeouts here.

Is that a known problem and do we have good heuristics when to prefer
one over the other?

I'm not aware of any.

On Thu, Oct 28, 2021 at 07:26:07AM -0700, Jakub Kicinski wrote:

The status of the command is re-checked after the loop, sleeping too
long should not cause timeouts here.

Fair point.  usleep_range() is likely the correct answer in this case.

Jörn

--
It is the mark of an educated mind to be able to entertain a thought
without accepting it.
-- Aristotle

On Wed, 2021-10-27 at 09:30 -0700, Jakub Kicinski wrote:

On Mon, 25 Oct 2021 10:55:07 -0700 Tony Nguyen wrote:

quoted

From: Jan Kundrát <redacted>

The driver tried to use Linux' native software I2C bus master
(i2c-algo-bits) for exporting the I2C interface that talks to the
SFP
cage(s) towards userspace. As-is, however, the physical SCL/SDA
pins
were not moving at all, staying at logical 1 all the time.

So targeting net-next because this never worked?

Correct. Would you prefer I send this patch via net instead?

Thanks,
Tony

On Thu, 28 Oct 2021 15:25:55 +0000 Nguyen, Anthony L wrote:

On Wed, 2021-10-27 at 09:30 -0700, Jakub Kicinski wrote:

quoted

On Mon, 25 Oct 2021 10:55:07 -0700 Tony Nguyen wrote:  

quoted

From: Jan Kundrát <redacted>

The driver tried to use Linux' native software I2C bus master
(i2c-algo-bits) for exporting the I2C interface that talks to the
SFP
cage(s) towards userspace. As-is, however, the physical SCL/SDA
pins
were not moving at all, staying at logical 1 all the time.  

So targeting net-next because this never worked?  

Correct. Would you prefer I send this patch via net instead?

I think net-next will be fine here. Only patch 1 needs rejigging then.

Previously, the kernel could spend up to 250 ms waiting for a command to
be submitted to an admin queue. This function is also called in a loop,
e.g., in i40e_get_module_eeprom() (through i40e_aq_get_phy_register()),
so the time spent in the kernel may be even higher. We observed
scheduling delays of over 2 seconds in production,
with stacktraces pointing to this code as the culprit.

Use usleep_range() instead of udelay() so the loop can yield the CPU.
Also compute the elapsed time using the jiffies counter rather than
assuming udelay() waits exactly the time interval requested.

Signed-off-by: Caleb Sander <redacted>
Reviewed-by: Joern Engel <redacted>
---
 drivers/net/ethernet/intel/i40e/i40e_adminq.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

Changed from v1:
Use usleep_range() instead of udelay() + cond_resched(),
to avoid using the CPU while waiting.
Use 50 us as the max for the range since hrtimers schedules the sleep
for the max (unless another timer interrupt occurs after the min).
Since checking if the command is done too frequently would waste time
context-switching, use half of the max (25 us) as the min for the range.

diff --git a/drivers/net/ethernet/intel/i40e/i40e_adminq.c b/drivers/net/ethernet/intel/i40e/i40e_adminq.c
index 593912b17..b2c27ab3b 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_adminq.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_adminq.c

@@ -902,7 +902,7 @@ i40e_status i40e_asq_send_command(struct i40e_hw *hw,
 	 * we need to wait for desc write back
 	 */
 	if (!details->async && !details->postpone) {
-		u32 total_delay = 0;
+		unsigned long timeout_end = jiffies + usecs_to_jiffies(hw->aq.asq_cmd_timeout);
 
 		do {
 			/* AQ designers suggest use of head for better

@@ -910,9 +910,8 @@ i40e_status i40e_asq_send_command(struct i40e_hw *hw,
 			 */
 			if (i40e_asq_done(hw))
 				break;
-			udelay(50);
-			total_delay += 50;
-		} while (total_delay < hw->aq.asq_cmd_timeout);
+			usleep_range(25, 50);
+		} while (time_before(jiffies, timeout_end));
 	}
 
 	/* if ready, copy the desc back to temp */

-- 
2.25.1