Re: [Intel-wired-lan] [PATCH net-next 08/12] ice: Add VLAN FDB support in switchdev mode

[Alexander Lobakin <aleksander.lobakin@intel.com>]

From: Wojciech Drewek <wojciech.drewek@intel.com>
Date: Mon, 17 Apr 2023 11:34:08 +0200

> 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>
> ---
>  .../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
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan