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

Drewek, Wojciech wojciech.drewek at intel.com
Tue May 9 11:25:40 UTC 2023



> -----Original Message-----
> From: Lobakin, Aleksander <aleksander.lobakin at intel.com>
> Sent: piątek, 21 kwietnia 2023 18:35
> To: Drewek, Wojciech <wojciech.drewek at intel.com>
> Cc: intel-wired-lan at lists.osuosl.org; netdev at vger.kernel.org; Ertman, David M <david.m.ertman at intel.com>;
> michal.swiatkowski at linux.intel.com; marcin.szycik at linux.intel.com; Chmielewski, Pawel <pawel.chmielewski at intel.com>;
> Samudrala, Sridhar <sridhar.samudrala at intel.com>
> Subject: Re: [PATCH net-next 09/12] ice: implement bridge port vlan
> 
> From: Wojciech Drewek <wojciech.drewek at intel.com>
> Date: Mon, 17 Apr 2023 11:34:09 +0200
> 
> > From: Michal Swiatkowski <michal.swiatkowski at 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?

sure

> 
> > +	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()?

To me it's not an error, port vlan is already configured

> 
> > +			 "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)

I don't think so, user is trying to configure something that is already done.

> 
> > +	}
> > +
> > +	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)


Again, is this an error really? We just don't support such case.
> 
> > +		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.

Sure

> 
> >  };
> >
> >  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.

Agree

> 
> > +}
> > +
> > +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.

Makes sense

> 
> > +
> > +	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.

Sure

> 
> > +		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


More information about the Intel-wired-lan mailing list