Hi Vivien,
On Tue, 20 Aug 2019 at 09:07, Vivien Didelot <vivien.dide...@gmail.com> wrote:
>
> On Tue, 20 Aug 2019 03:00:00 +0300, Vladimir Oltean <olte...@gmail.com> wrote:
> > Commit b2f81d304cee ("net: dsa: add CPU and DSA ports as VLAN members")
> > programs the VLAN from the bridge into the specified port as well as the
> > upstream port, with the same set of flags.
> >
> > Consider the typical case of installing pvid 1 on user port 1, pvid 2 on
> > user port 2, etc. The upstream port would end up having a pvid equal to
> > the last user port whose pvid was programmed from the bridge. Less than
> > useful.
> >
> > So just don't change the pvid of the upstream port and let it be
> > whatever the driver set it internally to be.
> >
> > Signed-off-by: Vladimir Oltean <olte...@gmail.com>
> > ---
> > net/dsa/switch.c | 16 ++++++++++++----
> > 1 file changed, 12 insertions(+), 4 deletions(-)
> >
> > diff --git a/net/dsa/switch.c b/net/dsa/switch.c
> > index 84ab2336131e..02ccc53f1926 100644
> > --- a/net/dsa/switch.c
> > +++ b/net/dsa/switch.c
> > @@ -239,17 +239,21 @@ dsa_switch_vlan_prepare_bitmap(struct dsa_switch *ds,
> > const struct switchdev_obj_port_vlan *vlan,
> > const unsigned long *bitmap)
> > {
> > + struct switchdev_obj_port_vlan v = *vlan;
> > int port, err;
> >
> > if (!ds->ops->port_vlan_prepare || !ds->ops->port_vlan_add)
> > return -EOPNOTSUPP;
> >
> > for_each_set_bit(port, bitmap, ds->num_ports) {
> > - err = dsa_port_vlan_check(ds, port, vlan);
> > + if (dsa_is_cpu_port(ds, port) || dsa_is_dsa_port(ds, port))
> > + v.flags &= ~BRIDGE_VLAN_INFO_PVID;
>
> So you keep the BRIDGE_VLAN_INFO_PVID flag cleared for all other ports that
> come after any CPU or DSA port?
>
It looks like the convenient hardware decision of making the CPU port
on my board also be the numerically highest one strikes again :)
I always find bugs when I change the CPU port to another number.
This is another example (also related to the inclusion of upstream
ports in the VLAN bitmap, like they all seem to be):
https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git/commit/?id=d34d2baa9173f6e0c0f22d005d18e83d1cb54d8d
> > +
> > + err = dsa_port_vlan_check(ds, port, &v);
> > if (err)
> > return err;
> >
> > - err = ds->ops->port_vlan_prepare(ds, port, vlan);
> > + err = ds->ops->port_vlan_prepare(ds, port, &v);
> > if (err)
> > return err;
> > }
> > @@ -262,10 +266,14 @@ dsa_switch_vlan_add_bitmap(struct dsa_switch *ds,
> > const struct switchdev_obj_port_vlan *vlan,
> > const unsigned long *bitmap)
> > {
> > + struct switchdev_obj_port_vlan v = *vlan;
> > int port;
> >
> > - for_each_set_bit(port, bitmap, ds->num_ports)
> > - ds->ops->port_vlan_add(ds, port, vlan);
> > + for_each_set_bit(port, bitmap, ds->num_ports) {
> > + if (dsa_is_cpu_port(ds, port) || dsa_is_dsa_port(ds, port))
> > + v.flags &= ~BRIDGE_VLAN_INFO_PVID;
>
> Same here. Did you intend to initialize your switchdev_obj_port_vlan structure
> _within_ the for_each_set_bit loop maybe?
>
Thanks for pointing this out.
> > + ds->ops->port_vlan_add(ds, port, &v);
> > + }
> > }
> >
> > static int dsa_switch_vlan_add(struct dsa_switch *ds,
>
> Do you even test your patches?
No.
Regards,
-Vladimir
