Re: [Intel-wired-lan] [PATCH net-next v2 3/4] ice: allow matching on meta data

[Alexander Lobakin <aleksander.lobakin@intel.com>]

From: Michal Swiatkowski <michal.swiatkowski@linux.intel.com>
Date: Tue,  4 Apr 2023 09:28:32 +0200

> Add meta data matching criteria in the same place as protocol matching
> criteria. There is no need to add meta data as special words after
> parsing all lookups. Trade meta data in the same why as other lookups.

[...]

> --- a/drivers/net/ethernet/intel/ice/ice_switch.c
> +++ b/drivers/net/ethernet/intel/ice/ice_switch.c
> @@ -4573,6 +4573,15 @@ static const struct ice_prot_ext_tbl_entry ice_prot_ext[ICE_PROTOCOL_LAST] = {
>  	{ ICE_L2TPV3,		{ 0, 2, 4, 6, 8, 10 } },
>  	{ ICE_VLAN_EX,          { 2, 0 } },
>  	{ ICE_VLAN_IN,          { 2, 0 } },
> +	{ ICE_HW_METADATA,	{ ICE_SOURCE_PORT_MDID_OFFSET,
> +				  ICE_PTYPE_MDID_OFFSET,
> +				  ICE_PACKET_LENGTH_MDID_OFFSET,
> +				  ICE_SOURCE_VSI_MDID_OFFSET,
> +				  ICE_PKT_VLAN_MDID_OFFSET,
> +				  ICE_PKT_TUNNEL_MDID_OFFSET,
> +				  ICE_PKT_TCP_MDID_OFFSET,
> +				  ICE_PKT_ERROR_MDID_OFFSET,
> +				}},

I don't think this is proper indenting. I believe it should like this:

	/* This line is unchanged except the opening brace at the end */
	{ ICE_VLAN_IN,          { 2, 0 } }, {
		ICE_HW_METADATA, {
			ICE_SOURCE_PORT_MDID_OFFSET,
			ICE_PTYPE_MDID_OFFSET,
			[...]
		/* Don't forget commas after last elements */
		},
	},

or

	{
		ICE_HW_METADATA,
		{
			ICE_SOURCE_PORT_MDID_OFFSET,
			ICE_PTYPE_MDID_OFFSET,
			[...]
		},
	},

(but I'd prefer the first one)

Also, I think anonymous initializers are now discouraged in favour of
designated, at least randstruct sometimes complains about that. Could
we start always specifying field names? You could define a macro for
this particular struct to not bloat the code.

>  };
>  
>  static struct ice_protocol_entry ice_prot_id_tbl[ICE_PROTOCOL_LAST] = {
> @@ -4597,6 +4606,7 @@ static struct ice_protocol_entry ice_prot_id_tbl[ICE_PROTOCOL_LAST] = {
>  	{ ICE_L2TPV3,		ICE_L2TPV3_HW },
>  	{ ICE_VLAN_EX,          ICE_VLAN_OF_HW },
>  	{ ICE_VLAN_IN,          ICE_VLAN_OL_HW },
> +	{ ICE_HW_METADATA,      ICE_META_DATA_ID_HW},

Please replace spaces with tabs (as it's done for ICE_L2TPV3_HW).
Also missing space before the last brace.

>  };
>  
>  /**

[...]

> @@ -5726,6 +5663,10 @@ ice_fill_adv_dummy_packet(struct ice_adv_lkup_elem *lkups, u16 lkups_cnt,
>  		 * was already checked when search for the dummy packet
>  		 */
>  		type = lkups[i].type;
> +		/* metadata isn't lockated in packet */

("located", but I'd say "metadata isn't present in the packet")

> +		if (type == ICE_HW_METADATA)
> +			continue;
> +
>  		for (j = 0; offsets[j].type != ICE_PROTOCOL_LAST; j++) {
>  			if (type == offsets[j].type) {
>  				offset = offsets[j].offset;
> @@ -5861,16 +5802,21 @@ ice_fill_adv_packet_tun(struct ice_hw *hw, enum ice_sw_tunnel_type tun_type,
>  
>  /**
>   * ice_fill_adv_packet_vlan - fill dummy packet with VLAN tag type
> + * @hw: pointer to hw structure
>   * @vlan_type: VLAN tag type
>   * @pkt: dummy packet to fill in
>   * @offsets: offset info for the dummy packet
>   */
>  static int
> -ice_fill_adv_packet_vlan(u16 vlan_type, u8 *pkt,
> +ice_fill_adv_packet_vlan(struct ice_hw *hw, u16 vlan_type, u8 *pkt,
>  			 const struct ice_dummy_pkt_offsets *offsets)
>  {
>  	u16 i;
>  
> +	/* Check if there is something to do */
> +	if (vlan_type == 0 || !ice_is_dvm_ena(hw))

`!vlan_type` is preferred over `== 0`.

> +		return 0;
> +
>  	/* Find VLAN header and insert VLAN TPID */
>  	for (i = 0; offsets[i].type != ICE_PROTOCOL_LAST; i++) {
>  		if (offsets[i].type == ICE_VLAN_OFOS ||
> @@ -5889,6 +5835,15 @@ ice_fill_adv_packet_vlan(u16 vlan_type, u8 *pkt,
>  	return -EIO;
>  }
>  
> +static bool ice_is_rule_info_the_same(struct ice_adv_rule_info *first,

Doesn't sound natural. "ice_rules_equal"?

> +				      struct ice_adv_rule_info *second)

The function is read-only, `const` for both arguments.

> +{
> +	return first->sw_act.flag == second->sw_act.flag &&
> +	       first->tun_type == second->tun_type &&
> +	       first->vlan_type == second->vlan_type &&
> +	       first->src_vsi == second->src_vsi;
> +}

[...]

> @@ -6121,7 +6088,12 @@ ice_add_adv_rule(struct ice_hw *hw, struct ice_adv_lkup_elem *lkups,
>  	if (rinfo->sw_act.fltr_act == ICE_FWD_TO_VSI)
>  		rinfo->sw_act.fwd_id.hw_vsi_id =
>  			ice_get_hw_vsi_num(hw, vsi_handle);
> -	rinfo->sw_act.src = ice_get_hw_vsi_num(hw, vsi_handle);
> +
> +	if (rinfo->src_vsi)
> +		rinfo->sw_act.src =
> +			ice_get_hw_vsi_num(hw, rinfo->src_vsi);

This fits into one line in my editor :D

> +	else
> +		rinfo->sw_act.src = ice_get_hw_vsi_num(hw, vsi_handle);
>  
>  	status = ice_add_adv_recipe(hw, lkups, lkups_cnt, rinfo, &rid);
>  	if (status)

[...]

> --- a/drivers/net/ethernet/intel/ice/ice_switch.h
> +++ b/drivers/net/ethernet/intel/ice/ice_switch.h
> @@ -186,11 +186,13 @@ struct ice_adv_rule_flags_info {
>  };
>  
>  struct ice_adv_rule_info {
> +	/* Store metadata values in rule info */
>  	enum ice_sw_tunnel_type tun_type;
> +	u16 vlan_type;
> +	u16 src_vsi;
>  	struct ice_sw_act_ctrl sw_act;
>  	u32 priority;
>  	u16 fltr_rule_id;
> -	u16 vlan_type;
>  	struct ice_adv_rule_flags_info flags_info;

Please check holes within the structure. I see at least one in between
`fltr_rule_id` and `flags_info`. Some fields can definitely be moved around.

>  };
>  
Thanks,
Olek