Re: [PATCH net-next 3/3] net: dsa: mv88e6xxx: fix vlan setup

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

On 2/18/20 11:09 AM, Vivien Didelot wrote:
> Hi Russell,
> 
> On Tue, 18 Feb 2020 11:46:20 +0000, Russell King <rmk+kernel@armlinux.org.uk> wrote:
>> DSA assumes that a bridge which has vlan filtering disabled is not
>> vlan aware, and ignores all vlan configuration. However, the kernel
>> software bridge code allows configuration in this state.
>>
>> This causes the kernel's idea of the bridge vlan state and the
>> hardware state to disagree, so "bridge vlan show" indicates a correct
>> configuration but the hardware lacks all configuration. Even worse,
>> enabling vlan filtering on a DSA bridge immediately blocks all traffic
>> which, given the output of "bridge vlan show", is very confusing.
>>
>> Provide an option that drivers can set to indicate they want to receive
>> vlan configuration even when vlan filtering is disabled. This is safe
>> for Marvell DSA bridges, which do not look up ingress traffic in the
>> VTU if the port is in 8021Q disabled state. Whether this change is
>> suitable for all DSA bridges is not known.
>>
>> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
>> ---
>>  drivers/net/dsa/mv88e6xxx/chip.c |  1 +
>>  include/net/dsa.h                |  1 +
>>  net/dsa/slave.c                  | 12 ++++++++----
>>  3 files changed, 10 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
>> index 629eb7bbbb23..e656e571ef7d 100644
>> --- a/drivers/net/dsa/mv88e6xxx/chip.c
>> +++ b/drivers/net/dsa/mv88e6xxx/chip.c
>> @@ -2934,6 +2934,7 @@ static int mv88e6xxx_setup(struct dsa_switch *ds)
>>  
>>  	chip->ds = ds;
>>  	ds->slave_mii_bus = mv88e6xxx_default_mdio_bus(chip);
>> +	ds->vlan_bridge_vtu = true;
>>  
>>  	mv88e6xxx_reg_lock(chip);
>>  
>> diff --git a/include/net/dsa.h b/include/net/dsa.h
>> index 63495e3443ac..d3a826646e8e 100644
>> --- a/include/net/dsa.h
>> +++ b/include/net/dsa.h
>> @@ -273,6 +273,7 @@ struct dsa_switch {
>>  	 * settings on ports if not hardware-supported
>>  	 */
>>  	bool			vlan_filtering_is_global;
>> +	bool			vlan_bridge_vtu;
>>  
>>  	/* In case vlan_filtering_is_global is set, the VLAN awareness state
>>  	 * should be retrieved from here and not from the per-port settings.
>> diff --git a/net/dsa/slave.c b/net/dsa/slave.c
>> index 088c886e609e..534d511b349e 100644
>> --- a/net/dsa/slave.c
>> +++ b/net/dsa/slave.c
>> @@ -318,7 +318,8 @@ static int dsa_slave_vlan_add(struct net_device *dev,
>>  	if (obj->orig_dev != dev)
>>  		return -EOPNOTSUPP;
>>  
>> -	if (dp->bridge_dev && !br_vlan_enabled(dp->bridge_dev))
>> +	if (dp->bridge_dev && !dp->ds->vlan_bridge_vtu &&
>> +	    !br_vlan_enabled(dp->bridge_dev))
>>  		return 0;
>>  
>>  	vlan = *SWITCHDEV_OBJ_PORT_VLAN(obj);
>> @@ -385,7 +386,8 @@ static int dsa_slave_vlan_del(struct net_device *dev,
>>  	if (obj->orig_dev != dev)
>>  		return -EOPNOTSUPP;
>>  
>> -	if (dp->bridge_dev && !br_vlan_enabled(dp->bridge_dev))
>> +	if (dp->bridge_dev && !dp->ds->vlan_bridge_vtu &&
>> +	    !br_vlan_enabled(dp->bridge_dev))
>>  		return 0;
>>  
>>  	/* Do not deprogram the CPU port as it may be shared with other user
>> @@ -1106,7 +1108,8 @@ static int dsa_slave_vlan_rx_add_vid(struct net_device *dev, __be16 proto,
>>  	 * need to emulate the switchdev prepare + commit phase.
>>  	 */
>>  	if (dp->bridge_dev) {
>> -		if (!br_vlan_enabled(dp->bridge_dev))
>> +		if (!dp->ds->vlan_bridge_vtu &&
>> +		    !br_vlan_enabled(dp->bridge_dev))
>>  			return 0;
>>  
>>  		/* br_vlan_get_info() returns -EINVAL or -ENOENT if the
>> @@ -1140,7 +1143,8 @@ static int dsa_slave_vlan_rx_kill_vid(struct net_device *dev, __be16 proto,
>>  	 * need to emulate the switchdev prepare + commit phase.
>>  	 */
>>  	if (dp->bridge_dev) {
>> -		if (!br_vlan_enabled(dp->bridge_dev))
>> +		if (!dp->ds->vlan_bridge_vtu &&
>> +		    !br_vlan_enabled(dp->bridge_dev))
>>  			return 0;
>>  
>>  		/* br_vlan_get_info() returns -EINVAL or -ENOENT if the
> 
> It is confusing to add a Marvell specific term (VTU) in the generic dsa_switch
> structure to bypass the fact that VLAN configuration in hardware was already
> bypassed for some reasons until vlan filtering is turned on. As you said,
> simply offloading the VLAN configuration in hardware and only turning the
> ports' 802.1Q mode to Secure once vlan_filtering is flipped to 1 should work
> in theory for both VLAN filtering aware and unaware scenarios, but this was
> causing problems if I'm not mistaken, I'll try to dig this out.
> 
> In the meantime, does the issue you're trying to solve here happens if you
> create a vlan-filtering aware bridge in the first place, before any VLAN
> configuration? i.e.:
> 
>     # ip link add name br0 type bridge vlan_filtering 1
>     # ip link set master br0 dev lan1 up
>     # bridge vlan add ...

That will work okay because then you do get around the restrictions
added by 2ea7a679ca2abd251c1ec03f20508619707e1749 ("net: dsa: Don't add
vlans when vlan filtering is disabled") and you will get VLAN objects
programming request to flow down your DSA driver. AFAICT, mlxsw
specifically prevents to toggle vlan_filtering at runtime for that
reason, because the VLAN objects notification are not "sent again" while
you toggle vlan_filtering. I really wonder if we should not do the same
in DSA as runtime toggling is of questionable use beyond just testing.

Russell, in your tests/examples, did the tagged traffic of $VN continue
to work after you toggled vlan_filtering on? If so, that must be because
on a bridge with VLAN filtering disabled, we still ended up calling down
to the lan1..6 ndo_vlan_rx_add_vid() and so we do have VLAN entries
programmed for $VN.
-- 
Florian