| Message ID | 20230726105054.295220-1-wojciech.drewek@intel.com (mailing list archive) |
|---|---|
| State | Awaiting Upstream |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next] ice: VLAN untagged traffic support | expand |
Dear Wojciech, Thank you for your patch. Could you use a statement as summary by using a verb (in imperative mood)? Maybe: Support untagged VLAN traffic Am 26.07.23 um 12:50 schrieb Wojciech Drewek: > When driver recives SWITCHDEV_FDB_ADD_TO_DEVICE notification receives > with vid = 1, it means that we have to offload untagged traffic. > This is achieved by adding vlan metadata lookup. > > Also remove unnecessary brackets in ice_eswitch_br_fdb_entry_create. For things starting with “Also” in the git commit message, that’s a good indicator for a separate commit. ;-) > Signed-off-by: Wojciech Drewek <wojciech.drewek@intel.com> > --- > drivers/net/ethernet/intel/ice/ice_eswitch_br.c | 9 +++++---- > drivers/net/ethernet/intel/ice/ice_eswitch_br.h | 9 --------- > 2 files changed, 5 insertions(+), 13 deletions(-) > > diff --git a/drivers/net/ethernet/intel/ice/ice_eswitch_br.c b/drivers/net/ethernet/intel/ice/ice_eswitch_br.c > index 67bfd1f61cdd..05bee706b946 100644 > --- a/drivers/net/ethernet/intel/ice/ice_eswitch_br.c > +++ b/drivers/net/ethernet/intel/ice/ice_eswitch_br.c > @@ -104,13 +104,15 @@ ice_eswitch_br_rule_delete(struct ice_hw *hw, struct ice_rule_query_data *rule) > static u16 > ice_eswitch_br_get_lkups_cnt(u16 vid) > { > - return ice_eswitch_br_is_vid_valid(vid) ? 2 : 1; > + return vid == 0 ? 1 : 2; Should some comment be added to the function to explain the behavior? > } > > static void > ice_eswitch_br_add_vlan_lkup(struct ice_adv_lkup_elem *list, u16 vid) > { > - if (ice_eswitch_br_is_vid_valid(vid)) { > + if (vid == 1) { > + ice_rule_add_vlan_metadata(&list[1]); > + } else if (vid > 1) { Why is vid == 1 treated differently from the others? Kind regards, Paul > 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); > @@ -400,11 +402,10 @@ ice_eswitch_br_fdb_entry_create(struct net_device *netdev, > unsigned long event; > int err; > > - /* untagged filtering is not yet supported */ > if (!(bridge->flags & ICE_ESWITCH_BR_VLAN_FILTERING) && vid) > return; > > - if ((bridge->flags & ICE_ESWITCH_BR_VLAN_FILTERING)) { > + if (bridge->flags & ICE_ESWITCH_BR_VLAN_FILTERING) { > vlan = ice_esw_br_port_vlan_lookup(bridge, br_port->vsi_idx, > vid); > if (IS_ERR(vlan)) { > diff --git a/drivers/net/ethernet/intel/ice/ice_eswitch_br.h b/drivers/net/ethernet/intel/ice/ice_eswitch_br.h > index 85a8fadb2928..cf7b0e5acfcb 100644 > --- a/drivers/net/ethernet/intel/ice/ice_eswitch_br.h > +++ b/drivers/net/ethernet/intel/ice/ice_eswitch_br.h > @@ -103,15 +103,6 @@ struct ice_esw_br_vlan { > struct ice_esw_br_fdb_work, \ > work) > > -static inline bool ice_eswitch_br_is_vid_valid(u16 vid) > -{ > - /* In trunk VLAN mode, for untagged traffic the bridge sends requests > - * to offload VLAN 1 with pvid and untagged flags set. Since these > - * flags are not supported, add a MAC filter instead. > - */ > - return vid > 1; > -} > - > void > ice_eswitch_br_offloads_deinit(struct ice_pf *pf); > int
Hi Paul, Thanks for review > -----Original Message----- > From: Paul Menzel <pmenzel@molgen.mpg.de> > Sent: środa, 26 lipca 2023 13:41 > To: Drewek, Wojciech <wojciech.drewek@intel.com> > Cc: intel-wired-lan@lists.osuosl.org; netdev@vger.kernel.org > Subject: Re: [Intel-wired-lan] [PATCH net-next] ice: VLAN untagged traffic support > > Dear Wojciech, > > > Thank you for your patch. > > Could you use a statement as summary by using a verb (in imperative > mood)? Maybe: > > Support untagged VLAN traffic sure > > Am 26.07.23 um 12:50 schrieb Wojciech Drewek: > > When driver recives SWITCHDEV_FDB_ADD_TO_DEVICE notification > > receives > > > with vid = 1, it means that we have to offload untagged traffic. > > This is achieved by adding vlan metadata lookup. > > > > Also remove unnecessary brackets in ice_eswitch_br_fdb_entry_create. > > For things starting with “Also” in the git commit message, that’s a good > indicator for a separate commit. ;-) ok > > > Signed-off-by: Wojciech Drewek <wojciech.drewek@intel.com> > > --- > > drivers/net/ethernet/intel/ice/ice_eswitch_br.c | 9 +++++---- > > drivers/net/ethernet/intel/ice/ice_eswitch_br.h | 9 --------- > > 2 files changed, 5 insertions(+), 13 deletions(-) > > > > diff --git a/drivers/net/ethernet/intel/ice/ice_eswitch_br.c b/drivers/net/ethernet/intel/ice/ice_eswitch_br.c > > index 67bfd1f61cdd..05bee706b946 100644 > > --- a/drivers/net/ethernet/intel/ice/ice_eswitch_br.c > > +++ b/drivers/net/ethernet/intel/ice/ice_eswitch_br.c > > @@ -104,13 +104,15 @@ ice_eswitch_br_rule_delete(struct ice_hw *hw, struct ice_rule_query_data *rule) > > static u16 > > ice_eswitch_br_get_lkups_cnt(u16 vid) > > { > > - return ice_eswitch_br_is_vid_valid(vid) ? 2 : 1; > > + return vid == 0 ? 1 : 2; > > Should some comment be added to the function to explain the behavior? Will do > > > } > > > > static void > > ice_eswitch_br_add_vlan_lkup(struct ice_adv_lkup_elem *list, u16 vid) > > { > > - if (ice_eswitch_br_is_vid_valid(vid)) { > > + if (vid == 1) { > > + ice_rule_add_vlan_metadata(&list[1]); > > + } else if (vid > 1) { > > Why is vid == 1 treated differently from the others? With vid ==1 we want to match on MAC but without vlan tags. To achieve this, we're adding vlan metadata (matching on all vlan tags) but we do not add ICE_VLAN_OFOS lookup type which gives us mac + not vlan matching criteria. > > > Kind regards, > > Paul > > > > 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); > > @@ -400,11 +402,10 @@ ice_eswitch_br_fdb_entry_create(struct net_device *netdev, > > unsigned long event; > > int err; > > > > - /* untagged filtering is not yet supported */ > > if (!(bridge->flags & ICE_ESWITCH_BR_VLAN_FILTERING) && vid) > > return; > > > > - if ((bridge->flags & ICE_ESWITCH_BR_VLAN_FILTERING)) { > > + if (bridge->flags & ICE_ESWITCH_BR_VLAN_FILTERING) { > > vlan = ice_esw_br_port_vlan_lookup(bridge, br_port->vsi_idx, > > vid); > > if (IS_ERR(vlan)) { > > diff --git a/drivers/net/ethernet/intel/ice/ice_eswitch_br.h b/drivers/net/ethernet/intel/ice/ice_eswitch_br.h > > index 85a8fadb2928..cf7b0e5acfcb 100644 > > --- a/drivers/net/ethernet/intel/ice/ice_eswitch_br.h > > +++ b/drivers/net/ethernet/intel/ice/ice_eswitch_br.h > > @@ -103,15 +103,6 @@ struct ice_esw_br_vlan { > > struct ice_esw_br_fdb_work, \ > > work) > > > > -static inline bool ice_eswitch_br_is_vid_valid(u16 vid) > > -{ > > - /* In trunk VLAN mode, for untagged traffic the bridge sends requests > > - * to offload VLAN 1 with pvid and untagged flags set. Since these > > - * flags are not supported, add a MAC filter instead. > > - */ > > - return vid > 1; > > -} > > - > > void > > ice_eswitch_br_offloads_deinit(struct ice_pf *pf); > > int
diff --git a/drivers/net/ethernet/intel/ice/ice_eswitch_br.c b/drivers/net/ethernet/intel/ice/ice_eswitch_br.c index 67bfd1f61cdd..05bee706b946 100644 --- a/drivers/net/ethernet/intel/ice/ice_eswitch_br.c +++ b/drivers/net/ethernet/intel/ice/ice_eswitch_br.c @@ -104,13 +104,15 @@ ice_eswitch_br_rule_delete(struct ice_hw *hw, struct ice_rule_query_data *rule) static u16 ice_eswitch_br_get_lkups_cnt(u16 vid) { - return ice_eswitch_br_is_vid_valid(vid) ? 2 : 1; + return vid == 0 ? 1 : 2; } static void ice_eswitch_br_add_vlan_lkup(struct ice_adv_lkup_elem *list, u16 vid) { - if (ice_eswitch_br_is_vid_valid(vid)) { + if (vid == 1) { + ice_rule_add_vlan_metadata(&list[1]); + } else if (vid > 1) { 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); @@ -400,11 +402,10 @@ ice_eswitch_br_fdb_entry_create(struct net_device *netdev, unsigned long event; int err; - /* untagged filtering is not yet supported */ if (!(bridge->flags & ICE_ESWITCH_BR_VLAN_FILTERING) && vid) return; - if ((bridge->flags & ICE_ESWITCH_BR_VLAN_FILTERING)) { + if (bridge->flags & ICE_ESWITCH_BR_VLAN_FILTERING) { vlan = ice_esw_br_port_vlan_lookup(bridge, br_port->vsi_idx, vid); if (IS_ERR(vlan)) { diff --git a/drivers/net/ethernet/intel/ice/ice_eswitch_br.h b/drivers/net/ethernet/intel/ice/ice_eswitch_br.h index 85a8fadb2928..cf7b0e5acfcb 100644 --- a/drivers/net/ethernet/intel/ice/ice_eswitch_br.h +++ b/drivers/net/ethernet/intel/ice/ice_eswitch_br.h @@ -103,15 +103,6 @@ struct ice_esw_br_vlan { struct ice_esw_br_fdb_work, \ work) -static inline bool ice_eswitch_br_is_vid_valid(u16 vid) -{ - /* In trunk VLAN mode, for untagged traffic the bridge sends requests - * to offload VLAN 1 with pvid and untagged flags set. Since these - * flags are not supported, add a MAC filter instead. - */ - return vid > 1; -} - void ice_eswitch_br_offloads_deinit(struct ice_pf *pf); int
When driver recives SWITCHDEV_FDB_ADD_TO_DEVICE notification with vid = 1, it means that we have to offload untagged traffic. This is achieved by adding vlan metadata lookup. Also remove unnecessary brackets in ice_eswitch_br_fdb_entry_create. Signed-off-by: Wojciech Drewek <wojciech.drewek@intel.com> --- drivers/net/ethernet/intel/ice/ice_eswitch_br.c | 9 +++++---- drivers/net/ethernet/intel/ice/ice_eswitch_br.h | 9 --------- 2 files changed, 5 insertions(+), 13 deletions(-)