DPDK patches and discussions
 help / color / mirror / Atom feed
* [dpdk-dev] [PATCH] net/ixgbe: fix fdirctrl register setting
@ 2020-12-11  1:35 dapengx.yu
  2020-12-15  3:00 ` Zhou, JunX W
                   ` (2 more replies)
  0 siblings, 3 replies; 8+ messages in thread
From: dapengx.yu @ 2020-12-11  1:35 UTC (permalink / raw)
  To: jia.guo; +Cc: dev, YU DAPENG, qi.z.zhang

From: YU DAPENG <dapengx.yu@intel.com>

The function ixgbe_fdir_set_flexbytes_offset is used when create FDir
rule for flexbytes. It set a register: FDIRCTRL.FLEX_OFFSET, which
cause that even if the FDir flexbytes rule is destroyed, the rule still
direct the packet and transfer it to the wrong place. It is because
Setting FDIRCTRL shall only be permitted on Flow Director
initialization flow or Clearing the Flow Director table, otherwise
unexpected happens. In order to evade the limit, add code to make
setting FDIRCTRL work without unexpected effects.

Fixes: f35fec63dde1 ("net/ixgbe: enable flex bytes for generic flow API")
Cc: qi.z.zhang@intel.com

Signed-off-by: YU DAPENG <dapengx.yu@intel.com>
---
 drivers/net/ixgbe/ixgbe_fdir.c | 23 +++++++++++++++++++++++
 drivers/net/ixgbe/ixgbe_flow.c |  7 +++----
 2 files changed, 26 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ixgbe/ixgbe_fdir.c b/drivers/net/ixgbe/ixgbe_fdir.c
index a0fab5070..56dddd56b 100644
--- a/drivers/net/ixgbe/ixgbe_fdir.c
+++ b/drivers/net/ixgbe/ixgbe_fdir.c
@@ -503,9 +503,32 @@ ixgbe_fdir_set_flexbytes_offset(struct rte_eth_dev *dev,
 				uint16_t offset)
 {
 	struct ixgbe_hw *hw = IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
+	struct ixgbe_hw_fdir_info *fdir_info =
+		IXGBE_DEV_PRIVATE_TO_FDIR_INFO(dev->data->dev_private);
 	uint32_t fdirctrl;
 	int i;
 
+	if (fdir_info->flex_bytes_offset == offset)
+		return 0;
+
+	fdir_info->flex_bytes_offset = offset;
+
+	/*
+	 * 82599 adapters flow director init flow cannot be restarted,
+	 * Workaround 82599 silicon errata by performing the following steps
+	 * before re-writing the FDIRCTRL control register with the same value.
+	 * - write 1 to bit 8 of FDIRCMD register &
+	 * - write 0 to bit 8 of FDIRCMD register
+	 */
+	IXGBE_WRITE_REG(hw, IXGBE_FDIRCMD,
+			(IXGBE_READ_REG(hw, IXGBE_FDIRCMD) |
+			 IXGBE_FDIRCMD_CLEARHT));
+	IXGBE_WRITE_FLUSH(hw);
+	IXGBE_WRITE_REG(hw, IXGBE_FDIRCMD,
+			(IXGBE_READ_REG(hw, IXGBE_FDIRCMD) &
+			 ~IXGBE_FDIRCMD_CLEARHT));
+	IXGBE_WRITE_FLUSH(hw);
+
 	fdirctrl = IXGBE_READ_REG(hw, IXGBE_FDIRCTRL);
 
 	fdirctrl &= ~IXGBE_FDIRCTRL_FLEX_MASK;
diff --git a/drivers/net/ixgbe/ixgbe_flow.c b/drivers/net/ixgbe/ixgbe_flow.c
index 39f6ed73f..b37541d9b 100644
--- a/drivers/net/ixgbe/ixgbe_flow.c
+++ b/drivers/net/ixgbe/ixgbe_flow.c
@@ -3137,8 +3137,6 @@ ixgbe_flow_create(struct rte_eth_dev *dev,
 				rte_memcpy(&fdir_info->mask,
 					&fdir_rule.mask,
 					sizeof(struct ixgbe_hw_fdir_mask));
-				fdir_info->flex_bytes_offset =
-					fdir_rule.flex_bytes_offset;
 
 				if (fdir_rule.mask.flex_bytes_mask)
 					ixgbe_fdir_set_flexbytes_offset(dev,
@@ -3161,8 +3159,9 @@ ixgbe_flow_create(struct rte_eth_dev *dev,
 				if (ret)
 					goto out;
 
-				if (fdir_info->flex_bytes_offset !=
-						fdir_rule.flex_bytes_offset)
+				if (fdir_rule.mask.flex_bytes_mask &&
+					(fdir_info->flex_bytes_offset !=
+						fdir_rule.flex_bytes_offset))
 					goto out;
 			}
 		}
-- 
2.26.2.windows.1


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

* Re: [dpdk-dev] [PATCH] net/ixgbe: fix fdirctrl register setting
  2020-12-11  1:35 [dpdk-dev] [PATCH] net/ixgbe: fix fdirctrl register setting dapengx.yu
@ 2020-12-15  3:00 ` Zhou, JunX W
  2020-12-15  4:36 ` Guo, Jia
  2020-12-15 10:10 ` [dpdk-dev] [PATCH v2] " dapengx.yu
  2 siblings, 0 replies; 8+ messages in thread
From: Zhou, JunX W @ 2020-12-15  3:00 UTC (permalink / raw)
  To: Yu, DapengX, Guo, Jia; +Cc: dev, Yu, DapengX, Zhang, Qi Z

Tested-by: Zhou, Jun <junx.w.zhou@intel.com>

-----Original Message-----
From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of dapengx.yu@intel.com
Sent: Friday, December 11, 2020 9:35 AM
To: Guo, Jia <jia.guo@intel.com>
Cc: dev@dpdk.org; Yu, DapengX <dapengx.yu@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com>
Subject: [dpdk-dev] [PATCH] net/ixgbe: fix fdirctrl register setting

From: YU DAPENG <dapengx.yu@intel.com>

The function ixgbe_fdir_set_flexbytes_offset is used when create FDir rule for flexbytes. It set a register: FDIRCTRL.FLEX_OFFSET, which cause that even if the FDir flexbytes rule is destroyed, the rule still direct the packet and transfer it to the wrong place. It is because Setting FDIRCTRL shall only be permitted on Flow Director initialization flow or Clearing the Flow Director table, otherwise unexpected happens. In order to evade the limit, add code to make setting FDIRCTRL work without unexpected effects.

Fixes: f35fec63dde1 ("net/ixgbe: enable flex bytes for generic flow API")
Cc: qi.z.zhang@intel.com

Signed-off-by: YU DAPENG <dapengx.yu@intel.com>
---
 drivers/net/ixgbe/ixgbe_fdir.c | 23 +++++++++++++++++++++++  drivers/net/ixgbe/ixgbe_flow.c |  7 +++----
 2 files changed, 26 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ixgbe/ixgbe_fdir.c b/drivers/net/ixgbe/ixgbe_fdir.c index a0fab5070..56dddd56b 100644
--- a/drivers/net/ixgbe/ixgbe_fdir.c
+++ b/drivers/net/ixgbe/ixgbe_fdir.c
@@ -503,9 +503,32 @@ ixgbe_fdir_set_flexbytes_offset(struct rte_eth_dev *dev,
 				uint16_t offset)
 {
 	struct ixgbe_hw *hw = IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
+	struct ixgbe_hw_fdir_info *fdir_info =
+		IXGBE_DEV_PRIVATE_TO_FDIR_INFO(dev->data->dev_private);
 	uint32_t fdirctrl;
 	int i;
 
+	if (fdir_info->flex_bytes_offset == offset)
+		return 0;
+
+	fdir_info->flex_bytes_offset = offset;
+
+	/*
+	 * 82599 adapters flow director init flow cannot be restarted,
+	 * Workaround 82599 silicon errata by performing the following steps
+	 * before re-writing the FDIRCTRL control register with the same value.
+	 * - write 1 to bit 8 of FDIRCMD register &
+	 * - write 0 to bit 8 of FDIRCMD register
+	 */
+	IXGBE_WRITE_REG(hw, IXGBE_FDIRCMD,
+			(IXGBE_READ_REG(hw, IXGBE_FDIRCMD) |
+			 IXGBE_FDIRCMD_CLEARHT));
+	IXGBE_WRITE_FLUSH(hw);
+	IXGBE_WRITE_REG(hw, IXGBE_FDIRCMD,
+			(IXGBE_READ_REG(hw, IXGBE_FDIRCMD) &
+			 ~IXGBE_FDIRCMD_CLEARHT));
+	IXGBE_WRITE_FLUSH(hw);
+
 	fdirctrl = IXGBE_READ_REG(hw, IXGBE_FDIRCTRL);
 
 	fdirctrl &= ~IXGBE_FDIRCTRL_FLEX_MASK; diff --git a/drivers/net/ixgbe/ixgbe_flow.c b/drivers/net/ixgbe/ixgbe_flow.c index 39f6ed73f..b37541d9b 100644
--- a/drivers/net/ixgbe/ixgbe_flow.c
+++ b/drivers/net/ixgbe/ixgbe_flow.c
@@ -3137,8 +3137,6 @@ ixgbe_flow_create(struct rte_eth_dev *dev,
 				rte_memcpy(&fdir_info->mask,
 					&fdir_rule.mask,
 					sizeof(struct ixgbe_hw_fdir_mask));
-				fdir_info->flex_bytes_offset =
-					fdir_rule.flex_bytes_offset;
 
 				if (fdir_rule.mask.flex_bytes_mask)
 					ixgbe_fdir_set_flexbytes_offset(dev,
@@ -3161,8 +3159,9 @@ ixgbe_flow_create(struct rte_eth_dev *dev,
 				if (ret)
 					goto out;
 
-				if (fdir_info->flex_bytes_offset !=
-						fdir_rule.flex_bytes_offset)
+				if (fdir_rule.mask.flex_bytes_mask &&
+					(fdir_info->flex_bytes_offset !=
+						fdir_rule.flex_bytes_offset))
 					goto out;
 			}
 		}
--
2.26.2.windows.1


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

* Re: [dpdk-dev] [PATCH] net/ixgbe: fix fdirctrl register setting
  2020-12-11  1:35 [dpdk-dev] [PATCH] net/ixgbe: fix fdirctrl register setting dapengx.yu
  2020-12-15  3:00 ` Zhou, JunX W
@ 2020-12-15  4:36 ` Guo, Jia
  2020-12-16  0:55   ` Yu, DapengX
  2020-12-15 10:10 ` [dpdk-dev] [PATCH v2] " dapengx.yu
  2 siblings, 1 reply; 8+ messages in thread
From: Guo, Jia @ 2020-12-15  4:36 UTC (permalink / raw)
  To: Yu, DapengX; +Cc: dev, Yu, DapengX, Zhang, Qi Z

Hi, dapeng

> -----Original Message-----
> From: dapengx.yu@intel.com <dapengx.yu@intel.com>
> Sent: Friday, December 11, 2020 9:35 AM
> To: Guo, Jia <jia.guo@intel.com>
> Cc: dev@dpdk.org; Yu, DapengX <dapengx.yu@intel.com>; Zhang, Qi Z
> <qi.z.zhang@intel.com>
> Subject: [PATCH] net/ixgbe: fix fdirctrl register setting
> 
> From: YU DAPENG <dapengx.yu@intel.com>
> 
> The function ixgbe_fdir_set_flexbytes_offset is used when create FDir rule
> for flexbytes. It set a register: FDIRCTRL.FLEX_OFFSET, which cause that even
> if the FDir flexbytes rule is destroyed, the rule still direct the packet and
> transfer it to the wrong place. It is because Setting FDIRCTRL shall only be
> permitted on Flow Director initialization flow or Clearing the Flow Director
> table, otherwise unexpected happens. In order to evade the limit, add code
> to make setting FDIRCTRL work without unexpected effects.
> 
> Fixes: f35fec63dde1 ("net/ixgbe: enable flex bytes for generic flow API")
> Cc: qi.z.zhang@intel.com
> 
> Signed-off-by: YU DAPENG <dapengx.yu@intel.com>
> ---
>  drivers/net/ixgbe/ixgbe_fdir.c | 23 +++++++++++++++++++++++
> drivers/net/ixgbe/ixgbe_flow.c |  7 +++----
>  2 files changed, 26 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/ixgbe/ixgbe_fdir.c b/drivers/net/ixgbe/ixgbe_fdir.c
> index a0fab5070..56dddd56b 100644
> --- a/drivers/net/ixgbe/ixgbe_fdir.c
> +++ b/drivers/net/ixgbe/ixgbe_fdir.c
> @@ -503,9 +503,32 @@ ixgbe_fdir_set_flexbytes_offset(struct rte_eth_dev
> *dev,
>  				uint16_t offset)
>  {
>  	struct ixgbe_hw *hw = IXGBE_DEV_PRIVATE_TO_HW(dev->data-
> >dev_private);
> +	struct ixgbe_hw_fdir_info *fdir_info =
> +		IXGBE_DEV_PRIVATE_TO_FDIR_INFO(dev->data-
> >dev_private);
>  	uint32_t fdirctrl;
>  	int i;
> 
> +	if (fdir_info->flex_bytes_offset == offset)
> +		return 0;
> +
> +	fdir_info->flex_bytes_offset = offset;

Should this value assign at the end of the polling checking?
And could you kindly help to check is the timeout checking (i >= IXGBE_FDIR_INIT_DONE_POLL)
also need in the "ixgbe_fdir_set_flexbytes_offset"?

> +
> +	/*

/* -> /**

> +	 * 82599 adapters flow director init flow cannot be restarted,
> +	 * Workaround 82599 silicon errata by performing the following steps
> +	 * before re-writing the FDIRCTRL control register with the same
> value.
> +	 * - write 1 to bit 8 of FDIRCMD register &
> +	 * - write 0 to bit 8 of FDIRCMD register
> +	 */
> +	IXGBE_WRITE_REG(hw, IXGBE_FDIRCMD,
> +			(IXGBE_READ_REG(hw, IXGBE_FDIRCMD) |
> +			 IXGBE_FDIRCMD_CLEARHT));
> +	IXGBE_WRITE_FLUSH(hw);
> +	IXGBE_WRITE_REG(hw, IXGBE_FDIRCMD,
> +			(IXGBE_READ_REG(hw, IXGBE_FDIRCMD) &
> +			 ~IXGBE_FDIRCMD_CLEARHT));
> +	IXGBE_WRITE_FLUSH(hw);
> +

So you mean the above steps should be done before re-writing the FDIRCTRL, just like
the Flow Director initialization flow or Clearing the Flow Director, right?
If so, please refine your commit log and make it more clear, since Setting FDIRCTRL shall is not only be
permitted on Flow Director initialization flow or Clearing the Flow Director table.

>  	fdirctrl = IXGBE_READ_REG(hw, IXGBE_FDIRCTRL);
> 
>  	fdirctrl &= ~IXGBE_FDIRCTRL_FLEX_MASK; diff --git
> a/drivers/net/ixgbe/ixgbe_flow.c b/drivers/net/ixgbe/ixgbe_flow.c index
> 39f6ed73f..b37541d9b 100644
> --- a/drivers/net/ixgbe/ixgbe_flow.c
> +++ b/drivers/net/ixgbe/ixgbe_flow.c
> @@ -3137,8 +3137,6 @@ ixgbe_flow_create(struct rte_eth_dev *dev,
>  				rte_memcpy(&fdir_info->mask,
>  					&fdir_rule.mask,
>  					sizeof(struct ixgbe_hw_fdir_mask));
> -				fdir_info->flex_bytes_offset =
> -					fdir_rule.flex_bytes_offset;
> 
>  				if (fdir_rule.mask.flex_bytes_mask)
>  					ixgbe_fdir_set_flexbytes_offset(dev,
> @@ -3161,8 +3159,9 @@ ixgbe_flow_create(struct rte_eth_dev *dev,
>  				if (ret)
>  					goto out;
> 
> -				if (fdir_info->flex_bytes_offset !=
> -						fdir_rule.flex_bytes_offset)
> +				if (fdir_rule.mask.flex_bytes_mask &&
> +					(fdir_info->flex_bytes_offset !=

Suggest you check the line align. 

> +						fdir_rule.flex_bytes_offset))

This line is the same as above.

>  					goto out;
>  			}
>  		}
> --
> 2.26.2.windows.1


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

* [dpdk-dev] [PATCH v2] net/ixgbe: fix fdirctrl register setting
  2020-12-11  1:35 [dpdk-dev] [PATCH] net/ixgbe: fix fdirctrl register setting dapengx.yu
  2020-12-15  3:00 ` Zhou, JunX W
  2020-12-15  4:36 ` Guo, Jia
@ 2020-12-15 10:10 ` dapengx.yu
  2020-12-22  6:51   ` Zhou, JunX W
  2 siblings, 1 reply; 8+ messages in thread
From: dapengx.yu @ 2020-12-15 10:10 UTC (permalink / raw)
  To: jia.guo; +Cc: dev, YU DAPENG, stable

From: YU DAPENG <dapengx.yu@intel.com>

The function ixgbe_fdir_set_flexbytes_offset is used when create FDir
rule for flexbytes. It set a register: FDIRCTRL.FLEX_OFFSET, which
cause that even if the FDir flexbytes rule is destroyed, the rule still
direct the packet and transfer it to the wrong place. It is because
setting FDIRCTRL shall only be permitted on Flow Director
initialization flow or clearing the Flow Director table according to
intel datasheet, otherwise unexpected happens. In order to evade the
limit, add code to set FDIRCMD.CLEARHT to 1b and then clear it back to
0b to make the setting act like the Flow Director initialization flow
or clearing the Flow Director table.

Fixes: f35fec63dde1 ("net/ixgbe: enable flex bytes for generic flow API")
Cc: stable@dpdk.org

Signed-off-by: YU DAPENG <dapengx.yu@intel.com>
---
 drivers/net/ixgbe/ixgbe_fdir.c | 29 +++++++++++++++++++++++++++++
 drivers/net/ixgbe/ixgbe_flow.c | 15 ++++++++-------
 2 files changed, 37 insertions(+), 7 deletions(-)

diff --git a/drivers/net/ixgbe/ixgbe_fdir.c b/drivers/net/ixgbe/ixgbe_fdir.c
index a0fab5070..11b9effeb 100644
--- a/drivers/net/ixgbe/ixgbe_fdir.c
+++ b/drivers/net/ixgbe/ixgbe_fdir.c
@@ -503,9 +503,30 @@ ixgbe_fdir_set_flexbytes_offset(struct rte_eth_dev *dev,
 				uint16_t offset)
 {
 	struct ixgbe_hw *hw = IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
+	struct ixgbe_hw_fdir_info *fdir_info =
+		IXGBE_DEV_PRIVATE_TO_FDIR_INFO(dev->data->dev_private);
 	uint32_t fdirctrl;
 	int i;
 
+	if (fdir_info->flex_bytes_offset == offset)
+		return 0;
+
+	/**
+	 * 82599 adapters flow director init flow cannot be restarted,
+	 * Workaround 82599 silicon errata by performing the following steps
+	 * before re-writing the FDIRCTRL control register with the same value.
+	 * - write 1 to bit 8 of FDIRCMD register &
+	 * - write 0 to bit 8 of FDIRCMD register
+	 */
+	IXGBE_WRITE_REG(hw, IXGBE_FDIRCMD,
+			(IXGBE_READ_REG(hw, IXGBE_FDIRCMD) |
+			 IXGBE_FDIRCMD_CLEARHT));
+	IXGBE_WRITE_FLUSH(hw);
+	IXGBE_WRITE_REG(hw, IXGBE_FDIRCMD,
+			(IXGBE_READ_REG(hw, IXGBE_FDIRCMD) &
+			 ~IXGBE_FDIRCMD_CLEARHT));
+	IXGBE_WRITE_FLUSH(hw);
+
 	fdirctrl = IXGBE_READ_REG(hw, IXGBE_FDIRCTRL);
 
 	fdirctrl &= ~IXGBE_FDIRCTRL_FLEX_MASK;
@@ -520,6 +541,14 @@ ixgbe_fdir_set_flexbytes_offset(struct rte_eth_dev *dev,
 			break;
 		msec_delay(1);
 	}
+
+	if (i >= IXGBE_FDIR_INIT_DONE_POLL) {
+		PMD_DRV_LOG(ERR, "Flow Director poll time exceeded!");
+		return -ETIMEDOUT;
+	}
+
+	fdir_info->flex_bytes_offset = offset;
+
 	return 0;
 }
 
diff --git a/drivers/net/ixgbe/ixgbe_flow.c b/drivers/net/ixgbe/ixgbe_flow.c
index 39f6ed73f..9aeb2e4a4 100644
--- a/drivers/net/ixgbe/ixgbe_flow.c
+++ b/drivers/net/ixgbe/ixgbe_flow.c
@@ -3137,13 +3137,13 @@ ixgbe_flow_create(struct rte_eth_dev *dev,
 				rte_memcpy(&fdir_info->mask,
 					&fdir_rule.mask,
 					sizeof(struct ixgbe_hw_fdir_mask));
-				fdir_info->flex_bytes_offset =
-					fdir_rule.flex_bytes_offset;
 
-				if (fdir_rule.mask.flex_bytes_mask)
-					ixgbe_fdir_set_flexbytes_offset(dev,
+				if (fdir_rule.mask.flex_bytes_mask) {
+					ret = ixgbe_fdir_set_flexbytes_offset(dev,
 						fdir_rule.flex_bytes_offset);
-
+					if (ret)
+						goto out;
+				}
 				ret = ixgbe_fdir_set_input_mask(dev);
 				if (ret)
 					goto out;
@@ -3161,8 +3161,9 @@ ixgbe_flow_create(struct rte_eth_dev *dev,
 				if (ret)
 					goto out;
 
-				if (fdir_info->flex_bytes_offset !=
-						fdir_rule.flex_bytes_offset)
+				if (fdir_rule.mask.flex_bytes_mask &&
+				    fdir_info->flex_bytes_offset !=
+				    fdir_rule.flex_bytes_offset)
 					goto out;
 			}
 		}
-- 
2.26.2.windows.1


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

* Re: [dpdk-dev] [PATCH] net/ixgbe: fix fdirctrl register setting
  2020-12-15  4:36 ` Guo, Jia
@ 2020-12-16  0:55   ` Yu, DapengX
  0 siblings, 0 replies; 8+ messages in thread
From: Yu, DapengX @ 2020-12-16  0:55 UTC (permalink / raw)
  To: Guo, Jia; +Cc: dev, Zhang, Qi Z

Hi Jia,
Thanks for your comments!
Patch v2 has been sent, all your comments have specified answer in patch v2.

-----Original Message-----
From: Guo, Jia 
Sent: Tuesday, December 15, 2020 12:37 PM
To: Yu, DapengX <dapengx.yu@intel.com>
Cc: dev@dpdk.org; Yu, DapengX <dapengx.yu@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com>
Subject: RE: [PATCH] net/ixgbe: fix fdirctrl register setting

Hi, dapeng

> -----Original Message-----
> From: dapengx.yu@intel.com <dapengx.yu@intel.com>
> Sent: Friday, December 11, 2020 9:35 AM
> To: Guo, Jia <jia.guo@intel.com>
> Cc: dev@dpdk.org; Yu, DapengX <dapengx.yu@intel.com>; Zhang, Qi Z 
> <qi.z.zhang@intel.com>
> Subject: [PATCH] net/ixgbe: fix fdirctrl register setting
> 
> From: YU DAPENG <dapengx.yu@intel.com>
> 
> The function ixgbe_fdir_set_flexbytes_offset is used when create FDir 
> rule for flexbytes. It set a register: FDIRCTRL.FLEX_OFFSET, which 
> cause that even if the FDir flexbytes rule is destroyed, the rule 
> still direct the packet and transfer it to the wrong place. It is 
> because Setting FDIRCTRL shall only be permitted on Flow Director 
> initialization flow or Clearing the Flow Director table, otherwise 
> unexpected happens. In order to evade the limit, add code to make setting FDIRCTRL work without unexpected effects.
> 
> Fixes: f35fec63dde1 ("net/ixgbe: enable flex bytes for generic flow 
> API")
> Cc: qi.z.zhang@intel.com
> 
> Signed-off-by: YU DAPENG <dapengx.yu@intel.com>
> ---
>  drivers/net/ixgbe/ixgbe_fdir.c | 23 +++++++++++++++++++++++ 
> drivers/net/ixgbe/ixgbe_flow.c |  7 +++----
>  2 files changed, 26 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/ixgbe/ixgbe_fdir.c 
> b/drivers/net/ixgbe/ixgbe_fdir.c index a0fab5070..56dddd56b 100644
> --- a/drivers/net/ixgbe/ixgbe_fdir.c
> +++ b/drivers/net/ixgbe/ixgbe_fdir.c
> @@ -503,9 +503,32 @@ ixgbe_fdir_set_flexbytes_offset(struct 
> rte_eth_dev *dev,
>  				uint16_t offset)
>  {
>  	struct ixgbe_hw *hw = IXGBE_DEV_PRIVATE_TO_HW(dev->data-
> >dev_private);
> +	struct ixgbe_hw_fdir_info *fdir_info =
> +		IXGBE_DEV_PRIVATE_TO_FDIR_INFO(dev->data-
> >dev_private);
>  	uint32_t fdirctrl;
>  	int i;
> 
> +	if (fdir_info->flex_bytes_offset == offset)
> +		return 0;
> +
> +	fdir_info->flex_bytes_offset = offset;

Should this value assign at the end of the polling checking?
And could you kindly help to check is the timeout checking (i >= IXGBE_FDIR_INIT_DONE_POLL) also need in the "ixgbe_fdir_set_flexbytes_offset"?

> +
> +	/*

/* -> /**

> +	 * 82599 adapters flow director init flow cannot be restarted,
> +	 * Workaround 82599 silicon errata by performing the following steps
> +	 * before re-writing the FDIRCTRL control register with the same
> value.
> +	 * - write 1 to bit 8 of FDIRCMD register &
> +	 * - write 0 to bit 8 of FDIRCMD register
> +	 */
> +	IXGBE_WRITE_REG(hw, IXGBE_FDIRCMD,
> +			(IXGBE_READ_REG(hw, IXGBE_FDIRCMD) |
> +			 IXGBE_FDIRCMD_CLEARHT));
> +	IXGBE_WRITE_FLUSH(hw);
> +	IXGBE_WRITE_REG(hw, IXGBE_FDIRCMD,
> +			(IXGBE_READ_REG(hw, IXGBE_FDIRCMD) &
> +			 ~IXGBE_FDIRCMD_CLEARHT));
> +	IXGBE_WRITE_FLUSH(hw);
> +

So you mean the above steps should be done before re-writing the FDIRCTRL, just like the Flow Director initialization flow or Clearing the Flow Director, right?
If so, please refine your commit log and make it more clear, since Setting FDIRCTRL shall is not only be permitted on Flow Director initialization flow or Clearing the Flow Director table.

>  	fdirctrl = IXGBE_READ_REG(hw, IXGBE_FDIRCTRL);
> 
>  	fdirctrl &= ~IXGBE_FDIRCTRL_FLEX_MASK; diff --git 
> a/drivers/net/ixgbe/ixgbe_flow.c b/drivers/net/ixgbe/ixgbe_flow.c 
> index 39f6ed73f..b37541d9b 100644
> --- a/drivers/net/ixgbe/ixgbe_flow.c
> +++ b/drivers/net/ixgbe/ixgbe_flow.c
> @@ -3137,8 +3137,6 @@ ixgbe_flow_create(struct rte_eth_dev *dev,
>  				rte_memcpy(&fdir_info->mask,
>  					&fdir_rule.mask,
>  					sizeof(struct ixgbe_hw_fdir_mask));
> -				fdir_info->flex_bytes_offset =
> -					fdir_rule.flex_bytes_offset;
> 
>  				if (fdir_rule.mask.flex_bytes_mask)
>  					ixgbe_fdir_set_flexbytes_offset(dev,
> @@ -3161,8 +3159,9 @@ ixgbe_flow_create(struct rte_eth_dev *dev,
>  				if (ret)
>  					goto out;
> 
> -				if (fdir_info->flex_bytes_offset !=
> -						fdir_rule.flex_bytes_offset)
> +				if (fdir_rule.mask.flex_bytes_mask &&
> +					(fdir_info->flex_bytes_offset !=

Suggest you check the line align. 

> +						fdir_rule.flex_bytes_offset))

This line is the same as above.

>  					goto out;
>  			}
>  		}
> --
> 2.26.2.windows.1


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

* Re: [dpdk-dev] [PATCH v2] net/ixgbe: fix fdirctrl register setting
  2020-12-15 10:10 ` [dpdk-dev] [PATCH v2] " dapengx.yu
@ 2020-12-22  6:51   ` Zhou, JunX W
  2020-12-22  7:23     ` Guo, Jia
  0 siblings, 1 reply; 8+ messages in thread
From: Zhou, JunX W @ 2020-12-22  6:51 UTC (permalink / raw)
  To: Yu, DapengX, Guo, Jia; +Cc: dev, Yu, DapengX, stable

Tested-by: Zhou, Jun <junx.w.zhou@intel.com> 

-----Original Message-----
From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of dapengx.yu@intel.com
Sent: Tuesday, December 15, 2020 6:11 PM
To: Guo, Jia <jia.guo@intel.com>
Cc: dev@dpdk.org; Yu, DapengX <dapengx.yu@intel.com>; stable@dpdk.org
Subject: [dpdk-dev] [PATCH v2] net/ixgbe: fix fdirctrl register setting

From: YU DAPENG <dapengx.yu@intel.com>

The function ixgbe_fdir_set_flexbytes_offset is used when create FDir rule for flexbytes. It set a register: FDIRCTRL.FLEX_OFFSET, which cause that even if the FDir flexbytes rule is destroyed, the rule still direct the packet and transfer it to the wrong place. It is because setting FDIRCTRL shall only be permitted on Flow Director initialization flow or clearing the Flow Director table according to intel datasheet, otherwise unexpected happens. In order to evade the limit, add code to set FDIRCMD.CLEARHT to 1b and then clear it back to 0b to make the setting act like the Flow Director initialization flow or clearing the Flow Director table.

Fixes: f35fec63dde1 ("net/ixgbe: enable flex bytes for generic flow API")
Cc: stable@dpdk.org

Signed-off-by: YU DAPENG <dapengx.yu@intel.com>
---
 drivers/net/ixgbe/ixgbe_fdir.c | 29 +++++++++++++++++++++++++++++  drivers/net/ixgbe/ixgbe_flow.c | 15 ++++++++-------
 2 files changed, 37 insertions(+), 7 deletions(-)

diff --git a/drivers/net/ixgbe/ixgbe_fdir.c b/drivers/net/ixgbe/ixgbe_fdir.c index a0fab5070..11b9effeb 100644
--- a/drivers/net/ixgbe/ixgbe_fdir.c
+++ b/drivers/net/ixgbe/ixgbe_fdir.c
@@ -503,9 +503,30 @@ ixgbe_fdir_set_flexbytes_offset(struct rte_eth_dev *dev,
 				uint16_t offset)
 {
 	struct ixgbe_hw *hw = IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
+	struct ixgbe_hw_fdir_info *fdir_info =
+		IXGBE_DEV_PRIVATE_TO_FDIR_INFO(dev->data->dev_private);
 	uint32_t fdirctrl;
 	int i;
 
+	if (fdir_info->flex_bytes_offset == offset)
+		return 0;
+
+	/**
+	 * 82599 adapters flow director init flow cannot be restarted,
+	 * Workaround 82599 silicon errata by performing the following steps
+	 * before re-writing the FDIRCTRL control register with the same value.
+	 * - write 1 to bit 8 of FDIRCMD register &
+	 * - write 0 to bit 8 of FDIRCMD register
+	 */
+	IXGBE_WRITE_REG(hw, IXGBE_FDIRCMD,
+			(IXGBE_READ_REG(hw, IXGBE_FDIRCMD) |
+			 IXGBE_FDIRCMD_CLEARHT));
+	IXGBE_WRITE_FLUSH(hw);
+	IXGBE_WRITE_REG(hw, IXGBE_FDIRCMD,
+			(IXGBE_READ_REG(hw, IXGBE_FDIRCMD) &
+			 ~IXGBE_FDIRCMD_CLEARHT));
+	IXGBE_WRITE_FLUSH(hw);
+
 	fdirctrl = IXGBE_READ_REG(hw, IXGBE_FDIRCTRL);
 
 	fdirctrl &= ~IXGBE_FDIRCTRL_FLEX_MASK; @@ -520,6 +541,14 @@ ixgbe_fdir_set_flexbytes_offset(struct rte_eth_dev *dev,
 			break;
 		msec_delay(1);
 	}
+
+	if (i >= IXGBE_FDIR_INIT_DONE_POLL) {
+		PMD_DRV_LOG(ERR, "Flow Director poll time exceeded!");
+		return -ETIMEDOUT;
+	}
+
+	fdir_info->flex_bytes_offset = offset;
+
 	return 0;
 }
 
diff --git a/drivers/net/ixgbe/ixgbe_flow.c b/drivers/net/ixgbe/ixgbe_flow.c index 39f6ed73f..9aeb2e4a4 100644
--- a/drivers/net/ixgbe/ixgbe_flow.c
+++ b/drivers/net/ixgbe/ixgbe_flow.c
@@ -3137,13 +3137,13 @@ ixgbe_flow_create(struct rte_eth_dev *dev,
 				rte_memcpy(&fdir_info->mask,
 					&fdir_rule.mask,
 					sizeof(struct ixgbe_hw_fdir_mask));
-				fdir_info->flex_bytes_offset =
-					fdir_rule.flex_bytes_offset;
 
-				if (fdir_rule.mask.flex_bytes_mask)
-					ixgbe_fdir_set_flexbytes_offset(dev,
+				if (fdir_rule.mask.flex_bytes_mask) {
+					ret = ixgbe_fdir_set_flexbytes_offset(dev,
 						fdir_rule.flex_bytes_offset);
-
+					if (ret)
+						goto out;
+				}
 				ret = ixgbe_fdir_set_input_mask(dev);
 				if (ret)
 					goto out;
@@ -3161,8 +3161,9 @@ ixgbe_flow_create(struct rte_eth_dev *dev,
 				if (ret)
 					goto out;
 
-				if (fdir_info->flex_bytes_offset !=
-						fdir_rule.flex_bytes_offset)
+				if (fdir_rule.mask.flex_bytes_mask &&
+				    fdir_info->flex_bytes_offset !=
+				    fdir_rule.flex_bytes_offset)
 					goto out;
 			}
 		}
--
2.26.2.windows.1


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

* Re: [dpdk-dev] [PATCH v2] net/ixgbe: fix fdirctrl register setting
  2020-12-22  6:51   ` Zhou, JunX W
@ 2020-12-22  7:23     ` Guo, Jia
  2021-01-05 12:09       ` [dpdk-dev] [dpdk-stable] " Ferruh Yigit
  0 siblings, 1 reply; 8+ messages in thread
From: Guo, Jia @ 2020-12-22  7:23 UTC (permalink / raw)
  To: Zhou, JunX W, Yu, DapengX; +Cc: dev, Yu, DapengX, stable

Acked-by: Jeff Guo <jia.guo@intel.com>

> -----Original Message-----
> From: Zhou, JunX W <junx.w.zhou@intel.com>
> Sent: Tuesday, December 22, 2020 2:51 PM
> To: Yu, DapengX <dapengx.yu@intel.com>; Guo, Jia <jia.guo@intel.com>
> Cc: dev@dpdk.org; Yu, DapengX <dapengx.yu@intel.com>; stable@dpdk.org
> Subject: RE: [dpdk-dev] [PATCH v2] net/ixgbe: fix fdirctrl register setting
> 
> Tested-by: Zhou, Jun <junx.w.zhou@intel.com>
> 
> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of
> dapengx.yu@intel.com
> Sent: Tuesday, December 15, 2020 6:11 PM
> To: Guo, Jia <jia.guo@intel.com>
> Cc: dev@dpdk.org; Yu, DapengX <dapengx.yu@intel.com>; stable@dpdk.org
> Subject: [dpdk-dev] [PATCH v2] net/ixgbe: fix fdirctrl register setting
> 
> From: YU DAPENG <dapengx.yu@intel.com>
> 
> The function ixgbe_fdir_set_flexbytes_offset is used when create FDir rule
> for flexbytes. It set a register: FDIRCTRL.FLEX_OFFSET, which cause that even
> if the FDir flexbytes rule is destroyed, the rule still direct the packet and
> transfer it to the wrong place. It is because setting FDIRCTRL shall only be
> permitted on Flow Director initialization flow or clearing the Flow Director
> table according to intel datasheet, otherwise unexpected happens. In order
> to evade the limit, add code to set FDIRCMD.CLEARHT to 1b and then clear it
> back to 0b to make the setting act like the Flow Director initialization flow or
> clearing the Flow Director table.
> 
> Fixes: f35fec63dde1 ("net/ixgbe: enable flex bytes for generic flow API")
> Cc: stable@dpdk.org
> 
> Signed-off-by: YU DAPENG <dapengx.yu@intel.com>
> ---
>  drivers/net/ixgbe/ixgbe_fdir.c | 29 +++++++++++++++++++++++++++++
> drivers/net/ixgbe/ixgbe_flow.c | 15 ++++++++-------
>  2 files changed, 37 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/net/ixgbe/ixgbe_fdir.c b/drivers/net/ixgbe/ixgbe_fdir.c
> index a0fab5070..11b9effeb 100644
> --- a/drivers/net/ixgbe/ixgbe_fdir.c
> +++ b/drivers/net/ixgbe/ixgbe_fdir.c
> @@ -503,9 +503,30 @@ ixgbe_fdir_set_flexbytes_offset(struct rte_eth_dev
> *dev,
>  				uint16_t offset)
>  {
>  	struct ixgbe_hw *hw = IXGBE_DEV_PRIVATE_TO_HW(dev->data-
> >dev_private);
> +	struct ixgbe_hw_fdir_info *fdir_info =
> +		IXGBE_DEV_PRIVATE_TO_FDIR_INFO(dev->data-
> >dev_private);
>  	uint32_t fdirctrl;
>  	int i;
> 
> +	if (fdir_info->flex_bytes_offset == offset)
> +		return 0;
> +
> +	/**
> +	 * 82599 adapters flow director init flow cannot be restarted,
> +	 * Workaround 82599 silicon errata by performing the following steps
> +	 * before re-writing the FDIRCTRL control register with the same
> value.
> +	 * - write 1 to bit 8 of FDIRCMD register &
> +	 * - write 0 to bit 8 of FDIRCMD register
> +	 */
> +	IXGBE_WRITE_REG(hw, IXGBE_FDIRCMD,
> +			(IXGBE_READ_REG(hw, IXGBE_FDIRCMD) |
> +			 IXGBE_FDIRCMD_CLEARHT));
> +	IXGBE_WRITE_FLUSH(hw);
> +	IXGBE_WRITE_REG(hw, IXGBE_FDIRCMD,
> +			(IXGBE_READ_REG(hw, IXGBE_FDIRCMD) &
> +			 ~IXGBE_FDIRCMD_CLEARHT));
> +	IXGBE_WRITE_FLUSH(hw);
> +
>  	fdirctrl = IXGBE_READ_REG(hw, IXGBE_FDIRCTRL);
> 
>  	fdirctrl &= ~IXGBE_FDIRCTRL_FLEX_MASK; @@ -520,6 +541,14 @@
> ixgbe_fdir_set_flexbytes_offset(struct rte_eth_dev *dev,
>  			break;
>  		msec_delay(1);
>  	}
> +
> +	if (i >= IXGBE_FDIR_INIT_DONE_POLL) {
> +		PMD_DRV_LOG(ERR, "Flow Director poll time exceeded!");
> +		return -ETIMEDOUT;
> +	}
> +
> +	fdir_info->flex_bytes_offset = offset;
> +
>  	return 0;
>  }
> 
> diff --git a/drivers/net/ixgbe/ixgbe_flow.c b/drivers/net/ixgbe/ixgbe_flow.c
> index 39f6ed73f..9aeb2e4a4 100644
> --- a/drivers/net/ixgbe/ixgbe_flow.c
> +++ b/drivers/net/ixgbe/ixgbe_flow.c
> @@ -3137,13 +3137,13 @@ ixgbe_flow_create(struct rte_eth_dev *dev,
>  				rte_memcpy(&fdir_info->mask,
>  					&fdir_rule.mask,
>  					sizeof(struct ixgbe_hw_fdir_mask));
> -				fdir_info->flex_bytes_offset =
> -					fdir_rule.flex_bytes_offset;
> 
> -				if (fdir_rule.mask.flex_bytes_mask)
> -					ixgbe_fdir_set_flexbytes_offset(dev,
> +				if (fdir_rule.mask.flex_bytes_mask) {
> +					ret =
> ixgbe_fdir_set_flexbytes_offset(dev,
>  						fdir_rule.flex_bytes_offset);
> -
> +					if (ret)
> +						goto out;
> +				}
>  				ret = ixgbe_fdir_set_input_mask(dev);
>  				if (ret)
>  					goto out;
> @@ -3161,8 +3161,9 @@ ixgbe_flow_create(struct rte_eth_dev *dev,
>  				if (ret)
>  					goto out;
> 
> -				if (fdir_info->flex_bytes_offset !=
> -						fdir_rule.flex_bytes_offset)
> +				if (fdir_rule.mask.flex_bytes_mask &&
> +				    fdir_info->flex_bytes_offset !=
> +				    fdir_rule.flex_bytes_offset)
>  					goto out;
>  			}
>  		}
> --
> 2.26.2.windows.1


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

* Re: [dpdk-dev] [dpdk-stable] [PATCH v2] net/ixgbe: fix fdirctrl register setting
  2020-12-22  7:23     ` Guo, Jia
@ 2021-01-05 12:09       ` Ferruh Yigit
  0 siblings, 0 replies; 8+ messages in thread
From: Ferruh Yigit @ 2021-01-05 12:09 UTC (permalink / raw)
  To: Guo, Jia, Zhou, JunX W, Yu, DapengX; +Cc: dev, stable, Zhang, Qi Z

On 12/22/2020 7:23 AM, Guo, Jia wrote:
> Acked-by: Jeff Guo <jia.guo@intel.com>
> 
>> -----Original Message-----
>> From: Zhou, JunX W <junx.w.zhou@intel.com>
>> Sent: Tuesday, December 22, 2020 2:51 PM
>> To: Yu, DapengX <dapengx.yu@intel.com>; Guo, Jia <jia.guo@intel.com>
>> Cc: dev@dpdk.org; Yu, DapengX <dapengx.yu@intel.com>; stable@dpdk.org
>> Subject: RE: [dpdk-dev] [PATCH v2] net/ixgbe: fix fdirctrl register setting
>>
>> Tested-by: Zhou, Jun <junx.w.zhou@intel.com>
>>
>> -----Original Message-----
>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of
>> dapengx.yu@intel.com
>> Sent: Tuesday, December 15, 2020 6:11 PM
>> To: Guo, Jia <jia.guo@intel.com>
>> Cc: dev@dpdk.org; Yu, DapengX <dapengx.yu@intel.com>; stable@dpdk.org
>> Subject: [dpdk-dev] [PATCH v2] net/ixgbe: fix fdirctrl register setting
>>
>> From: YU DAPENG <dapengx.yu@intel.com>
>>
>> The function ixgbe_fdir_set_flexbytes_offset is used when create FDir rule
>> for flexbytes. It set a register: FDIRCTRL.FLEX_OFFSET, which cause that even
>> if the FDir flexbytes rule is destroyed, the rule still direct the packet and
>> transfer it to the wrong place. It is because setting FDIRCTRL shall only be
>> permitted on Flow Director initialization flow or clearing the Flow Director
>> table according to intel datasheet, otherwise unexpected happens. In order
>> to evade the limit, add code to set FDIRCMD.CLEARHT to 1b and then clear it
>> back to 0b to make the setting act like the Flow Director initialization flow or
>> clearing the Flow Director table.
>>
>> Fixes: f35fec63dde1 ("net/ixgbe: enable flex bytes for generic flow API")
>> Cc: stable@dpdk.org
>>
>> Signed-off-by: YU DAPENG <dapengx.yu@intel.com>

Updating the commit log as following:

     net/ixgbe: fix flexbytes flow director rule

     When a flexbytes flow director rule is created, the FDIRCTRL.FLEX_OFFSET
     register is set, and it keeps its affect even after the flow director
     flexbytes rule is destroyed, causing packets to be transferred to the
     wrong place.

     It is because setting FDIRCTRL shall only be permitted on Flow Director
     initialization flow or clearing the Flow Director table according to the
     datasheet, otherwise device may behave unexpectedly.

     In order to evade this limitation, simulate the Flow Director
     initialization flow or clearing the Flow Director table by setting
     FDIRCMD.CLEARHT to 0x1B and then clear it back to 0x0B.

     Fixes: f35fec63dde1 ("net/ixgbe: enable flex bytes for generic flow API")
     Cc: stable@dpdk.org

     Signed-off-by: Dapeng Yu <dapengx.yu@intel.com>
     Tested-by: Jun Zhou <junx.w.zhou@intel.com>
     Acked-by: Jeff Guo <jia.guo@intel.com>

Please prefer the "Dapeng Yu <dapengx.yu@intel.com>" signature to be consistent.

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

end of thread, other threads:[~2021-01-05 12:10 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-12-11  1:35 [dpdk-dev] [PATCH] net/ixgbe: fix fdirctrl register setting dapengx.yu
2020-12-15  3:00 ` Zhou, JunX W
2020-12-15  4:36 ` Guo, Jia
2020-12-16  0:55   ` Yu, DapengX
2020-12-15 10:10 ` [dpdk-dev] [PATCH v2] " dapengx.yu
2020-12-22  6:51   ` Zhou, JunX W
2020-12-22  7:23     ` Guo, Jia
2021-01-05 12:09       ` [dpdk-dev] [dpdk-stable] " Ferruh Yigit

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).