net: ethernet: strip vlan headers if id is zero #83734

clamattia · 2025-01-09T10:16:04Z

Allow to receive vlan frames with id zero even when the vlan interface is disabled. They are forwarded to the native interface or in other words, the vlan header is simply stripped and ignored.

#83662

maass-hamburg · 2025-01-09T10:24:47Z

It might be a good idea to have this behavior behind a Kconfig and not by default. What do the others think?

clamattia · 2025-01-09T10:26:50Z

It might be a good idea to have this behavior behind a Kconfig and not by default. What do the others think?

I do not have a copy of the standard at hand. But it is my understanding, that this is the ieee 802.1q standardized behavior. Can someone confirm?

If so, I believe it should be the hard-coded option and not behind a feature flag.

clamattia · 2025-01-09T10:27:16Z

This would also fix issue #83277 for me.

jukkar · 2025-01-09T11:51:58Z

I could not locate the spec where this is described. The feature is called VLAN 0 priority tagging, some notes about this can be found in
https://en.wikipedia.org/wiki/IEEE_802.1Q
and
https://www.cisco.com/c/en/us/td/docs/switches/connectedgrid/cg-switch-sw-master/software/configuration/guide/vlan0/b_vlan_0.html

We should enable this unconditionally so no Kconfig option for this is needed.

clamattia · 2025-01-09T12:45:57Z

Does someone have time to help with the tests?

jukkar · 2025-01-09T12:59:19Z

include/zephyr/net/ethernet.h

-#if defined(CONFIG_NET_VLAN)
 #define _NET_ETH_MAX_HDR_SIZE		(sizeof(struct net_eth_vlan_hdr))
-#else
-#define _NET_ETH_MAX_HDR_SIZE		(sizeof(struct net_eth_hdr))
-#endif


Why do we need this? If VLAN is disabled, then we only support normal Ethernet header lengths.

The hole point of this PR is to allow to receive VLAN 0 priority tagged frames, even if VLAN is disabled. This can only be achieved, if there is enough space in the header for the priority tagged header.
In practice, if this is not adjusted, the frames with maximum length can not be received (see linked issue in stm driver).

I reverted this. Users, who want to receive priority tagged frames need to enable CONFIG_NET_VLAN now.

jukkar · 2025-01-09T13:05:41Z

subsys/net/l2/ethernet/ethernet.c

-			enum net_verdict verdict;
-
-			net_pkt_set_vlan_tci(pkt, ntohs(hdr_vlan->vlan.tci));
+	if (type == NET_ETH_PTYPE_VLAN) {


If VLAN is disabled, then we should never receive VLAN frames. So with this in mind, I think we do need to have the config option check, no?

I have assumed, that we want to receive priority tagged VLAN even if it is disabled.
An alternative approach would be, that it has to be enabled, but the VLAN_COUNT=0 of actual virtual interfaces is zero. Is this what you have in mind?

Ie. If someone whats to receive priority tagged vlan headers, they need to enable CONFIG_NET_VLAN but they will not have any actual virtual LAN interface?

As mentioned above. I reverted this logic. User must now enable CONFIG_NET_VLAN to receive priority tagged frames.

clamattia · 2025-01-09T13:45:36Z

@jukkar If someone whats to receive vlan 0 priority tagged frames. Should they set:

CONFIG_NET_VLAN=y
CONFIG_NET_VLAN_COUNT=0

?
For now minimum of CONFIG_NET_VLAN_COUNT is 1. I have assumed, that they will always want this feature. Let me know what you think?

Also CONFIG_NET_CONFIG_MY_VLAN_ID defaulting to 0 is likely an issue.

clamattia · 2025-01-10T13:43:10Z

@jukkar If someone whats to receive vlan 0 priority tagged frames. Should they set:
CONFIG_NET_VLAN=y
CONFIG_NET_VLAN_COUNT=0
? For now minimum of CONFIG_NET_VLAN_COUNT is 1. I have assumed, that they will always want this feature. Let me know what you think?

Also CONFIG_NET_CONFIG_MY_VLAN_ID defaulting to 0 is likely an issue.

@jukkar One reason, that speaks against that solution is, that we do not actually need/want vlan.c in the build in this situation. Should vlan.c only be build in, if VLAN_COUNT>0?

I am curious about your preferred way forward?

clamattia · 2025-01-13T13:47:07Z

Idea is now, that configuration to receive only priority tagged frames without actually enabling a virtual interface looks like:

CONFIG_NET_VLAN=y
CONFIG_NET_VLAN_COUNT=0

I will have to test, if this actually works.

Feedback is welcome :)

clamattia · 2025-01-14T13:38:30Z

Testing it leads to a problem.
With CONFIG_NET_VLAN_COUNT=1:

uart:~$ net iface
Hostname: xxx


Interface eth0 (0x20001ac0) (Ethernet) [1]
===================================
Link addr : 00:80:E1:96:0E:73
MTU       : 1500
Flags     : AUTO_START,IPv4
Device    : ethernet@40028000 (0x8076c44)
Ethernet capabilities supported:
        Virtual LAN
        10 Mbits
        100 Mbits
IPv4 unicast addresses (max 1):
        <none>
IPv4 multicast addresses (max 2):
        224.0.0.1
IPv4 gateway : 0.0.0.0

Interface net0 (0x20001ba4) (Virtual) [2]
==================================

With CONFIG_NET_VLAN_COUNT=0 gives:

uart:~$ net iface
Hostname: xxx


Interface eth0 (0x20001ab8) (Ethernet) [1]
===================================
Link addr : 00:80:E1:96:0E:73
MTU       : 1500
Flags     : AUTO_START,IPv4
Device    : ethernet@40028000 (0x8076780)
Ethernet capabilities supported:
	Virtual LAN
	10 Mbits
	100 Mbits
IPv4 not enabled for this interface.

Why does it say Ipv4 not enabled ? Probably CONFIG_NET_VLAN_COUNT=0 is breaking an assumption?

jukkar · 2025-01-14T13:41:03Z

I am not really sure if my understanding of this feature is correct, but I thought about VLAN 0 like this:

The CONFIG_NET_VLAN must be enabled to receive VLAN frames. If VLAN is disabled, then user is not interested in VLAN support so VLAN 0 support is not needed either.
If we receive a VLAN frame with tag value 0, then the packet is fed into the underlying network interface instead of feeding it into VLAN interface.
Normal VLAN traffic (tags > 0) are placed to the proper VLAN interface
The CONFIG_NET_VLAN_COUNT is handled normally, IMHO it should have min value of 1 (because if 0, then basically VLAN is disabled which is not a point here).

WDYT?

jukkar · 2025-01-14T13:43:48Z

subsys/net/l2/ethernet/Kconfig

@@ -41,7 +41,7 @@ config NET_VLAN
 config NET_VLAN_COUNT
 	int "Max VLAN tags supported in the system"
 	default 1
-	range 1 $(UINT8_MAX)
+	range 0 $(UINT8_MAX)


IMHO this is not needed because it basically disables VLAN

See my explenation: #83734 (comment)

I try to find a way to make room for the use case of receiving VLAN 0 frames without having an actual virtual interface enabled. This is my use case.

Hmm, I am not sure you need to enable the VLAN virtual interface. I mean the enabled indicates that the interface has a tag assigned to it. But to receive tag 0, this is not needed.

Hmm, I am not sure you need to enable the VLAN virtual interface. I mean the enabled indicates that the interface has a tag assigned to it. But to receive tag 0, this is not needed.

Are you saying, that I should go back to initial draft of always (without CONFIG_VLAN enabled) forwarding tag 0 to the native interface?

This would be my preferred solution. But it has other consequences. It requires, the increase in the expected header size as in the initial draft for example.

I have reverted. Solution no longer requires CONFIG_VLAN.

clamattia · 2025-01-14T13:47:02Z

The CONFIG_NET_VLAN must be enabled to receive VLAN frames. If VLAN is disabled, then user is not interested in VLAN support so VLAN 0 support is not needed either.

Fair enough. My initial draft wanted to not require the user to select CONFIG_NET_VLAN to have support for VLAN 0. But I can get down with this decision.

If we receive a VLAN frame with tag value 0, then the packet is fed into the underlying network interface instead of feeding it into VLAN interface.

Agree.

Normal VLAN traffic (tags > 0) are placed to the proper VLAN interface

Agree.

The CONFIG_NET_VLAN_COUNT is handled normally, IMHO it should have min value of 1 (because if 0, then basically VLAN is disabled which is not a point here).

Disagree. How else is the user supposed to express their interest in selecting VLAN 0 without having any virtual interface for normal VLAN traffic (tags > 0). This is my usecase. I.E. They want VLAN 0 traffic but no actual proper virtual interface.

jukkar · 2025-01-14T14:43:20Z

They want VLAN 0 traffic but no actual proper virtual interface.

I don't think this is really feasible without major changes (not fully sure about this). Fortunately you do not need to enable the virtual interface to support VLAN 0.

clamattia · 2025-01-14T14:46:30Z

They want VLAN 0 traffic but no actual proper virtual interface.

I don't think this is really feasible without major changes (not fully sure about this). Fortunately you do not need to enable the virtual interface to support VLAN 0.

Ok, will revert to initial draft about this respect. 👍

clamattia · 2025-01-14T15:01:29Z

I have reverted this aspect. Now everyone will be able to receive vlan frames with id 0 on their native interface. No need to enable CONFIG_VLAN for this.

clamattia · 2025-01-14T15:12:16Z

Is it not compliant to reference a config (CONFIG_VLAN) in a comment?

jukkar · 2025-01-15T12:11:39Z

include/zephyr/net/ethernet.h

-#if defined(CONFIG_NET_VLAN)
+/*
+ * Leave enough space in the header for vlan, even if CONFIG_VLAN is not enabled,
+ * so that frames with a vlan-header and vlan-id NET_VLAN_ID_NATIVE can be
+ * forwarded to the native interface.
+ */
 #define _NET_ETH_MAX_HDR_SIZE		(sizeof(struct net_eth_vlan_hdr))
-#else
-#define _NET_ETH_MAX_HDR_SIZE		(sizeof(struct net_eth_hdr))
-#endif


If we do this change (which I think is not needed), then the net_pkt tests would need to be fixed as they are failing because of this.
If CONFIG_NET_VLAN is disabled, then we should not be able to receive Ethernet frames that have the VLAN information in it. So we should not increase the max frame size in that case.

Hmm, I am not sure you need to enable the VLAN virtual interface. I mean the enabled indicates that the interface has a tag assigned to it. But to receive tag 0, this is not needed.

It is needed. Otherwise id 0 frames can not be received.

I can try to fix, but I would like to get your approval on the approach first.

reverted and created separate issue: #84023

clamattia · 2025-01-15T13:30:13Z

As discussed in discord, have reverted to more basic solution and added issue for optimized solution here: #84023

clamattia · 2025-01-20T09:01:00Z

I rebased, please re-review @jukkar

Also, ideally, we would find a solution for #84023

jukkar · 2025-01-21T11:47:26Z

I pushed my proposal over the original PR. It has changes from #84158 in order to make testing possible. The actual VLAN tag 0 handling can be found in the last commit 3ce3777

Packets are forwarded to the native interface or in other words, the vlan header is simply stripped and ignored. This feature is called 'priority tagging'. Signed-off-by: Cla Mattia Galliard <[email protected]> Signed-off-by: Jukka Rissanen <[email protected]>

jukkar · 2025-01-22T06:55:59Z

Added tests for VLAN tag 0
Rebased to latest main

Make sure we are able to receive VLAN tag 0 packet, and verify that reply packet is sent correctly. Signed-off-by: Jukka Rissanen <[email protected]>

zephyrbot added area: Ethernet area: Networking labels Jan 9, 2025

zephyrbot requested review from decsny, jukkar, lmajewski, maass-hamburg, pdgendt, rlubos, ssharks and tbursztyka January 9, 2025 10:16

zephyrbot assigned rlubos and jukkar Jan 9, 2025

clamattia force-pushed the vlan_natively_tagged branch from 3908ebd to a1d8cbc Compare January 9, 2025 10:21

jukkar reviewed Jan 9, 2025

View reviewed changes

clamattia force-pushed the vlan_natively_tagged branch from a1d8cbc to 68ff9bf Compare January 13, 2025 13:39

clamattia requested a review from jukkar January 13, 2025 13:58

jukkar reviewed Jan 14, 2025

View reviewed changes

clamattia requested a review from jukkar January 14, 2025 14:27

clamattia force-pushed the vlan_natively_tagged branch 3 times, most recently from e5f6721 to 1fc86fe Compare January 14, 2025 15:00

jukkar reviewed Jan 15, 2025

View reviewed changes

clamattia force-pushed the vlan_natively_tagged branch from 1fc86fe to da5842f Compare January 15, 2025 13:07

clamattia mentioned this pull request Jan 15, 2025

net: ethernet: allow receive priority tagged vlan frames without needing a virtual interface #84023

Closed

clamattia requested a review from jukkar January 15, 2025 13:16

clamattia force-pushed the vlan_natively_tagged branch from da5842f to 5903359 Compare January 20, 2025 08:58

jukkar force-pushed the vlan_natively_tagged branch from 5903359 to 3ce3777 Compare January 21, 2025 11:42

jukkar force-pushed the vlan_natively_tagged branch from 3ce3777 to 16c4735 Compare January 22, 2025 06:55

tests: net: vlan: Add VLAN tag 0 test

b918a3b

Make sure we are able to receive VLAN tag 0 packet, and verify that reply packet is sent correctly. Signed-off-by: Jukka Rissanen <[email protected]>

jukkar force-pushed the vlan_natively_tagged branch from 16c4735 to b918a3b Compare January 22, 2025 07:36

pdgendt approved these changes Jan 22, 2025

View reviewed changes

maass-hamburg approved these changes Jan 22, 2025

View reviewed changes

rlubos approved these changes Jan 22, 2025

View reviewed changes

kartben merged commit 7488810 into zephyrproject-rtos:main Jan 22, 2025
25 checks passed

This was referenced Jan 23, 2025

net/ethernet/ethernet.c: Handle ieee 802.1q (vlan) headers with id 0 (even if CONFIG_VLAN is not enabled) #83662

Closed

drivers/ethernet/eth_stm32_hal.c: net_pkt_write fails, when packet has size 1518 #83277

Closed

jukkar mentioned this pull request Jan 30, 2025

net: vlan: Allow VLAN count to be set to 0 #84873

Merged

clamattia mentioned this pull request Apr 23, 2025

Contributor Nomination: clamattia #88944

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

net: ethernet: strip vlan headers if id is zero #83734

net: ethernet: strip vlan headers if id is zero #83734

clamattia commented Jan 9, 2025

maass-hamburg commented Jan 9, 2025

clamattia commented Jan 9, 2025

clamattia commented Jan 9, 2025

jukkar commented Jan 9, 2025

clamattia commented Jan 9, 2025

jukkar Jan 9, 2025

clamattia Jan 9, 2025

clamattia Jan 13, 2025

jukkar Jan 9, 2025

clamattia Jan 9, 2025

clamattia Jan 13, 2025

clamattia commented Jan 9, 2025

clamattia commented Jan 10, 2025

clamattia commented Jan 13, 2025 •

edited

Loading

clamattia commented Jan 14, 2025

jukkar commented Jan 14, 2025

jukkar Jan 14, 2025

clamattia Jan 14, 2025

clamattia Jan 14, 2025

jukkar Jan 14, 2025

clamattia Jan 14, 2025

clamattia Jan 14, 2025

clamattia commented Jan 14, 2025 •

edited

Loading

jukkar commented Jan 14, 2025

clamattia commented Jan 14, 2025

clamattia commented Jan 14, 2025

clamattia commented Jan 14, 2025

jukkar Jan 15, 2025

clamattia Jan 15, 2025

clamattia Jan 15, 2025

clamattia commented Jan 15, 2025

clamattia commented Jan 20, 2025

jukkar commented Jan 21, 2025

jukkar commented Jan 22, 2025

net: ethernet: strip vlan headers if id is zero #83734

net: ethernet: strip vlan headers if id is zero #83734

Conversation

clamattia commented Jan 9, 2025

maass-hamburg commented Jan 9, 2025

clamattia commented Jan 9, 2025

clamattia commented Jan 9, 2025

jukkar commented Jan 9, 2025

clamattia commented Jan 9, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clamattia commented Jan 9, 2025

clamattia commented Jan 10, 2025

clamattia commented Jan 13, 2025 • edited Loading

clamattia commented Jan 14, 2025

jukkar commented Jan 14, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clamattia commented Jan 14, 2025 • edited Loading

jukkar commented Jan 14, 2025

clamattia commented Jan 14, 2025

clamattia commented Jan 14, 2025

clamattia commented Jan 14, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clamattia commented Jan 15, 2025

clamattia commented Jan 20, 2025

jukkar commented Jan 21, 2025

jukkar commented Jan 22, 2025

clamattia commented Jan 13, 2025 •

edited

Loading

clamattia commented Jan 14, 2025 •

edited

Loading