Re: [PATCH net] net: dsa: mv88e6xxx: Fix opps when adding vlan bridge

[Andrew Lunn]

On Sun, Dec 11, 2016 at 04:02:37PM -0500, Vivien Didelot wrote:
> Hi Andrew,
> 
> Andrew Lunn <and...@lunn.ch> writes:
> 
> > @@ -1804,6 +1807,9 @@ static int mv88e6xxx_port_check_hw_vlan(struct 
> > dsa_switch *ds, int port,
> >                         chip->ports[port].bridge_dev)
> >                             break; /* same bridge, check next VLAN */
> >  
> > +                   if (!chip->ports[i].bridge_dev)
> > +                           continue;
> > +
> 
> The above truncated test:
> 
>                       if (chip->ports[i].bridge_dev ==
>                           chip->ports[port].bridge_dev)
>                               break; /* same bridge, check next VLAN */
> 
> should handle the case where bridge_dev is NULL, but if you want to
> explicitly test it, I'd move it before this statement.
> 
> >                     netdev_warn(ds->ports[port].netdev,
> >                                 "hardware VLAN %d already used by %s\n",
> >                                 vlan.vid,

Hi Vivien

I don't think you comment is correct. Here is the loop, with my two
additions.

                for (i = 0; i < mv88e6xxx_num_ports(chip); ++i) {
                        if (dsa_is_dsa_port(ds, i) || dsa_is_cpu_port(ds, i))
                                continue;

                        if (!ds->ports[port].netdev)
                                continue;

                        if (vlan.data[i] ==
                            GLOBAL_VTU_DATA_MEMBER_TAG_NON_MEMBER)
                                continue;

                        if (chip->ports[i].bridge_dev ==
                            chip->ports[port].bridge_dev)
                                break; /* same bridge, check next VLAN */

                        if (!chip->ports[i].bridge_dev)
                                continue;

                        netdev_warn(ds->ports[port].netdev,
                                    "hardware VLAN %d already used by %s\n",
                                    vlan.vid,
                                    netdev_name(chip->ports[i].bridge_dev));
                        err = -EOPNOTSUPP;
                        goto unlock;
                }

The opps was occurring in netdev_name(). I did not check which
one. The obvious one, or the netdev_warn(netdev,... one.

> The above truncated test:
> 
>                       if (chip->ports[i].bridge_dev ==
>                           chip->ports[port].bridge_dev)
>                               break; /* same bridge, check next VLAN */
>
> should handle the case where bridge_dev is NULL, but if you want to
> explicitly test it, I'd move it before this statement.

This will not stop chip->ports[i].bridge_dev == NULL from reaching the
netdev_warn().

        Andrew