Re: [PATCH v2 net-next 2/2] net: dsa: tag_8021q: Restore bridge VLANs when enabling vlan_filtering

[Vivien Didelot <vivien.didelot@gmail.com>]

Hi Vladimir,

On Thu, 29 Aug 2019 14:50:14 +0300, Vladimir Oltean <olteanv@gmail.com> wrote:
> > +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