Hi Vladimir,

On Thu, 29 Aug 2019 14:50:14 +0300, Vladimir Oltean [off-list ref] wrote:

quoted

+static int dsa_8021q_restore_pvid(struct dsa_switch *ds, int port)
+{
+       struct bridge_vlan_info vinfo;
+       struct net_device *slave;
+       u16 pvid;
+       int err;
+
+       if (!dsa_is_user_port(ds, port))
+               return 0;
+
+       slave = ds->ports[port].slave;
+
+       err = br_vlan_get_pvid(slave, &pvid);
+       if (err < 0) {
+               dev_err(ds->dev, "Couldn't determine bridge PVID\n");
+               return err;
+       }
+
+       err = br_vlan_get_info(slave, pvid, &vinfo);
+       if (err < 0) {
+               dev_err(ds->dev, "Couldn't determine PVID attributes\n");
+               return err;
+       }
+
+       return dsa_port_vid_add(&ds->ports[port], pvid, vinfo.flags);

If the bridge had installed a dsa_8021q VLAN here, I need to use the
dsa_slave_vid_add logic to restore it. The dsa_8021q flags on the CPU
port are "ingress tagged", but that may not be the case for the bridge
VLAN.
Should I expose dsa_slave_vlan_add in dsa_priv.h, or should I just
open-code another dsa_port_vid_add for dp->cpu_dp, duplicating a bit
of code from dsa_slave_vlan_add?

dsa_slave_* functions are the entry points for operations performed on the
net_device structures exposed to userspace. Using them elsewhere seems wrong.

dsa_port_* functions scope any dsa_port structure regardless its type though,
so I'd suggest duplicating a bit of code in tag_8021q.c to implement this
specific use case, until we figure out something nice to factorize.


Thank you,

	Vivien