netdev - RE: [Intel-wired-lan] [PATCH net-next] ice: VLAN untagged traffic support

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <MW4PR11MB57765D0996877C9E782D6067FD01A@MW4PR11MB5776.namprd11.prod.outlook.com>
Date: Thu, 27 Jul 2023 09:18:28 +0000
From: "Drewek, Wojciech" <wojciech.drewek@...el.com>
To: Paul Menzel <pmenzel@...gen.mpg.de>
CC: "intel-wired-lan@...ts.osuosl.org" <intel-wired-lan@...ts.osuosl.org>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: RE: [Intel-wired-lan] [PATCH net-next] ice: VLAN untagged traffic
 support

Hi Paul,

Thanks for review

> -----Original Message-----
> From: Paul Menzel <pmenzel@...gen.mpg.de>
> Sent: środa, 26 lipca 2023 13:41
> To: Drewek, Wojciech <wojciech.drewek@...el.com>
> Cc: intel-wired-lan@...ts.osuosl.org; netdev@...r.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@...el.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