Re: [Intel-wired-lan] [PATCH net-next 09/12] ice: implement bridge port vlan

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

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

> From: Michal Swiatkowski <michal.swiatkowski@linux.intel.com>
> 
> Port VLAN in this case means push and pop VLAN action on specific vid.
> There are a few limitation in hardware:
> - push and pop can't be used separately
> - if port VLAN is used there can't be any trunk VLANs, because pop
>   action is done on all trafic received by VSI in port VLAN mode
> - port VLAN mode on uplink port isn't supported

[...]

> @@ -610,11 +612,26 @@ ice_eswitch_br_vlan_filtering_set(struct ice_esw_br *bridge, bool enable)
>  		bridge->flags &= ~ICE_ESWITCH_BR_VLAN_FILTERING;
>  }
>  
> +static void
> +ice_eswitch_br_clear_pvid(struct ice_esw_br_port *port)
> +{
> +	struct ice_vsi_vlan_ops *vlan_ops =
> +		ice_get_compat_vsi_vlan_ops(port->vsi);
> +

Deref in a separate line to avoid breaking?

> +	vlan_ops->clear_port_vlan(port->vsi);
> +
> +	ice_vf_vsi_disable_port_vlan(port->vsi);
> +
> +	port->pvid = 0;
> +}
> +
>  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);
> +	if (port->pvid == vlan->vid)
> +		ice_eswitch_br_clear_pvid(port);
>  	kfree(vlan);
>  }
>  
> @@ -627,9 +644,50 @@ static void ice_eswitch_br_port_vlans_flush(struct ice_esw_br_port *port)
>  		ice_eswitch_br_vlan_cleanup(port, vlan);
>  }
>  
> +static int
> +ice_eswitch_br_set_pvid(struct ice_esw_br_port *port,
> +			struct ice_esw_br_vlan *vlan)
> +{
> +	struct ice_vlan port_vlan = ICE_VLAN(ETH_P_8021Q, vlan->vid, 0);
> +	struct device *dev = ice_pf_to_dev(port->vsi->back);
> +	struct ice_vsi_vlan_ops *vlan_ops;
> +	int err;
> +
> +	if (port->pvid == vlan->vid || vlan->vid == 1)
> +		return 0;
> +
> +	/* Setting port vlan on uplink isn't supported by hw */
> +	if (port->type == ICE_ESWITCH_BR_UPLINK_PORT)
> +		return -EOPNOTSUPP;
> +
> +	if (port->pvid) {
> +		dev_info(dev,

dev_err()?

> +			 "Port VLAN (vsi=%u, vid=%u) already exists on the port, remove it before adding new one\n",
> +			 port->vsi_idx, port->pvid);
> +		return -EEXIST;

Hmm, isn't -EBUSY more common for such cases?

(below as well)

> +	}
> +
> +	ice_vf_vsi_enable_port_vlan(port->vsi);

[...]

> @@ -639,14 +697,29 @@ ice_eswitch_br_vlan_create(u16 vid, u16 flags, struct ice_esw_br_port *port)
>  
>  	vlan->vid = vid;
>  	vlan->flags = flags;
> +	if ((flags & BRIDGE_VLAN_INFO_PVID) &&
> +	    (flags & BRIDGE_VLAN_INFO_UNTAGGED)) {
> +		err = ice_eswitch_br_set_pvid(port, vlan);
> +		if (err)
> +			goto err_set_pvid;
> +	} else if ((flags & BRIDGE_VLAN_INFO_PVID) ||
> +		   (flags & BRIDGE_VLAN_INFO_UNTAGGED)) {
> +		dev_info(dev, "VLAN push and pop are supported only simultaneously\n");

(same for dev_err(), as well as below)

> +		return ERR_PTR(-EOPNOTSUPP);
> +	}

[...]

> diff --git a/drivers/net/ethernet/intel/ice/ice_eswitch_br.h b/drivers/net/ethernet/intel/ice/ice_eswitch_br.h
> index cf3e2615a62a..b6eef068ea81 100644
> --- a/drivers/net/ethernet/intel/ice/ice_eswitch_br.h
> +++ b/drivers/net/ethernet/intel/ice/ice_eswitch_br.h
> @@ -43,6 +43,7 @@ struct ice_esw_br_port {
>  	struct ice_vsi *vsi;
>  	u16 vsi_idx;
>  	struct xarray vlans;
> +	u16 pvid;

Oh, or you can just stack ::vsi_idx with ::pvid here to avoid spawning
holes.

>  };
>  
>  enum {
> diff --git a/drivers/net/ethernet/intel/ice/ice_vf_vsi_vlan_ops.c b/drivers/net/ethernet/intel/ice/ice_vf_vsi_vlan_ops.c
> index b1ffb81893d4..447b4e6ef7e4 100644
> --- a/drivers/net/ethernet/intel/ice/ice_vf_vsi_vlan_ops.c
> +++ b/drivers/net/ethernet/intel/ice/ice_vf_vsi_vlan_ops.c
> @@ -21,6 +21,108 @@ noop_vlan(struct ice_vsi __always_unused *vsi)
>  	return 0;
>  }
>  
> +static void ice_port_vlan_on(struct ice_vsi *vsi)
> +{
> +	struct ice_vsi_vlan_ops *vlan_ops;
> +	struct ice_pf *pf = vsi->back;
> +
> +	if (ice_is_dvm_ena(&pf->hw)) {
> +		vlan_ops = &vsi->outer_vlan_ops;
> +
> +		/* setup outer VLAN ops */
> +		vlan_ops->set_port_vlan = ice_vsi_set_outer_port_vlan;
> +		vlan_ops->clear_port_vlan = ice_vsi_clear_outer_port_vlan;
> +		vlan_ops->clear_port_vlan = ice_vsi_clear_outer_port_vlan;
> +		vlan_ops->ena_rx_filtering =
> +			ice_vsi_ena_rx_vlan_filtering;
> +
> +		/* setup inner VLAN ops */
> +		vlan_ops = &vsi->inner_vlan_ops;
> +		vlan_ops->add_vlan = noop_vlan_arg;
> +		vlan_ops->del_vlan = noop_vlan_arg;
> +		vlan_ops->ena_stripping = ice_vsi_ena_inner_stripping;
> +		vlan_ops->dis_stripping = ice_vsi_dis_inner_stripping;
> +		vlan_ops->ena_insertion = ice_vsi_ena_inner_insertion;
> +		vlan_ops->dis_insertion = ice_vsi_dis_inner_insertion;
> +	} else {
> +		vlan_ops = &vsi->inner_vlan_ops;
> +
> +		vlan_ops->set_port_vlan = ice_vsi_set_inner_port_vlan;
> +		vlan_ops->clear_port_vlan = ice_vsi_clear_inner_port_vlan;
> +		vlan_ops->clear_port_vlan = ice_vsi_clear_inner_port_vlan;
> +		vlan_ops->ena_rx_filtering =
> +			ice_vsi_ena_rx_vlan_filtering;
> +	}

->ena_rx_filtering is filled with just one possible value, so it could
be done outside ifs.

> +}
> +
> +static void ice_port_vlan_off(struct ice_vsi *vsi)
> +{
> +	struct ice_vsi_vlan_ops *vlan_ops;
> +	struct ice_pf *pf = vsi->back;
> +
> +	if (ice_is_dvm_ena(&pf->hw)) {
> +		/* setup inner VLAN ops */
> +		vlan_ops = &vsi->inner_vlan_ops;
> +
> +		vlan_ops->ena_stripping = ice_vsi_ena_inner_stripping;
> +		vlan_ops->dis_stripping = ice_vsi_dis_inner_stripping;
> +		vlan_ops->ena_insertion = ice_vsi_ena_inner_insertion;
> +		vlan_ops->dis_insertion = ice_vsi_dis_inner_insertion;
> +
> +		vlan_ops = &vsi->outer_vlan_ops;
> +
> +		vlan_ops->del_vlan = ice_vsi_del_vlan;
> +		vlan_ops->ena_stripping = ice_vsi_ena_outer_stripping;
> +		vlan_ops->dis_stripping = ice_vsi_dis_outer_stripping;
> +		vlan_ops->ena_insertion = ice_vsi_ena_outer_insertion;
> +		vlan_ops->dis_insertion = ice_vsi_dis_outer_insertion;
> +	} else {
> +		vlan_ops = &vsi->inner_vlan_ops;
> +
> +		vlan_ops->del_vlan = ice_vsi_del_vlan;
> +		vlan_ops->ena_stripping = ice_vsi_ena_inner_stripping;
> +		vlan_ops->dis_stripping = ice_vsi_dis_inner_stripping;
> +		vlan_ops->ena_insertion = ice_vsi_ena_inner_insertion;
> +		vlan_ops->dis_insertion = ice_vsi_dis_inner_insertion;
> +	}

The whole ->inner_vlan_ops is filled with the same values, the only
difference is ->del_vlan, which can be left in `else`, the rest can be
set up unconditionally.

> +
> +	if (!test_bit(ICE_FLAG_VF_VLAN_PRUNING, pf->flags))
> +		vlan_ops->ena_rx_filtering = noop_vlan;
> +	else
> +		vlan_ops->ena_rx_filtering =
> +			ice_vsi_ena_rx_vlan_filtering;
> +}
> +
> +/**
> + * ice_vf_vsi_enable_port_vlan - Set VSI VLAN ops to support port VLAN
> + * @vsi: VF's VSI being configured
> + *
> + * The function won't create port VLAN, it only allows to create port VLAN
> + * using VLAN ops on the VF VSI.
> + */
> +void ice_vf_vsi_enable_port_vlan(struct ice_vsi *vsi)
> +{
> +	if (WARN_ON(!vsi->vf))

I'd use WARN_ON_ONCE(). Otherwise, it may be possible to flood kernel
log buffer (-> CPU) from the userspace.

> +		return;
> +
> +	ice_port_vlan_on(vsi);
> +}
> +
> +/**
> + * ice_vf_vsi_disable_port_vlan - Clear VSI support for creating port VLAN
> + * @vsi: VF's VSI being configured
> + *
> + * The function should be called after removing port VLAN on VSI
> + * (using VLAN ops)
> + */
> +void ice_vf_vsi_disable_port_vlan(struct ice_vsi *vsi)
> +{
> +	if (WARN_ON(!vsi->vf))

(same)

> +		return;
> +
> +	ice_port_vlan_off(vsi);
> +}

[...]

> +	info->valid_sections = cpu_to_le16(ICE_AQ_VSI_PROP_VLAN_VALID |
> +					   ICE_AQ_VSI_PROP_SW_VALID);
> +
> +	ret = ice_update_vsi(hw, vsi->idx, ctxt, NULL);
> +	if (ret)
> +		dev_info(ice_hw_to_dev(hw), "update VSI for port VLAN failed, err %d aq_err %s\n",

(dev_err())
(+ %pe)

> +			 ret, ice_aq_str(hw->adminq.sq_last_status));
> +
> +	kfree(ctxt);
> +	return ret;
> +}
[...]

Thanks,
Olek
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan