drivers/at86rf2xx: Fix L2 addr genration/handling

[maribu]

Contribution description

• Moved address generation to at86rf2xx_setup() to generate the L2 address only once
  • Resets will no longer change the L2 address
• Do not use uint16_t or uint64_t for short/long addresses to avoid confusion between host and network byte order
• No longer shuffling bytes of the LUID generated for use as L2 address:
  • Blindly setting/clearing the bits marking globally unique L2 addresses and multicast addresses no longer changes the byte luid_get() mutates, so no collisions of locally generated L2 addresses can occur any more
  • I still think it is a poor design decision to manipulate a LUID by hand and expect it to still be locally unique, but at least now it is (in the context of the current implementation of luid_get()) true

Testing procedure

• Check if two notes with an at86rf2xx transceiver can still communicate
• Check if the remain able to communicate after multiple resets
• Check if two at86rf2xx transceivers (of the same type!) attached on the same board now get two different L2 addresses.
• Check if ifconfig still provides the same output except for the L2 addresses

Issues/PRs references

Fixes #9656

[miri64]

* Check if `ifconfig` still provides the same output.

I added this test case to check if NETOPT is formatted properly.

[maribu]

Note that opaque byte arrays received from luid_get are no longer shuffled arround, so on little endian systems the address will be "the other way round" (plus 4 bits changed). On the plus side, the same CPU ID will now generate the same L2 address regardless of endianess; which makes sense to me.

The change of the L2 addresses is pretty bad in OTA updates. But having in mind that a the way they were generated could result in e.g. four at86rf233 transceivers on the same board getting the exact same L2 addresses up to now, the generation of the addresses needed to be fixed. And I saw no value in having the L2 addresses being almost identical except for the colliding bits, so I optimised the byte order conversion of the byte array out. Maybe it is even better if the change in L2 addresses is more visible, than a sneaky change of only the colliding bits.

[miri64]

Note that opaque byte arrays received from luid_get are no longer shuffled arround, so on little endian systems the address will be "the other way round" (plus 4 bits changed). On the plus side, the same CPU ID will now generate the same L2 address regardless of endianess; which makes sense to me.

Yes, and this is a problem, because now they are not exposed as legal EUI-64s to the upper layers any more (because you don't reorder them on get()/set(). Because of this, link-local address resolution in 6LoWPAN will fail as it relies on the address being in the correct order.

[maribu]

Didn't we agree that byte arrays do not have a byte order?

[miri64]

Didn't we agree that byte arrays do not have a byte order?

Yes, but it still matters which order you hand it up. The bits you are flipping after luid_get() are called the U/L bit and the G/L bit. They have significance in an EUI-64 and they have significance in the IID of an IPv6 address (where the U/L logic is flipped). If you put them now at the end of the byte array, and IPv6 and 6LoWPAN expect them at the beginning, this will lead problems we fought very hard in the past to get rid of.

[miri64]

Or to say it differently: by deciding to flip the bits of the last bytes you made your byte array an EUI-64 in little endian-order and the last byte the MSB. This isn't a problem as long as you just interact with the device this way. But when you hand up the long address to another layer it will become a problem, as the EUI-64 is now in the wrong order.

See IEEE guidelines of use of EUI, RFC 4291, RFC 4944, RFC 6775,

[maribu]

If there is no byte order in a byte array, it cannot be in the wrong order. So I assume what you are saying is, that I flipped the wrong bits in the address. But I didn't change the bits that were flipped.

Here the relevant part of the code in master:

     /* make sure we mark the address as non-multicast and not globally unique */ 
     addr_long.uint8[0] &= ~(0x01);                                               
     addr_long.uint8[0] |=  (0x02);                                               
    /* set short and long address */                                             
     at86rf2xx_set_addr_long(dev, ntohll(addr_long.uint64.u64));

With ntohll this will move the byte previously at index zero to index 7.

[miri64]

Here the relevant part of the code in master:

     /* make sure we mark the address as non-multicast and not globally unique */ 
     addr_long.uint8[0] &= ~(0x01);                                               
     addr_long.uint8[0] |=  (0x02);                                               
    /* set short and long address */                                             
     at86rf2xx_set_addr_long(dev, ntohll(addr_long.uint64.u64));

With ntohll this will move the byte previously at index zero to index 7.

Concerning this part of the code you are right, but at the line I commented in #12600 (comment), you hand the EUI-64 in little endian order (as the last byte got its bits flipped it became the MSB) up the stack, but the doc of NETOPT_ADDRESS_LONG states it must be return in network byte order (aka big endian order).

[benpicco]

Maybe the general confusion could be reduced by fixing the insanity contained inside the at86rf2xx_set_addr_long() function…

[maribu]

Maybe the general confusion could be reduced by fixing the insanity contained inside the at86rf2xx_set_addr_long() function…

Thought the same and did so! 😆

[maribu]

Concerning this part of the code you are right

So the old driver generated the L2 address in big endian, converted it to little endian prior to the call to at86rf2xx_set_addr_long(), in which it was converted back to big endian?

[miri64]

Concerning this part of the code you are right

So the old driver generated the L2 address in big endian, converted it to little endian prior to the call to at86rf2xx_set_addr_long(), in which it was converted back to big endian?

Yes... it's stupid.. That much we agree on.

[miri64]

The only thing I wanted from the original state of the PR is that this patch is applied

diff --git a/drivers/at86rf2xx/at86rf2xx_netdev.c b/drivers/at86rf2xx/at86rf2xx_netdev.c
index 0532c7571..133729cc2 100644
--- a/drivers/at86rf2xx/at86rf2xx_netdev.c
+++ b/drivers/at86rf2xx/at86rf2xx_netdev.c
@@ -473,11 +473,16 @@ static int _set(netdev_t *netdev, netopt_t opt, const void *val, size_t len)
             at86rf2xx_set_addr_short(dev, val);
             /* don't set res to set netdev_ieee802154_t::short_addr */
             break;
-        case NETOPT_ADDRESS_LONG:
+        case NETOPT_ADDRESS_LONG: {
+            uint64_t addr;
+            const eui64_t *eui64 = val;
+
             assert(len <= sizeof(uint64_t));
-            at86rf2xx_set_addr_long(dev, val);
+            addr = byteorder_ntohll(eui64->uint64);
+            at86rf2xx_set_addr_long(dev, (uint64_t *)&addr);
             /* don't set res to set netdev_ieee802154_t::long_addr */
             break;
+        }
         case NETOPT_NID:
             assert(len <= sizeof(uint16_t));
             at86rf2xx_set_pan(dev, *((const uint16_t *)val));

and dev->long_addr is in the order expected by netopt and netdev_ieee802154 -.-

[miri64]

I don't really care what happens internally in the device [edit]as long as it works ;-)[/edit], but the network stack expects a long address where the U/L and G/L bits are in the first byte (when read as byte array). This needs to happen, everything else I don't care about ;-).

[maribu]

If I understood you correct, the driver now sets the U/L and G/L bits correctly, right? So no need to shuffle bytes around any more, right?

[maribu]

Sorry, forgot to push.

[miri64]

Have you tested pinging with IPv6 to a link-local address? I'm not sure the address is now configured internally.

[fjmolinas]

@maribu how much of your test procedure still apply here?

[maribu]

I rebased to resolve merge conflicts and updated to use the common ieee802154_setup() function for the address generation as well. As the alignment mismatch between uint8_t * and eui64_t * is not actually present (eui64_t has an __attribute__((packed)) buried somewhere in its definition), I just updated the code to cast between those.

[maribu]

All green

[maribu]

Thanks for the reviews :-)