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

[Yu, DapengX]

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 at intel.com>
Cc: dev at dpdk.org; Yu, DapengX <dapengx.yu at intel.com>; Zhang, Qi Z <qi.z.zhang at intel.com>
Subject: RE: [PATCH] net/ixgbe: fix fdirctrl register setting

Hi, dapeng

> -----Original Message-----
> From: dapengx.yu at intel.com <dapengx.yu at intel.com>
> Sent: Friday, December 11, 2020 9:35 AM
> To: Guo, Jia <jia.guo at intel.com>
> Cc: dev at dpdk.org; Yu, DapengX <dapengx.yu at intel.com>; Zhang, Qi Z 
> <qi.z.zhang at intel.com>
> Subject: [PATCH] net/ixgbe: fix fdirctrl register setting
> 
> From: YU DAPENG <dapengx.yu at 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 at intel.com
> 
> Signed-off-by: YU DAPENG <dapengx.yu at 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