Re: [PATCH v2 net-next 8/8] net: dsa: mv88e6xxx: Offload bridge broadcast flooding flag

On 3/18/2021 7:35 AM, Vladimir Oltean wrote:

On Thu, Mar 18, 2021 at 03:15:50PM +0100, Tobias Waldekranz wrote:

quoted

These switches have two modes of classifying broadcast:

1. Broadcast is multicast.
2. Broadcast is its own unique thing that is always flooded
   everywhere.

This driver uses the first option, making sure to load the broadcast
address into all active databases. Because of this, we can support
per-port broadcast flooding by (1) making sure to only set the subset
of ports that have it enabled whenever joining a new bridge or VLAN,
and (2) by updating all active databases whenever the setting is
changed on a port.

Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com>
---
 drivers/net/dsa/mv88e6xxx/chip.c | 73 +++++++++++++++++++++++++++++++-
 1 file changed, 72 insertions(+), 1 deletion(-)

diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index 7976fb699086..3baa4dadb0dd 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c

@@ -1982,6 +1982,21 @@ static int mv88e6xxx_broadcast_setup(struct mv88e6xxx_chip *chip, u16 vid)
 	int err;
 
 	for (port = 0; port < mv88e6xxx_num_ports(chip); port++) {
+		struct dsa_port *dp = dsa_to_port(chip->ds, port);
+		struct net_device *brport;
+
+		if (dsa_is_unused_port(chip->ds, port))
+			continue;
+
+		brport = dsa_port_to_bridge_port(dp);
+
+		if (dp->bridge_dev &&
+		    !br_port_flag_is_set(brport, BR_BCAST_FLOOD))

I think I would have liked to see a dsa_port_to_bridge_port helper that
actually returns NULL when dp->bridge_dev is NULL.

This would make your piece of code look as follows:

		brport = dsa_port_to_bridge_port(dp);
		if (brport && !br_port_flag_is_set(brport, BR_BCAST_FLOOD)
			continue;

Agreed, with that fixed:

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
-- 
Florian