[net-next,08/12] ice: Add VLAN FDB support in switchdev mode

Commit Message

Wojciech Drewek April 17, 2023, 9:34 a.m. UTC

From: Marcin Szycik <[e-mail]>

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 <[e-mail]>
---
 .../net/ethernet/intel/ice/ice_eswitch_br.c   | 319 +++++++++++++++++-
 .../net/ethernet/intel/ice/ice_eswitch_br.h   |  12 +
 2 files changed, 317 insertions(+), 14 deletions(-)

Comments

Alexander Lobakin April 21, 2023, 3:25 p.m. UTC | #1

From: Wojciech Drewek <[e-mail]>
Date: Mon, 17 Apr 2023 11:34:08 +0200

> From: Marcin Szycik <[e-mail]>
> 
> 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 <[e-mail]>
> ---
>  .../net/ethernet/intel/ice/ice_eswitch_br.c   | 319 +++++++++++++++++-
>  .../net/ethernet/intel/ice/ice_eswitch_br.h   |  12 +
>  2 files changed, 317 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/ice/ice_eswitch_br.c b/drivers/net/ethernet/intel/ice/ice_eswitch_br.c
> index 49381e4bf62a..56d36e397b12 100644
> --- a/drivers/net/ethernet/intel/ice/ice_eswitch_br.c
> +++ b/drivers/net/ethernet/intel/ice/ice_eswitch_br.c
> @@ -59,13 +59,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, bool vlan, u16 vid,

Could we use one combined argument? Doesn't `!!vid == !!vlan`? VID 0 is
reserved IIRC...

(same in all the places below)

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

[...]

> @@ -344,10 +389,33 @@ ice_eswitch_br_fdb_entry_create(struct net_device *netdev,
>  	struct device *dev = ice_pf_to_dev(pf);
>  	struct ice_esw_br_fdb_entry *fdb_entry;
>  	struct ice_esw_br_flow *flow;
> +	struct ice_esw_br_vlan *vlan;
>  	struct ice_hw *hw = &pf->hw;
> +	bool add_vlan = false;
>  	unsigned long event;
>  	int err;
>  
> +	/* FIXME: untagged filtering is not yet supported
> +	 */

Shouldn't be present in release code I believe. I mean, the sentence is
fine (just don't forget dot at the end), but without "FIXME:". And it
can be one-liner.

> +	if (!(bridge->flags & ICE_ESWITCH_BR_VLAN_FILTERING) && vid)
> +		return;

[...]

> +static void
> +ice_eswitch_br_vlan_filtering_set(struct ice_esw_br *bridge, bool enable)
> +{
> +	bool filtering = bridge->flags & ICE_ESWITCH_BR_VLAN_FILTERING;
> +
> +	if (filtering == enable)
> +		return;

	if (enable == !!(bridge->flags & ICE_ESWITCH_BR_VLAN_FILTERING))

?

> +
> +	ice_eswitch_br_fdb_flush(bridge);
> +	if (enable)
> +		bridge->flags |= ICE_ESWITCH_BR_VLAN_FILTERING;
> +	else
> +		bridge->flags &= ~ICE_ESWITCH_BR_VLAN_FILTERING;
> +}

[...]

> +	port = xa_load(&bridge->ports, vsi_idx);
> +	if (!port)
> +		return -EINVAL;
> +
> +	vlan = xa_load(&port->vlans, vid);
> +	if (vlan) {
> +		if (vlan->flags == flags)
> +			return 0;
> +
> +		ice_eswitch_br_vlan_cleanup(port, vlan);
> +	}
> +
> +	vlan = ice_eswitch_br_vlan_create(vid, flags, port);
> +	if (IS_ERR(vlan)) {
> +		NL_SET_ERR_MSG_MOD(extack, "Failed to create VLAN entry");

FYI, there's NL_SET_ERR_MSG_FMT_MOD() landed recently (a couple releases
back), which supports format strings. E.g. you could pass VID, VSI ID,
flags etc. there to have more meaningful output (right in userspace).

> +		return PTR_ERR(vlan);
> +	}
> +
> +	return 0;
> +}

[...]

> +static int
> +ice_eswitch_br_port_obj_add(struct net_device *netdev, const void *ctx,
> +			    const struct switchdev_obj *obj,
> +			    struct netlink_ext_ack *extack)
> +{
> +	struct ice_esw_br_port *br_port = ice_eswitch_br_netdev_to_port(netdev);
> +	struct switchdev_obj_port_vlan *vlan;
> +	int err;
> +
> +	if (!br_port)
> +		return -EINVAL;
> +
> +	switch (obj->id) {
> +	case SWITCHDEV_OBJ_ID_PORT_VLAN:
> +		vlan = SWITCHDEV_OBJ_PORT_VLAN(obj);
> +		err = ice_eswitch_br_port_vlan_add(br_port->bridge,
> +						   br_port->vsi_idx, vlan->vid,
> +						   vlan->flags, extack);

return right here? You have `default` in the switch block, so the
compiler shouldn't complain if you remove it from the end of the func.

> +		break;
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +
> +	return err;
> +}
> +
> +static int
> +ice_eswitch_br_port_obj_del(struct net_device *netdev, const void *ctx,
> +			    const struct switchdev_obj *obj)
> +{
> +	struct ice_esw_br_port *br_port = ice_eswitch_br_netdev_to_port(netdev);
> +	struct switchdev_obj_port_vlan *vlan;
> +
> +	if (!br_port)
> +		return -EINVAL;
> +
> +	switch (obj->id) {
> +	case SWITCHDEV_OBJ_ID_PORT_VLAN:
> +		vlan = SWITCHDEV_OBJ_PORT_VLAN(obj);
> +		ice_eswitch_br_port_vlan_del(br_port->bridge, br_port->vsi_idx,
> +					     vlan->vid);

(same)

> +		break;
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +
> +	return 0;
> +}
> +
> +static int
> +ice_eswitch_br_port_obj_attr_set(struct net_device *netdev, const void *ctx,
> +				 const struct switchdev_attr *attr,
> +				 struct netlink_ext_ack *extack)
> +{
> +	struct ice_esw_br_port *br_port = ice_eswitch_br_netdev_to_port(netdev);
> +
> +	if (!br_port)
> +		return -EINVAL;
> +
> +	switch (attr->id) {
> +	case SWITCHDEV_ATTR_ID_BRIDGE_VLAN_FILTERING:
> +		ice_eswitch_br_vlan_filtering_set(br_port->bridge,
> +						  attr->u.vlan_filtering);

(and here)

> +		break;
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +
> +	return 0;
> +}

[...]

> +	br_offloads->switchdev_blk.notifier_call =
> +		ice_eswitch_br_event_blocking;

Oh, you have two usages of ->switchdev_blk here, so you can add an
intermediate variable to avoid line breaking, which would also shorten
the line below :D

	nb = &br_offloads->switchdev_blk;
	nb->notifier_call = ice_eswitch_br_event_blocking;
	...

> +	err = register_switchdev_blocking_notifier(&br_offloads->switchdev_blk);
> +	if (err) {
> +		dev_err(dev,
> +			"Failed to register bridge blocking switchdev notifier\n");
> +		goto err_reg_switchdev_blk;
> +	}
> +
>  	br_offloads->netdev_nb.notifier_call = ice_eswitch_br_port_event;
>  	err = register_netdevice_notifier(&br_offloads->netdev_nb);

(here the same, but no line breaks, so up to you. You could reuse the
 same variable or leave it as it is)

>  	if (err) {

[...]

> diff --git a/drivers/net/ethernet/intel/ice/ice_eswitch_br.h b/drivers/net/ethernet/intel/ice/ice_eswitch_br.h
> index 73ad81bad655..cf3e2615a62a 100644
> --- a/drivers/net/ethernet/intel/ice/ice_eswitch_br.h
> +++ b/drivers/net/ethernet/intel/ice/ice_eswitch_br.h
> @@ -42,10 +42,16 @@ struct ice_esw_br_port {
>  	enum ice_esw_br_port_type type;
>  	struct ice_vsi *vsi;
>  	u16 vsi_idx;
> +	struct xarray vlans;

Hmm, I feel like you can make ::type u16 and then stack it with
::vsi_idx, so that you avoid a hole here.

> +};
> +
> +enum {
> +	ICE_ESWITCH_BR_VLAN_FILTERING = BIT(0),
>  };
>  
>  struct ice_esw_br {
>  	struct ice_esw_br_offloads *br_offloads;
> +	int flags;

Unsigned types fit flags better I think?

>  	int ifindex;

(BTW, ifindex is also usually unsigned unless it's not an error)

>  
>  	struct xarray ports;
[...]

Thanks,
Olek

Wojciech Drewek April 27, 2023, 10:28 a.m. UTC | #2

> -----Original Message-----
> From: Lobakin, Aleksander <[e-mail]>
> Sent: piątek, 21 kwietnia 2023 17:25
> To: Drewek, Wojciech <[e-mail]>
> Cc: [e-mail]; [e-mail]; Lobakin, Aleksander <[e-mail]>; Ertman, David M
> <[e-mail]>; [e-mail]; [e-mail]; Chmielewski, Pawel
> <[e-mail]>; Samudrala, Sridhar <[e-mail]>
> Subject: Re: [PATCH net-next 08/12] ice: Add VLAN FDB support in switchdev mode
> 
> From: Wojciech Drewek <[e-mail]>
> Date: Mon, 17 Apr 2023 11:34:08 +0200
> 
> > From: Marcin Szycik <[e-mail]>
> >
> > 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 <[e-mail]>
> > ---
> >  .../net/ethernet/intel/ice/ice_eswitch_br.c   | 319 +++++++++++++++++-
> >  .../net/ethernet/intel/ice/ice_eswitch_br.h   |  12 +
> >  2 files changed, 317 insertions(+), 14 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/intel/ice/ice_eswitch_br.c b/drivers/net/ethernet/intel/ice/ice_eswitch_br.c
> > index 49381e4bf62a..56d36e397b12 100644
> > --- a/drivers/net/ethernet/intel/ice/ice_eswitch_br.c
> > +++ b/drivers/net/ethernet/intel/ice/ice_eswitch_br.c
> > @@ -59,13 +59,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, bool vlan, u16 vid,
> 
> Could we use one combined argument? Doesn't `!!vid == !!vlan`? VID 0 is
> reserved IIRC...
> 
> (same in all the places below)

Makes sense

> 
> >  				  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);
> 
> [...]
> 
> > @@ -344,10 +389,33 @@ ice_eswitch_br_fdb_entry_create(struct net_device *netdev,
> >  	struct device *dev = ice_pf_to_dev(pf);
> >  	struct ice_esw_br_fdb_entry *fdb_entry;
> >  	struct ice_esw_br_flow *flow;
> > +	struct ice_esw_br_vlan *vlan;
> >  	struct ice_hw *hw = &pf->hw;
> > +	bool add_vlan = false;
> >  	unsigned long event;
> >  	int err;
> >
> > +	/* FIXME: untagged filtering is not yet supported
> > +	 */
> 
> Shouldn't be present in release code I believe. I mean, the sentence is
> fine (just don't forget dot at the end), but without "FIXME:". And it
> can be one-liner.

Sure

> 
> > +	if (!(bridge->flags & ICE_ESWITCH_BR_VLAN_FILTERING) && vid)
> > +		return;
> 
> [...]
> 
> > +static void
> > +ice_eswitch_br_vlan_filtering_set(struct ice_esw_br *bridge, bool enable)
> > +{
> > +	bool filtering = bridge->flags & ICE_ESWITCH_BR_VLAN_FILTERING;
> > +
> > +	if (filtering == enable)
> > +		return;
> 
> 	if (enable == !!(bridge->flags & ICE_ESWITCH_BR_VLAN_FILTERING))
> 
> ?

I like it

> 
> > +
> > +	ice_eswitch_br_fdb_flush(bridge);
> > +	if (enable)
> > +		bridge->flags |= ICE_ESWITCH_BR_VLAN_FILTERING;
> > +	else
> > +		bridge->flags &= ~ICE_ESWITCH_BR_VLAN_FILTERING;
> > +}
> 
> [...]
> 
> > +	port = xa_load(&bridge->ports, vsi_idx);
> > +	if (!port)
> > +		return -EINVAL;
> > +
> > +	vlan = xa_load(&port->vlans, vid);
> > +	if (vlan) {
> > +		if (vlan->flags == flags)
> > +			return 0;
> > +
> > +		ice_eswitch_br_vlan_cleanup(port, vlan);
> > +	}
> > +
> > +	vlan = ice_eswitch_br_vlan_create(vid, flags, port);
> > +	if (IS_ERR(vlan)) {
> > +		NL_SET_ERR_MSG_MOD(extack, "Failed to create VLAN entry");
> 
> FYI, there's NL_SET_ERR_MSG_FMT_MOD() landed recently (a couple releases
> back), which supports format strings. E.g. you could pass VID, VSI ID,
> flags etc. there to have more meaningful output (right in userspace).

Sure, I guess I can improve log msgs in other patches now.

> 
> > +		return PTR_ERR(vlan);
> > +	}
> > +
> > +	return 0;
> > +}
> 
> [...]
> 
> > +static int
> > +ice_eswitch_br_port_obj_add(struct net_device *netdev, const void *ctx,
> > +			    const struct switchdev_obj *obj,
> > +			    struct netlink_ext_ack *extack)
> > +{
> > +	struct ice_esw_br_port *br_port = ice_eswitch_br_netdev_to_port(netdev);
> > +	struct switchdev_obj_port_vlan *vlan;
> > +	int err;
> > +
> > +	if (!br_port)
> > +		return -EINVAL;
> > +
> > +	switch (obj->id) {
> > +	case SWITCHDEV_OBJ_ID_PORT_VLAN:
> > +		vlan = SWITCHDEV_OBJ_PORT_VLAN(obj);
> > +		err = ice_eswitch_br_port_vlan_add(br_port->bridge,
> > +						   br_port->vsi_idx, vlan->vid,
> > +						   vlan->flags, extack);
> 
> return right here? You have `default` in the switch block, so the
> compiler shouldn't complain if you remove it from the end of the func.

Sure

> 
> > +		break;
> > +	default:
> > +		return -EOPNOTSUPP;
> > +	}
> > +
> > +	return err;
> > +}
> > +
> > +static int
> > +ice_eswitch_br_port_obj_del(struct net_device *netdev, const void *ctx,
> > +			    const struct switchdev_obj *obj)
> > +{
> > +	struct ice_esw_br_port *br_port = ice_eswitch_br_netdev_to_port(netdev);
> > +	struct switchdev_obj_port_vlan *vlan;
> > +
> > +	if (!br_port)
> > +		return -EINVAL;
> > +
> > +	switch (obj->id) {
> > +	case SWITCHDEV_OBJ_ID_PORT_VLAN:
> > +		vlan = SWITCHDEV_OBJ_PORT_VLAN(obj);
> > +		ice_eswitch_br_port_vlan_del(br_port->bridge, br_port->vsi_idx,
> > +					     vlan->vid);
> 
> (same)
> 
> > +		break;
> > +	default:
> > +		return -EOPNOTSUPP;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int
> > +ice_eswitch_br_port_obj_attr_set(struct net_device *netdev, const void *ctx,
> > +				 const struct switchdev_attr *attr,
> > +				 struct netlink_ext_ack *extack)
> > +{
> > +	struct ice_esw_br_port *br_port = ice_eswitch_br_netdev_to_port(netdev);
> > +
> > +	if (!br_port)
> > +		return -EINVAL;
> > +
> > +	switch (attr->id) {
> > +	case SWITCHDEV_ATTR_ID_BRIDGE_VLAN_FILTERING:
> > +		ice_eswitch_br_vlan_filtering_set(br_port->bridge,
> > +						  attr->u.vlan_filtering);
> 
> (and here)
> 
> > +		break;
> > +	default:
> > +		return -EOPNOTSUPP;
> > +	}
> > +
> > +	return 0;
> > +}
> 
> [...]
> 
> > +	br_offloads->switchdev_blk.notifier_call =
> > +		ice_eswitch_br_event_blocking;
> 
> Oh, you have two usages of ->switchdev_blk here, so you can add an
> intermediate variable to avoid line breaking, which would also shorten
> the line below :D
> 
> 	nb = &br_offloads->switchdev_blk;
> 	nb->notifier_call = ice_eswitch_br_event_blocking;
> 	...

Hmmm, I feel like it is more readable right now. It's clear that we're registering
switchdev blocking notifier block (switchdev_blk). Introducing generic variable (nb)
might a bit ambiguous IMO. So if you have nothing against it I'd leave it as it is.

> 
> > +	err = register_switchdev_blocking_notifier(&br_offloads->switchdev_blk);
> > +	if (err) {
> > +		dev_err(dev,
> > +			"Failed to register bridge blocking switchdev notifier\n");
> > +		goto err_reg_switchdev_blk;
> > +	}
> > +
> >  	br_offloads->netdev_nb.notifier_call = ice_eswitch_br_port_event;
> >  	err = register_netdevice_notifier(&br_offloads->netdev_nb);
> 
> (here the same, but no line breaks, so up to you. You could reuse the
>  same variable or leave it as it is)

(same here)

> 
> >  	if (err) {
> 
> [...]
> 
> > diff --git a/drivers/net/ethernet/intel/ice/ice_eswitch_br.h b/drivers/net/ethernet/intel/ice/ice_eswitch_br.h
> > index 73ad81bad655..cf3e2615a62a 100644
> > --- a/drivers/net/ethernet/intel/ice/ice_eswitch_br.h
> > +++ b/drivers/net/ethernet/intel/ice/ice_eswitch_br.h
> > @@ -42,10 +42,16 @@ struct ice_esw_br_port {
> >  	enum ice_esw_br_port_type type;
> >  	struct ice_vsi *vsi;
> >  	u16 vsi_idx;
> > +	struct xarray vlans;
> 
> Hmm, I feel like you can make ::type u16 and then stack it with
> ::vsi_idx, so that you avoid a hole here.
> 
> > +};
> > +
> > +enum {
> > +	ICE_ESWITCH_BR_VLAN_FILTERING = BIT(0),
> >  };
> >
> >  struct ice_esw_br {
> >  	struct ice_esw_br_offloads *br_offloads;
> > +	int flags;
> 
> Unsigned types fit flags better I think?

Yup

> 
> >  	int ifindex;
> 
> (BTW, ifindex is also usually unsigned unless it's not an error)

It is defined as int in net_device, so I don’t know

> 
> >
> >  	struct xarray ports;
> [...]
> 
> Thanks,
> Olek

Alexander Lobakin May 8, 2023, 2:09 p.m. UTC | #3

From: Wojciech Drewek <[e-mail]>
Date: Thu, 27 Apr 2023 12:28:21 +0200

> 
> 
>> -----Original Message-----
>> From: Lobakin, Aleksander <[e-mail]>
>> Sent: piątek, 21 kwietnia 2023 17:25
>> To: Drewek, Wojciech <[e-mail]>
>> Cc: [e-mail]; [e-mail]; Lobakin, Aleksander <[e-mail]>; Ertman, David M
>> <[e-mail]>; [e-mail]; [e-mail]; Chmielewski, Pawel
>> <[e-mail]>; Samudrala, Sridhar <[e-mail]>
>> Subject: Re: [PATCH net-next 08/12] ice: Add VLAN FDB support in switchdev mode

[...]

>>> +		break;
>>> +	default:
>>> +		return -EOPNOTSUPP;
>>> +	}
>>> +
>>> +	return 0;
>>> +}
>>
>> [...]
>>
>>> +	br_offloads->switchdev_blk.notifier_call =
>>> +		ice_eswitch_br_event_blocking;
>>
>> Oh, you have two usages of ->switchdev_blk here, so you can add an
>> intermediate variable to avoid line breaking, which would also shorten
>> the line below :D
>>
>> 	nb = &br_offloads->switchdev_blk;
>> 	nb->notifier_call = ice_eswitch_br_event_blocking;
>> 	...
> 
> Hmmm, I feel like it is more readable right now. It's clear that we're registering
> switchdev blocking notifier block (switchdev_blk). Introducing generic variable (nb)
> might a bit ambiguous IMO. So if you have nothing against it I'd leave it as it is.

Noprob, up to you :)

[...]

>>>  	int ifindex;
>>
>> (BTW, ifindex is also usually unsigned unless it's not an error)
> 
> It is defined as int in net_device, so I don’t know

I know, but back then people don't care much and were usually defining
everything that doesn't need explicit size as `int` :D No interfaces
have a negative index, indexes start from 1, which is reserved for
loopback, so even 0 is not used. Up to you anyway, I just usually don't
use sign when it's not needed. But it's more of a personal.

[...]

Thanks,
Olek

Patch

diff --git a/drivers/net/ethernet/intel/ice/ice_eswitch_br.c b/drivers/net/ethernet/intel/ice/ice_eswitch_br.c
index 49381e4bf62a..56d36e397b12 100644
--- a/drivers/net/ethernet/intel/ice/ice_eswitch_br.c
+++ b/drivers/net/ethernet/intel/ice/ice_eswitch_br.c
@@ -59,13 +59,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, bool vlan, 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 (vlan) {
+		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 = vf_vsi_idx;
 	rule_info->sw_act.flag |= ICE_FLTR_RX;
 	rule_info->sw_act.src = pf_id;
@@ -75,13 +81,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, bool vlan, 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 (vlan) {
+		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;
@@ -105,12 +117,12 @@  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, u16 vsi_idx, int port_type,
-			       const unsigned char *mac)
+			       const unsigned char *mac, bool vlan, 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 = vlan ? 2 : 1;
 	int err;
 
 	rule = kzalloc(sizeof(*rule), GFP_KERNEL);
@@ -125,12 +137,12 @@  ice_eswitch_br_fwd_rule_create(struct ice_hw *hw, u16 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);
+		ice_eswitch_br_egress_rule_setup(list, &rule_info, mac, vlan,
+						 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);
+		ice_eswitch_br_ingress_rule_setup(list, &rule_info, mac, vlan,
+						  vid, hw->pf_id, vsi_idx);
 		break;
 	default:
 		err = -EINVAL;
@@ -159,12 +171,12 @@  ice_eswitch_br_fwd_rule_create(struct ice_hw *hw, u16 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, bool vlan, 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;
+	u16 lkups_cnt = vlan ? 2 : 1;
 	int err;
 
 	rule = kzalloc(sizeof(*rule), GFP_KERNEL);
@@ -183,6 +195,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 (vlan) {
+		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;
@@ -204,7 +222,8 @@  ice_eswitch_br_guard_rule_create(struct ice_hw *hw, u16 vsi_idx,
 
 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)
+			   int port_type, const unsigned char *mac,
+			   bool add_vlan, u16 vid)
 {
 	struct ice_rule_query_data *fwd_rule, *guard_rule;
 	struct ice_esw_br_flow *flow;
@@ -214,7 +233,8 @@  ice_eswitch_br_flow_create(struct device *dev, struct ice_hw *hw, u16 vsi_idx,
 	if (!flow)
 		return ERR_PTR(-ENOMEM);
 
-	fwd_rule = ice_eswitch_br_fwd_rule_create(hw, vsi_idx, port_type, mac);
+	fwd_rule = ice_eswitch_br_fwd_rule_create(hw, vsi_idx, port_type, mac,
+						  add_vlan, vid);
 	if (IS_ERR(fwd_rule)) {
 		err = PTR_ERR(fwd_rule);
 		dev_err(dev, "Failed to create eswitch bridge %sgress forward rule, err: %d\n",
@@ -223,7 +243,8 @@  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);
+	guard_rule = ice_eswitch_br_guard_rule_create(hw, vsi_idx, mac,
+						      add_vlan, vid);
 	if (IS_ERR(guard_rule)) {
 		err = PTR_ERR(guard_rule);
 		dev_err(dev, "Failed to create eswitch bridge %sgress guard rule, err: %d\n",
@@ -276,6 +297,30 @@  ice_eswitch_br_flow_delete(struct ice_pf *pf, struct ice_esw_br_flow *flow)
 	kfree(flow);
 }
 
+static struct ice_esw_br_vlan *
+ice_esw_br_port_vlan_lookup(struct ice_esw_br *bridge, u16 vsi_idx, u16 vid)
+{
+	struct ice_pf *pf = bridge->br_offloads->pf;
+	struct device *dev = ice_pf_to_dev(pf);
+	struct ice_esw_br_port *port;
+	struct ice_esw_br_vlan *vlan;
+
+	port = xa_load(&bridge->ports, vsi_idx);
+	if (!port) {
+		dev_info(dev, "Bridge port lookup failed (vsi=%u)\n", vsi_idx);
+		return ERR_PTR(-EINVAL);
+	}
+
+	vlan = xa_load(&port->vlans, vid);
+	if (!vlan) {
+		dev_info(dev, "Bridge port vlan metadata lookup failed (vsi=%u)\n",
+			 vsi_idx);
+		return ERR_PTR(-EINVAL);
+	}
+
+	return vlan;
+}
+
 static void
 ice_eswitch_br_fdb_entry_delete(struct ice_esw_br *bridge,
 				struct ice_esw_br_fdb_entry *fdb_entry)
@@ -344,10 +389,33 @@  ice_eswitch_br_fdb_entry_create(struct net_device *netdev,
 	struct device *dev = ice_pf_to_dev(pf);
 	struct ice_esw_br_fdb_entry *fdb_entry;
 	struct ice_esw_br_flow *flow;
+	struct ice_esw_br_vlan *vlan;
 	struct ice_hw *hw = &pf->hw;
+	bool add_vlan = false;
 	unsigned long event;
 	int err;
 
+	/* FIXME: untagged filtering is not yet supported
+	 */
+	if (!(bridge->flags & ICE_ESWITCH_BR_VLAN_FILTERING) && vid)
+		return;
+
+	/* 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.
+	 */
+	if ((bridge->flags & ICE_ESWITCH_BR_VLAN_FILTERING) && vid != 1) {
+		vlan = ice_esw_br_port_vlan_lookup(bridge, br_port->vsi_idx,
+						   vid);
+		if (IS_ERR(vlan)) {
+			dev_err(dev, "Failed to find vlan lookup, err: %ld\n",
+				PTR_ERR(vlan));
+			return;
+		}
+
+		add_vlan = true;
+	}
+
 	fdb_entry = ice_eswitch_br_fdb_find(bridge, mac, vid);
 	if (fdb_entry)
 		ice_eswitch_br_fdb_entry_notify_and_cleanup(bridge, fdb_entry);
@@ -359,7 +427,7 @@  ice_eswitch_br_fdb_entry_create(struct net_device *netdev,
 	}
 
 	flow = ice_eswitch_br_flow_create(dev, hw, br_port->vsi_idx,
-					  br_port->type, mac);
+					  br_port->type, mac, add_vlan, vid);
 	if (IS_ERR(flow)) {
 		err = PTR_ERR(flow);
 		goto err_add_flow;
@@ -519,6 +587,214 @@  ice_eswitch_br_switchdev_event(struct notifier_block *nb,
 	return NOTIFY_DONE;
 }
 
+static void ice_eswitch_br_fdb_flush(struct ice_esw_br *bridge)
+{
+	struct ice_esw_br_fdb_entry *entry, *tmp;
+
+	list_for_each_entry_safe(entry, tmp, &bridge->fdb_list, list)
+		ice_eswitch_br_fdb_entry_notify_and_cleanup(bridge, entry);
+}
+
+static void
+ice_eswitch_br_vlan_filtering_set(struct ice_esw_br *bridge, bool enable)
+{
+	bool filtering = bridge->flags & ICE_ESWITCH_BR_VLAN_FILTERING;
+
+	if (filtering == enable)
+		return;
+
+	ice_eswitch_br_fdb_flush(bridge);
+	if (enable)
+		bridge->flags |= ICE_ESWITCH_BR_VLAN_FILTERING;
+	else
+		bridge->flags &= ~ICE_ESWITCH_BR_VLAN_FILTERING;
+}
+
+static void
+ice_eswitch_br_vlan_cleanup(struct ice_esw_br_port *port,
+			    struct ice_esw_br_vlan *vlan)
+{
+	xa_erase(&port->vlans, vlan->vid);
+	kfree(vlan);
+}
+
+static void ice_eswitch_br_port_vlans_flush(struct ice_esw_br_port *port)
+{
+	struct ice_esw_br_vlan *vlan;
+	unsigned long index;
+
+	xa_for_each(&port->vlans, index, vlan)
+		ice_eswitch_br_vlan_cleanup(port, vlan);
+}
+
+static struct ice_esw_br_vlan *
+ice_eswitch_br_vlan_create(u16 vid, u16 flags, struct ice_esw_br_port *port)
+{
+	struct ice_esw_br_vlan *vlan;
+	int err;
+
+	vlan = kzalloc(sizeof(*vlan), GFP_KERNEL);
+	if (!vlan)
+		return ERR_PTR(-ENOMEM);
+
+	vlan->vid = vid;
+	vlan->flags = flags;
+
+	err = xa_insert(&port->vlans, vlan->vid, vlan, GFP_KERNEL);
+	if (err) {
+		kfree(vlan);
+		return ERR_PTR(err);
+	}
+
+	return vlan;
+}
+
+static int
+ice_eswitch_br_port_vlan_add(struct ice_esw_br *bridge, u16 vsi_idx, u16 vid,
+			     u16 flags, struct netlink_ext_ack *extack)
+{
+	struct ice_esw_br_port *port;
+	struct ice_esw_br_vlan *vlan;
+
+	port = xa_load(&bridge->ports, vsi_idx);
+	if (!port)
+		return -EINVAL;
+
+	vlan = xa_load(&port->vlans, vid);
+	if (vlan) {
+		if (vlan->flags == flags)
+			return 0;
+
+		ice_eswitch_br_vlan_cleanup(port, vlan);
+	}
+
+	vlan = ice_eswitch_br_vlan_create(vid, flags, port);
+	if (IS_ERR(vlan)) {
+		NL_SET_ERR_MSG_MOD(extack, "Failed to create VLAN entry");
+		return PTR_ERR(vlan);
+	}
+
+	return 0;
+}
+
+static void
+ice_eswitch_br_port_vlan_del(struct ice_esw_br *bridge, u16 vsi_idx, u16 vid)
+{
+	struct ice_esw_br_port *port;
+	struct ice_esw_br_vlan *vlan;
+
+	port = xa_load(&bridge->ports, vsi_idx);
+	if (!port)
+		return;
+
+	vlan = xa_load(&port->vlans, vid);
+	if (!vlan)
+		return;
+
+	ice_eswitch_br_vlan_cleanup(port, vlan);
+}
+
+static int
+ice_eswitch_br_port_obj_add(struct net_device *netdev, const void *ctx,
+			    const struct switchdev_obj *obj,
+			    struct netlink_ext_ack *extack)
+{
+	struct ice_esw_br_port *br_port = ice_eswitch_br_netdev_to_port(netdev);
+	struct switchdev_obj_port_vlan *vlan;
+	int err;
+
+	if (!br_port)
+		return -EINVAL;
+
+	switch (obj->id) {
+	case SWITCHDEV_OBJ_ID_PORT_VLAN:
+		vlan = SWITCHDEV_OBJ_PORT_VLAN(obj);
+		err = ice_eswitch_br_port_vlan_add(br_port->bridge,
+						   br_port->vsi_idx, vlan->vid,
+						   vlan->flags, extack);
+		break;
+	default:
+		return -EOPNOTSUPP;
+	}
+
+	return err;
+}
+
+static int
+ice_eswitch_br_port_obj_del(struct net_device *netdev, const void *ctx,
+			    const struct switchdev_obj *obj)
+{
+	struct ice_esw_br_port *br_port = ice_eswitch_br_netdev_to_port(netdev);
+	struct switchdev_obj_port_vlan *vlan;
+
+	if (!br_port)
+		return -EINVAL;
+
+	switch (obj->id) {
+	case SWITCHDEV_OBJ_ID_PORT_VLAN:
+		vlan = SWITCHDEV_OBJ_PORT_VLAN(obj);
+		ice_eswitch_br_port_vlan_del(br_port->bridge, br_port->vsi_idx,
+					     vlan->vid);
+		break;
+	default:
+		return -EOPNOTSUPP;
+	}
+
+	return 0;
+}
+
+static int
+ice_eswitch_br_port_obj_attr_set(struct net_device *netdev, const void *ctx,
+				 const struct switchdev_attr *attr,
+				 struct netlink_ext_ack *extack)
+{
+	struct ice_esw_br_port *br_port = ice_eswitch_br_netdev_to_port(netdev);
+
+	if (!br_port)
+		return -EINVAL;
+
+	switch (attr->id) {
+	case SWITCHDEV_ATTR_ID_BRIDGE_VLAN_FILTERING:
+		ice_eswitch_br_vlan_filtering_set(br_port->bridge,
+						  attr->u.vlan_filtering);
+		break;
+	default:
+		return -EOPNOTSUPP;
+	}
+
+	return 0;
+}
+
+static int
+ice_eswitch_br_event_blocking(struct notifier_block *nb, unsigned long event,
+			      void *ptr)
+{
+	struct net_device *dev = switchdev_notifier_info_to_dev(ptr);
+	int err;
+
+	switch (event) {
+	case SWITCHDEV_PORT_OBJ_ADD:
+		err = switchdev_handle_port_obj_add(dev, ptr,
+						    ice_eswitch_br_is_dev_valid,
+						    ice_eswitch_br_port_obj_add);
+		break;
+	case SWITCHDEV_PORT_OBJ_DEL:
+		err = switchdev_handle_port_obj_del(dev, ptr,
+						    ice_eswitch_br_is_dev_valid,
+						    ice_eswitch_br_port_obj_del);
+		break;
+	case SWITCHDEV_PORT_ATTR_SET:
+		err = switchdev_handle_port_attr_set(dev, ptr,
+						     ice_eswitch_br_is_dev_valid,
+						     ice_eswitch_br_port_obj_attr_set);
+		break;
+	default:
+		err = 0;
+	}
+
+	return notifier_from_errno(err);
+}
+
 static void
 ice_eswitch_br_port_deinit(struct ice_esw_br *bridge,
 			   struct ice_esw_br_port *br_port)
@@ -537,6 +813,7 @@  ice_eswitch_br_port_deinit(struct ice_esw_br *bridge,
 		vsi->vf->repr->br_port = NULL;
 
 	xa_erase(&bridge->ports, br_port->vsi_idx);
+	ice_eswitch_br_port_vlans_flush(br_port);
 	kfree(br_port);
 }
 
@@ -549,6 +826,8 @@  ice_eswitch_br_port_init(struct ice_esw_br *bridge)
 	if (!br_port)
 		return ERR_PTR(-ENOMEM);
 
+	xa_init(&br_port->vlans);
+
 	br_port->bridge = bridge;
 
 	return br_port;
@@ -852,6 +1131,7 @@  ice_eswitch_br_offloads_deinit(struct ice_pf *pf)
 		return;
 
 	unregister_netdevice_notifier(&br_offloads->netdev_nb);
+	unregister_switchdev_blocking_notifier(&br_offloads->switchdev_blk);
 	unregister_switchdev_notifier(&br_offloads->switchdev_nb);
 	destroy_workqueue(br_offloads->wq);
 	/* Although notifier block is unregistered just before,
@@ -895,6 +1175,15 @@  ice_eswitch_br_offloads_init(struct ice_pf *pf)
 		goto err_reg_switchdev_nb;
 	}
 
+	br_offloads->switchdev_blk.notifier_call =
+		ice_eswitch_br_event_blocking;
+	err = register_switchdev_blocking_notifier(&br_offloads->switchdev_blk);
+	if (err) {
+		dev_err(dev,
+			"Failed to register bridge blocking switchdev notifier\n");
+		goto err_reg_switchdev_blk;
+	}
+
 	br_offloads->netdev_nb.notifier_call = ice_eswitch_br_port_event;
 	err = register_netdevice_notifier(&br_offloads->netdev_nb);
 	if (err) {
@@ -906,6 +1195,8 @@  ice_eswitch_br_offloads_init(struct ice_pf *pf)
 	return 0;
 
 err_reg_netdev_nb:
+	unregister_switchdev_blocking_notifier(&br_offloads->switchdev_blk);
+err_reg_switchdev_blk:
 	unregister_switchdev_notifier(&br_offloads->switchdev_nb);
 err_reg_switchdev_nb:
 	destroy_workqueue(br_offloads->wq);
diff --git a/drivers/net/ethernet/intel/ice/ice_eswitch_br.h b/drivers/net/ethernet/intel/ice/ice_eswitch_br.h
index 73ad81bad655..cf3e2615a62a 100644
--- a/drivers/net/ethernet/intel/ice/ice_eswitch_br.h
+++ b/drivers/net/ethernet/intel/ice/ice_eswitch_br.h
@@ -42,10 +42,16 @@  struct ice_esw_br_port {
 	enum ice_esw_br_port_type type;
 	struct ice_vsi *vsi;
 	u16 vsi_idx;
+	struct xarray vlans;
+};
+
+enum {
+	ICE_ESWITCH_BR_VLAN_FILTERING = BIT(0),
 };
 
 struct ice_esw_br {
 	struct ice_esw_br_offloads *br_offloads;
+	int flags;
 	int ifindex;
 
 	struct xarray ports;
@@ -57,6 +63,7 @@  struct ice_esw_br_offloads {
 	struct ice_pf *pf;
 	struct ice_esw_br *bridge;
 	struct notifier_block netdev_nb;
+	struct notifier_block switchdev_blk;
 	struct notifier_block switchdev_nb;
 
 	struct workqueue_struct *wq;
@@ -70,6 +77,11 @@  struct ice_esw_br_fdb_work {
 	unsigned long event;
 };
 
+struct ice_esw_br_vlan {
+	u16 vid;
+	u16 flags;
+};
+
 #define ice_nb_to_br_offloads(nb, nb_name) \
 	container_of(nb, \
 		     struct ice_esw_br_offloads, \