Re: [PATCH iwl-next v4 10/13] ice: Add VLAN FDB support in switchdev mode

[Simon Horman <simon.horman@corigine.com>]

On Wed, May 24, 2023 at 02:21:18PM +0200, Wojciech Drewek wrote:
> From: Marcin Szycik <marcin.szycik@intel.com>
> 
> Add support for matching on VLAN tag in bridge offloads.
> Currently only trunk mode is supported.
> 
> To enable VLAN filtering (existing FDB entries will be deleted):
> ip link set $BR type bridge vlan_filtering 1
> 
> To add VLANs to bridge in trunk mode:
> bridge vlan add dev $PF1 vid 110-111
> bridge vlan add dev $VF1_PR vid 110-111
> 
> Signed-off-by: Marcin Szycik <marcin.szycik@intel.com>
> Signed-off-by: Wojciech Drewek <wojciech.drewek@intel.com>

Hi Wojciech,

some minor feedback on this one from my side.

...

> diff --git a/drivers/net/ethernet/intel/ice/ice_eswitch_br.c b/drivers/net/ethernet/intel/ice/ice_eswitch_br.c
> index 19481decffe4..820b3296da60 100644
> --- a/drivers/net/ethernet/intel/ice/ice_eswitch_br.c
> +++ b/drivers/net/ethernet/intel/ice/ice_eswitch_br.c
> @@ -64,13 +64,19 @@ ice_eswitch_br_netdev_to_port(struct net_device *dev)
>  static void
>  ice_eswitch_br_ingress_rule_setup(struct ice_adv_lkup_elem *list,
>  				  struct ice_adv_rule_info *rule_info,
> -				  const unsigned char *mac,
> +				  const unsigned char *mac, u16 vid,
>  				  u8 pf_id, u16 vf_vsi_idx)
>  {
>  	list[0].type = ICE_MAC_OFOS;
>  	ether_addr_copy(list[0].h_u.eth_hdr.dst_addr, mac);
>  	eth_broadcast_addr(list[0].m_u.eth_hdr.dst_addr);
>  
> +	if (ice_eswitch_is_vid_valid(vid)) {
> +		list[1].type = ICE_VLAN_OFOS;
> +		list[1].h_u.vlan_hdr.vlan = cpu_to_be16(vid & VLAN_VID_MASK);
> +		list[1].m_u.vlan_hdr.vlan = cpu_to_be16(0xFFFF);
> +	}

nit: the above code seems to be (largely) duplicated in (at least)
     ice_eswitch_br_egress_rule_setup(). Perhaps a helper function
     would be appropriate.

> +
>  	rule_info->sw_act.vsi_handle = vf_vsi_idx;
>  	rule_info->sw_act.flag |= ICE_FLTR_RX;
>  	rule_info->sw_act.src = pf_id;
> @@ -80,13 +86,19 @@ ice_eswitch_br_ingress_rule_setup(struct ice_adv_lkup_elem *list,
>  static void
>  ice_eswitch_br_egress_rule_setup(struct ice_adv_lkup_elem *list,
>  				 struct ice_adv_rule_info *rule_info,
> -				 const unsigned char *mac,
> +				 const unsigned char *mac, u16 vid,
>  				 u16 pf_vsi_idx)
>  {
>  	list[0].type = ICE_MAC_OFOS;
>  	ether_addr_copy(list[0].h_u.eth_hdr.dst_addr, mac);
>  	eth_broadcast_addr(list[0].m_u.eth_hdr.dst_addr);
>  
> +	if (ice_eswitch_is_vid_valid(vid)) {
> +		list[1].type = ICE_VLAN_OFOS;
> +		list[1].h_u.vlan_hdr.vlan = cpu_to_be16(vid & VLAN_VID_MASK);
> +		list[1].m_u.vlan_hdr.vlan = cpu_to_be16(0xFFFF);
> +	}
> +
>  	rule_info->sw_act.vsi_handle = pf_vsi_idx;
>  	rule_info->sw_act.flag |= ICE_FLTR_TX;
>  	rule_info->flags_info.act = ICE_SINGLE_ACT_LAN_ENABLE;
> @@ -110,14 +122,19 @@ ice_eswitch_br_rule_delete(struct ice_hw *hw, struct ice_rule_query_data *rule)
>  
>  static struct ice_rule_query_data *
>  ice_eswitch_br_fwd_rule_create(struct ice_hw *hw, int vsi_idx, int port_type,
> -			       const unsigned char *mac)
> +			       const unsigned char *mac, u16 vid)
>  {
>  	struct ice_adv_rule_info rule_info = { 0 };
>  	struct ice_rule_query_data *rule;
>  	struct ice_adv_lkup_elem *list;
> -	u16 lkups_cnt = 1;
> +	u16 lkups_cnt;
>  	int err;
>  
> +	if (ice_eswitch_is_vid_valid(vid))
> +		lkups_cnt = 2;
> +	else
> +		lkups_cnt = 1;

nit: The above condition could be more succinctly expressed as
     (completely untested):

	lkups_cnt = ice_eswitch_is_vid_valid(vid) ? 2 : 1;

     Also, the above condition appears elsewhere in this patch.
     Perhaps a helper is appropriate.

> +
>  	rule = kzalloc(sizeof(*rule), GFP_KERNEL);
>  	if (!rule)
>  		return ERR_PTR(-ENOMEM);
> @@ -131,11 +148,11 @@ ice_eswitch_br_fwd_rule_create(struct ice_hw *hw, int vsi_idx, int port_type,
>  	switch (port_type) {
>  	case ICE_ESWITCH_BR_UPLINK_PORT:
>  		ice_eswitch_br_egress_rule_setup(list, &rule_info, mac,
> -						 vsi_idx);
> +						 vid, vsi_idx);
>  		break;
>  	case ICE_ESWITCH_BR_VF_REPR_PORT:
>  		ice_eswitch_br_ingress_rule_setup(list, &rule_info, mac,
> -						  hw->pf_id, vsi_idx);
> +						  vid, hw->pf_id, vsi_idx);
>  		break;
>  	default:
>  		err = -EINVAL;
> @@ -164,13 +181,18 @@ ice_eswitch_br_fwd_rule_create(struct ice_hw *hw, int vsi_idx, int port_type,
>  
>  static struct ice_rule_query_data *
>  ice_eswitch_br_guard_rule_create(struct ice_hw *hw, u16 vsi_idx,
> -				 const unsigned char *mac)
> +				 const unsigned char *mac, u16 vid)
>  {
>  	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 = -ENOMEM;
> +	u16 lkups_cnt;
> +
> +	if (ice_eswitch_is_vid_valid(vid))
> +		lkups_cnt = 2;
> +	else
> +		lkups_cnt = 1;
>  
>  	rule = kzalloc(sizeof(*rule), GFP_KERNEL);
>  	if (!rule)
> @@ -184,6 +206,12 @@ ice_eswitch_br_guard_rule_create(struct ice_hw *hw, u16 vsi_idx,
>  	ether_addr_copy(list[0].h_u.eth_hdr.src_addr, mac);
>  	eth_broadcast_addr(list[0].m_u.eth_hdr.src_addr);
>  
> +	if (ice_eswitch_is_vid_valid(vid)) {
> +		list[1].type = ICE_VLAN_OFOS;
> +		list[1].h_u.vlan_hdr.vlan = cpu_to_be16(vid & VLAN_VID_MASK);
> +		list[1].m_u.vlan_hdr.vlan = cpu_to_be16(0xFFFF);
> +	}
> +
>  	rule_info.allow_pass_l2 = true;
>  	rule_info.sw_act.vsi_handle = vsi_idx;
>  	rule_info.sw_act.fltr_act = ICE_NOP;

...