| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <ZHy9yNtc5GMr6UbR@corigine.com>
Date: Sun, 4 Jun 2023 18:37:28 +0200
From: Simon Horman <simon.horman@...igine.com>
To: Wojciech Drewek <wojciech.drewek@...el.com>
Cc: intel-wired-lan@...ts.osuosl.org, netdev@...r.kernel.org,
alexandr.lobakin@...el.com, david.m.ertman@...el.com,
michal.swiatkowski@...ux.intel.com, marcin.szycik@...ux.intel.com,
pawel.chmielewski@...el.com, sridhar.samudrala@...el.com,
pmenzel@...gen.mpg.de, dan.carpenter@...aro.org
Subject: Re: [PATCH iwl-next v4 10/13] ice: Add VLAN FDB support in switchdev
mode
On Wed, May 24, 2023 at 02:21:18PM +0200, Wojciech Drewek wrote:
> From: Marcin Szycik <marcin.szycik@...el.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@...el.com>
> Signed-off-by: Wojciech Drewek <wojciech.drewek@...el.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;
...
Powered by blists - more mailing lists