Allow gPTP over VLAN #8615

jukkar · 2018-06-28T13:58:28Z

No description provided.

tbursztyka

Why can't this eth header filling be factorized? it's the same exact code copy pasted all over.

well in short having a higher level protocol messing up with link layer is a super mess. It's what we've been doing so far and that's going to disappear (it has to, better have a single place for filling in l2 header than scattered everywhere).

tbursztyka · 2018-06-28T14:05:03Z

subsys/net/l2/ethernet/gptp/gptp_messages.c

 	NET_ASSERT(iface);

+#if defined(CONFIG_NET_VLAN)
+	eth_ctx = net_if_l2_data(iface);


shouldn't you be using net_eth_is_vlan_enabled() here?

Btw, I can't recall why net_eth_is_vlan_enabled() has to take the context in parameter? Since we pass the iface, it can get the context. Or is this function meant to be used with a context that does not belong to the iface? (which sounds a bit weird)

There can be more than one (vlan) interface specified in one context.

tbursztyka · 2018-06-28T14:06:00Z

subsys/net/l2/ethernet/gptp/gptp_messages.c

 	struct gptp_hdr *hdr;

+#if defined(CONFIG_NET_VLAN)
+	struct net_eth_vlan_hdr *hdr_vlan;


move that after line 202

Hmm, I think that is against the coding style. Meaning we should introduce variables at the beginning of the function.

no it's not against if you do:

if (net_eth_get_vlan_info(iface, &eth_len)) {
struct net_eth_vlan_hdr *hdr_vlan;
...
}

Actually all can go in that if, no need of the vlan_enabled at all.

Just move all that at line 213

This cannot be done because hdr_vlan is filled later in the function. For that we also need the vlan_enabled flag.

tbursztyka · 2018-06-28T14:06:11Z

subsys/net/l2/ethernet/gptp/gptp_messages.c


+#if defined(CONFIG_NET_VLAN)
+	if (vlan_enabled) {
+		hdr_vlan = (struct net_eth_vlan_hdr *)NET_ETH_HDR(pkt);


move that after line 202

tbursztyka · 2018-06-28T14:07:16Z

subsys/net/l2/ethernet/gptp/gptp_messages.c

 	struct net_buf *frag;

+#if defined(CONFIG_NET_VLAN)
+	struct net_eth_vlan_hdr *hdr_vlan;


same, move after line 317

tbursztyka · 2018-06-28T14:07:24Z

subsys/net/l2/ethernet/gptp/gptp_messages.c

+#if defined(CONFIG_NET_VLAN)
+	if (vlan_enabled) {
+		hdr_vlan = (struct net_eth_vlan_hdr *)NET_ETH_HDR(pkt);
+		net_pkt_set_vlan_tag(pkt, net_pkt_vlan_tag(sync));


move these 2 lines after 317

tbursztyka · 2018-06-28T14:07:46Z

subsys/net/l2/ethernet/gptp/gptp_messages.c

 	struct gptp_hdr *hdr;

+#if defined(CONFIG_NET_VLAN)
+	struct net_eth_vlan_hdr *hdr_vlan;


well you know the song now.

codecov-io · 2018-06-28T14:27:22Z

Codecov Report

Merging #8615 into master will decrease coverage by 0.01%.
The diff coverage is 33.33%.

@@            Coverage Diff             @@
##           master    #8615      +/-   ##
==========================================
- Coverage   52.24%   52.22%   -0.02%     
==========================================
  Files         195      195              
  Lines       24655    24673      +18     
  Branches     5123     5124       +1     
==========================================
+ Hits        12880    12886       +6     
- Misses       9709     9721      +12     
  Partials     2066     2066

Impacted Files	Coverage Δ
include/net/ethernet.h	`38.46% <ø> (ø)`	⬆️
subsys/net/l2/ethernet/ethernet_mgmt.c	`43.85% <0%> (-5.16%)`	⬇️
include/net/ethernet_mgmt.h	`50% <100%> (+50%)`	⬆️
subsys/net/l2/ethernet/ethernet.c	`51.22% <25%> (-0.66%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 536d77a...dfe3194. Read the comment docs.

Send network management event if VLAN tag is enabled or disabled. Signed-off-by: Jukka Rissanen <[email protected]>

tbursztyka · 2018-07-03T12:55:16Z

subsys/net/l2/ethernet/ethernet_mgmt.c

+void ethernet_mgmt_raise_vlan_enabled_event(struct net_if *iface, u16_t tag)
+{
+#if defined(CONFIG_NET_MGMT_EVENT_INFO)
+	net_mgmt_event_notify_with_info(NET_EVENT_ETHERNET_VLAN_TAG_ENABLED,


Do you really need that version? Because it's possible to retrieve the tag from the iface right? So then no need to give it through the mgmt interface as well.

For enabled event we could get it from iface, but that would not work for disabled event. Thus in order to keep the functions looking similar, I opted to have the tag in the enabled API.

tbursztyka · 2018-07-03T12:58:53Z

subsys/net/l2/ethernet/gptp/gptp_messages.c

 	struct gptp_hdr *hdr;

+#if defined(CONFIG_NET_VLAN)
+	struct net_eth_vlan_hdr *hdr_vlan;


no it's not against if you do:

if (net_eth_get_vlan_info(iface, &eth_len)) {
struct net_eth_vlan_hdr *hdr_vlan;
...
}

Actually all can go in that if, no need of the vlan_enabled at all.

Just move all that at line 213

tbursztyka · 2018-07-03T12:59:40Z

subsys/net/l2/ethernet/ethernet.c

I don't like the fact it does 2 things: saying it's vlan enabled and returning the eth header size.

Lets get rid of that second parameter, it is not really needed.

tbursztyka · 2018-07-03T13:03:42Z

subsys/net/l2/ethernet/gptp/gptp_messages.c

here it would be:

if (IS_ENABLED(CONFIG_NET_GPTP_VLAN)) { setup_eth_vlan_hdr(iface, pkt); } else { eth->type = htons(NET_ETH_PTYPE_PTP); }

tbursztyka · 2018-07-03T13:06:44Z

subsys/net/l2/ethernet/gptp/gptp_messages.c

line 166:

frag = gptp_get_frag() and make that function, aware of vlan so reserving the right amount etc...
Then no need of eth_len being set here if vlan or not. This logic is repeated all over below, so it's worth getting 1 place doing it.

jukkar · 2018-07-03T14:08:13Z

More refactoring done

A small helper function will return information whether a given network interface has VLAN enabled or not. Signed-off-by: Jukka Rissanen <[email protected]>

Allow this setup as Linux supports this too. Signed-off-by: Jukka Rissanen <[email protected]>

By default gPTP is not run over VLAN but if needed that can be done by setting CONFIG_NET_GPTP_VLAN and CONFIG_NET_GPTP_VLAN_TAG options. Signed-off-by: Jukka Rissanen <[email protected]>

tbursztyka

Yeah now it's just right

jukkar added the area: Networking label Jun 28, 2018

jukkar requested review from pfalcon and tbursztyka as code owners June 28, 2018 13:58

tbursztyka requested changes Jun 28, 2018

View reviewed changes

net: mgmt: Add VLAN enabled / disabled event support

fc86c8a

Send network management event if VLAN tag is enabled or disabled. Signed-off-by: Jukka Rissanen <[email protected]>

jukkar force-pushed the gptp-2 branch from 2ef1106 to 3747064 Compare July 3, 2018 12:32

tbursztyka requested changes Jul 3, 2018

View reviewed changes

jukkar force-pushed the gptp-2 branch from 3747064 to cb73318 Compare July 3, 2018 14:08

jukkar added 3 commits July 3, 2018 17:26

net: eth: Add helper to return VLAN info for an interface

04533d2

A small helper function will return information whether a given network interface has VLAN enabled or not. Signed-off-by: Jukka Rissanen <[email protected]>

net: gptp: Allow gPTP to run over VLAN

2ea942e

Allow this setup as Linux supports this too. Signed-off-by: Jukka Rissanen <[email protected]>

samples: net: gptp: Allow running gPTP over VLAN

dfe3194

By default gPTP is not run over VLAN but if needed that can be done by setting CONFIG_NET_GPTP_VLAN and CONFIG_NET_GPTP_VLAN_TAG options. Signed-off-by: Jukka Rissanen <[email protected]>

jukkar force-pushed the gptp-2 branch from cb73318 to dfe3194 Compare July 3, 2018 14:30

tbursztyka approved these changes Jul 3, 2018

View reviewed changes

jukkar merged commit ad15056 into zephyrproject-rtos:master Jul 3, 2018

jukkar mentioned this pull request Jul 3, 2018

drivers: eth: native_posix: Enable gPTP support #8620

Merged

jukkar deleted the gptp-2 branch July 20, 2018 19:09

Allow gPTP over VLAN #8615

Allow gPTP over VLAN #8615

Uh oh!

Conversation

jukkar commented Jun 28, 2018

Uh oh!

tbursztyka left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jukkar Jul 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov-io commented Jun 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tbursztyka Jul 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jukkar commented Jul 3, 2018

Uh oh!

tbursztyka left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jukkar Jul 2, 2018 •

edited

Loading

codecov-io commented Jun 28, 2018 •

edited

Loading

tbursztyka Jul 3, 2018 •

edited

Loading