Message-ID: <87pmzynib9.fsf@waldekranz.com>
Date:   Wed, 17 Mar 2021 12:14:18 +0100
From:   Tobias Waldekranz <tobias@...dekranz.com>
To:     Vladimir Oltean <olteanv@...il.com>
Cc:     davem@...emloft.net, kuba@...nel.org, andrew@...n.ch,
        vivien.didelot@...il.com, f.fainelli@...il.com,
        netdev@...r.kernel.org
Subject: Re: [PATCH net-next 5/5] net: dsa: mv88e6xxx: Offload bridge broadcast flooding flag

On Tue, Mar 16, 2021 at 11:39, Vladimir Oltean <olteanv@...il.com> wrote:
> On Mon, Mar 15, 2021 at 10:14:00PM +0100, Tobias Waldekranz wrote:
>> 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@...dekranz.com>
>> ---
>>  drivers/net/dsa/mv88e6xxx/chip.c | 68 +++++++++++++++++++++++++++++++-
>>  1 file changed, 67 insertions(+), 1 deletion(-)
>> 
>> diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
>> index 48e65f22641e..e6987c501fb7 100644
>> --- a/drivers/net/dsa/mv88e6xxx/chip.c
>> +++ b/drivers/net/dsa/mv88e6xxx/chip.c
>> @@ -1950,6 +1950,18 @@ 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);
>> +
>> +		if (dsa_is_unused_port(chip->ds, port))
>> +			continue;
>> +
>> +		if (dsa_is_user_port(chip->ds, port) && dp->bridge_dev &&
>> +		    !br_port_flag_is_set(dp->slave, BR_BCAST_FLOOD))
>
> What if dp->slave is not the bridge port, but a LAG? br_port_flag_is_set
> will return false.

Good point. I see two ways forward:

- My first idea was to cache a vector per switch that would act as the
  template when creating a new entry. This avoids having the driver
  layer knowing about stacked netdevs etc. But I think that Andrew is
  generally opposed to caching?

- Add a new helper at the dsa layer that takes a dp and returns the
  netdev that is attached to the bridge, if any:

  struct net_device *dsa_port_to_bridge_port(struct dsa_port *dp)

Any preference or other ideas?

> Speaking of, shouldn't mv88e6xxx_port_vlan_join also be called from
> mv88e6xxx_port_bridge_join somehow, or are we waiting for the bridge
> facility to replay VLANs added to the LAG when we emit the offload
> notification for it?

I do not think so. VLANs are always added via the .port_vlan_add
callback, no?

Potentially the bridge is of the non-filtering variety, so it could be
that no VLANs are ever added.

Or do you mean (the most confusingly named feature Marvell LinkStreet
devices) port-based VLANs? Those are setup on a bridge join via
mv88e6xxx_port_vlan_map and mv88e6xxx_pvt_map.

>> +			/* Skip bridged user ports where broadcast
>> +			 * flooding is disabled.
>> +			 */
>> +			continue;
>> +
>>  		err = mv88e6xxx_port_add_broadcast(chip, port, vid);
>>  		if (err)
>>  			return err;
>> @@ -1958,6 +1970,51 @@ static int mv88e6xxx_broadcast_setup(struct mv88e6xxx_chip *chip, u16 vid)
>>  	return 0;
>>  }
>>  
>> +struct mv88e6xxx_port_broadcast_sync_ctx {
>> +	int port;
>> +	bool flood;
>> +};
>> +
>> +static int
>> +mv88e6xxx_port_broadcast_sync_vlan(struct mv88e6xxx_chip *chip,
>> +				   const struct mv88e6xxx_vtu_entry *vlan,
>> +				   void *_ctx)
>> +{
>> +	const char broadcast[6] = { 0xff, 0xff, 0xff, 0xff, 0xff, 0xff };
>
> MAC addresses are usually defined as unsigned char[ETH_ALEN]. You can
> also use eth_broadcast_addr(broadcast) for initialization.

I was going for uniformity with mv88e6xxx_port_add_broadcast. But I will
add a clean-up commit that fixes the existing code first, and then adds
this definition in the proper way.

>> +	struct mv88e6xxx_port_broadcast_sync_ctx *ctx = _ctx;
>> +	u8 state;
>> +
>> +	if (ctx->flood)
>> +		state = MV88E6XXX_G1_ATU_DATA_STATE_MC_STATIC;
>> +	else
>> +		state = MV88E6XXX_G1_ATU_DATA_STATE_MC_UNUSED;
>> +
>> +	return mv88e6xxx_port_db_load_purge(chip, ctx->port, broadcast,
>> +					    vlan->vid, state);
>> +}
>> +
>> +static int mv88e6xxx_port_broadcast_sync(struct mv88e6xxx_chip *chip, int port,
>> +					 bool flood)
>> +{
>> +	struct mv88e6xxx_port_broadcast_sync_ctx ctx = {
>> +		.port = port,
>> +		.flood = flood,
>> +	};
>> +	struct mv88e6xxx_vtu_entry vid0 = {
>> +		.vid = 0,
>> +	};
>> +	int err;
>> +
>> +	/* Update the port's private database... */
>> +	err = mv88e6xxx_port_broadcast_sync_vlan(chip, &vid0, &ctx);
>> +	if (err)
>> +		return err;
>> +
>> +	/* ...and the database for all VLANs. */
>> +	return mv88e6xxx_vtu_walk(chip, mv88e6xxx_port_broadcast_sync_vlan,
>> +				  &ctx);
>> +}
>> +
>>  static int mv88e6xxx_port_vlan_join(struct mv88e6xxx_chip *chip, int port,
>>  				    u16 vid, u8 member, bool warn)
>>  {
>> @@ -5431,7 +5488,8 @@ static int mv88e6xxx_port_pre_bridge_flags(struct dsa_switch *ds, int port,
>>  	struct mv88e6xxx_chip *chip = ds->priv;
>>  	const struct mv88e6xxx_ops *ops;
>>  
>> -	if (flags.mask & ~(BR_LEARNING | BR_FLOOD | BR_MCAST_FLOOD))
>> +	if (flags.mask & ~(BR_LEARNING | BR_FLOOD | BR_MCAST_FLOOD |
>> +			   BR_BCAST_FLOOD))
>>  		return -EINVAL;
>>  
>>  	ops = chip->info->ops;
>> @@ -5480,6 +5538,14 @@ static int mv88e6xxx_port_bridge_flags(struct dsa_switch *ds, int port,
>>  			goto out;
>>  	}
>>  
>> +	if (flags.mask & BR_BCAST_FLOOD) {
>> +		bool broadcast = !!(flags.val & BR_BCAST_FLOOD);
>> +
>> +		err = mv88e6xxx_port_broadcast_sync(chip, port, broadcast);
>> +		if (err)
>> +			goto out;
>> +	}
>> +
>>  out:
>>  	mv88e6xxx_reg_unlock(chip);
>>  
>> -- 
>> 2.25.1
>> 

Powered by blists - more mailing lists