Re: [PATCH net-next v2 02/12] net: dsa: vsc73xx: Add vlan filtering

[Vladimir Oltean <olteanv@gmail.com>]

On Wed, Jun 19, 2024 at 10:52:08PM +0200, Pawel Dembicki wrote:
> This patch implements VLAN filtering for the vsc73xx driver.
> 
> After starting VLAN filtering, the switch is reconfigured from QinQ to
> a simple VLAN aware mode. This is required because VSC73XX chips do not
> support inner VLAN tag filtering.
> 
> Signed-off-by: Pawel Dembicki <paweldembicki@gmail.com>
> ---
> v2:
>   - removed not needed INIT_LIST_HEAD
>   - fix vsc73xx_vlan removing procedure
>   - fix code spell
>   - handle return codes from 'vsc73xx_vlan_commit*' functions
>   - move 'vsc73xx_vlan_commit*' call from port_setup to port_enable to
>     avoid unused port configuration
> v1:
>   - refactored pvid, untagged and vlan filter configuration
>   - fix typo
>   - simplification

Thanks for the changes. This is much simpler for my mind to process.
Hopefully you won't mind a few nitpicks below.

> +static int vsc73xx_set_vlan_conf(struct vsc73xx *vsc, int port,
> +				 enum vsc73xx_port_vlan_conf port_vlan_conf)
> +{
> +	u32 val = 0;
> +	int ret;
> +
> +	if (port_vlan_conf == VSC73XX_VLAN_IGNORE)
> +		val = VSC73XX_CAT_VLAN_MISC_VLAN_TCI_IGNORE_ENA |
> +		      VSC73XX_CAT_VLAN_MISC_VLAN_KEEP_TAG_ENA;
> +
> +	ret = vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port,
> +				  VSC73XX_CAT_VLAN_MISC,
> +				  VSC73XX_CAT_VLAN_MISC_VLAN_TCI_IGNORE_ENA |
> +				  VSC73XX_CAT_VLAN_MISC_VLAN_KEEP_TAG_ENA, val);
> +	if (ret)
> +		return ret;
> +
> +	val = (port_vlan_conf == VSC73XX_VLAN_FILTER) ?
> +	      VSC73XX_TXUPDCFG_TX_INSERT_TAG : 0;
> +
> +	return vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port,
> +				   VSC73XX_TXUPDCFG,
> +				   VSC73XX_TXUPDCFG_TX_INSERT_TAG, val);
> +}
> +
> +static int vsc73xx_vlan_commit_conf(struct vsc73xx *vsc, int port)

Could you please add a kernel-doc stating what is the logical input and
output of this routine? Something like:

/**
 * vsc73xx_vlan_commit_conf - Update VLAN configuration of a port
 * @vsc: Switch private data structure
 * @port: Port index on which to operate
 *
 * Update the VLAN behavior of a port to make sure that when it is under
 * a VLAN filtering bridge, the port is either filtering with tag
 * preservation, or filtering with all VLANs egress-untagged. Otherwise,
 * the port ignores VLAN tags from packets and applies the port-based
 * VID.
 *
 * Must be called when changes are made to:
 * - the bridge VLAN filtering state of the port
 * - the number or attributes of VLANs from the bridge VLAN table,
 *   while the port is currently VLAN-aware
 */

> +{
> +	enum vsc73xx_port_vlan_conf port_vlan_conf = VSC73XX_VLAN_IGNORE;
> +	struct dsa_port *dp = dsa_to_port(vsc->ds, port);
> +
> +	if (port == CPU_PORT) {
> +		port_vlan_conf = VSC73XX_VLAN_FILTER;
> +	} else if (dsa_port_is_vlan_filtering(dp)) {
> +		struct vsc73xx_vlan_summary summary;
> +
> +		port_vlan_conf = VSC73XX_VLAN_FILTER;
> +
> +		vsc73xx_bridge_vlan_summary(vsc, port, &summary, VLAN_N_VID);
> +		if (summary.num_tagged == 0)
> +			port_vlan_conf = VSC73XX_VLAN_FILTER_UNTAG_ALL;
> +	}
> +
> +	return vsc73xx_set_vlan_conf(vsc, port, port_vlan_conf);
> +}
> +
> +static int
> +vsc73xx_vlan_change_untagged(struct vsc73xx *vsc, int port, u16 vid, bool set)
> +{
> +	u32 val = 0;
> +
> +	if (set)
> +		val = VSC73XX_TXUPDCFG_TX_UNTAGGED_VID_ENA |
> +		      ((vid << VSC73XX_TXUPDCFG_TX_UNTAGGED_VID_SHIFT) &
> +		       VSC73XX_TXUPDCFG_TX_UNTAGGED_VID);
> +
> +	return vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port,
> +				   VSC73XX_TXUPDCFG,
> +				   VSC73XX_TXUPDCFG_TX_UNTAGGED_VID_ENA |
> +				   VSC73XX_TXUPDCFG_TX_UNTAGGED_VID, val);
> +}
> +
> +static int vsc73xx_vlan_commit_untagged(struct vsc73xx *vsc, int port)

/**
 * vsc73xx_vlan_commit_untagged - Update native VLAN of a port
 * @vsc: Switch private data structure
 * @port: Port index on which to operate
 *
 * Update the native VLAN of a port (the one VLAN which is transmitted
 * as egress-tagged on a trunk port).
 *
 * TODO: I need to figure out more about the implementation :)
 */

> +{
> +	struct vsc73xx_portinfo *portinfo = &vsc->portinfo[port];
> +	bool valid = portinfo->untagged_tag_8021q_configured;
> +	struct dsa_port *dp = dsa_to_port(vsc->ds, port);
> +	u16 vid = portinfo->untagged_tag_8021q;

Question: when tag_8021q is active and the port is VSC73XX_VLAN_IGNORE,
does the untagged_tag_8021q VID matter at all?

> +
> +	if (dsa_port_is_vlan_filtering(dp)) {

If not, wouldn't it be better to just return early and do nothing
if !dsa_port_is_vlan_filtering(dp)?

> +		struct vsc73xx_vlan_summary summary;
> +
> +		vsc73xx_bridge_vlan_summary(vsc, port, &summary, VLAN_N_VID);
> +
> +		if (summary.num_untagged > 1)
> +			/* Port must untag all vlans in that case.
> +			 * No need to commit untagged config change.
> +			 */
> +			return 0;
> +
> +		valid = (summary.num_untagged == 1);
> +		if (valid)
> +			vid = vsc73xx_find_first_vlan_untagged(vsc, port);
> +	}
> +
> +	return vsc73xx_vlan_change_untagged(vsc, port, vid, valid);
> +}
> +
> +static int
> +vsc73xx_vlan_change_pvid(struct vsc73xx *vsc, int port, u16 vid, bool set)
> +{
> +	u32 val = 0;
> +	int ret;
> +
> +	val = set ? 0 : VSC73XX_CAT_DROP_UNTAGGED_ENA;
> +
> +	ret = vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port,
> +				  VSC73XX_CAT_DROP,
> +				  VSC73XX_CAT_DROP_UNTAGGED_ENA, val);
> +	if (!set || ret)
> +		return ret;
> +
> +	return vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port,
> +				   VSC73XX_CAT_PORT_VLAN,
> +				   VSC73XX_CAT_PORT_VLAN_VLAN_VID,
> +				   vid & VSC73XX_CAT_PORT_VLAN_VLAN_VID);
> +}
> +

/**
 * vsc73xx_vlan_commit_pvid - Update port-based default VLAN of a port
 * @vsc: Switch private data structure
 * @port: Port index on which to operate
 *
 * Update the PVID of a port so that it follows either the bridge PVID
 * configuration, when the bridge is currently VLAN-aware, or the PVID
 * from tag_8021q, when the port is standalone or under a VLAN-unaware
 * bridge. A port with no PVID drops all untagged and VID 0 tagged
 * traffic.
 *
 * Must be called when changes are made to:
 * - the bridge VLAN filtering state of the port
 * - the number or attributes of VLANs from the bridge VLAN table,
 *   while the port is currently VLAN-aware
 */

> +static int vsc73xx_vlan_commit_pvid(struct vsc73xx *vsc, int port)
> +{
> +	struct vsc73xx_portinfo *portinfo = &vsc->portinfo[port];
> +	bool valid = portinfo->pvid_tag_8021q_configured;
> +	struct dsa_port *dp = dsa_to_port(vsc->ds, port);
> +	u16 vid = portinfo->pvid_tag_8021q;
> +
> +	if (dsa_port_is_vlan_filtering(dp)) {
> +		vid = portinfo->pvid_vlan_filtering;
> +		valid = portinfo->pvid_vlan_filtering_configured;
> +	}
> +
> +	return vsc73xx_vlan_change_pvid(vsc, port, vid, valid);
> +}
> +
>  static int vsc73xx_port_enable(struct dsa_switch *ds, int port,
>  			       struct phy_device *phy)
>  {
>  	struct vsc73xx *vsc = ds->priv;
> +	int ret;
>  
>  	dev_info(vsc->dev, "enable port %d\n", port);
>  	vsc73xx_init_port(vsc, port);
>  
> -	return 0;
> +	ret = vsc73xx_vlan_commit_untagged(vsc, port);
> +	if (ret)
> +		return ret;
> +
> +	ret = vsc73xx_vlan_commit_pvid(vsc, port);
> +	if (ret)
> +		return ret;
> +
> +	return vsc73xx_vlan_commit_conf(vsc, port);
>  }
>  
>  static void vsc73xx_port_disable(struct dsa_switch *ds, int port)
> @@ -1032,6 +1361,184 @@ static void vsc73xx_phylink_get_caps(struct dsa_switch *dsa, int port,
>  	config->mac_capabilities = MAC_SYM_PAUSE | MAC_10 | MAC_100 | MAC_1000;
>  }
>  
> +static int
> +vsc73xx_port_vlan_filtering(struct dsa_switch *ds, int port,
> +			    bool vlan_filtering, struct netlink_ext_ack *extack)
> +{
> +	struct vsc73xx *vsc = ds->priv;
> +	int ret;
> +
> +	/* The commit to hardware processed below is required because vsc73xx
> +	 * is using tag_8021q. When vlan_filtering is disabled, tag_8021q uses
> +	 * pvid/untagged vlans for port recognition. The values configured for
> +	 * vlans and pvid/untagged states are stored in portinfo structure.
> +	 * When vlan_filtering is enabled, we need to restore pvid/untagged from
> +	 * portinfo structure. Analogous routine is processed when
> +	 * vlan_filtering is disabled, but values used for tag_8021q are
> +	 * restored.
> +	 */
> +	ret = vsc73xx_vlan_commit_untagged(vsc, port);
> +	if (ret)
> +		return ret;
> +
> +	ret = vsc73xx_vlan_commit_pvid(vsc, port);
> +	if (ret)
> +		return ret;
> +
> +	return vsc73xx_vlan_commit_conf(vsc, port);

Could you please group these 3 calls together in a separate subroutine,
like vsc73xx_port_update_vlan(), and just call that? The pattern is
repeated in a number of places.

> +}
> +
> +static int vsc73xx_port_vlan_add(struct dsa_switch *ds, int port,
> +				 const struct switchdev_obj_port_vlan *vlan,
> +				 struct netlink_ext_ack *extack)
> +{
> +	bool untagged = vlan->flags & BRIDGE_VLAN_INFO_UNTAGGED;
> +	bool pvid = vlan->flags & BRIDGE_VLAN_INFO_PVID;
> +	struct dsa_port *dp = dsa_to_port(ds, port);
> +	struct vsc73xx_bridge_vlan *vsc73xx_vlan;
> +	struct vsc73xx_vlan_summary summary;
> +	struct vsc73xx_portinfo *portinfo;
> +	struct vsc73xx *vsc = ds->priv;
> +	bool commit_to_hardware;
> +	int ret = 0;
> +
> +	/* Be sure to deny alterations to the configuration done by tag_8021q.
> +	 */
> +	if (vid_is_dsa_8021q(vlan->vid)) {
> +		NL_SET_ERR_MSG_MOD(extack,
> +				   "Range 3072-4095 reserved for dsa_8021q operation");
> +		return -EBUSY;
> +	}
> +
> +	/* The processed vlan->vid is excluded from the search because the VLAN
> +	 * can be re-added with a different set of flags, so it's easiest to
> +	 * ignore its old flags from the VLAN database software copy.
> +	 */
> +	vsc73xx_bridge_vlan_summary(vsc, port, &summary, vlan->vid);
> +
> +	/* VSC73XX allow only three untagged states: none, one or all */

allows

> +	if ((untagged && summary.num_tagged > 0 && summary.num_untagged > 0) ||
> +	    (!untagged && summary.num_untagged > 1)) {
> +		NL_SET_ERR_MSG_MOD(extack,
> +				   "Port can have only none, one or all untagged vlan");
> +		return -EBUSY;
> +	}
> +
> +	vsc73xx_vlan = vsc73xx_bridge_vlan_find(vsc, vlan->vid);
> +
> +	if (!vsc73xx_vlan) {
> +		vsc73xx_vlan = kzalloc(sizeof(*vsc73xx_vlan), GFP_KERNEL);
> +		if (!vsc73xx_vlan)
> +			return -ENOMEM;
> +
> +		vsc73xx_vlan->vid = vlan->vid;
> +		vsc73xx_vlan->portmask = 0;
> +		vsc73xx_vlan->untagged = 0;

kzalloc gives you 0-initialized memory, so "portmask" and "untagged" are
already 0.

> +
> +		list_add_tail(&vsc73xx_vlan->list, &vsc->vlans);
> +	}
> +
> +	/* CPU port must be always tagged because port separation is based on
> +	 * tag_8021q.

I would say "source port identification" rather than separation.

> +	 */
> +	if (port == CPU_PORT)
> +		goto update_vlan_table;
> +
> +	vsc73xx_vlan->portmask |= BIT(port);
> +
> +	if (untagged)
> +		vsc73xx_vlan->untagged |= BIT(port);
> +	else
> +		vsc73xx_vlan->untagged &= ~BIT(port);
> +
> +	portinfo = &vsc->portinfo[port];
> +
> +	if (pvid) {
> +		portinfo->pvid_vlan_filtering_configured = true;
> +		portinfo->pvid_vlan_filtering = vlan->vid;
> +	} else if (portinfo->pvid_vlan_filtering_configured &&
> +		   portinfo->pvid_vlan_filtering == vlan->vid) {
> +		portinfo->pvid_vlan_filtering_configured = false;
> +	}
> +
> +	commit_to_hardware = !vsc73xx_tag_8021q_active(dp);
> +	if (commit_to_hardware) {
> +		ret = vsc73xx_vlan_commit_untagged(vsc, port);
> +		if (ret)
> +			goto err;
> +
> +		ret = vsc73xx_vlan_commit_pvid(vsc, port);
> +		if (ret)
> +			goto err;
> +
> +		ret =  vsc73xx_vlan_commit_conf(vsc, port);
> +		if (ret)
> +			goto err;
> +	}
> +
> +update_vlan_table:
> +	ret = vsc73xx_update_vlan_table(vsc, port, vlan->vid, true);
> +	if (!ret)
> +		return 0;
> +err:
> +	vsc73xx_bridge_vlan_remove_port(vsc73xx_vlan, port);
> +	return ret;
> +}
> +
> +static int vsc73xx_port_vlan_del(struct dsa_switch *ds, int port,
> +				 const struct switchdev_obj_port_vlan *vlan)
> +{
> +	struct vsc73xx_bridge_vlan *vsc73xx_vlan;
> +	struct vsc73xx_portinfo *portinfo;
> +	struct vsc73xx *vsc = ds->priv;
> +	bool commit_to_hardware;
> +	int ret;
> +
> +	ret = vsc73xx_update_vlan_table(vsc, port, vlan->vid, false);
> +	if (ret)
> +		return ret;
> +
> +	portinfo = &vsc->portinfo[port];
> +
> +	if (portinfo->pvid_vlan_filtering_configured &&
> +	    portinfo->pvid_vlan_filtering == vlan->vid)
> +		portinfo->pvid_vlan_filtering_configured = false;
> +
> +	vsc73xx_vlan = vsc73xx_bridge_vlan_find(vsc, vlan->vid);
> +
> +	if (vsc73xx_vlan)
> +		vsc73xx_bridge_vlan_remove_port(vsc73xx_vlan, port);
> +
> +	commit_to_hardware = !vsc73xx_tag_8021q_active(dsa_to_port(ds, port));
> +	if (commit_to_hardware) {
> +		ret = vsc73xx_vlan_commit_untagged(vsc, port);
> +		if (ret)
> +			return ret;
> +
> +		ret = vsc73xx_vlan_commit_pvid(vsc, port);
> +		if (ret)
> +			return ret;
> +
> +		return vsc73xx_vlan_commit_conf(vsc, port);
> +	}
> +
> +	return 0;
> +}
> +
> +static int vsc73xx_port_setup(struct dsa_switch *ds, int port)
> +{
> +	struct vsc73xx_portinfo *portinfo;
> +	struct vsc73xx *vsc = ds->priv;
> +
> +	portinfo = &vsc->portinfo[port];
> +
> +	portinfo->pvid_vlan_filtering_configured = false;
> +	portinfo->pvid_tag_8021q_configured = false;
> +	portinfo->untagged_tag_8021q_configured = false;

Initialization with 0, false etc isn't really needed. Not worth adding a
DSA callback for no useful action.

> +
> +	return 0;
> +}
> +
>  static void vsc73xx_refresh_fwd_map(struct dsa_switch *ds, int port, u8 state)
>  {
>  	struct dsa_port *other_dp, *dp = dsa_to_port(ds, port);
> @@ -1126,11 +1633,15 @@ static const struct dsa_switch_ops vsc73xx_ds_ops = {
>  	.get_strings = vsc73xx_get_strings,
>  	.get_ethtool_stats = vsc73xx_get_ethtool_stats,
>  	.get_sset_count = vsc73xx_get_sset_count,
> +	.port_setup = vsc73xx_port_setup,
>  	.port_enable = vsc73xx_port_enable,
>  	.port_disable = vsc73xx_port_disable,
>  	.port_change_mtu = vsc73xx_change_mtu,
>  	.port_max_mtu = vsc73xx_get_max_mtu,
>  	.port_stp_state_set = vsc73xx_port_stp_state_set,
> +	.port_vlan_filtering = vsc73xx_port_vlan_filtering,
> +	.port_vlan_add = vsc73xx_port_vlan_add,
> +	.port_vlan_del = vsc73xx_port_vlan_del,
>  	.phylink_get_caps = vsc73xx_phylink_get_caps,
>  };
>  
> diff --git a/drivers/net/dsa/vitesse-vsc73xx.h b/drivers/net/dsa/vitesse-vsc73xx.h
> index 2997f7e108b1..3c7586868e1b 100644
> --- a/drivers/net/dsa/vitesse-vsc73xx.h
> +++ b/drivers/net/dsa/vitesse-vsc73xx.h
> @@ -14,6 +14,27 @@
>   */
>  #define VSC73XX_MAX_NUM_PORTS	8
>  
> +/**
> + * struct vsc73xx_portinfo - port data structure: contains storage data
> + * @pvid_vlan_filtering_configured: imforms if port have configured pvid in vlan
> + *	filtering mode
> + * @pvid_vlan_filtering: pvid vlan number used in vlan filtering mode
> + * @pvid_tag_8021q_configured: imforms if port have configured pvid in tag_8021q
> + *	mode
> + * @pvid_tag_8021q: pvid vlan number used in tag_8021q mode
> + * @untagged_tag_8021q_configured: imforms if port have configured untagged vlan
> + *	in tag_8021q mode
> + * @untagged_tag_8021q: untagged vlan number used in tag_8021q mode
> + */

s/imforms/informs/
s/port have/port has/

> +struct vsc73xx_portinfo {
> +	bool		pvid_vlan_filtering_configured;
> +	u16		pvid_vlan_filtering;
> +	bool		pvid_tag_8021q_configured;
> +	u16		pvid_tag_8021q;
> +	bool		untagged_tag_8021q_configured;
> +	u16		untagged_tag_8021q;
> +};
> +

I think it may be more economical to put all u16 elements first, then
all bool elements. Pahole should be able to tell.

>  /**
>   * struct vsc73xx - VSC73xx state container: main data structure
>   * @dev: The device pointer
> @@ -25,6 +46,10 @@
>   * @addr: MAC address used in flow control frames
>   * @ops: Structure with hardware-dependent operations
>   * @priv: Pointer to the configuration interface structure
> + * @portinfo: Storage table portinfo structructures
> + * @vlans: List of configured vlans. Contains port mask and untagged status of
> + *	every vlan configured in port vlan operation. It doesn't cover tag_8021q
> + *	vlans.
>   */
>  struct vsc73xx {
>  	struct device			*dev;
> @@ -35,6 +60,8 @@ struct vsc73xx {
>  	u8				addr[ETH_ALEN];
>  	const struct vsc73xx_ops	*ops;
>  	void				*priv;
> +	struct vsc73xx_portinfo		portinfo[VSC73XX_MAX_NUM_PORTS];
> +	struct list_head		vlans;
>  };
>  
>  /**
> @@ -49,6 +76,21 @@ struct vsc73xx_ops {
>  		     u32 val);
>  };
>  
> +/**
> + * struct vsc73xx_bridge_vlan - VSC73xx driver structure which keeps vlan
> + *	database copy
> + * @vid: VLAN number
> + * @portmask: each bit represents one port
> + * @untagged: each bit represents one port configured with @vid untagged
> + * @list: list structure
> + */
> +struct vsc73xx_bridge_vlan {
> +	u16 vid;
> +	u8 portmask;
> +	u8 untagged;
> +	struct list_head list;
> +};
> +
>  int vsc73xx_is_addr_valid(u8 block, u8 subblock);
>  int vsc73xx_probe(struct vsc73xx *vsc);
>  void vsc73xx_remove(struct vsc73xx *vsc);
> -- 
> 2.34.1
>