Skip to contentSkip to conversation

net/ixgbe: fix TDH register setting issue

PATCH Superseded Yanglong Wu 2 participants 6 messages Nov 16, 2018 – Nov 20, 2018 Network drivers

lore
Share & tools
lore

Yanglong Wu

author
#0 lore raw

PATCH 1/1 net/ixgbe: fix TDH register setting issue

The only time that software should write to the TDH register is after a reset (hardware reset or CTRL.RST) and before enabling the transmit function (TXDCTL.ENABLE). If software were to write to this register while the transmit function was enabled, the on-chip descriptor buffers might be invalidated and the hardware could become confused.

Fixes a8cdaf0964f7 ("net/ixgbe: remove redundant queue id checks")
Signed-off-by Yanglong Wu <[email protected]>
---
 drivers/net/ixgbe/ixgbe_rxtx.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/net/ixgbe/ixgbe_rxtx.c +1 -1
index 2f0262ae1..ddc7efa87 100644
@@ -5264,6 +5264,7 @@ ixgbe_dev_tx_queue_start(struct rte_eth_dev *dev, uint16_t tx_queue_id)
 	hw = IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
 
 	txq = dev->data->tx_queues[tx_queue_id];
+	IXGBE_WRITE_REG(hw, IXGBE_TDH(txq->reg_idx), 0);
 	txdctl = IXGBE_READ_REG(hw, IXGBE_TXDCTL(txq->reg_idx));
 	txdctl |= IXGBE_TXDCTL_ENABLE;
 	IXGBE_WRITE_REG(hw, IXGBE_TXDCTL(txq->reg_idx), txdctl);

@@ -5281,7 +5282,6 @@ ixgbe_dev_tx_queue_start(struct rte_eth_dev *dev, uint16_t tx_queue_id)
 				tx_queue_id);
 	}
 	rte_wmb();
-	IXGBE_WRITE_REG(hw, IXGBE_TDH(txq->reg_idx), 0);
 	IXGBE_WRITE_REG(hw, IXGBE_TDT(txq->reg_idx), 0);
 	dev->data->tx_queue_state[tx_queue_id] = RTE_ETH_QUEUE_STATE_STARTED;
Signature · 1 line
2.11.0
#1 lore raw

Hi Yanglong:

Quoted 9 lines

-----Original Message-----
From: Wu, Yanglong
Sent: Thursday, November 15, 2018 6:32 PM
To: [email protected]
Cc: Zhang, Qi Z <[email protected]>; Wu, Jingjing <[email protected]>; Byrne, Stephen1 <[email protected]>; Wu, Yanglong <[email protected]>
Subject: [PATCH] net/ixgbe: fix TDH register setting issue

Quoted 8 lines

The only time that software should write to the TDH register is after a reset (hardware reset or CTRL.RST) and before enabling the transmit function (TXDCTL.ENABLE).
If software were to write to this register while the transmit function was enabled, the on-chip descriptor buffers might be invalidated and the hardware could become confused.

Fixes a8cdaf0964f7 ("net/ixgbe: remove redundant queue id checks")

I try to understand why this patch is going to fix above commit.
seems the commit didn't change the register write order, and even if we go much early I saw the TDH register is always be written after TXDCTL.ENABL. Did I missed something?
btw, I'm also curious why only TDH matters but TDT does not.

Quoted 30 lines
Signed-off-by Yanglong Wu <[email protected]>
---
 drivers/net/ixgbe/ixgbe_rxtx.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/net/ixgbe/ixgbe_rxtx.c +1 -0
index 2f0262ae1..ddc7efa87 100644
@@ -5264,6 +5264,7 @@ ixgbe_dev_tx_queue_start(struct rte_eth_dev *dev,
 uint16_t tx_queue_id)
 	hw = IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
 
 	txq = dev->data->tx_queues[tx_queue_id];
+	IXGBE_WRITE_REG(hw, IXGBE_TDH(txq->reg_idx), 0);
 	txdctl = IXGBE_READ_REG(hw, IXGBE_TXDCTL(txq->reg_idx));
 	txdctl |= IXGBE_TXDCTL_ENABLE;

IXGBE_WRITE_REG(hw, IXGBE_TXDCTL(txq->reg_idx), txdctl); @@
-5281,7 +5282,6 @@ ixgbe_dev_tx_queue_start(struct rte_eth_dev *dev, uint16_t tx_queue_id)

drivers/net/ixgbe/ixgbe_rxtx.c +0 -1
 				tx_queue_id);
 	}
 	rte_wmb();
-	IXGBE_WRITE_REG(hw, IXGBE_TDH(txq->reg_idx), 0);
 	IXGBE_WRITE_REG(hw, IXGBE_TDT(txq->reg_idx), 0);
 	dev->data->tx_queue_state[tx_queue_id] =

RTE_ETH_QUEUE_STATE_STARTED;

--
2.11.0

author
#2 lore raw

Hi, qi
I don't understand why dose this patch not change register write order. THD is write before TXDCTL.ENABL.

The Intel 82599 data sheet (https://www.intel.com/content/dam/www/public/us/en/documents/datasheets/82599-10-gbe-controller-datasheet.pdf, §8.2.3.9.8) states that "The only time that software should write to [the TDH] register is after a reset (hardware reset or CTRL.RST) and before enabling the transmit function (TXDCTL.ENABLE). If software were to write to this register while the transmit function was enabled, the on-chip descriptor buffers might be invalidated and the hardware could become confused".

Regard
yanglong

-----Original Message-----
From: Zhang, Qi Z
Sent: Saturday, November 17, 2018 3:52 AM
To: Wu, Yanglong <[email protected]>; [email protected]
Cc: Wu, Jingjing <[email protected]>; Byrne, Stephen1 <[email protected]>
Subject: RE: [PATCH] net/ixgbe: fix TDH register setting issue

Hi Yanglong:

Quoted 9 lines

-----Original Message-----
From: Wu, Yanglong
Sent: Thursday, November 15, 2018 6:32 PM
To: [email protected]
Cc: Zhang, Qi Z <[email protected]>; Wu, Jingjing <[email protected]>; Byrne, Stephen1 <[email protected]>; Wu, Yanglong <[email protected]>
Subject: [PATCH] net/ixgbe: fix TDH register setting issue

Quoted 8 lines

The only time that software should write to the TDH register is after a reset (hardware reset or CTRL.RST) and before enabling the transmit function (TXDCTL.ENABLE).
If software were to write to this register while the transmit function was enabled, the on-chip descriptor buffers might be invalidated and the hardware could become confused.

Fixes a8cdaf0964f7 ("net/ixgbe: remove redundant queue id checks")

I try to understand why this patch is going to fix above commit.
seems the commit didn't change the register write order, and even if we go much early I saw the TDH register is always be written after TXDCTL.ENABL. Did I missed something?
btw, I'm also curious why only TDH matters but TDT does not.

Quoted 30 lines
Signed-off-by Yanglong Wu <[email protected]>
---
 drivers/net/ixgbe/ixgbe_rxtx.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
(quoted diff) +0 -0

b/drivers/net/ixgbe/ixgbe_rxtx.c index 2f0262ae1..ddc7efa87 100644

drivers/net/ixgbe/ixgbe_rxtx.c +1 -0
@@ -5264,6 +5264,7 @@ ixgbe_dev_tx_queue_start(struct rte_eth_dev 
 *dev, uint16_t tx_queue_id)
 	hw = IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
 
 	txq = dev->data->tx_queues[tx_queue_id];
+	IXGBE_WRITE_REG(hw, IXGBE_TDH(txq->reg_idx), 0);
 	txdctl = IXGBE_READ_REG(hw, IXGBE_TXDCTL(txq->reg_idx));
 	txdctl |= IXGBE_TXDCTL_ENABLE;

IXGBE_WRITE_REG(hw, IXGBE_TXDCTL(txq->reg_idx), txdctl); @@
-5281,7 +5282,6 @@ ixgbe_dev_tx_queue_start(struct rte_eth_dev *dev, uint16_t tx_queue_id)

drivers/net/ixgbe/ixgbe_rxtx.c +0 -1
 				tx_queue_id);
 	}
 	rte_wmb();
-	IXGBE_WRITE_REG(hw, IXGBE_TDH(txq->reg_idx), 0);
 	IXGBE_WRITE_REG(hw, IXGBE_TDT(txq->reg_idx), 0);
 	dev->data->tx_queue_state[tx_queue_id] =

RTE_ETH_QUEUE_STATE_STARTED;

--
2.11.0

Tested-by Waterman Cao Acked-by Thomas Monjalon
#3 lore raw
Quoted 14 lines

-----Original Message-----
From: Wu, Yanglong
Sent: Monday, November 19, 2018 6:12 PM
To: Zhang, Qi Z <[email protected]>; [email protected]
Cc: Wu, Jingjing <[email protected]>; Byrne, Stephen1 <[email protected]>
Subject: RE: [PATCH] net/ixgbe: fix TDH register setting issue

Hi, qi
I don't understand why dose this patch not change register write order. THD is write before TXDCTL.ENABL.

Quoted 8 lines

The Intel 82599 data sheet
(https://www.intel.com/content/dam/www/public/us/en/documents/datashe
ets/82599-10-gbe-controller-datasheet.pdf, §8.2.3.9.8) states that "The only time that software should write to [the TDH] register is after a reset (hardware reset or CTRL.RST) and before enabling the transmit function (TXDCTL.ENABLE). If software were to write to this register while the transmit function was enabled, the on-chip descriptor buffers might be invalidated and the hardware could become confused".

OK got it, I think you can use below commit as to fix.

commit 029fd06d40fa8cff7f562dc85d7cc4609d5c91fb

Author: Ouyang Changchun <[email protected]>
Date:   Wed May 28 16:06:37 2014 +0800

ixgbe: queue start and stop

This patch implements queue start and stop functionality in IXGBE PMD; it also enable hardware loopback for VMDQ mode in IXGBE PMD.

Signed-off-by Ouyang Changchun <[email protected]>
Tested-by Waterman Cao <[email protected]>
Acked-by Thomas Monjalon <[email protected]>
Quoted 68 lines

Regard
yanglong

-----Original Message-----
From: Zhang, Qi Z
Sent: Saturday, November 17, 2018 3:52 AM
To: Wu, Yanglong <[email protected]>; [email protected]
Cc: Wu, Jingjing <[email protected]>; Byrne, Stephen1 <[email protected]>
Subject: RE: [PATCH] net/ixgbe: fix TDH register setting issue

Hi Yanglong:

Quoted 9 lines

-----Original Message-----
From: Wu, Yanglong
Sent: Thursday, November 15, 2018 6:32 PM
To: [email protected]
Cc: Zhang, Qi Z <[email protected]>; Wu, Jingjing <[email protected]>; Byrne, Stephen1 <[email protected]>; Wu, Yanglong <[email protected]>
Subject: [PATCH] net/ixgbe: fix TDH register setting issue

Quoted 8 lines

The only time that software should write to the TDH register is after a reset (hardware reset or CTRL.RST) and before enabling the transmit function (TXDCTL.ENABLE).
If software were to write to this register while the transmit function was enabled, the on-chip descriptor buffers might be invalidated and the hardware could become confused.

Fixes a8cdaf0964f7 ("net/ixgbe: remove redundant queue id checks")

I try to understand why this patch is going to fix above commit. seems the commit didn't change the register write order, and even if we go much early I saw the TDH register is always be written after TXDCTL.ENABL. Did I missed something?
btw, I'm also curious why only TDH matters but TDT does not.

Quoted 30 lines
Signed-off-by Yanglong Wu <[email protected]>
---
 drivers/net/ixgbe/ixgbe_rxtx.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
(quoted diff) +0 -0

b/drivers/net/ixgbe/ixgbe_rxtx.c index 2f0262ae1..ddc7efa87 100644

drivers/net/ixgbe/ixgbe_rxtx.c +1 -0
@@ -5264,6 +5264,7 @@ ixgbe_dev_tx_queue_start(struct rte_eth_dev
 *dev, uint16_t tx_queue_id)
 	hw = IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
 
 	txq = dev->data->tx_queues[tx_queue_id];
+	IXGBE_WRITE_REG(hw, IXGBE_TDH(txq->reg_idx), 0);
 	txdctl = IXGBE_READ_REG(hw, IXGBE_TXDCTL(txq->reg_idx));
 	txdctl |= IXGBE_TXDCTL_ENABLE;

IXGBE_WRITE_REG(hw, IXGBE_TXDCTL(txq->reg_idx), txdctl); @@
-5281,7 +5282,6 @@ ixgbe_dev_tx_queue_start(struct rte_eth_dev *dev, uint16_t tx_queue_id)

drivers/net/ixgbe/ixgbe_rxtx.c +0 -1
 				tx_queue_id);
 	}
 	rte_wmb();
-	IXGBE_WRITE_REG(hw, IXGBE_TDH(txq->reg_idx), 0);
 	IXGBE_WRITE_REG(hw, IXGBE_TDT(txq->reg_idx), 0);
 	dev->data->tx_queue_state[tx_queue_id] =

RTE_ETH_QUEUE_STATE_STARTED;

--
2.11.0

author
#4 lore raw

PATCH v2 1/1 net/ixgbe: fix TDH register setting issue

In reply to Yanglong Wu

The only time that software should write to the TDH register is after a reset (hardware reset or CTRL.RST) and before enabling the transmit function (TXDCTL.ENABLE). If software were to write to this register while the transmit function was enabled, the on-chip descriptor buffers might be invalidated and the hardware could become confused.

cc [email protected]

Signed-off-by Yanglong Wu <[email protected]>

---
v2:
change fix commit

---
 drivers/net/ixgbe/ixgbe_rxtx.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/net/ixgbe/ixgbe_rxtx.c +1 -1
index 2f0262ae1..ddc7efa87 100644
@@ -5264,6 +5264,7 @@ ixgbe_dev_tx_queue_start(struct rte_eth_dev *dev, uint16_t tx_queue_id)
 	hw = IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
 
 	txq = dev->data->tx_queues[tx_queue_id];
+	IXGBE_WRITE_REG(hw, IXGBE_TDH(txq->reg_idx), 0);
 	txdctl = IXGBE_READ_REG(hw, IXGBE_TXDCTL(txq->reg_idx));
 	txdctl |= IXGBE_TXDCTL_ENABLE;
 	IXGBE_WRITE_REG(hw, IXGBE_TXDCTL(txq->reg_idx), txdctl);

@@ -5281,7 +5282,6 @@ ixgbe_dev_tx_queue_start(struct rte_eth_dev *dev, uint16_t tx_queue_id)
 				tx_queue_id);
 	}
 	rte_wmb();
-	IXGBE_WRITE_REG(hw, IXGBE_TDH(txq->reg_idx), 0);
 	IXGBE_WRITE_REG(hw, IXGBE_TDT(txq->reg_idx), 0);
 	dev->data->tx_queue_state[tx_queue_id] = RTE_ETH_QUEUE_STATE_STARTED;
Signature · 1 line
2.11.0
Acked-by on v2 1/1 Applied
#5 lore raw
Quoted 10 lines

-----Original Message-----
From: Wu, Yanglong
Sent: Monday, November 19, 2018 9:59 PM
To: [email protected]
Cc: Zhang, Qi Z <[email protected]>; Wu, Jingjing <[email protected]>; Wu, Yanglong <[email protected]>
Subject: [PATCH v2] net/ixgbe: fix TDH register setting issue

Quoted 8 lines

The only time that software should write to the TDH register is after a reset (hardware reset or CTRL.RST) and before enabling the transmit function (TXDCTL.ENABLE).
If software were to write to this register while the transmit function was enabled, the on-chip descriptor buffers might be invalidated and the hardware could become confused.

cc [email protected]

Cc [email protected]
Quoted 1 line
Signed-off-by Yanglong Wu <[email protected]>
Acked-by Qi Zhang <[email protected]>

Applied to dpdk-next-net-intel with adding below fix line

Fixes 029fd06d40fa ("ixgbe: queue start and stop")

Thanks
Qi