Re: [Intel-wired-lan] [PATCH net-next 06/12] ice: Add guard rule when creating FDB in switchdev

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

From: Wojciech Drewek <wojciech.drewek@intel.com>
Date: Mon, 17 Apr 2023 11:34:06 +0200

> From: Marcin Szycik <marcin.szycik@intel.com>
> 
> Introduce new "guard" rule upon FDB entry creation.
> 
> It matches on src_mac, has valid bit unset, allow_pass_l2 set
> and has a nop action.

[...]

> +static struct ice_rule_query_data *
> +ice_eswitch_br_guard_rule_create(struct ice_hw *hw, u16 vsi_idx,
> +				 const unsigned char *mac)
> +{
> +	struct ice_adv_rule_info rule_info = { 0 };
> +	struct ice_rule_query_data *rule;
> +	struct ice_adv_lkup_elem *list;
> +	const u16 lkups_cnt = 1;
> +	int err;

You can initialize it with -%ENOMEM right here in order to...

> +
> +	rule = kzalloc(sizeof(*rule), GFP_KERNEL);
> +	if (!rule) {
> +		err = -ENOMEM;
> +		goto err_exit;
> +	}
> +
> +	list = kcalloc(lkups_cnt, sizeof(*list), GFP_ATOMIC);
> +	if (!list) {
> +		err = -ENOMEM;
> +		goto err_list_alloc;
> +	}

...make those 2 ifs goto-oneliners :3 As...

> +
> +	list[0].type = ICE_MAC_OFOS;
> +	ether_addr_copy(list[0].h_u.eth_hdr.src_addr, mac);
> +	eth_broadcast_addr(list[0].m_u.eth_hdr.src_addr);
> +
> +	rule_info.allow_pass_l2 = true;
> +	rule_info.sw_act.vsi_handle = vsi_idx;
> +	rule_info.sw_act.fltr_act = ICE_NOP;
> +	rule_info.priority = 5;
> +
> +	err = ice_add_adv_rule(hw, list, lkups_cnt, &rule_info, rule);

...it's overwritten here anyway, so it is safe to init it with an error
value.

> +	if (err)
> +		goto err_add_rule;
> +
> +	return rule;
> +
> +err_add_rule:
> +	kfree(list);
> +err_list_alloc:
> +	kfree(rule);
> +err_exit:
> +	return ERR_PTR(err);
> +}
> +
>  static struct ice_esw_br_flow *
>  ice_eswitch_br_flow_create(struct device *dev, struct ice_hw *hw, u16 vsi_idx,
>  			   int port_type, const unsigned char *mac)
>  {
> -	struct ice_rule_query_data *fwd_rule;
> +	struct ice_rule_query_data *fwd_rule, *guard_rule;
>  	struct ice_esw_br_flow *flow;
>  	int err;
>  
> @@ -155,10 +202,22 @@ ice_eswitch_br_flow_create(struct device *dev, struct ice_hw *hw, u16 vsi_idx,
>  		goto err_fwd_rule;
>  	}
>  
> +	guard_rule = ice_eswitch_br_guard_rule_create(hw, vsi_idx, mac);
> +	if (IS_ERR(guard_rule)) {
> +		err = PTR_ERR(guard_rule);

Aaah ok, that's what you meant in the previous mails. I see now.
You can either leave it like that or there's an alternative -- pick the
one that you like the most:

	guard_rule = ice_eswitch_...
	err = PTR_ERR(guard_rule);
	if (err) {
		...

> +		dev_err(dev, "Failed to create eswitch bridge %sgress guard rule, err: %d\n",
> +			port_type == ICE_ESWITCH_BR_UPLINK_PORT ? "e" : "in",
> +			err);

You still can print it via "%pe" + @guard_rule instead of @err :p (same
with @fwd_rule above)

> +		goto err_guard_rule;
> +	}
> +
>  	flow->fwd_rule = fwd_rule;
> +	flow->guard_rule = guard_rule;
>  
>  	return flow;

[...]

> @@ -4624,7 +4628,7 @@ static struct ice_protocol_entry ice_prot_id_tbl[ICE_PROTOCOL_LAST] = {
>   */
>  static u16
>  ice_find_recp(struct ice_hw *hw, struct ice_prot_lkup_ext *lkup_exts,
> -	      enum ice_sw_tunnel_type tun_type)
> +	      struct ice_adv_rule_info *rinfo)

Can be const I think?

>  {
>  	bool refresh_required = true;
>  	struct ice_sw_recipe *recp;

[...]

> @@ -5075,6 +5082,14 @@ ice_add_sw_recipe(struct ice_hw *hw, struct ice_sw_recipe *rm,
>  		set_bit(buf[recps].recipe_indx,
>  			(unsigned long *)buf[recps].recipe_bitmap);
>  		buf[recps].content.act_ctrl_fwd_priority = rm->priority;
> +
> +		if (rm->need_pass_l2)
> +			buf[recps].content.act_ctrl |=
> +				ICE_AQ_RECIPE_ACT_NEED_PASS_L2;
> +
> +		if (rm->allow_pass_l2)
> +			buf[recps].content.act_ctrl |=
> +				ICE_AQ_RECIPE_ACT_ALLOW_PASS_L2;

I don't like these line breaks :s

		type_of_content *cont;
		...

		/* As far as I can see, it can be used above as well */
		cont = &buf[recps].content;

		if (rm->need_pass_l2)
			cont->act_ctrl |= ICE_AQ_RECIPE_ACT_NEED_PASS_L2;
		if (rm->allow_pass_l2)
			cont->act_ctrl |= ICE_AQ_RECIPE_ACT_ALLOW_PASS_L2;

>  		recps++;
>  	}
>  

[...]

> @@ -6166,6 +6190,11 @@ ice_add_adv_rule(struct ice_hw *hw, struct ice_adv_lkup_elem *lkups,
>  		act |= ICE_SINGLE_ACT_VSI_FORWARDING | ICE_SINGLE_ACT_DROP |
>  		       ICE_SINGLE_ACT_VALID_BIT;
>  		break;
> +	case ICE_NOP:
> +		act |= (rinfo->sw_act.fwd_id.hw_vsi_id <<
> +			ICE_SINGLE_ACT_VSI_ID_S) & ICE_SINGLE_ACT_VSI_ID_M;

`FIELD_PREP(ICE_SINGLE_ACT_VSI_ID_M, rinfo->sw_act.fwd_id.hw_vsi_id)`?

> +		act &= ~ICE_SINGLE_ACT_VALID_BIT;
> +		break;
>  	default:
>  		status = -EIO;
>  		goto err_ice_add_adv_rule;
> @@ -6446,7 +6475,7 @@ ice_rem_adv_rule(struct ice_hw *hw, struct ice_adv_lkup_elem *lkups,
>  			return -EIO;
>  	}
>  
> -	rid = ice_find_recp(hw, &lkup_exts, rinfo->tun_type);
> +	rid = ice_find_recp(hw, &lkup_exts, rinfo);
>  	/* If did not find a recipe that match the existing criteria */
>  	if (rid == ICE_MAX_NUM_RECIPES)
>  		return -EINVAL;
> diff --git a/drivers/net/ethernet/intel/ice/ice_switch.h b/drivers/net/ethernet/intel/ice/ice_switch.h
> index c84b56fe84a5..5ecce39cf1f5 100644
> --- a/drivers/net/ethernet/intel/ice/ice_switch.h
> +++ b/drivers/net/ethernet/intel/ice/ice_switch.h
> @@ -191,6 +191,8 @@ struct ice_adv_rule_info {
>  	u16 vlan_type;
>  	u16 fltr_rule_id;
>  	u32 priority;
> +	u8 need_pass_l2;
> +	u8 allow_pass_l2;

They can be either true or false, nothing else, right? I'd make them
occupy 1 bit per var then:

	u16 need_pass_l2:1;
	u16 allow_pass_l2:1;
	u16 src_vsi;

+14 free bits for more flags, no holes (stacked with ::src_vsi).

>  	u16 src_vsi;
>  	struct ice_sw_act_ctrl sw_act;
>  	struct ice_adv_rule_flags_info flags_info;
> @@ -254,6 +256,9 @@ struct ice_sw_recipe {
>  	 */
>  	u8 priority;
>  
> +	u8 need_pass_l2;
> +	u8 allow_pass_l2;

(same with bitfields here, just use u8 :1 instead of u16 here to stack
 with ::priority)

> +
>  	struct list_head rg_list;

[...]

Thanks,
Olek

_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan