[net,v2] i40e: Fix ethtool rx-flow-hash setting for X722

From: Slawomir Laba <slawomirx.laba@intel.com>

When enabling flow type for RSS hash via ethtool:

  ethtool -N $pf rx-flow-hash tcp4|tcp6|udp4|udp6 s|d

the driver would fail to setup this setting on X722
device since it was using the mask on the register
dedicated for X710 devices.

Implement a bitmap to collect the flow pc types that
shall be applied on the inset and hena registers.
Apply a different mask on the register when setting the
RSS hash for the X722 device.

When displaying the flow types enabled via ethtool:

  ethtool -n $pf rx-flow-hash tcp4|tcp6|udp4|udp6

the driver would print wrong values for X722 device.

Fix this issue by testing masks for X722 device in
i40e_get_rss_hash_opts function.

Fixes: eb0dd6e4a3b3 (i40e: Allow RSS Hash set with less than four parameters)
Signed-off-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
Signed-off-by: Slawomir Laba <slawomirx.laba@intel.com>
---
 .../net/ethernet/intel/i40e/i40e_ethtool.c    | 103 +++++++++++-------
 drivers/net/ethernet/intel/i40e/i40e_type.h   |   4 +
 2 files changed, 67 insertions(+), 40 deletions(-)

Dear Aleksandr, dear Slawomir,


Thank you for the patch.

Am 14.06.22 um 19:29 schrieb Loktionov, Aleksandr:
> From: Slawomir Laba <slawomirx.laba@intel.com>
> 
> When enabling flow type for RSS hash via ethtool:
> 
>    ethtool -N $pf rx-flow-hash tcp4|tcp6|udp4|udp6 s|d
> 
> the driver would fail to setup this setting on X722

The verb is spelled with a space: to set up

> device since it was using the mask on the register
> dedicated for X710 devices.
> 
> Implement a bitmap to collect the flow pc types that
> shall be applied on the inset and hena registers.
> Apply a different mask on the register when setting the
> RSS hash for the X722 device.
> 
> When displaying the flow types enabled via ethtool:
> 
>    ethtool -n $pf rx-flow-hash tcp4|tcp6|udp4|udp6
> 
> the driver would print wrong values for X722 device.
> 
> Fix this issue by testing masks for X722 device in
> i40e_get_rss_hash_opts function.

The display problem sounds separate or only loosely related). Could it 
go in a separate commit?

Please reflow the commit message for 75 characters per line.

Also, to ease review, maybe reference the datasheet name, revision, and 
section, where the bits are documented.

> Fixes: eb0dd6e4a3b3 (i40e: Allow RSS Hash set with less than four parameters)
> Signed-off-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
> Signed-off-by: Slawomir Laba <slawomirx.laba@intel.com>
> ---
>   .../net/ethernet/intel/i40e/i40e_ethtool.c    | 103 +++++++++++-------
>   drivers/net/ethernet/intel/i40e/i40e_type.h   |   4 +
>   2 files changed, 67 insertions(+), 40 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/i40e/i40e_ethtool.c b/drivers/net/ethernet/intel/i40e/i40e_ethtool.c
> index 19704f5..bc9e921 100644
> --- a/drivers/net/ethernet/intel/i40e/i40e_ethtool.c
> +++ b/drivers/net/ethernet/intel/i40e/i40e_ethtool.c
> @@ -3098,10 +3098,17 @@ static int i40e_get_rss_hash_opts(struct i40e_pf *pf, struct ethtool_rxnfc *cmd)
>   
>   		if (cmd->flow_type == TCP_V4_FLOW ||
>   		    cmd->flow_type == UDP_V4_FLOW) {
> -			if (i_set & I40E_L3_SRC_MASK)
> -				cmd->data |= RXH_IP_SRC;
> -			if (i_set & I40E_L3_DST_MASK)
> -				cmd->data |= RXH_IP_DST;
> +			if (hw->mac.type == I40E_MAC_X722) {
> +				if (i_set & I40E_X722_L3_SRC_MASK)
> +					cmd->data |= RXH_IP_SRC;
> +				if (i_set & I40E_X722_L3_DST_MASK)
> +					cmd->data |= RXH_IP_DST;
> +			} else {
> +				if (i_set & I40E_L3_SRC_MASK)

Should these macros be renamed first in a commit in front of this cone?

> +					cmd->data |= RXH_IP_SRC;
> +				if (i_set & I40E_L3_DST_MASK)
> +					cmd->data |= RXH_IP_DST;
> +			}

I’d do it like further down

     src_l3_mask = hw->mac.type == I40E_MAC_X722 ? I40E_X722_L3_SRC_MASK 
: I40E_L3_SRC_MASK;
     dest_l3_mask = …

and then `if (i_set & src_l3_mask)` and so on. This should be shorter, 
and easier to grasp.

>   		} else if (cmd->flow_type == TCP_V6_FLOW ||
>   			  cmd->flow_type == UDP_V6_FLOW) {
>   			if (i_set & I40E_L3_V6_SRC_MASK)
> @@ -3459,12 +3466,15 @@ static int i40e_get_rxnfc(struct net_device *netdev, struct ethtool_rxnfc *cmd,
>   
>   /**
>    * i40e_get_rss_hash_bits - Read RSS Hash bits from register
> + * @hw: hw structure
>    * @nfc: pointer to user request
>    * @i_setc: bits currently set
>    *
>    * Returns value of bits to be set per user request
>    **/
> -static u64 i40e_get_rss_hash_bits(struct ethtool_rxnfc *nfc, u64 i_setc)
> +static u64 i40e_get_rss_hash_bits(struct i40e_hw *hw,
> +				  struct ethtool_rxnfc *nfc,
> +				  u64 i_setc)
>   {
>   	u64 i_set = i_setc;
>   	u64 src_l3 = 0, dst_l3 = 0;
> @@ -3483,8 +3493,13 @@ static u64 i40e_get_rss_hash_bits(struct ethtool_rxnfc *nfc, u64 i_setc)
>   		dst_l3 = I40E_L3_V6_DST_MASK;
>   	} else if (nfc->flow_type == TCP_V4_FLOW ||
>   		  nfc->flow_type == UDP_V4_FLOW) {
> -		src_l3 = I40E_L3_SRC_MASK;
> -		dst_l3 = I40E_L3_DST_MASK;
> +		if (hw->mac.type == I40E_MAC_X722) {
> +			src_l3 = I40E_X722_L3_SRC_MASK;
> +			dst_l3 = I40E_X722_L3_DST_MASK;
> +		} else {
> +			src_l3 = I40E_L3_SRC_MASK;
> +			dst_l3 = I40E_L3_DST_MASK;
> +		}

I’d use the ternary operator.

>   	} else {
>   		/* Any other flow type are not supported here */
>   		return i_set;
> @@ -3502,6 +3517,8 @@ static u64 i40e_get_rss_hash_bits(struct ethtool_rxnfc *nfc, u64 i_setc)
>   	return i_set;
>   }
>   
> +#define FLOW_PCTYPES_SIZE 64

Why is this define necessary?

> +
>   /**
>    * i40e_set_rss_hash_opt - Enable/Disable flow types for RSS hash
>    * @pf: pointer to the physical function struct
> @@ -3514,9 +3531,11 @@ static int i40e_set_rss_hash_opt(struct i40e_pf *pf, struct ethtool_rxnfc *nfc)
>   	struct i40e_hw *hw = &pf->hw;
>   	u64 hena = (u64)i40e_read_rx_ctl(hw, I40E_PFQF_HENA(0)) |
>   		   ((u64)i40e_read_rx_ctl(hw, I40E_PFQF_HENA(1)) << 32);
> -	u8 flow_pctype = 0;
> +	DECLARE_BITMAP(flow_pctypes, FLOW_PCTYPES_SIZE);
>   	u64 i_set, i_setc;
>   
> +	bitmap_zero(flow_pctypes, FLOW_PCTYPES_SIZE);
> +
>   	if (pf->flags & I40E_FLAG_MFP_ENABLED) {
>   		dev_err(&pf->pdev->dev,
>   			"Change of RSS hash input set is not supported when MFP mode is enabled\n");
> @@ -3532,36 +3551,35 @@ static int i40e_set_rss_hash_opt(struct i40e_pf *pf, struct ethtool_rxnfc *nfc)
>   
>   	switch (nfc->flow_type) {
>   	case TCP_V4_FLOW:
> -		flow_pctype = I40E_FILTER_PCTYPE_NONF_IPV4_TCP;
> +		set_bit(I40E_FILTER_PCTYPE_NONF_IPV4_TCP, flow_pctypes);
>   		if (pf->hw_features & I40E_HW_MULTIPLE_TCP_UDP_RSS_PCTYPE)
> -			hena |=
> -			  BIT_ULL(I40E_FILTER_PCTYPE_NONF_IPV4_TCP_SYN_NO_ACK);
> +			set_bit(I40E_FILTER_PCTYPE_NONF_IPV4_TCP_SYN_NO_ACK,
> +				flow_pctypes);
>   		break;
>   	case TCP_V6_FLOW:
> -		flow_pctype = I40E_FILTER_PCTYPE_NONF_IPV6_TCP;
> +		set_bit(I40E_FILTER_PCTYPE_NONF_IPV6_TCP, flow_pctypes);
>   		if (pf->hw_features & I40E_HW_MULTIPLE_TCP_UDP_RSS_PCTYPE)
> -			hena |=
> -			  BIT_ULL(I40E_FILTER_PCTYPE_NONF_IPV4_TCP_SYN_NO_ACK);
> -		if (pf->hw_features & I40E_HW_MULTIPLE_TCP_UDP_RSS_PCTYPE)
> -			hena |=
> -			  BIT_ULL(I40E_FILTER_PCTYPE_NONF_IPV6_TCP_SYN_NO_ACK);
> +			set_bit(I40E_FILTER_PCTYPE_NONF_IPV6_TCP_SYN_NO_ACK,
> +				flow_pctypes);
>   		break;
>   	case UDP_V4_FLOW:
> -		flow_pctype = I40E_FILTER_PCTYPE_NONF_IPV4_UDP;
> -		if (pf->hw_features & I40E_HW_MULTIPLE_TCP_UDP_RSS_PCTYPE)
> -			hena |=
> -			  BIT_ULL(I40E_FILTER_PCTYPE_NONF_UNICAST_IPV4_UDP) |
> -			  BIT_ULL(I40E_FILTER_PCTYPE_NONF_MULTICAST_IPV4_UDP);
> -
> +		set_bit(I40E_FILTER_PCTYPE_NONF_IPV4_UDP, flow_pctypes);
> +		if (pf->hw_features & I40E_HW_MULTIPLE_TCP_UDP_RSS_PCTYPE) {
> +			set_bit(I40E_FILTER_PCTYPE_NONF_UNICAST_IPV4_UDP,
> +				flow_pctypes);
> +			set_bit(I40E_FILTER_PCTYPE_NONF_MULTICAST_IPV4_UDP,
> +				flow_pctypes);
> +		}
>   		hena |= BIT_ULL(I40E_FILTER_PCTYPE_FRAG_IPV4);
>   		break;
>   	case UDP_V6_FLOW:
> -		flow_pctype = I40E_FILTER_PCTYPE_NONF_IPV6_UDP;
> -		if (pf->hw_features & I40E_HW_MULTIPLE_TCP_UDP_RSS_PCTYPE)
> -			hena |=
> -			  BIT_ULL(I40E_FILTER_PCTYPE_NONF_UNICAST_IPV6_UDP) |
> -			  BIT_ULL(I40E_FILTER_PCTYPE_NONF_MULTICAST_IPV6_UDP);
> -
> +		set_bit(I40E_FILTER_PCTYPE_NONF_IPV6_UDP, flow_pctypes);
> +		if (pf->hw_features & I40E_HW_MULTIPLE_TCP_UDP_RSS_PCTYPE) {
> +			set_bit(I40E_FILTER_PCTYPE_NONF_UNICAST_IPV6_UDP,
> +				flow_pctypes);
> +			set_bit(I40E_FILTER_PCTYPE_NONF_MULTICAST_IPV6_UDP,
> +				flow_pctypes);
> +		}
>   		hena |= BIT_ULL(I40E_FILTER_PCTYPE_FRAG_IPV6);
>   		break;
>   	case AH_ESP_V4_FLOW:
> @@ -3594,17 +3612,24 @@ static int i40e_set_rss_hash_opt(struct i40e_pf *pf, struct ethtool_rxnfc *nfc)
>   		return -EINVAL;
>   	}
>   
> -	if (flow_pctype) {
> -		i_setc = (u64)i40e_read_rx_ctl(hw, I40E_GLQF_HASH_INSET(0,
> -					       flow_pctype)) |
> -			((u64)i40e_read_rx_ctl(hw, I40E_GLQF_HASH_INSET(1,
> -					       flow_pctype)) << 32);
> -		i_set = i40e_get_rss_hash_bits(nfc, i_setc);
> -		i40e_write_rx_ctl(hw, I40E_GLQF_HASH_INSET(0, flow_pctype),
> -				  (u32)i_set);
> -		i40e_write_rx_ctl(hw, I40E_GLQF_HASH_INSET(1, flow_pctype),
> -				  (u32)(i_set >> 32));
> -		hena |= BIT_ULL(flow_pctype);
> +	if (bitmap_weight(flow_pctypes, FLOW_PCTYPES_SIZE)) {
> +		u8 flow_id;
> +
> +		for_each_set_bit(flow_id, flow_pctypes, FLOW_PCTYPES_SIZE) {
> +			i_setc = (u64)i40e_read_rx_ctl(hw,
> +						       I40E_GLQF_HASH_INSET
> +						       (0, flow_id)) |
> +				 ((u64)i40e_read_rx_ctl(hw,
> +							I40E_GLQF_HASH_INSET
> +							(1, flow_id)) << 32);
> +			i_set = i40e_get_rss_hash_bits(&pf->hw, nfc, i_setc);
> +
> +			i40e_write_rx_ctl(hw, I40E_GLQF_HASH_INSET(0, flow_id),
> +					  (u32)i_set);
> +			i40e_write_rx_ctl(hw, I40E_GLQF_HASH_INSET(1, flow_id),
> +					  (u32)(i_set >> 32));
> +			hena |= BIT_ULL(flow_id);
> +		};
>   	}
>   
>   	i40e_write_rx_ctl(hw, I40E_PFQF_HENA(0), (u32)hena);
> diff --git a/drivers/net/ethernet/intel/i40e/i40e_type.h b/drivers/net/ethernet/intel/i40e/i40e_type.h
> index 7b3f30b..388c3d3 100644
> --- a/drivers/net/ethernet/intel/i40e/i40e_type.h
> +++ b/drivers/net/ethernet/intel/i40e/i40e_type.h
> @@ -1404,6 +1404,10 @@ struct i40e_lldp_variables {
>   #define I40E_PFQF_CTL_0_HASHLUTSIZE_512	0x00010000
>   
>   /* INPUT SET MASK for RSS, flow director, and flexible payload */
> +#define I40E_X722_L3_SRC_SHIFT		49
> +#define I40E_X722_L3_SRC_MASK		(0x3ULL << I40E_X722_L3_SRC_SHIFT)
> +#define I40E_X722_L3_DST_SHIFT		41
> +#define I40E_X722_L3_DST_MASK		(0x3ULL << I40E_X722_L3_DST_SHIFT)
>   #define I40E_L3_SRC_SHIFT		47
>   #define I40E_L3_SRC_MASK		(0x3ULL << I40E_L3_SRC_SHIFT)
>   #define I40E_L3_V6_SRC_SHIFT		43


Kind regards,

Paul