[Intel-wired-lan] [jkirsher/next-queue PATCH 0/2] Address issues with reset on systems with high completion timeout times

[Intel-wired-lan] [jkirsher/next-queue PATCH 0/2] Address issues with reset on systems with high completion timeout times

[Alexander Duyck]

This patch series is meant to address issues seen with ixgbe on systems
with high completion timeout times. Specifically the Tx and Rx queue
disable functions are supposed to poll until the enable bit is actually
cleared, and isntead of doing that we were failing immediately after just
100us in the case of Rx and weren't bothering to test in the case of Tx.
Instead this patch set adds polling logic with the upper bounds defined
using the intervals defined in the completion timeout logic.

In addition I have reorganized the code to parallelize this as much as
possible so that the Tx and Rx should hopefully flush out at around the
same time so we only experience on wait for all of the queues instead of
waiting per queue.

---

Alexander Duyck (2):
      ixgbe: Reorder Tx/Rx shutdown to reduce time needed to stop device
      ixgbe: Refactor queue disable logic to take completion time into account


 drivers/net/ethernet/intel/ixgbe/ixgbe.h         |    3 
 drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c |   32 +-
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c    |  290 +++++++++++++++++-----
 3 files changed, 229 insertions(+), 96 deletions(-)

--

[Intel-wired-lan] [jkirsher/next-queue PATCH 1/2] ixgbe: Reorder Tx/Rx shutdown to reduce time needed to stop device

[Alexander Duyck]

This change is meant to help reduce the time needed to shutdown the
transmit and receive paths for the device. Specifically what we now do
after this patch is disable the transmit path first at the netdev level,
and then work on disabling the Rx. This way while we are waiting on the Rx
queues to be disabled the Tx queues have an opportunity to drain out.

In addition I have droped the 10ms timeout that was left in the ixgbe_down
function that seems to have been carried through from back in e1000 as far
as I can tell. We shouldn't need it since we don't actually disable the Tx
until much later and we have additional logic in place for verifying the Tx
queues have been disabled.

Signed-off-by: Alexander Duyck <alexander.h.duyck@intel.com>
---
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c |   14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
index 372174c..7eef089 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
@@ -5814,6 +5814,13 @@ void ixgbe_down(struct ixgbe_adapter *adapter)
 	if (test_and_set_bit(__IXGBE_DOWN, &adapter->state))
 		return; /* do nothing if already down */
 
+	/* Shut off incoming Tx traffic */
+	netif_tx_stop_all_queues(netdev);
+
+	/* call carrier off first to avoid false dev_watchdog timeouts */
+	netif_carrier_off(netdev);
+	netif_tx_disable(netdev);
+
 	/* disable receives */
 	hw->mac.ops.disable_rx(hw);
 
@@ -5822,16 +5829,9 @@ void ixgbe_down(struct ixgbe_adapter *adapter)
 		/* this call also flushes the previous write */
 		ixgbe_disable_rx_queue(adapter, adapter->rx_ring[i]);
 
-	usleep_range(10000, 20000);
-
 	/* synchronize_sched() needed for pending XDP buffers to drain */
 	if (adapter->xdp_ring[0])
 		synchronize_sched();
-	netif_tx_stop_all_queues(netdev);
-
-	/* call carrier off first to avoid false dev_watchdog timeouts */
-	netif_carrier_off(netdev);
-	netif_tx_disable(netdev);
 
 	ixgbe_irq_disable(adapter);
 

[Intel-wired-lan] [jkirsher/next-queue PATCH 2/2] ixgbe: Refactor queue disable logic to take completion time into account

[Alexander Duyck]

This change is meant to allow us to take completion time into account when
disabling queues. Previously we were just working with hard coded values
for how long we should wait. This worked fine for the standard case where
completion timeout was operating in the 50us to 50ms range, however on
platforms that have higher completion timeout times this was resulting in
Rx queues disable messages being displayed as we weren't waiting long
enough for outstanding Rx DMA completions.

Signed-off-by: Alexander Duyck <alexander.h.duyck@intel.com>
---
 drivers/net/ethernet/intel/ixgbe/ixgbe.h         |    3 
 drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c |   32 +--
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c    |  280 +++++++++++++++++-----
 3 files changed, 224 insertions(+), 91 deletions(-)

diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe.h b/drivers/net/ethernet/intel/ixgbe/ixgbe.h
index 144d5fe..4fc906c 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe.h
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe.h
@@ -855,7 +855,8 @@ enum ixgbe_boards {
 void ixgbe_free_tx_resources(struct ixgbe_ring *);
 void ixgbe_configure_rx_ring(struct ixgbe_adapter *, struct ixgbe_ring *);
 void ixgbe_configure_tx_ring(struct ixgbe_adapter *, struct ixgbe_ring *);
-void ixgbe_disable_rx_queue(struct ixgbe_adapter *adapter, struct ixgbe_ring *);
+void ixgbe_disable_rx(struct ixgbe_adapter *adapter);
+void ixgbe_disable_tx(struct ixgbe_adapter *adapter);
 void ixgbe_update_stats(struct ixgbe_adapter *adapter);
 int ixgbe_init_interrupt_scheme(struct ixgbe_adapter *adapter);
 bool ixgbe_wol_supported(struct ixgbe_adapter *adapter, u16 device_id,
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c
index 1d68884..e5a8461 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c
@@ -1698,35 +1698,17 @@ static int ixgbe_intr_test(struct ixgbe_adapter *adapter, u64 *data)
 
 static void ixgbe_free_desc_rings(struct ixgbe_adapter *adapter)
 {
-	struct ixgbe_ring *tx_ring = &adapter->test_tx_ring;
-	struct ixgbe_ring *rx_ring = &adapter->test_rx_ring;
-	struct ixgbe_hw *hw = &adapter->hw;
-	u32 reg_ctl;
-
-	/* shut down the DMA engines now so they can be reinitialized later */
+	/* Shut down the DMA engines now so they can be reinitialized later,
+	 * since the test rings and normally used rings should overlap on
+	 * queue 0 we can just use the standard disable Rx/Tx calls and they
+	 * will take care of disabling the test rings for us.
+	 */
 
 	/* first Rx */
-	hw->mac.ops.disable_rx(hw);
-	ixgbe_disable_rx_queue(adapter, rx_ring);
+	ixgbe_disable_rx(adapter);
 
 	/* now Tx */
-	reg_ctl = IXGBE_READ_REG(hw, IXGBE_TXDCTL(tx_ring->reg_idx));
-	reg_ctl &= ~IXGBE_TXDCTL_ENABLE;
-	IXGBE_WRITE_REG(hw, IXGBE_TXDCTL(tx_ring->reg_idx), reg_ctl);
-
-	switch (hw->mac.type) {
-	case ixgbe_mac_82599EB:
-	case ixgbe_mac_X540:
-	case ixgbe_mac_X550:
-	case ixgbe_mac_X550EM_x:
-	case ixgbe_mac_x550em_a:
-		reg_ctl = IXGBE_READ_REG(hw, IXGBE_DMATXCTL);
-		reg_ctl &= ~IXGBE_DMATXCTL_TE;
-		IXGBE_WRITE_REG(hw, IXGBE_DMATXCTL, reg_ctl);
-		break;
-	default:
-		break;
-	}
+	ixgbe_disable_tx(adapter);
 
 	ixgbe_reset(adapter);
 
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
index 7eef089..951655b 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
@@ -4022,38 +4022,6 @@ static void ixgbe_rx_desc_queue_enable(struct ixgbe_adapter *adapter,
 	}
 }
 
-void ixgbe_disable_rx_queue(struct ixgbe_adapter *adapter,
-			    struct ixgbe_ring *ring)
-{
-	struct ixgbe_hw *hw = &adapter->hw;
-	int wait_loop = IXGBE_MAX_RX_DESC_POLL;
-	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;
-
-	/* write value back with RXDCTL.ENABLE bit cleared */
-	IXGBE_WRITE_REG(hw, IXGBE_RXDCTL(reg_idx), rxdctl);
-
-	if (hw->mac.type == ixgbe_mac_82598EB &&
-	    !(IXGBE_READ_REG(hw, IXGBE_LINKS) & IXGBE_LINKS_UP))
-		return;
-
-	/* the hardware may take up to 100us to really disable the rx queue */
-	do {
-		udelay(10);
-		rxdctl = IXGBE_READ_REG(hw, IXGBE_RXDCTL(reg_idx));
-	} while (--wait_loop && (rxdctl & IXGBE_RXDCTL_ENABLE));
-
-	if (!wait_loop) {
-		e_err(drv, "RXDCTL.ENABLE on Rx queue %d not cleared within "
-		      "the polling period\n", reg_idx);
-	}
-}
-
 void ixgbe_configure_rx_ring(struct ixgbe_adapter *adapter,
 			     struct ixgbe_ring *ring)
 {
@@ -4063,9 +4031,13 @@ void ixgbe_configure_rx_ring(struct ixgbe_adapter *adapter,
 	u32 rxdctl;
 	u8 reg_idx = ring->reg_idx;
 
-	/* disable queue to avoid issues while updating state */
+	/* disable queue to avoid use of these values while updating state */
 	rxdctl = IXGBE_READ_REG(hw, IXGBE_RXDCTL(reg_idx));
-	ixgbe_disable_rx_queue(adapter, ring);
+	rxdctl &= ~IXGBE_RXDCTL_ENABLE;
+
+	/* write value back with RXDCTL.ENABLE bit cleared */
+	IXGBE_WRITE_REG(hw, IXGBE_RXDCTL(reg_idx), rxdctl);
+	IXGBE_WRITE_FLUSH(hw);
 
 	IXGBE_WRITE_REG(hw, IXGBE_RDBAL(reg_idx), (rdba & DMA_BIT_MASK(32)));
 	IXGBE_WRITE_REG(hw, IXGBE_RDBAH(reg_idx), (rdba >> 32));
@@ -5633,6 +5605,212 @@ void ixgbe_up(struct ixgbe_adapter *adapter)
 	ixgbe_up_complete(adapter);
 }
 
+unsigned long ixgbe_get_completion_timeout(struct ixgbe_adapter *adapter)
+{
+	u16 devctl2;
+
+	pcie_capability_read_word(adapter->pdev, PCI_EXP_DEVCTL2, &devctl2);
+
+	switch (devctl2 & IXGBE_PCIDEVCTRL2_TIMEO_MASK) {
+	case IXGBE_PCIDEVCTRL2_17_34s:
+	case IXGBE_PCIDEVCTRL2_4_8s:
+		/* For now we cap the upper limit on delay to 2 seconds
+		 * as we end up going up to 34 seconds of delay in worst
+		 * case timeout value.
+		 */
+	case IXGBE_PCIDEVCTRL2_1_2s:
+		return 2000000ul;	/* 2.0 s */
+	case IXGBE_PCIDEVCTRL2_260_520ms:
+		return 520000ul;	/* 520 ms */
+	case IXGBE_PCIDEVCTRL2_65_130ms:
+		return 130000ul;	/* 130 ms */
+	case IXGBE_PCIDEVCTRL2_16_32ms:
+		return 32000ul;		/* 32 ms */
+	case IXGBE_PCIDEVCTRL2_1_2ms:
+		return 2000ul;		/* 2 ms */
+	case IXGBE_PCIDEVCTRL2_50_100us:
+		return 100ul;		/* 100 us */
+	case IXGBE_PCIDEVCTRL2_16_32ms_def:
+		return 32000ul;		/* 32 ms */
+	default:
+		break;
+	}
+
+	/* We shouldn't need to hit this path, but just in case default as
+	 * though completion timeout is not supported and support 32ms.
+	 */
+	return 32000ul;
+}
+
+void ixgbe_disable_rx(struct ixgbe_adapter *adapter)
+{
+	unsigned long wait_delay, delay_interval;
+	struct ixgbe_hw *hw = &adapter->hw;
+	int i, wait_loop;
+	u32 rxdctl;
+
+	/* disable receives */
+	hw->mac.ops.disable_rx(hw);
+
+	if (ixgbe_removed(hw->hw_addr))
+		return;
+
+	/* disable all enabled Rx queues */
+	for (i = 0; i < adapter->num_rx_queues; i++) {
+		struct ixgbe_ring *ring = adapter->rx_ring[i];
+		u8 reg_idx = ring->reg_idx;
+
+		rxdctl = IXGBE_READ_REG(hw, IXGBE_RXDCTL(reg_idx));
+		rxdctl &= ~IXGBE_RXDCTL_ENABLE;
+		rxdctl |= IXGBE_RXDCTL_SWFLSH;
+
+		/* write value back with RXDCTL.ENABLE bit cleared */
+		IXGBE_WRITE_REG(hw, IXGBE_RXDCTL(reg_idx), rxdctl);
+	}
+
+	/* RXDCTL.EN may not change 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))
+		return;
+
+	/* Determine our minimum delay interval. We will increase this value
+	 * with each subsequent test. This way if the device returns quickly
+	 * we should spend as little time as possible waiting, however as
+	 * the time increases we will wait for larger periods of time.
+	 *
+	 * The trick here is that we increase the interval using the
+	 * following pattern: 1x 3x 5x 7x 9x 11x 13x 15x 17x 19x. The result
+	 * of that wait is that it totals up to 100x whatever interval we
+	 * choose. Since our minimum wait is 100us we can just divide the
+	 * total timeout by 100 to get our minimum delay interval.
+	 */
+	delay_interval = ixgbe_get_completion_timeout(adapter) / 100;
+
+	wait_loop = IXGBE_MAX_RX_DESC_POLL;
+	wait_delay = delay_interval;
+
+	while (wait_loop--) {
+		usleep_range(wait_delay, wait_delay + 10);
+		wait_delay += delay_interval * 2;
+		rxdctl = 0;
+
+		/* OR together the reading of all the active RXDCTL registers,
+		 * and then test the result. We need the disable to complete
+		 * before we start freeing the memory and invalidating the
+		 * DMA mappings.
+		 */
+		for (i = 0; i < adapter->num_rx_queues; i++) {
+			struct ixgbe_ring *ring = adapter->rx_ring[i];
+			u8 reg_idx = ring->reg_idx;
+
+			rxdctl |= IXGBE_READ_REG(hw, IXGBE_RXDCTL(reg_idx));
+		}
+
+		if (!(rxdctl & IXGBE_RXDCTL_ENABLE))
+			return;
+	}
+
+	e_err(drv,
+	      "RXDCTL.ENABLE for one or more queues not cleared within the polling period\n");
+}
+
+void ixgbe_disable_tx(struct ixgbe_adapter *adapter)
+{
+	unsigned long wait_delay, delay_interval;
+	struct ixgbe_hw *hw = &adapter->hw;
+	int i, wait_loop;
+	u32 txdctl;
+
+	if (ixgbe_removed(hw->hw_addr))
+		return;
+
+	/* disable all enabled Tx queues */
+	for (i = 0; i < adapter->num_tx_queues; i++) {
+		struct ixgbe_ring *ring = adapter->tx_ring[i];
+		u8 reg_idx = ring->reg_idx;
+
+		IXGBE_WRITE_REG(hw, IXGBE_TXDCTL(reg_idx), IXGBE_TXDCTL_SWFLSH);
+	}
+
+	/* disable all enabled XDP Tx queues */
+	for (i = 0; i < adapter->num_xdp_queues; i++) {
+		struct ixgbe_ring *ring = adapter->xdp_ring[i];
+		u8 reg_idx = ring->reg_idx;
+
+		IXGBE_WRITE_REG(hw, IXGBE_TXDCTL(reg_idx), IXGBE_TXDCTL_SWFLSH);
+	}
+
+	/* If the link is now up there shouldn't be much in the way of
+	 * pending transactions. Those that are left will be flushed out
+	 * when the reset logic goes through the flush sequence to clean out
+	 * the pending Tx transactions.
+	 */
+	if (!(IXGBE_READ_REG(hw, IXGBE_LINKS) & IXGBE_LINKS_UP))
+		goto dma_engine_disable;
+
+	/* Determine our minimum delay interval. We will increase this value
+	 * with each subsequent test. This way if the device returns quickly
+	 * we should spend as little time as possible waiting, however as
+	 * the time increases we will wait for larger periods of time.
+	 *
+	 * The trick here is that we increase the interval using the
+	 * following pattern: 1x 3x 5x 7x 9x 11x 13x 15x 17x 19x. The result
+	 * of that wait is that it totals up to 100x whatever interval we
+	 * choose. Since our minimum wait is 100us we can just divide the
+	 * total timeout by 100 to get our minimum delay interval.
+	 */
+	delay_interval = ixgbe_get_completion_timeout(adapter) / 100;
+
+	wait_loop = IXGBE_MAX_RX_DESC_POLL;
+	wait_delay = delay_interval;
+
+	while (wait_loop--) {
+		usleep_range(wait_delay, wait_delay + 10);
+		wait_delay += delay_interval * 2;
+		txdctl = 0;
+
+		/* OR together the reading of all the active TXDCTL registers,
+		 * and then test the result. We need the disable to complete
+		 * before we start freeing the memory and invalidating the
+		 * DMA mappings.
+		 */
+		for (i = 0; i < adapter->num_tx_queues; i++) {
+			struct ixgbe_ring *ring = adapter->tx_ring[i];
+			u8 reg_idx = ring->reg_idx;
+
+			txdctl |= IXGBE_READ_REG(hw, IXGBE_TXDCTL(reg_idx));
+		}
+		for (i = 0; i < adapter->num_xdp_queues; i++) {
+			struct ixgbe_ring *ring = adapter->xdp_ring[i];
+			u8 reg_idx = ring->reg_idx;
+
+			txdctl |= IXGBE_READ_REG(hw, IXGBE_TXDCTL(reg_idx));
+		}
+
+		if (!(txdctl & IXGBE_TXDCTL_ENABLE))
+			goto dma_engine_disable;
+	}
+
+	e_err(drv,
+	      "TXDCTL.ENABLE for one or more queues not cleared within the polling period\n");
+
+dma_engine_disable:
+	/* Disable the Tx DMA engine on 82599 and later MAC */
+	switch (hw->mac.type) {
+	case ixgbe_mac_82599EB:
+	case ixgbe_mac_X540:
+	case ixgbe_mac_X550:
+	case ixgbe_mac_X550EM_x:
+	case ixgbe_mac_x550em_a:
+		IXGBE_WRITE_REG(hw, IXGBE_DMATXCTL,
+				(IXGBE_READ_REG(hw, IXGBE_DMATXCTL) &
+				 ~IXGBE_DMATXCTL_TE));
+		/* fall through */
+	default:
+		break;
+	}
+}
+
 void ixgbe_reset(struct ixgbe_adapter *adapter)
 {
 	struct ixgbe_hw *hw = &adapter->hw;
@@ -5821,13 +5999,8 @@ void ixgbe_down(struct ixgbe_adapter *adapter)
 	netif_carrier_off(netdev);
 	netif_tx_disable(netdev);
 
-	/* disable receives */
-	hw->mac.ops.disable_rx(hw);
-
-	/* disable all enabled rx queues */
-	for (i = 0; i < adapter->num_rx_queues; i++)
-		/* this call also flushes the previous write */
-		ixgbe_disable_rx_queue(adapter, adapter->rx_ring[i]);
+	/* Disable Rx */
+	ixgbe_disable_rx(adapter);
 
 	/* synchronize_sched() needed for pending XDP buffers to drain */
 	if (adapter->xdp_ring[0])
@@ -5859,30 +6032,7 @@ void ixgbe_down(struct ixgbe_adapter *adapter)
 	}
 
 	/* disable transmits in the hardware now that interrupts are off */
-	for (i = 0; i < adapter->num_tx_queues; i++) {
-		u8 reg_idx = adapter->tx_ring[i]->reg_idx;
-		IXGBE_WRITE_REG(hw, IXGBE_TXDCTL(reg_idx), IXGBE_TXDCTL_SWFLSH);
-	}
-	for (i = 0; i < adapter->num_xdp_queues; i++) {
-		u8 reg_idx = adapter->xdp_ring[i]->reg_idx;
-
-		IXGBE_WRITE_REG(hw, IXGBE_TXDCTL(reg_idx), IXGBE_TXDCTL_SWFLSH);
-	}
-
-	/* Disable the Tx DMA engine on 82599 and later MAC */
-	switch (hw->mac.type) {
-	case ixgbe_mac_82599EB:
-	case ixgbe_mac_X540:
-	case ixgbe_mac_X550:
-	case ixgbe_mac_X550EM_x:
-	case ixgbe_mac_x550em_a:
-		IXGBE_WRITE_REG(hw, IXGBE_DMATXCTL,
-				(IXGBE_READ_REG(hw, IXGBE_DMATXCTL) &
-				 ~IXGBE_DMATXCTL_TE));
-		break;
-	default:
-		break;
-	}
+	ixgbe_disable_tx(adapter);
 
 	if (!pci_channel_offline(adapter->pdev))
 		ixgbe_reset(adapter);

[Intel-wired-lan] [jkirsher/next-queue PATCH 2/2] ixgbe: Refactor queue disable logic to take completion time into account

[kbuild test robot]

Hi Alexander,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on jkirsher-next-queue/dev-queue]
[also build test WARNING on v4.18-rc5 next-20180720]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Alexander-Duyck/ixgbe-Reorder-Tx-Rx-shutdown-to-reduce-time-needed-to-stop-device/20180721-074030
base:   https://git.kernel.org/pub/scm/linux/kernel/git/jkirsher/next-queue.git dev-queue
reproduce:
        # apt-get install sparse
        make ARCH=x86_64 allmodconfig
        make C=1 CF=-D__CHECK_ENDIAN__


sparse warnings: (new ones prefixed by >>)

   drivers/net/ethernet/intel/ixgbe/ixgbe_main.c:2723:19: sparse: expression using sizeof(void)
   drivers/net/ethernet/intel/ixgbe/ixgbe_main.c:2723:19: sparse: expression using sizeof(void)
   drivers/net/ethernet/intel/ixgbe/ixgbe_main.c:3181:35: sparse: expression using sizeof(void)
   drivers/net/ethernet/intel/ixgbe/ixgbe_main.c:3205:16: sparse: expression using sizeof(void)
   drivers/net/ethernet/intel/ixgbe/ixgbe_main.c:3205:16: sparse: expression using sizeof(void)
   drivers/net/ethernet/intel/ixgbe/ixgbe_main.c:4823:16: sparse: expression using sizeof(void)
   drivers/net/ethernet/intel/ixgbe/ixgbe_main.c:4990:29: sparse: expression using sizeof(void)
>> drivers/net/ethernet/intel/ixgbe/ixgbe_main.c:5608:15: sparse: symbol 'ixgbe_get_completion_timeout' was not declared. Should it be static?
   drivers/net/ethernet/intel/ixgbe/ixgbe_main.c:6167:15: sparse: expression using sizeof(void)
   drivers/net/ethernet/intel/ixgbe/ixgbe_main.c:6167:15: sparse: expression using sizeof(void)
   drivers/net/ethernet/intel/ixgbe/ixgbe_main.c:6172:16: sparse: expression using sizeof(void)
   include/linux/slab.h:631:13: sparse: undefined identifier '__builtin_mul_overflow'
   drivers/net/ethernet/intel/ixgbe/ixgbe_main.c:8140:24: sparse: expression using sizeof(void)
   drivers/net/ethernet/intel/ixgbe/ixgbe_main.c:8140:24: sparse: expression using sizeof(void)
   drivers/net/ethernet/intel/ixgbe/ixgbe_main.c:9575:19: sparse: expression using sizeof(void)
   drivers/net/ethernet/intel/ixgbe/ixgbe_main.c:9575:19: sparse: expression using sizeof(void)
   drivers/net/ethernet/intel/ixgbe/ixgbe_main.c:9979:42: sparse: expression using sizeof(void)
   drivers/net/ethernet/intel/ixgbe/ixgbe_main.c:9979:42: sparse: expression using sizeof(void)
   drivers/net/ethernet/intel/ixgbe/ixgbe_main.c:9983:42: sparse: expression using sizeof(void)
   drivers/net/ethernet/intel/ixgbe/ixgbe_main.c:9983:42: sparse: expression using sizeof(void)
   drivers/net/ethernet/intel/ixgbe/ixgbe_main.c:10681:26: sparse: expression using sizeof(void)
   drivers/net/ethernet/intel/ixgbe/ixgbe_main.c:10766:32: sparse: expression using sizeof(void)
   include/linux/slab.h:631:13: sparse: call with no type!

Please review and possibly fold the followup patch.

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[Intel-wired-lan] [RFC PATCH] ixgbe: ixgbe_get_completion_timeout() can be static

[kbuild test robot]

Fixes: 23f3effc3f69 ("ixgbe: Refactor queue disable logic to take completion time into account")
Signed-off-by: kbuild test robot <fengguang.wu@intel.com>
---
 ixgbe_main.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
index d4395f1..e3fac6a 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
@@ -5605,7 +5605,7 @@ void ixgbe_up(struct ixgbe_adapter *adapter)
 	ixgbe_up_complete(adapter);
 }
 
-unsigned long ixgbe_get_completion_timeout(struct ixgbe_adapter *adapter)
+static unsigned long ixgbe_get_completion_timeout(struct ixgbe_adapter *adapter)
 {
 	u16 devctl2;
 

[Intel-wired-lan] [RFC PATCH] ixgbe: ixgbe_get_completion_timeout() can be static

[Alexander Duyck]

On Fri, Jul 20, 2018 at 10:53 PM, kbuild test robot
<fengguang.wu@intel.com> wrote:
>
> Fixes: 23f3effc3f69 ("ixgbe: Refactor queue disable logic to take completion time into account")
> Signed-off-by: kbuild test robot <fengguang.wu@intel.com>

Jeff do you think you could fold this into the current completion timeout patch?

Thanks.

- Alex

[Intel-wired-lan] [RFC PATCH] ixgbe: ixgbe_get_completion_timeout() can be static

[Jeff Kirsher]

On Mon, 2018-07-23 at 09:24 -0700, Alexander Duyck wrote:
> On Fri, Jul 20, 2018 at 10:53 PM, kbuild test robot
> <fengguang.wu@intel.com> wrote:
> > 
> > Fixes: 23f3effc3f69 ("ixgbe: Refactor queue disable logic to take
> > completion time into account")
> > Signed-off-by: kbuild test robot <fengguang.wu@intel.com>
> 
> Jeff do you think you could fold this into the current completion
> timeout patch?

Yes, I can do that.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: This is a digitally signed message part
URL: <http://lists.osuosl.org/pipermail/intel-wired-lan/attachments/20180723/b1d30cb7/attachment-0001.asc>

[Intel-wired-lan] [jkirsher/next-queue PATCH 2/2] ixgbe: Refactor queue disable logic to take completion time into account

[Shannon Nelson]

On 7/20/2018 3:29 PM, Alexander Duyck wrote:
> This change is meant to allow us to take completion time into account when
> disabling queues. Previously we were just working with hard coded values
> for how long we should wait. This worked fine for the standard case where
> completion timeout was operating in the 50us to 50ms range, however on
> platforms that have higher completion timeout times this was resulting in
> Rx queues disable messages being displayed as we weren't waiting long
> enough for outstanding Rx DMA completions.
> 
> Signed-off-by: Alexander Duyck <alexander.h.duyck@intel.com>
> ---

[...]

> +
> +void ixgbe_disable_tx(struct ixgbe_adapter *adapter)
> +{
> +	unsigned long wait_delay, delay_interval;
> +	struct ixgbe_hw *hw = &adapter->hw;
> +	int i, wait_loop;
> +	u32 txdctl;
> +
> +	if (ixgbe_removed(hw->hw_addr))
> +		return;
> +
> +	/* disable all enabled Tx queues */
> +	for (i = 0; i < adapter->num_tx_queues; i++) {
> +		struct ixgbe_ring *ring = adapter->tx_ring[i];
> +		u8 reg_idx = ring->reg_idx;
> +
> +		IXGBE_WRITE_REG(hw, IXGBE_TXDCTL(reg_idx), IXGBE_TXDCTL_SWFLSH);
> +	}
> +
> +	/* disable all enabled XDP Tx queues */
> +	for (i = 0; i < adapter->num_xdp_queues; i++) {
> +		struct ixgbe_ring *ring = adapter->xdp_ring[i];
> +		u8 reg_idx = ring->reg_idx;
> +
> +		IXGBE_WRITE_REG(hw, IXGBE_TXDCTL(reg_idx), IXGBE_TXDCTL_SWFLSH);
> +	}
> +
> +	/* If the link is now up there shouldn't be much in the way of

I think you mean "not" up rather than "now" up, which has the opposite 
meaning and could be a bit confusing.

> +	 * pending transactions. Those that are left will be flushed out
> +	 * when the reset logic goes through the flush sequence to clean out
> +	 * the pending Tx transactions.
> +	 */
> +	if (!(IXGBE_READ_REG(hw, IXGBE_LINKS) & IXGBE_LINKS_UP))
> +		goto dma_engine_disable;
> +
> +	/* Determine our minimum delay interval. We will increase this value
> +	 * with each subsequent test. This way if the device returns quickly
> +	 * we should spend as little time as possible waiting, however as
> +	 * the time increases we will wait for larger periods of time.
> +	 *
> +	 * The trick here is that we increase the interval using the
> +	 * following pattern: 1x 3x 5x 7x 9x 11x 13x 15x 17x 19x. The result
> +	 * of that wait is that it totals up to 100x whatever interval we
> +	 * choose. Since our minimum wait is 100us we can just divide the
> +	 * total timeout by 100 to get our minimum delay interval.
> +	 */
> +	delay_interval = ixgbe_get_completion_timeout(adapter) / 100;
> +
> +	wait_loop = IXGBE_MAX_RX_DESC_POLL;
> +	wait_delay = delay_interval;
> +
> +	while (wait_loop--) {
> +		usleep_range(wait_delay, wait_delay + 10);
> +		wait_delay += delay_interval * 2;
> +		txdctl = 0;
> +
> +		/* OR together the reading of all the active TXDCTL registers,
> +		 * and then test the result. We need the disable to complete
> +		 * before we start freeing the memory and invalidating the
> +		 * DMA mappings.
> +		 */
> +		for (i = 0; i < adapter->num_tx_queues; i++) {
> +			struct ixgbe_ring *ring = adapter->tx_ring[i];
> +			u8 reg_idx = ring->reg_idx;
> +
> +			txdctl |= IXGBE_READ_REG(hw, IXGBE_TXDCTL(reg_idx));
> +		}
> +		for (i = 0; i < adapter->num_xdp_queues; i++) {
> +			struct ixgbe_ring *ring = adapter->xdp_ring[i];
> +			u8 reg_idx = ring->reg_idx;
> +
> +			txdctl |= IXGBE_READ_REG(hw, IXGBE_TXDCTL(reg_idx));
> +		}
> +
> +		if (!(txdctl & IXGBE_TXDCTL_ENABLE))
> +			goto dma_engine_disable;
> +	}
> +
> +	e_err(drv,
> +	      "TXDCTL.ENABLE for one or more queues not cleared within the polling period\n");
> +
> +dma_engine_disable:
> +	/* Disable the Tx DMA engine on 82599 and later MAC */
> +	switch (hw->mac.type) {
> +	case ixgbe_mac_82599EB:
> +	case ixgbe_mac_X540:
> +	case ixgbe_mac_X550:
> +	case ixgbe_mac_X550EM_x:
> +	case ixgbe_mac_x550em_a:
> +		IXGBE_WRITE_REG(hw, IXGBE_DMATXCTL,
> +				(IXGBE_READ_REG(hw, IXGBE_DMATXCTL) &
> +				 ~IXGBE_DMATXCTL_TE));
> +		/* fall through */
> +	default:
> +		break;
> +	}

Picky bike-shed thoughts: I realize you're just moving the previously 
existing code, but with the comment "... and later MAC" I would think 
the translation to code puts all the later MACs into the default and 
have the WRITE_REG() as the default action.  This would make it default 
to the correct action for any new MACs if someone forgot to fix up this 
part of the code.

	switch (hw->mac.type) {
	case ixgbe_mac_82598EB:
		break;

	case ixgbe_mac_82599EB:
	case ixgbe_mac_X540:
	case ixgbe_mac_X550:
	case ixgbe_mac_X550EM_x:
	case ixgbe_mac_x550em_a:
	default:
		IXGBE_WRITE_REG(hw, IXGBE_DMATXCTL,
				(IXGBE_READ_REG(hw, IXGBE_DMATXCTL) &
				 ~IXGBE_DMATXCTL_TE));
		break;
	}

Or even simpler,

	if (hw->mac.type != ixgbe_mac_82598EB)
		IXGBE_WRITE_REG(hw, IXGBE_DMATXCTL,
				(IXGBE_READ_REG(hw, IXGBE_DMATXCTL) &
				 ~IXGBE_DMATXCTL_TE));

Okay, I'll stop now.

Cheers,
sln

[Intel-wired-lan] [jkirsher/next-queue PATCH 2/2] ixgbe: Refactor queue disable logic to take completion time into account

[Alexander Duyck]

On Mon, Jul 23, 2018 at 9:34 AM, Shannon Nelson
<shannon.nelson@oracle.com> wrote:
> On 7/20/2018 3:29 PM, Alexander Duyck wrote:
>>
>> This change is meant to allow us to take completion time into account when
>> disabling queues. Previously we were just working with hard coded values
>> for how long we should wait. This worked fine for the standard case where
>> completion timeout was operating in the 50us to 50ms range, however on
>> platforms that have higher completion timeout times this was resulting in
>> Rx queues disable messages being displayed as we weren't waiting long
>> enough for outstanding Rx DMA completions.
>>
>> Signed-off-by: Alexander Duyck <alexander.h.duyck@intel.com>
>> ---
>
>
> [...]
>
>
>> +
>> +void ixgbe_disable_tx(struct ixgbe_adapter *adapter)
>> +{
>> +       unsigned long wait_delay, delay_interval;
>> +       struct ixgbe_hw *hw = &adapter->hw;
>> +       int i, wait_loop;
>> +       u32 txdctl;
>> +
>> +       if (ixgbe_removed(hw->hw_addr))
>> +               return;
>> +
>> +       /* disable all enabled Tx queues */
>> +       for (i = 0; i < adapter->num_tx_queues; i++) {
>> +               struct ixgbe_ring *ring = adapter->tx_ring[i];
>> +               u8 reg_idx = ring->reg_idx;
>> +
>> +               IXGBE_WRITE_REG(hw, IXGBE_TXDCTL(reg_idx),
>> IXGBE_TXDCTL_SWFLSH);
>> +       }
>> +
>> +       /* disable all enabled XDP Tx queues */
>> +       for (i = 0; i < adapter->num_xdp_queues; i++) {
>> +               struct ixgbe_ring *ring = adapter->xdp_ring[i];
>> +               u8 reg_idx = ring->reg_idx;
>> +
>> +               IXGBE_WRITE_REG(hw, IXGBE_TXDCTL(reg_idx),
>> IXGBE_TXDCTL_SWFLSH);
>> +       }
>> +
>> +       /* If the link is now up there shouldn't be much in the way of
>
>
> I think you mean "not" up rather than "now" up, which has the opposite
> meaning and could be a bit confusing.

Yeah, that is a typo.

>
>> +        * pending transactions. Those that are left will be flushed out
>> +        * when the reset logic goes through the flush sequence to clean
>> out
>> +        * the pending Tx transactions.
>> +        */
>> +       if (!(IXGBE_READ_REG(hw, IXGBE_LINKS) & IXGBE_LINKS_UP))
>> +               goto dma_engine_disable;
>> +
>> +       /* Determine our minimum delay interval. We will increase this
>> value
>> +        * with each subsequent test. This way if the device returns
>> quickly
>> +        * we should spend as little time as possible waiting, however as
>> +        * the time increases we will wait for larger periods of time.
>> +        *
>> +        * The trick here is that we increase the interval using the
>> +        * following pattern: 1x 3x 5x 7x 9x 11x 13x 15x 17x 19x. The
>> result
>> +        * of that wait is that it totals up to 100x whatever interval we
>> +        * choose. Since our minimum wait is 100us we can just divide the
>> +        * total timeout by 100 to get our minimum delay interval.
>> +        */
>> +       delay_interval = ixgbe_get_completion_timeout(adapter) / 100;
>> +
>> +       wait_loop = IXGBE_MAX_RX_DESC_POLL;
>> +       wait_delay = delay_interval;
>> +
>> +       while (wait_loop--) {
>> +               usleep_range(wait_delay, wait_delay + 10);
>> +               wait_delay += delay_interval * 2;
>> +               txdctl = 0;
>> +
>> +               /* OR together the reading of all the active TXDCTL
>> registers,
>> +                * and then test the result. We need the disable to
>> complete
>> +                * before we start freeing the memory and invalidating the
>> +                * DMA mappings.
>> +                */
>> +               for (i = 0; i < adapter->num_tx_queues; i++) {
>> +                       struct ixgbe_ring *ring = adapter->tx_ring[i];
>> +                       u8 reg_idx = ring->reg_idx;
>> +
>> +                       txdctl |= IXGBE_READ_REG(hw,
>> IXGBE_TXDCTL(reg_idx));
>> +               }
>> +               for (i = 0; i < adapter->num_xdp_queues; i++) {
>> +                       struct ixgbe_ring *ring = adapter->xdp_ring[i];
>> +                       u8 reg_idx = ring->reg_idx;
>> +
>> +                       txdctl |= IXGBE_READ_REG(hw,
>> IXGBE_TXDCTL(reg_idx));
>> +               }
>> +
>> +               if (!(txdctl & IXGBE_TXDCTL_ENABLE))
>> +                       goto dma_engine_disable;
>> +       }
>> +
>> +       e_err(drv,
>> +             "TXDCTL.ENABLE for one or more queues not cleared within the
>> polling period\n");
>> +
>> +dma_engine_disable:
>> +       /* Disable the Tx DMA engine on 82599 and later MAC */
>> +       switch (hw->mac.type) {
>> +       case ixgbe_mac_82599EB:
>> +       case ixgbe_mac_X540:
>> +       case ixgbe_mac_X550:
>> +       case ixgbe_mac_X550EM_x:
>> +       case ixgbe_mac_x550em_a:
>> +               IXGBE_WRITE_REG(hw, IXGBE_DMATXCTL,
>> +                               (IXGBE_READ_REG(hw, IXGBE_DMATXCTL) &
>> +                                ~IXGBE_DMATXCTL_TE));
>> +               /* fall through */
>> +       default:
>> +               break;
>> +       }
>
>
> Picky bike-shed thoughts: I realize you're just moving the previously
> existing code, but with the comment "... and later MAC" I would think the
> translation to code puts all the later MACs into the default and have the
> WRITE_REG() as the default action.  This would make it default to the
> correct action for any new MACs if someone forgot to fix up this part of the
> code.
>
>         switch (hw->mac.type) {
>         case ixgbe_mac_82598EB:
>                 break;
>
>         case ixgbe_mac_82599EB:
>         case ixgbe_mac_X540:
>         case ixgbe_mac_X550:
>         case ixgbe_mac_X550EM_x:
>         case ixgbe_mac_x550em_a:
>         default:
>                 IXGBE_WRITE_REG(hw, IXGBE_DMATXCTL,
>                                 (IXGBE_READ_REG(hw, IXGBE_DMATXCTL) &
>                                  ~IXGBE_DMATXCTL_TE));
>                 break;
>         }
>
> Or even simpler,
>
>         if (hw->mac.type != ixgbe_mac_82598EB)
>                 IXGBE_WRITE_REG(hw, IXGBE_DMATXCTL,
>                                 (IXGBE_READ_REG(hw, IXGBE_DMATXCTL) &
>                                  ~IXGBE_DMATXCTL_TE));
>
> Okay, I'll stop now.
>
> Cheers,
> sln

Yeah, for now I will probably leave the code as is, but this is
something that could be updated int he future. Either that or you are
always free to submit a patch.. :-)

- Alex