net/link_layer: implement EUI provider#14634
Conversation
benpicco
added
Area: network
benpicco
requested review from
PeterKietzmann,
cgundogan,
daniel-k,
dylad,
haukepetersen,
jia200x,
miri64 and
smlng
as code owners
There are some more boards changed in this PR. What is the reasoning for not requiring to testing them? |
|
I think @chudov is the only one who has a |
|
I can provide photo of |
|
Flash
|
benpicco
added
CI: ready for build
|
Rebased to make use of #14778 |
|
Rebased again as #12600 already includes some of the changes of this PR. |
|
No impact if no openmote-bmasterthis PR |
fjmolinas
left a comment
fjmolinas
left a comment
There was a problem hiding this comment.
I have some questions (to be clear I understand this api correctly):
-
radio provides a EUI: this might imply a dependency to the radio module, would we use
ifndef MODULE_%for this? The provider type would make sure we don't mix them up. -
multiple interfaces on same radio (
at86rf215): there is aNETDEV_AT86RF215type which interface would use the providedeuiwill there be two? -
why use
edbgfor the radiomacaddress? what happens ifethosis used, orusb? We want it to be constant I guess, so which interface to choose? When the radio device provides theeui64this is pretty straightforward, but I'm not clear when it relates to other providers. This point can probably be solved with some doc.
Otherwise I think this looks nice, i'll give it a test now.
fjmolinas
left a comment
fjmolinas
left a comment
There was a problem hiding this comment.
Tested on samr21-xpro it matches the address on the board MAC: 000425191801CCA0
2020-08-25 11:21:35,647 # ifconfig
2020-08-25 11:21:35,654 # Iface 7 HWaddr: 4C:A0 Channel: 26 Page: 0 NID: 0x23 PHY: O-QPSK
2020-08-25 11:21:35,655 #
2020-08-25 11:21:35,660 # Long HWaddr: 00:04:25:19:18:01:CC:A0
2020-08-25 11:21:35,667 # TX-Power: 0dBm State: IDLE max. Retrans.: 3 CSMA Retries: 4
2020-08-25 11:21:35,674 # AUTOACK ACK_REQ CSMA L2-PDU:102 MTU:1280 HL:64 RTR
2020-08-25 11:21:35,676 # 6LO IPHC
2020-08-25 11:21:35,679 # Source address length: 8
2020-08-25 11:21:35,682 # Link type: wireless
2020-08-25 11:21:35,690 # inet6 addr: fe80::204:2519:1801:cca0 scope: link VAL
2020-08-25 11:21:35,691 # inet6 group: ff02::2
2020-08-25 11:21:35,693 # inet6 group: ff02::1
2020-08-25 11:21:35,697 # inet6 group: ff02::1:ff01:cca0
2020-08-25 11:21:35,698 #
2020-08-25 11:21:35,701 # Statistics for Layer 2
2020-08-25 11:21:35,704 # RX packets 0 bytes 0
2020-08-25 11:21:35,708 # TX packets 2 (Multicast: 2) bytes 86
2020-08-25 11:21:35,712 # TX succeeded 2 errors 0
2020-08-25 11:21:35,714 # Statistics for IPv6
2020-08-25 11:21:35,717 # RX packets 0 bytes 0
2020-08-25 11:21:35,722 # TX packets 2 (Multicast: 2) bytes 128
2020-08-25 11:21:35,725 # TX succeeded 2 errors 0
2020-08-25 11:21:35,725 #
ifconfig 7 set state reset
2020-08-25 11:21:56,659 # ifconfig 7 set state reset
2020-08-25 11:21:56,664 # success: set state of interface 7 to RESET
> ifconfig
2020-08-25 11:21:58,561 # ifconfig
2020-08-25 11:21:58,568 # Iface 7 HWaddr: 4C:A0 Channel: 26 Page: 0 NID: 0x23 PHY: O-QPSK
2020-08-25 11:21:58,569 #
2020-08-25 11:21:58,573 # Long HWaddr: 00:04:25:19:18:01:CC:A0
2020-08-25 11:21:58,580 # TX-Power: 0dBm State: IDLE max. Retrans.: 3 CSMA Retries: 4
2020-08-25 11:21:58,587 # AUTOACK ACK_REQ CSMA L2-PDU:102 MTU:1280 HL:64 RTR
2020-08-25 11:21:58,589 # 6LO IPHC
2020-08-25 11:21:58,592 # Source address length: 2
2020-08-25 11:21:58,595 # Link type: wireless
2020-08-25 11:21:58,601 # inet6 addr: fe80::204:2519:1801:cca0 scope: link VAL
2020-08-25 11:21:58,604 # inet6 group: ff02::2
2020-08-25 11:21:58,606 # inet6 group: ff02::1
2020-08-25 11:21:58,610 # inet6 group: ff02::1:ff01:cca0
2020-08-25 11:21:58,611 #
2020-08-25 11:21:58,614 # Statistics for Layer 2
2020-08-25 11:21:58,617 # RX packets 0 bytes 0
2020-08-25 11:21:58,622 # TX packets 4 (Multicast: 4) bytes 172
2020-08-25 11:21:58,625 # TX succeeded 4 errors 0
2020-08-25 11:21:58,628 # Statistics for IPv6
2020-08-25 11:21:58,631 # RX packets 0 bytes 0
2020-08-25 11:21:58,635 # TX packets 4 (Multicast: 4) bytes 256
2020-08-25 11:21:58,638 # TX succeeded 4 errors 0
2020-08-25 11:21:58,639 #
perfect!
What I meant with this question is what was the reasoning behind this, I agree with this matching, I just think it would be nice to document, so that others adding this module will follow the same reasoning (at least for the default configuration that goes in master). e.g.:
what do you think? |
benpicco
removed
the
CI: ready for build
|
I extended the documentation. I hope it's a bit more understandable now :) |
Looks good! |
|
Some more testing:
|
|
So I took a look at #12641 where this PR comes from. I'm trying to figure out if it fulfills requirements expressed there. From #12641 (comment), I think all points are achieved since the @miri64 concern #12641 (review) seems to have been addressed with #13746. Also should https://github.com/RIOT-OS/RIOT/pull/13232/files be adapted as well? @maribu @miri64 can you confirm that this new attempt fits your expectations? |
Converting the remaining netdevs to register themselves and use the But the old luid approach will keep working, so there is no urgency in the transition. |
This adds the possibility to define EUI sources and assign them to a network device.
benpicco
added
the
CI: ready for build
|
no answer :P |
|
@herjulf will be happy about this too 😄 |
|
Oh yes. Thanks! |
| static inline void eui_short_from_eui64(eui64_t *addr_long, | ||
| network_uint16_t *addr_short) | ||
| { | ||
| /* https://tools.ietf.org/html/rfc4944#section-12 requires the first bit to | ||
| * 0 for unicast addresses */ | ||
| addr_short->u8[0] = addr_long->uint8[6] & 0x7F; | ||
| addr_short->u8[1] = addr_long->uint8[7]; | ||
| } |
There was a problem hiding this comment.
I'm realizing just now that this is not implementing the same workaround to have unique short address that is done for luid_get_short().
luid_base(addr, sizeof(*addr));
addr->u8[1] ^= lastused++;
/* https://tools.ietf.org/html/rfc4944#section-12 requires the first bit to
* 0 for unicast addresses */
addr->u8[0] &= 0x7F;
Should this call luid_get_short instead? OR change it to:
static inline void eui_short_from_eui64(eui64_t *addr_long,
network_uint16_t *addr_short)
{
/* https://tools.ietf.org/html/rfc4944#section-12 requires the first bit to
* 0 for unicast addresses */
addr_short->u8[0] = addr_long->uint8[0] & 0x7F;
addr_short->u8[1] ^= addr_long->uint8[1];
}
There was a problem hiding this comment.
Huh? Byte 7 will change if we got the EUI64 from luid and there are multiple interfaces
void luid_get_eui64(eui64_t *addr)
{
luid_base(addr, sizeof(*addr));
addr->uint8[7] ^= lastused++;
eui64_set_local(addr);
eui64_clear_group(addr);
}
void luid_netdev_get_eui64(const netdev_t *netdev, eui64_t *addr)
{
luid_base(addr, sizeof(*addr));
#ifdef MODULE_NETDEV_REGISTER
addr->uint8[6] ^= netdev->type;
addr->uint8[7] ^= netdev->index;
#else
/* we should only get here with gnrc_netif_single */
(void)netdev;
#endif
eui64_set_local(addr);
eui64_clear_group(addr);
}There was a problem hiding this comment.
I think its the same issue described in #13358, on multiple iotl-lab-m3 they all get the same short-addres 13:15 .
There was a problem hiding this comment.
Huh? Byte 7 will change if we got the EUI64 from luid and there are multiple interfaces
I'm talking about uniqueness across platforms, not interfaces.
There was a problem hiding this comment.
@benpicco do you have an issue with the proposed change?
static inline void eui_short_from_eui64(eui64_t *addr_long,
network_uint16_t *addr_short)
{
/* https://tools.ietf.org/html/rfc4944#section-12 requires the first bit to
* 0 for unicast addresses */
addr_short->u8[0] = addr_long->uint8[0] & 0x7F;
addr_short->u8[1] ^= addr_long->uint8[1];
}There was a problem hiding this comment.
@aabadie do you have some input on the m3? Could we do this?
There was a problem hiding this comment.
I haven't followed all the discussion in detail here. What I see is that there's a regression introduced on iotlab-m3. Why is it not possible to compute the EUI-64 from cpuid and eventually the interface ?
There was a problem hiding this comment.
Why is it not possible to compute the EUI-64 from cpuid and eventually the interface ?
This is what happens, but it seems like the last bytes of the generated EUI-64 are not very rich in entropy.
There was a problem hiding this comment.
This is what happens, but it seems like the last bytes of the generated EUI-64 are not very rich in entropy.
Then why the logic from #13362 was changed ?
There was a problem hiding this comment.
#13362 replaced the logic in the driver with a common helper function.
That it 'solved' this problem was by accident.
The root cause is that the CPU ID (especially the uniform parts) is reflected too directly in the generated ID.
How about we shuffle the bytes a bit?
|
Hmm |
|
Sure! See #15083 |
6 participants
Contribution description
This implements EUI providers that can be implemented by a board to provide EUIs (MAC addresses) to network interfaces.
Testing procedure
Flash
examples/gnrc_networkingonsamr21-xpro.You can observe that the
Long HWaddrinifconfignow matches the MAC address printed on the reverse of the board.Flash
examples/gnrc_networkingonavr-rss2.You can observe that the
Long HWaddrinifconfignow matches the MAC address printed on the reverse of the board.Issues/PRs references
split off & waiting for #13746