Re: [PATCH net-next 3/3] net: dsa: mv88e6xxx: add MDB support

[Sergei Shtylyov]

Hello.

On 08/31/2016 05:46 PM, Vivien Didelot wrote:

> > > diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
> > > index 93abfff..812cb47 100644
> > > --- a/drivers/net/dsa/mv88e6xxx/chip.c
> > > +++ b/drivers/net/dsa/mv88e6xxx/chip.c
> > > @@ -2240,6 +2240,15 @@ static int mv88e6xxx_port_db_dump_one(struct mv88e6xxx_chip *chip,
> > >  				fdb->ndm_state = NUD_NOARP;
> > >  			else
> > >  				fdb->ndm_state = NUD_REACHABLE;
> > > +		} else {
> >
> > Rather than else, i think it would be safer to do
> >
> > 		if (obj->id == SWITCHDEV_OBJ_ID_PORT_MDB) {
> > > +			struct switchdev_obj_port_mdb *mdb;
> > > +
> > > +			if (!is_multicast_ether_addr(addr.mac))
> > > +				continue;
> > > +
> > > +			mdb = SWITCHDEV_OBJ_PORT_MDB(obj);
> > > +			mdb->vid = vid;
> > > +			ether_addr_copy(mdb->addr, addr.mac);
> > >  		}
> >
> > It should not happen, but the day it does, we get very confused...
>
> Do you mean the something like this?
>
>     if (obj->id == SWITCHDEV_OBJ_ID_PORT_FDB) {
>         ...
>     } else if (obj->id == SWITCHDEV_OBJ_ID_PORT_MDB) {
>         ...
>     } else {
>         return -EOPNOTSUPP;
>     }
>
> I'm OK with that if you think it is better.

   Just code it as a *switch*, please. :-)

[...]

MBR, Sergei