diff mbox series

mv88e6xxx: fixed adding vlan 0

Message ID 20210517062506.5045-1-eldargasanov2@gmail.com
State Superseded
Headers show
Series mv88e6xxx: fixed adding vlan 0 | expand

Commit Message

Eldar Gasanov May 17, 2021, 6:25 a.m. UTC 
8021q module adds vlan 0 to all interfaces when it starts.
When 8021q module is loaded it isn't possible to create bond
with mv88e6xxx interfaces, bonding module dipslay error
"Couldn't add bond vlan ids", because it tries to add vlan 0
to slave interfaces.

Signed-off-by: Eldar Gasanov <eldargasanov2@gmail.com>
---
 drivers/net/dsa/mv88e6xxx/chip.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

Comments

Vladimir Oltean May 17, 2021, 3 p.m. UTC | #1 
Hi Eldar,

On Mon, May 17, 2021 at 09:25:06AM +0300, Eldar Gasanov wrote:
> 8021q module adds vlan 0 to all interfaces when it starts.
> When 8021q module is loaded it isn't possible to create bond
> with mv88e6xxx interfaces, bonding module dipslay error
> "Couldn't add bond vlan ids", because it tries to add vlan 0
> to slave interfaces.
> 
> Signed-off-by: Eldar Gasanov <eldargasanov2@gmail.com>
> ---
>  drivers/net/dsa/mv88e6xxx/chip.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
> index eca285aaf72f..961fa6b75cad 100644
> --- a/drivers/net/dsa/mv88e6xxx/chip.c
> +++ b/drivers/net/dsa/mv88e6xxx/chip.c
> @@ -1618,9 +1618,6 @@ static int mv88e6xxx_port_check_hw_vlan(struct dsa_switch *ds, int port,
>  	struct mv88e6xxx_vtu_entry vlan;
>  	int i, err;
>  
> -	if (!vid)
> -		return -EOPNOTSUPP;
> -
>  	/* DSA and CPU ports have to be members of multiple vlans */
>  	if (dsa_is_dsa_port(ds, port) || dsa_is_cpu_port(ds, port))
>  		return 0;
> @@ -2109,6 +2106,9 @@ static int mv88e6xxx_port_vlan_add(struct dsa_switch *ds, int port,
>  	u8 member;
>  	int err;
>  
> +	if (!vlan->vid)
> +		return 0;
> +
>  	err = mv88e6xxx_port_vlan_prepare(ds, port, vlan);
>  	if (err)
>  		return err;
> -- 
> 2.25.1
> 

Thank you for your patch.

The reason why the 8021q module adds VLAN 0 to the hardware filtering
list of the network device is to ensure that 802.1p-tagged packets are
always received and treated as untagged, while on the other hand
preserving their VLAN ID of 0.

This is described in commit ad1afb003939 ("vlan_dev: VLAN 0 should be
treated as "no vlan tag" (802.1p packet)").

When we look at vlan_device_event(), we see that vlan_vid_add() is
called without checking the error code. So when mv88e6xxx returns
-EOPNOTSUPP, the code carries along.

I spot an inconsistency within the 8021q module, because
vlan_vids_add_by_dev() then checks the return code of vlan_vid_add(),
resulting in the problem you are trying to fix here. I think it would be
more self-consistent if vlan_vids_add_by_dev() would propagate the error
only if vid is != 0.

It can be said that the bug you are fixing is a regression introduced by
my commit 9b236d2a69da ("net: dsa: Advertise the VLAN offload netdev
ability only if switch supports it"). Tobias too tried to fix it here:
https://patchwork.kernel.org/project/netdevbpf/cover/20210308150405.3694678-1-tobias@waldekranz.com/

However that is not the central point. Ignoring the errors operates in
the good faith that the real_dev driver knows what it's doing, and
satisfies the requirements of commit ad1afb003939 mentioned above. This
is user-visible behavior, so it should better be compliant. And this is
where I am not 100% clear on what is going on.

The mv88e6xxx driver can refuse the configuration of VLAN ID 0 if it
does the right thing and we can make either the mv88e6xxx layer, or the
DSA layer (Tobias's patch) or the 8021q layer ignore that error code.
But I would like to make sure that in either case, a packet with VID 0
received by the mv88e6xxx ports will still contain VID 0, in the
following situations:

- The port is standalone
- The port is under a VLAN-unaware bridge
- The port is under a VLAN-aware bridge

I happen to have a mv88e6xxx testing device, and what I see is not
exactly that. In the first two cases, we are ok, but when the port is
under a VLAN-aware bridge, the packets are received as PVID-tagged:

# ip link add br0 type bridge vlan_filtering 1
# ip link set lan24 master br0
# bridge vlan add dev lan24 vid 34 pvid untagged
# tcpdump -i lan24 -e -n
tcpdump: verbose output suppressed, use -v or -vv for full protocol decode
listening on lan24, link-type EN10MB (Ethernet), capture size 262144 bytes
17:32:29.896736 00:1f:7b:63:02:08 > d8:58:d7:00:ca:6d, ethertype 802.1Q (0x8100), length 64: vlan 34, p 0, ethertype 0x88f7,
        0x0000:  0402 0000 0000 0000 15f1 2ef4 a9a7 d040  ...............@
        0x0010:  0000 0000 0000 0000 0000 0000 0000 0001  ................
        0x0020:  0000 dead beef dead beef dead beef       ..............
17:32:29.898292 00:1f:7b:63:02:08 > d8:58:d7:00:ca:6d, ethertype 802.1Q (0x8100), length 64: vlan 34, p 0, ethertype 0x88f7,
        0x0000:  0402 0000 0000 0000 15f1 2ef4 a9b7 1280  ................
        0x0010:  0000 0000 0000 0000 0000 0000 0000 0002  ................
        0x0020:  0000 dead beef dead beef dead beef       ..............
17:32:29.898713 00:1f:7b:63:02:08 > d8:58:d7:00:ca:6d, ethertype 802.1Q (0x8100), length 64: vlan 34, p 0, ethertype 0x88f7,
        0x0000:  0402 0000 0000 0000 15f1 2ef4 a9c6 54c0  ..............T.
        0x0010:  0000 0000 0000 0000 0000 0000 0000 0003  ................
        0x0020:  0000 dead beef dead beef dead beef       ..............
17:32:29.899674 00:1f:7b:63:02:08 > d8:58:d7:00:ca:6d, ethertype 802.1Q (0x8100), length 64: vlan 34, p 0, ethertype 0x88f7,
        0x0000:  0402 0000 0000 0000 15f1 2ef4 a9d5 9700  ................
        0x0010:  0000 0000 0000 0000 0000 0000 0000 0004  ................
        0x0020:  0000 dead beef dead beef dead beef       ..............
17:32:29.902377 00:1f:7b:63:02:08 > d8:58:d7:00:ca:6d, ethertype 802.1Q (0x8100), length 64: vlan 34, p 0, ethertype 0x88f7,
        0x0000:  0402 0000 0000 0000 15f1 2ef4 a9e4 d940  ...............@
        0x0010:  0000 0000 0000 0000 0000 0000 0000 0005  ................
        0x0020:  0000 dead beef dead beef dead beef       ..............
17:32:29.904425 00:1f:7b:63:02:08 > d8:58:d7:00:ca:6d, ethertype 802.1Q (0x8100), length 64: vlan 34, p 0, ethertype 0x88f7,
        0x0000:  0402 0000 0000 0000 15f1 2ef4 a9f4 1b80  ................
        0x0010:  0000 0000 0000 0000 0000 0000 0000 0006  ................
        0x0020:  0000 dead beef dead beef dead beef       ..............
17:32:29.905049 00:1f:7b:63:02:08 > d8:58:d7:00:ca:6d, ethertype 802.1Q (0x8100), length 64: vlan 34, p 0, ethertype 0x88f7,
        0x0000:  0402 0000 0000 0000 15f1 2ef4 aa03 5dc0  ..............].
        0x0010:  0000 0000 0000 0000 0000 0000 0000 0007  ................
        0x0020:  0000 dead beef dead beef dead beef       ..............
17:32:29.906571 00:1f:7b:63:02:08 > d8:58:d7:00:ca:6d, ethertype 802.1Q (0x8100), length 64: vlan 34, p 0, ethertype 0x88f7,
        0x0000:  0402 0000 0000 0000 15f1 2ef4 aa12 a000  ................
        0x0010:  0000 0000 0000 0000 0000 0000 0000 0008  ................
        0x0020:  0000 dead beef dead beef dead beef       ..............
17:32:29.907048 00:1f:7b:63:02:08 > d8:58:d7:00:ca:6d, ethertype 802.1Q (0x8100), length 64: vlan 34, p 0, ethertype 0x88f7,
        0x0000:  0402 0000 0000 0000 15f1 2ef4 aa21 e240  .............!.@
        0x0010:  0000 0000 0000 0000 0000 0000 0000 0009  ................
        0x0020:  0000 dead beef dead beef dead beef       ..............
17:32:29.907575 00:1f:7b:63:02:08 > d8:58:d7:00:ca:6d, ethertype 802.1Q (0x8100), length 64: vlan 34, p 0, ethertype 0x88f7,
        0x0000:  0402 0000 0000 0000 15f1 2ef4 aa31 2480  .............1$.
        0x0010:  0000 0000 0000 0000 0000 0000 0000 000a  ................
        0x0020:  0000 dead beef dead beef dead beef       ..............
# ip link set br0 type bridge vlan_filtering 0
# tcpdump -i lan24 -e -n
tcpdump: verbose output suppressed, use -v or -vv for full protocol decode
listening on lan24, link-type EN10MB (Ethernet), capture size 262144 bytes
17:34:39.884637 00:1f:7b:63:02:08 > d8:58:d7:00:ca:6d, ethertype 802.1Q (0x8100), length 64: vlan 0, p 0, ethertype 0x88f7,
        0x0000:  0402 0000 0000 0000 15f1 2f12 ee42 6440  ........../..Bd@
        0x0010:  0000 0000 0000 0000 0000 0000 0000 0001  ................
        0x0020:  0000 dead beef dead beef dead beef       ..............
17:34:39.885968 00:1f:7b:63:02:08 > d8:58:d7:00:ca:6d, ethertype 802.1Q (0x8100), length 64: vlan 0, p 0, ethertype 0x88f7,
        0x0000:  0402 0000 0000 0000 15f1 2f12 ee51 a680  ........../..Q..
        0x0010:  0000 0000 0000 0000 0000 0000 0000 0002  ................
        0x0020:  0000 dead beef dead beef dead beef       ..............
17:34:39.886179 00:1f:7b:63:02:08 > d8:58:d7:00:ca:6d, ethertype 802.1Q (0x8100), length 64: vlan 0, p 0, ethertype 0x88f7,
        0x0000:  0402 0000 0000 0000 15f1 2f12 ee60 e8c0  ........../..`..
        0x0010:  0000 0000 0000 0000 0000 0000 0000 0003  ................
        0x0020:  0000 dead beef dead beef dead beef       ..............
17:34:39.886559 00:1f:7b:63:02:08 > d8:58:d7:00:ca:6d, ethertype 802.1Q (0x8100), length 64: vlan 0, p 0, ethertype 0x88f7,
        0x0000:  0402 0000 0000 0000 15f1 2f12 ee70 2b00  ........../..p+.
        0x0010:  0000 0000 0000 0000 0000 0000 0000 0004  ................
        0x0020:  0000 dead beef dead beef dead beef       ..............
17:34:39.887116 00:1f:7b:63:02:08 > d8:58:d7:00:ca:6d, ethertype 802.1Q (0x8100), length 64: vlan 0, p 0, ethertype 0x88f7,
        0x0000:  0402 0000 0000 0000 15f1 2f12 ee7f 6d40  ........../...m@
        0x0010:  0000 0000 0000 0000 0000 0000 0000 0005  ................
        0x0020:  0000 dead beef dead beef dead beef       ..............
17:34:39.888090 00:1f:7b:63:02:08 > d8:58:d7:00:ca:6d, ethertype 802.1Q (0x8100), length 64: vlan 0, p 0, ethertype 0x88f7,
        0x0000:  0402 0000 0000 0000 15f1 2f12 ee8e af80  ........../.....
        0x0010:  0000 0000 0000 0000 0000 0000 0000 0006  ................
        0x0020:  0000 dead beef dead beef dead beef       ..............
17:34:39.890813 00:1f:7b:63:02:08 > d8:58:d7:00:ca:6d, ethertype 802.1Q (0x8100), length 64: vlan 0, p 0, ethertype 0x88f7,
        0x0000:  0402 0000 0000 0000 15f1 2f12 ee9d f1c0  ........../.....
        0x0010:  0000 0000 0000 0000 0000 0000 0000 0007  ................
        0x0020:  0000 dead beef dead beef dead beef       ..............
17:34:39.892526 00:1f:7b:63:02:08 > d8:58:d7:00:ca:6d, ethertype 802.1Q (0x8100), length 64: vlan 0, p 0, ethertype 0x88f7,
        0x0000:  0402 0000 0000 0000 15f1 2f12 eead 3400  ........../...4.
        0x0010:  0000 0000 0000 0000 0000 0000 0000 0008  ................
        0x0020:  0000 dead beef dead beef dead beef       ..............
17:34:39.893230 00:1f:7b:63:02:08 > d8:58:d7:00:ca:6d, ethertype 802.1Q (0x8100), length 64: vlan 0, p 0, ethertype 0x88f7,
        0x0000:  0402 0000 0000 0000 15f1 2f12 eebc 7640  ........../...v@
        0x0010:  0000 0000 0000 0000 0000 0000 0000 0009  ................
        0x0020:  0000 dead beef dead beef dead beef       ..............
17:34:39.895026 00:1f:7b:63:02:08 > d8:58:d7:00:ca:6d, ethertype 802.1Q (0x8100), length 64: vlan 0, p 0, ethertype 0x88f7,
        0x0000:  0402 0000 0000 0000 15f1 2f12 eecb b880  ........../.....
        0x0010:  0000 0000 0000 0000 0000 0000 0000 000a  ................
        0x0020:  0000 dead beef dead beef dead beef       ..............

The packets that were sent to the switch were the same in both cases.

Strange, huh?

There must be some VLAN retagging going on in the switch between VLAN ID
0 and the port's PVID, but I can't find it.
diff mbox series

Patch

diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index eca285aaf72f..961fa6b75cad 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -1618,9 +1618,6 @@  static int mv88e6xxx_port_check_hw_vlan(struct dsa_switch *ds, int port,
 	struct mv88e6xxx_vtu_entry vlan;
 	int i, err;
 
-	if (!vid)
-		return -EOPNOTSUPP;
-
 	/* DSA and CPU ports have to be members of multiple vlans */
 	if (dsa_is_dsa_port(ds, port) || dsa_is_cpu_port(ds, port))
 		return 0;
@@ -2109,6 +2106,9 @@  static int mv88e6xxx_port_vlan_add(struct dsa_switch *ds, int port,
 	u8 member;
 	int err;
 
+	if (!vlan->vid)
+		return 0;
+
 	err = mv88e6xxx_port_vlan_prepare(ds, port, vlan);
 	if (err)
 		return err;