Skip to content

drivers/enc28j60: register with netdev#15083

Merged
miri64 merged 1 commit intoRIOT-OS:masterfrom
benpicco:drivers/enc28j60-regster
Oct 8, 2020
Merged

drivers/enc28j60: register with netdev#15083
miri64 merged 1 commit intoRIOT-OS:masterfrom
benpicco:drivers/enc28j60-regster

Conversation

@benpicco
Copy link
Contributor

Contribution description

Register enc28j60 so we can use eui_provider to assign the module a MAC address.

Testing procedure

Issues/PRs references

#14634 (comment)

@benpicco benpicco requested a review from jia200x September 24, 2020 16:04
@benpicco benpicco added Area: drivers Area: Device drivers Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Sep 24, 2020
@benpicco benpicco force-pushed the drivers/enc28j60-regster branch from 30992a9 to b1a8b3c Compare September 24, 2020 16:05
@benpicco benpicco added State: waiting for CI update State: The PR requires an Update to CI to be performed first and removed State: waiting for CI update State: The PR requires an Update to CI to be performed first labels Sep 24, 2020
@herjulf
Copy link
Contributor

herjulf commented Sep 24, 2020

Like this. But getting a bit confused ...
Flashed a border-router with avr-rss2/AtMega256RFR2 and enc28j60
Having a EUI64 HW chip: FC:C2:3D:00:00:00:37:DB

Iface  X  HWaddr: 8A:8B:21:3D:38:B0  Link: down 
          inet6 addr: fe80::888b:21ff:fe3d:38b0  scope: link  VAL
  
Iface  Y  HWaddr: 37:DB  Channel: 25  Page: 0  NID: 0xfeed  PHY: O-QPSK 
          Long HWaddr: FC:C2:3D:00:00:00:37:DB 
          inet6 addr: fe80::fec2:3d00:0:37db  scope: link  VAL

A hack we've used sets EUI48 from the EUI64 to get:

Iface  X  HWaddr: FE:C2:3D:00:37:DB  Link: down 
          inet6 addr: fe80::fcc2:3dff:fe00:37db  scope: link  VAL

Comment?

@benpicco
Copy link
Contributor Author

The EUI64 chip only provides an EUI64, no EUI48.
So what you did there could be achieved with

static inline int _at24mac_get_eui48_hack(const void *arg, eui48_t *addr)
{
    eui64_t eui64;
    int res = at24mac_get_eui64((uintptr_t)arg, eui64);
    memcpy(addr, &eui64, sizeof(eui48_t));
    return res;
}

#define EUI48_PROVIDER_FUNC   _at24mac_get_eui48_hack
#define EUI48_PROVIDER_TYPE   NETDEV_ANY
#define EUI48_PROVIDER_INDEX  0

in your board.h.

This will discard the lower bytes of the EUI-64. But keep in mind that now it's no longer unique.

@herjulf
Copy link
Contributor

herjulf commented Sep 24, 2020

I was cherry-picking bits to reuse vendor and use most volatile bits,

  at24mac_get_eui64(0, &eui64);
  /* Mangle the EUI64 into a 48-bit Ethernet address. */
  memcpy(&macbuf[0], &eui64.uint8[0], 3); /* Preserve vendor id */
  memcpy(&macbuf[3], &eui64.uint8[5], 3); /* Use most volatile bits */

@benpicco
Copy link
Contributor Author

benpicco commented Sep 24, 2020

so that would be

static inline int _at24mac_get_eui48_hack(const void *arg, eui48_t *addr)
{
    eui64_t eui64;
    int res = at24mac_get_eui64((uintptr_t)arg, eui64);

    /* Mangle the EUI64 into a 48-bit Ethernet address. */
    memcpy(&addr->uint8[0], &eui64.uint8[0], 3); /* Preserve vendor id */
    memcpy(&addr->uint8[3], &eui64.uint8[5], 3); /* Use most volatile bits */

    return res;
}

@herjulf
Copy link
Contributor

herjulf commented Sep 24, 2020

Yes would prefer that. At least from unique a EUI64 where we double it's use and also stick it to Ethernet module. The volatile bits should give pretty good protection from collisions,

@maribu
Copy link
Member

maribu commented Sep 25, 2020

I think reusing the EUI-64 to also provide EUI-48 might be something we could provide as module, provided we set the bit indicating locally/globally unique address to only locally unique.

@miri64
Copy link
Member

miri64 commented Sep 29, 2020

The EUI64 chip only provides an EUI64, no EUI48.

The EUI-64 is for the radio, right? Then using that one for the Ethernet device seems dangerous in terms of the global uniqueness of the Ethernet device's MAC address. See also https://www.avrfreaks.net/comment/2614676#comment-2614676.

@herjulf
Copy link
Contributor

herjulf commented Sep 29, 2020

Right. ip6 as 6lowpan uses EUI64 The chip can be used for both. Devices that leaves MAC to users are challenging there is a risk unless we buy an address and put on devices. So how mitigate this situation? IMO in the end it's the network administrator or user that is responsible. OS can only give options/tools.

In this case we preserve the vendor id. So so no clashes with other vendors. Also one can meditate a bit over the need for global uniqueness at least nowadays where we partition networks hard and limit link sizes. We mostly keep the link where we have the weak MCU w. ethernet extra isolated/small not be vulnerable from internet. Maribu suggestion mark as locally unique seems correct. We've found this scheme attractive to easily map 802.15.4 and ip4 addresses. Yes including maribu suggestions to make it locally unique and a module.

This particular chip has unique 128 bit and 64 bits but not 48 bits. :(

@benpicco benpicco force-pushed the drivers/enc28j60-regster branch from b1a8b3c to 1afe72a Compare October 8, 2020 08:37
@benpicco benpicco added Area: drivers Area: Device drivers and removed Area: drivers Area: Device drivers labels Oct 8, 2020
@benpicco benpicco requested a review from maribu October 8, 2020 08:37
Copy link
Member

@maribu maribu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK. Code looks good and testing result provided in #15083 (comment)

@miri64 miri64 merged commit 453403c into RIOT-OS:master Oct 8, 2020
@benpicco benpicco deleted the drivers/enc28j60-regster branch October 8, 2020 13:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area: drivers Area: Device drivers CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants