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

Drewek, Wojciech wojciech.drewek at intel.com
Tue Apr 25 09:17:36 UTC 2023



> -----Original Message-----
> From: Lobakin, Aleksander <aleksander.lobakin at intel.com>
> Sent: piątek, 21 kwietnia 2023 16:23
> To: Drewek, Wojciech <wojciech.drewek at intel.com>
> Cc: intel-wired-lan at lists.osuosl.org; netdev at vger.kernel.org; Lobakin, Aleksander <aleksander.lobakin at intel.com>; Ertman, David M
> <david.m.ertman at intel.com>; michal.swiatkowski at linux.intel.com; marcin.szycik at linux.intel.com; Chmielewski, Pawel
> <pawel.chmielewski at intel.com>; Samudrala, Sridhar <sridhar.samudrala at intel.com>
> Subject: Re: [PATCH net-next 06/12] ice: Add guard rule when creating FDB in switchdev
> 
> From: Wojciech Drewek <wojciech.drewek at intel.com>
> Date: Mon, 17 Apr 2023 11:34:06 +0200
> 
> > From: Marcin Szycik <marcin.szycik at 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.

Makes sense, thanks.

> 
> > +	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) {
> 		...
> 

I like it, less ptr <-> macros

> > +		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?

Agree

> 
> >  {
> >  	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:

Correct

> 
> 	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



More information about the Intel-wired-lan mailing list