All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH net-next 0/4][pull request] 40GbE Intel Wired LAN Driver Updates 2021-10-25
@ 2021-10-25 17:55 Tony Nguyen
  2021-10-25 17:55 ` [PATCH net-next 1/4] i40e: avoid spin loop in i40e_asq_send_command() Tony Nguyen
                   ` (4 more replies)
  0 siblings, 5 replies; 16+ messages in thread
From: Tony Nguyen @ 2021-10-25 17:55 UTC (permalink / raw)
  To: davem, kuba; +Cc: Tony Nguyen, netdev

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


^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH net-next 1/4] i40e: avoid spin loop in i40e_asq_send_command()
  2021-10-25 17:55 [PATCH net-next 0/4][pull request] 40GbE Intel Wired LAN Driver Updates 2021-10-25 Tony Nguyen
@ 2021-10-25 17:55 ` Tony Nguyen
  2021-10-27 16:01   ` Jakub Kicinski
  2021-10-25 17:55 ` [PATCH net-next 2/4] intel: Simplify bool conversion Tony Nguyen
                   ` (3 subsequent siblings)
  4 siblings, 1 reply; 16+ messages in thread
From: Tony Nguyen @ 2021-10-25 17:55 UTC (permalink / raw)
  To: davem, kuba
  Cc: Caleb Sander, netdev, anthony.l.nguyen, sassmann, Joern Engel,
	Tony Brelinski

From: Caleb Sander <csander@purestorage.com>

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 <csander@purestorage.com>
Reviewed-by: Joern Engel <joern@purestorage.com>
Tested-by: Tony Brelinski <tony.brelinski@intel.com>
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


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH net-next 2/4] intel: Simplify bool conversion
  2021-10-25 17:55 [PATCH net-next 0/4][pull request] 40GbE Intel Wired LAN Driver Updates 2021-10-25 Tony Nguyen
  2021-10-25 17:55 ` [PATCH net-next 1/4] i40e: avoid spin loop in i40e_asq_send_command() Tony Nguyen
@ 2021-10-25 17:55 ` Tony Nguyen
  2021-10-25 17:55 ` [PATCH net-next 3/4] igb: unbreak I2C bit-banging on i350 Tony Nguyen
                   ` (2 subsequent siblings)
  4 siblings, 0 replies; 16+ messages in thread
From: Tony Nguyen @ 2021-10-25 17:55 UTC (permalink / raw)
  To: davem, kuba
  Cc: Yang Li, netdev, anthony.l.nguyen, sassmann, magnus.karlsson,
	maciej.fijalkowski, Abaci Robot

From: Yang Li <yang.lee@linux.alibaba.com>

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 <abaci@linux.alibaba.com>
Signed-off-by: Yang Li <yang.lee@linux.alibaba.com>
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


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH net-next 3/4] igb: unbreak I2C bit-banging on i350
  2021-10-25 17:55 [PATCH net-next 0/4][pull request] 40GbE Intel Wired LAN Driver Updates 2021-10-25 Tony Nguyen
  2021-10-25 17:55 ` [PATCH net-next 1/4] i40e: avoid spin loop in i40e_asq_send_command() Tony Nguyen
  2021-10-25 17:55 ` [PATCH net-next 2/4] intel: Simplify bool conversion Tony Nguyen
@ 2021-10-25 17:55 ` Tony Nguyen
  2021-10-27 16:30   ` Jakub Kicinski
  2021-10-25 17:55 ` [PATCH net-next 4/4] net: ixgbevf: Remove redundant initialization of variable ret_val Tony Nguyen
  2021-10-27 15:51 ` [PATCH net-next 0/4][pull request] 40GbE Intel Wired LAN Driver Updates 2021-10-25 Nguyen, Anthony L
  4 siblings, 1 reply; 16+ messages in thread
From: Tony Nguyen @ 2021-10-25 17:55 UTC (permalink / raw)
  To: davem, kuba
  Cc: Jan Kundrát, netdev, anthony.l.nguyen, Jesse Brandeburg,
	Tony Brelinski

From: Jan Kundrát <jan.kundrat@cesnet.cz>

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 <jan.kundrat@cesnet.cz>
See-also: https://www.spinics.net/lists/netdev/msg490554.html
Reviewed-by: Jesse Brandeburg <jesse.brandeburg@intel.com>
Tested-by: Tony Brelinski <tony.brelinski@intel.com>
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


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH net-next 4/4] net: ixgbevf: Remove redundant initialization of variable ret_val
  2021-10-25 17:55 [PATCH net-next 0/4][pull request] 40GbE Intel Wired LAN Driver Updates 2021-10-25 Tony Nguyen
                   ` (2 preceding siblings ...)
  2021-10-25 17:55 ` [PATCH net-next 3/4] igb: unbreak I2C bit-banging on i350 Tony Nguyen
@ 2021-10-25 17:55 ` Tony Nguyen
  2021-10-27 15:51 ` [PATCH net-next 0/4][pull request] 40GbE Intel Wired LAN Driver Updates 2021-10-25 Nguyen, Anthony L
  4 siblings, 0 replies; 16+ messages in thread
From: Tony Nguyen @ 2021-10-25 17:55 UTC (permalink / raw)
  To: davem, kuba; +Cc: Colin Ian King, netdev, anthony.l.nguyen

From: Colin Ian King <colin.king@canonical.com>

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 <colin.king@canonical.com>
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


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* Re: [PATCH net-next 0/4][pull request] 40GbE Intel Wired LAN Driver Updates 2021-10-25
  2021-10-25 17:55 [PATCH net-next 0/4][pull request] 40GbE Intel Wired LAN Driver Updates 2021-10-25 Tony Nguyen
                   ` (3 preceding siblings ...)
  2021-10-25 17:55 ` [PATCH net-next 4/4] net: ixgbevf: Remove redundant initialization of variable ret_val Tony Nguyen
@ 2021-10-27 15:51 ` Nguyen, Anthony L
  2021-10-27 15:59   ` Jakub Kicinski
  4 siblings, 1 reply; 16+ messages in thread
From: Nguyen, Anthony L @ 2021-10-27 15:51 UTC (permalink / raw)
  To: davem, kuba; +Cc: netdev

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=*


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH net-next 0/4][pull request] 40GbE Intel Wired LAN Driver Updates 2021-10-25
  2021-10-27 15:51 ` [PATCH net-next 0/4][pull request] 40GbE Intel Wired LAN Driver Updates 2021-10-25 Nguyen, Anthony L
@ 2021-10-27 15:59   ` Jakub Kicinski
  0 siblings, 0 replies; 16+ messages in thread
From: Jakub Kicinski @ 2021-10-27 15:59 UTC (permalink / raw)
  To: Nguyen, Anthony L; +Cc: davem, netdev

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:
> > 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.

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH net-next 1/4] i40e: avoid spin loop in i40e_asq_send_command()
  2021-10-25 17:55 ` [PATCH net-next 1/4] i40e: avoid spin loop in i40e_asq_send_command() Tony Nguyen
@ 2021-10-27 16:01   ` Jakub Kicinski
  2021-10-28 11:49     ` Jörn Engel
  0 siblings, 1 reply; 16+ messages in thread
From: Jakub Kicinski @ 2021-10-27 16:01 UTC (permalink / raw)
  To: Tony Nguyen
  Cc: davem, Caleb Sander, netdev, sassmann, Joern Engel, Tony Brelinski

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?

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH net-next 3/4] igb: unbreak I2C bit-banging on i350
  2021-10-25 17:55 ` [PATCH net-next 3/4] igb: unbreak I2C bit-banging on i350 Tony Nguyen
@ 2021-10-27 16:30   ` Jakub Kicinski
  2021-10-28 15:25     ` Nguyen, Anthony L
  0 siblings, 1 reply; 16+ messages in thread
From: Jakub Kicinski @ 2021-10-27 16:30 UTC (permalink / raw)
  To: Tony Nguyen
  Cc: davem, Jan Kundrát, netdev, Jesse Brandeburg, Tony Brelinski

On Mon, 25 Oct 2021 10:55:07 -0700 Tony Nguyen wrote:
> From: Jan Kundrát <jan.kundrat@cesnet.cz>
> 
> 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?

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH net-next 1/4] i40e: avoid spin loop in i40e_asq_send_command()
  2021-10-27 16:01   ` Jakub Kicinski
@ 2021-10-28 11:49     ` Jörn Engel
  2021-10-28 14:26       ` Jakub Kicinski
  0 siblings, 1 reply; 16+ messages in thread
From: Jörn Engel @ 2021-10-28 11:49 UTC (permalink / raw)
  To: Jakub Kicinski
  Cc: Tony Nguyen, davem, Caleb Sander, netdev, sassmann, Tony Brelinski

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:
> > +			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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH net-next 1/4] i40e: avoid spin loop in i40e_asq_send_command()
  2021-10-28 11:49     ` Jörn Engel
@ 2021-10-28 14:26       ` Jakub Kicinski
  2021-10-28 14:44         ` Jörn Engel
  0 siblings, 1 reply; 16+ messages in thread
From: Jakub Kicinski @ 2021-10-28 14:26 UTC (permalink / raw)
  To: Jörn Engel
  Cc: Tony Nguyen, davem, Caleb Sander, netdev, sassmann, Tony Brelinski

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:
> > 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?  
> 
> 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.

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH net-next 1/4] i40e: avoid spin loop in i40e_asq_send_command()
  2021-10-28 14:26       ` Jakub Kicinski
@ 2021-10-28 14:44         ` Jörn Engel
  2021-11-01 17:38             ` [Intel-wired-lan] " Caleb Sander
  0 siblings, 1 reply; 16+ messages in thread
From: Jörn Engel @ 2021-10-28 14:44 UTC (permalink / raw)
  To: Jakub Kicinski
  Cc: Tony Nguyen, davem, Caleb Sander, netdev, sassmann, Tony Brelinski

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH net-next 3/4] igb: unbreak I2C bit-banging on i350
  2021-10-27 16:30   ` Jakub Kicinski
@ 2021-10-28 15:25     ` Nguyen, Anthony L
  2021-10-28 15:32       ` Jakub Kicinski
  0 siblings, 1 reply; 16+ messages in thread
From: Nguyen, Anthony L @ 2021-10-28 15:25 UTC (permalink / raw)
  To: kuba; +Cc: jan.kundrat, davem, netdev, Brandeburg, Jesse, Brelinski, Tony

On Wed, 2021-10-27 at 09:30 -0700, Jakub Kicinski wrote:
> On Mon, 25 Oct 2021 10:55:07 -0700 Tony Nguyen wrote:
> > From: Jan Kundrát <jan.kundrat@cesnet.cz>
> > 
> > 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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH net-next 3/4] igb: unbreak I2C bit-banging on i350
  2021-10-28 15:25     ` Nguyen, Anthony L
@ 2021-10-28 15:32       ` Jakub Kicinski
  0 siblings, 0 replies; 16+ messages in thread
From: Jakub Kicinski @ 2021-10-28 15:32 UTC (permalink / raw)
  To: Nguyen, Anthony L
  Cc: jan.kundrat, davem, netdev, Brandeburg, Jesse, Brelinski, 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:
> > On Mon, 25 Oct 2021 10:55:07 -0700 Tony Nguyen wrote:  
> > > From: Jan Kundrát <jan.kundrat@cesnet.cz>
> > > 
> > > 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.

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH v2] i40e: avoid spin loop in i40e_asq_send_command()
  2021-10-28 14:44         ` Jörn Engel
@ 2021-11-01 17:38             ` Caleb Sander
  0 siblings, 0 replies; 16+ messages in thread
From: Caleb Sander @ 2021-11-01 17:38 UTC (permalink / raw)
  To: intel-wired-lan
  Cc: Tony Nguyen, Caleb Sander, Joern Engel, Tony Brelinski, davem,
	Jakub Kicinski, netdev, sassmann

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 <csander@purestorage.com>
Reviewed-by: Joern Engel <joern@purestorage.com>
---
 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


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [Intel-wired-lan] [PATCH v2] i40e: avoid spin loop in i40e_asq_send_command()
@ 2021-11-01 17:38             ` Caleb Sander
  0 siblings, 0 replies; 16+ messages in thread
From: Caleb Sander @ 2021-11-01 17:38 UTC (permalink / raw)
  To: intel-wired-lan

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 <csander@purestorage.com>
Reviewed-by: Joern Engel <joern@purestorage.com>
---
 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


^ permalink raw reply related	[flat|nested] 16+ messages in thread

end of thread, other threads:[~2021-11-01 17:39 UTC | newest]

Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-10-25 17:55 [PATCH net-next 0/4][pull request] 40GbE Intel Wired LAN Driver Updates 2021-10-25 Tony Nguyen
2021-10-25 17:55 ` [PATCH net-next 1/4] i40e: avoid spin loop in i40e_asq_send_command() Tony Nguyen
2021-10-27 16:01   ` Jakub Kicinski
2021-10-28 11:49     ` Jörn Engel
2021-10-28 14:26       ` Jakub Kicinski
2021-10-28 14:44         ` Jörn Engel
2021-11-01 17:38           ` [PATCH v2] " Caleb Sander
2021-11-01 17:38             ` [Intel-wired-lan] " Caleb Sander
2021-10-25 17:55 ` [PATCH net-next 2/4] intel: Simplify bool conversion Tony Nguyen
2021-10-25 17:55 ` [PATCH net-next 3/4] igb: unbreak I2C bit-banging on i350 Tony Nguyen
2021-10-27 16:30   ` Jakub Kicinski
2021-10-28 15:25     ` Nguyen, Anthony L
2021-10-28 15:32       ` Jakub Kicinski
2021-10-25 17:55 ` [PATCH net-next 4/4] net: ixgbevf: Remove redundant initialization of variable ret_val Tony Nguyen
2021-10-27 15:51 ` [PATCH net-next 0/4][pull request] 40GbE Intel Wired LAN Driver Updates 2021-10-25 Nguyen, Anthony L
2021-10-27 15:59   ` Jakub Kicinski

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.