From mboxrd@z Thu Jan 1 00:00:00 1970 From: Subject: RE: dsa: dsa_slave_port_obj_del calls multiple times with SWITCHDEV_OBJ_ID_HOST_MDB obj id Date: Wed, 6 Dec 2017 19:31:55 +0000 Message-ID: <93AF473E2DA327428DE3D46B72B1E9FD4113E1AD@CHN-SV-EXMX02.mchp-main.com> References: <93AF473E2DA327428DE3D46B72B1E9FD4113DE40@CHN-SV-EXMX02.mchp-main.com> <20171206135619.GE27063@lunn.ch> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 8BIT Cc: , , To: Return-path: Received: from esa2.microchip.iphmx.com ([68.232.149.84]:11162 "EHLO esa2.microchip.iphmx.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751208AbdLFTb4 (ORCPT ); Wed, 6 Dec 2017 14:31:56 -0500 In-Reply-To: <20171206135619.GE27063@lunn.ch> Content-Language: en-US Sender: netdev-owner@vger.kernel.org List-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.