Re: [Patch RFC net-next 4/4] net: dsa: microchip: use private pvid for bridge_vlan_unwaware

[Vladimir Oltean <olteanv@gmail.com>]

On Fri, Jul 29, 2022 at 08:47:33PM +0530, Arun Ramadoss wrote:
> diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c
> index 516fb9d35c87..8a5583b1f2f4 100644
> --- a/drivers/net/dsa/microchip/ksz_common.c
> +++ b/drivers/net/dsa/microchip/ksz_common.c
> @@ -161,6 +161,7 @@ static const struct ksz_dev_ops ksz8_dev_ops = {
>  	.vlan_filtering = ksz8_port_vlan_filtering,
>  	.vlan_add = ksz8_port_vlan_add,
>  	.vlan_del = ksz8_port_vlan_del,
> +	.drop_untagged = ksz8_port_enable_pvid,

You'll have to explain this one. What impact does PVID insertion on KSZ8
have upon dropping/not dropping untagged packets? This patch is saying
that when untagged packets should be dropped, PVID insertion should be
enabled, and when untagged packets should be accepted, PVID insertion
should be disabled. How come?

>  	.mirror_add = ksz8_port_mirror_add,
>  	.mirror_del = ksz8_port_mirror_del,
>  	.get_caps = ksz8_get_caps,
> @@ -187,6 +188,7 @@ static const struct ksz_dev_ops ksz9477_dev_ops = {
>  	.vlan_filtering = ksz9477_port_vlan_filtering,
>  	.vlan_add = ksz9477_port_vlan_add,
>  	.vlan_del = ksz9477_port_vlan_del,
> +	.drop_untagged = ksz9477_port_drop_untagged,
>  	.mirror_add = ksz9477_port_mirror_add,
>  	.mirror_del = ksz9477_port_mirror_del,
>  	.get_caps = ksz9477_get_caps,
> @@ -220,6 +222,7 @@ static const struct ksz_dev_ops lan937x_dev_ops = {
>  	.vlan_filtering = ksz9477_port_vlan_filtering,
>  	.vlan_add = ksz9477_port_vlan_add,
>  	.vlan_del = ksz9477_port_vlan_del,
> +	.drop_untagged = ksz9477_port_drop_untagged,
>  	.mirror_add = ksz9477_port_mirror_add,
>  	.mirror_del = ksz9477_port_mirror_del,
>  	.get_caps = lan937x_phylink_get_caps,
> @@ -1254,6 +1257,9 @@ static int ksz_enable_port(struct dsa_switch *ds, int port,
>  {
>  	struct ksz_device *dev = ds->priv;
>  
> +	dev->dev_ops->vlan_add(dev, port, KSZ_DEFAULT_VLAN,
> +			       BRIDGE_VLAN_INFO_UNTAGGED);
> +

How many times can this be executed before the VLAN add operation fails
due to the VLAN already being present on the port? I notice you're
ignoring the return code. Wouldn't it be better to do this at
port_setup() time instead?

(side note, the PVID for standalone mode can be added at port_setup
time. The PVID to use for bridges in VLAN-unaware mode can be allocated
at port_bridge_join time)

>  	if (!dsa_is_user_port(ds, port))
>  		return 0;
>  
> +static int ksz_commit_pvid(struct dsa_switch *ds, int port)
> +{
> +	struct dsa_port *dp = dsa_to_port(ds, port);
> +	struct net_device *br = dsa_port_bridge_dev_get(dp);
> +	struct ksz_device *dev = ds->priv;
> +	u16 pvid = KSZ_DEFAULT_VLAN;
> +	bool drop_untagged = false;
> +	struct ksz_port *p;
> +
> +	p = &dev->ports[port];
> +
> +	if (br && br_vlan_enabled(br)) {
> +		pvid = p->bridge_pvid.vid;
> +		drop_untagged = !p->bridge_pvid.valid;
> +	}

This is better in the sense that it resolves the need for the
configure_vlan_while_not_filtering hack. But standalone and VLAN-unaware
bridge ports still share the same PVID. Even more so, standalone ports
have address learning enabled, which will poison the address database of
VLAN-unaware bridge ports (and of other standalone ports):
https://patchwork.kernel.org/project/netdevbpf/patch/20220802002636.3963025-1-vladimir.oltean@nxp.com/

Are you going to do further work in this area?

> +
> +	ksz_set_pvid(dev, port, pvid);
> +
> +	if (dev->dev_ops->drop_untagged)
> +		dev->dev_ops->drop_untagged(dev, port, drop_untagged);
> +
> +	return 0;
> +}