[dpdk-dev] [PATCH] net/ixgbe: fix TDH register setting issue

[Wu, Yanglong]

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 <yanglong.wu at intel.com>; dev at dpdk.org
Cc: Wu, Jingjing <jingjing.wu at intel.com>; Byrne, Stephen1 <stephen1.byrne at intel.com>
Subject: RE: [PATCH] net/ixgbe: fix TDH register setting issue

Hi Yanglong:

> -----Original Message-----
> From: Wu, Yanglong
> Sent: Thursday, November 15, 2018 6:32 PM
> To: dev at dpdk.org
> Cc: Zhang, Qi Z <qi.z.zhang at intel.com>; Wu, Jingjing 
> <jingjing.wu at intel.com>; Byrne, Stephen1 <stephen1.byrne at intel.com>; 
> Wu, Yanglong <yanglong.wu at intel.com>
> Subject: [PATCH] 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")

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.

> Signed-off-by: Yanglong Wu <yanglong.wu at intel.com>
> ---
>  drivers/net/ixgbe/ixgbe_rxtx.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ixgbe/ixgbe_rxtx.c 
> b/drivers/net/ixgbe/ixgbe_rxtx.c index 2f0262ae1..ddc7efa87 100644
> --- a/drivers/net/ixgbe/ixgbe_rxtx.c
> +++ b/drivers/net/ixgbe/ixgbe_rxtx.c
> @@ -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;
> 
> --
> 2.11.0