Re: [PATCH v4 net-next 10/10] net: dsa: microchip: add support for vlan operations

[Vladimir Oltean <olteanv@gmail.com>]

On Thu, Oct 07, 2021 at 08:42:00PM +0530, Prasanna Vengateshan wrote:
>  static int lan937x_read_table(struct ksz_device *dev, u32 *table)
>  {
>  	int ret;
> @@ -193,6 +292,102 @@ static void lan937x_port_stp_state_set(struct dsa_switch *ds, int port,
>  		ksz_update_port_member(dev, port);
>  }
>  
> +static int lan937x_port_vlan_filtering(struct dsa_switch *ds, int port,
> +				       bool flag,
> +				       struct netlink_ext_ack *extack)
> +{
> +	struct ksz_device *dev = ds->priv;
> +	int ret;
> +
> +	ret = lan937x_cfg(dev, REG_SW_LUE_CTRL_0, SW_VLAN_ENABLE,
> +			  flag);

If you're going to resend anyway, can you please check the entire
submission for this pattern, where you can eliminate the intermediary
"ret" variable and just return the function call directly?

	return lan937x_cfg(...)

Do you have an explanation for what SW_VLAN_ENABLE does exactly?

> +
> +	return ret;
> +}
> +
> +static int lan937x_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;
> +	struct ksz_device *dev = ds->priv;
> +	struct lan937x_vlan vlan_entry;
> +	int ret;
> +
> +	ret = lan937x_get_vlan_table(dev, vlan->vid, &vlan_entry);
> +	if (ret < 0) {
> +		NL_SET_ERR_MSG_MOD(extack, "Failed to get vlan table");
> +		return ret;
> +	}
> +
> +	vlan_entry.fid = lan937x_get_fid(vlan->vid);
> +	vlan_entry.valid = true;
> +
> +	/* set/clear switch port when updating vlan table registers */
> +	if (untagged)
> +		vlan_entry.untag_prtmap |= BIT(port);
> +	else
> +		vlan_entry.untag_prtmap &= ~BIT(port);
> +
> +	vlan_entry.fwd_map |= BIT(port);
> +
> +	ret = lan937x_set_vlan_table(dev, vlan->vid, &vlan_entry);
> +	if (ret < 0) {
> +		NL_SET_ERR_MSG_MOD(extack, "Failed to set vlan table");
> +		return ret;
> +	}
> +
> +	/* change PVID */
> +	if (vlan->flags & BRIDGE_VLAN_INFO_PVID) {
> +		ret = lan937x_pwrite16(dev, port, REG_PORT_DEFAULT_VID,
> +				       vlan->vid);
> +		if (ret < 0) {
> +			NL_SET_ERR_MSG_MOD(extack, "Failed to set pvid");
> +			return ret;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +static int lan937x_port_vlan_del(struct dsa_switch *ds, int port,
> +				 const struct switchdev_obj_port_vlan *vlan)
> +{
> +	bool untagged = vlan->flags & BRIDGE_VLAN_INFO_UNTAGGED;
> +	struct ksz_device *dev = ds->priv;
> +	struct lan937x_vlan vlan_entry;
> +	u16 pvid;
> +	int ret;
> +
> +	lan937x_pread16(dev, port, REG_PORT_DEFAULT_VID, &pvid);
> +	pvid &= 0xFFF;
> +
> +	ret = lan937x_get_vlan_table(dev, vlan->vid, &vlan_entry);
> +	if (ret < 0) {
> +		dev_err(dev->dev, "Failed to get vlan table\n");
> +		return ret;
> +	}
> +	/* clear port fwd map */
> +	vlan_entry.fwd_map &= ~BIT(port);
> +
> +	if (untagged)
> +		vlan_entry.untag_prtmap &= ~BIT(port);

This is bogus.
The user can add a VLAN entry using:

bridge vlan add dev lan0 vid 100 pvid untagged

and remove it using

bridge vlan del dev lan0 vid 100

so BRIDGE_VLAN_INFO_UNTAGGED is not set on removal.

Considering the fact that it doesn't matter whether the port is
egress-tagged or not when it isn't in the fwd_map in the first place,
I suggest you completely drop this condition.

> +
> +	ret = lan937x_set_vlan_table(dev, vlan->vid, &vlan_entry);
> +	if (ret < 0) {
> +		dev_err(dev->dev, "Failed to set vlan table\n");
> +		return ret;
> +	}
> +
> +	ret = lan937x_pwrite16(dev, port, REG_PORT_DEFAULT_VID, pvid);

What is the point of reading the pvid and writing it back unmodified?
Is the AND-ing with 0xFFF supposed to do anything? Because when you
write to REG_PORT_DEFAULT_VID, you write it with nothing in the upper
bits, so I expect there to be nothing in the upper bits when you read it
back either.

> +	if (ret < 0) {
> +		dev_err(dev->dev, "Failed to set pvid\n");
> +		return ret;
> +	}
> +
> +	return 0;
> +}

Also, consider the following set of commands:

ip link add br0 type bridge vlan_filtering 1
ip link set lan0 master br0
bridge vlan add dev lan0 vid 100 pvid untagged
bridge vlan del dev lan0 vid 100
ip link set br0 type bridge vlan_filtering 0

The expectation is that the switch, being VLAN-unaware as it is currently
configured, receives and sends any packet regardless of VLAN ID.
If you put an IP on br0 in this state, are you able to ping an outside host?