Date:   Wed, 6 Dec 2017 19:31:55 +0000
From:   <Tristram.Ha@...rochip.com>
To:     <andrew@...n.ch>
CC:     <f.fainelli@...il.com>, <netdev@...r.kernel.org>,
        <UNGLinuxDriver@...rochip.com>
Subject: RE: dsa: dsa_slave_port_obj_del calls multiple times with
 SWITCHDEV_OBJ_ID_HOST_MDB obj id

> SWITCHDEV_OBJ_ID_HOST_MDB is used, when there is a join/leave on the
> bridge interface. It happens for each interface in the bridge, and it
> means, packets which match the group that ingress on that interface
> should be forwarded to the CPU.
> 
> > As the base driver cannot find an entry with that host port, it returns an
> error
> > and so users will see a lot of failures from the DSA switch.
> 
> You have a few options:
> 
> 1) Just forward all multicast traffic to the cpu, and ignore
>    SWITCHDEV_OBJ_ID_HOST_MDB.
> 
> 2) Implement SWITCHDEV_OBJ_ID_HOST_MDB so you setup your tables to
>    just forward the requested multicast to the cpu.
> 
> 3) You can also forward a bit too much, e.g. if you cannot set filters
>    per ingress port, just send all the traffic for the group from any
>    port.
> 
> 
> The bridge will discard whatever it does not need.
> 
> > Is this a new behavior and the driver needs to handle that?  In previous
> versions
> > I do not think I saw that.
> 
> SWITCHDEV_OBJ_ID_HOST_MDB is new. However,
> dsa_slave_port_obj_del()
> can be called for all sorts of objects, and you should only be
> reacting on those your support. So adding a new object should not of
> changed anything.
> 
> > Typical operation is a PC connected to a port in a switch wants to send
> multicast
> > packets.  It broadcasts an IGMP membership join message.  Function
> > dsa_slave_port_obj_add is called to setup an entry in the lookup table.
> When
> > IGMP membership leave message is received dsa_slave_port_obj_del will
> be
> > called after a delay.  But then it is called for each port with host port as the
> > parameter.
> 
> Correct. switchdev is a generic API. It also needs to work for Top of
> Rack switches, which generally have a match/action architecture. I can
> imagine that this match/action happens per port, so we need to call
> switchdev per port. However, switches supported by DSA tend to have
> central management of all ports, so one call would be sufficient. With
> a DSA driver, you just need to expect redundant calls, and do the
> right thing.

The base DSA driver only knows about port_mdb_add and port_mdb_del.
It does not really know about SWITCHDEV_OBJ_ID_HOST_MDB.

It is fine to use port_mdb_add and port_mdb_del for
SWITCHDEV_OBJ_ID_HOST_MDB as hardware can program an entry for
the host port, but receiving many port_mdb_del calls with the same
parameter makes it difficult to handle.  Example:

port_mdb_add 2 xx:xx:xx:xx:xx:xx
port_mdb_del 2 xx:xx:xx:xx:xx:xx
port_mdb_del 4 xx:xx:xx:xx:xx:xx; lan1
port_mdb_del 4 xx:xx:xx:xx:xx:xx; lan2
port_mdb_del 4 xx:xx:xx:xx:xx:xx; lan3

There is no indication in the port_mdb_del call to show this call is for
port 1, 2, and so on.

port_mdb_add can be used to add an entry for the host.  Again it is
called as many times as number of ports.  I think the port_mdb_* call
needs an extra parameter to make it useful in this situation.

An easy fix is not to return any error in port_mdb_del, or ignore host port.

It seems port_mdb_prepare is used to indicate an entry can be added
before the actual port_mdb_add call, which returns void and so cannot
indicate failure to add.  Currently there is no implementation for that
port_mdb_prepare call and the Marvell driver just displays a warning
inside its port_mdb_add function.

Powered by blists - more mailing lists