Re: [PATCH RFC 1/2] net: dsa: integrate with SWITCHDEV for HW bridging

[Florian Fainelli <f.fainelli@gmail.com>]

On 17/02/15 17:19, Andrew Lunn wrote:
>> +/* Return a bitmask of all ports being currently bridged. Note that on
>> + * leave, the mask will still return the bitmask of ports currently bridged,
>> + * prior to port removal, and this is exactly what we want.
>> + */
>> +static u32 dsa_slave_br_port_mask(struct dsa_switch *ds)
>> +{
>> +	unsigned int port;
>> +	u32 mask = 0;
>> +
>> +	for (port = 0; port < DSA_MAX_PORTS; port++) {
>> +		if (!((1 << port) & ds->phys_port_mask))
>> +			continue;
>> +
>> +		if (ds->ports[port]->priv_flags & IFF_BRIDGE_PORT)
>> +			mask |= 1 << port;
>> +	}
>> +
>> +       return mask;
>> +}
>> +
>> +static int dsa_slave_bridge_port_join(struct net_device *dev,
>> +				      struct net_device *bridge)
>> +{
>> +	struct dsa_slave_priv *p = netdev_priv(dev);
>> +	struct dsa_switch *ds = p->parent;
>> +	int ret = -EOPNOTSUPP;
>> +
>> +	if (ds->drv->port_join_bridge)
>> +		ret = ds->drv->port_join_bridge(ds, p->port,
>> +						dsa_slave_br_port_mask(ds));
> 
> Hi Florian
> 
> Shouldn't this bridge port mask also be dependent on bridge?

Yes, you are very right, thankfully this is a RFC patch because of that ;)

> 
> I'm thinking of cases like
> 
> brctl addbr br0
> brctl addif br0 lan0
> brctl addif br0 lan1
> 
> brctl addbr br1
> brctl addif br1 lan2

mask = 0x4 | 0x2 | 0x1 # FAIL

> brctl addif br1 lan3

mask = 0x8 | 0x4 | 0x2 | 0x1 # FAIL

> 
> We have two software bridges, so need two masks.  It does look like
> your hardware and the Marvell hardware supports this, disjoint sets of
> bridged ports.  But with the current implementation, your going to end
> up with one hardware bridge with four ports, and broken STP.

Yep, I will rework that patch set to address that, and I can actually
test that, which is even better, thanks!
-- 
Florian