Page MenuHomeFreeBSD

dwc(4) Ethernet MAC address setting/generating
Needs ReviewPublic

Authored by bz on Jan 12 2020, 3:03 PM.

Details

Reviewers
br
manu
mmel
kevans
Summary

Currently dwc(4) supports an Ethernet MAC address from hardware (usually set by the boot loader), or by generating a random one with a "BSD" prefix for the OUI.

Change the logic to first try a HW set address, or if that fails check the FDT. If neither works return an error and after if_alloc() in the attach function use ether_gen_addr() to create a (random) Ethernet MAC under the FreeBSD OUI. If we do support getting a cpuid and setting the host uuid based on that the generated MAC should also be stable. This seems to be a better solution than using a pseudo-random on a locally administered OUI.

Setting the "local-mac-address" from loader can be done, e.g., on a NanoPC-T4 with:
fdt mkprop /ethernet@fe300000/local-mac-address [ 0x58 0x9c 0xfc 0xde 0xc0 0xde ]

Test Plan

Cannot test the HW (boot loader) set address currently; the other two options seem to work.

Diff Detail

Repository
rS FreeBSD src repository
Lint
Lint Skipped
Unit
Unit Tests Skipped
Build Status
Buildable 29020

Event Timeline

bz created this revision.Jan 12 2020, 3:03 PM
manu added a comment.Jan 12 2020, 3:09 PM

The bindings (https://github.com/torvalds/linux/blob/master/Documentation/devicetree/bindings/net/ethernet-controller.yaml#L16) seems to indicate
that mac-address should be used if it exists and is prefered to local-mac-address.
Maybe change dwc_get_hwaddr_fdt to handle that and make it a generic helper ?

bz added a comment.Jan 12 2020, 4:30 PM
In D23145#507260, @manu wrote:

The bindings (https://github.com/torvalds/linux/blob/master/Documentation/devicetree/bindings/net/ethernet-controller.yaml#L16) seems to indicate
that mac-address should be used if it exists and is prefered to local-mac-address.
Maybe change dwc_get_hwaddr_fdt to handle that and make it a generic helper ?

Should we support both as USB does here? https://svnweb.freebsd.org/base/head/sys/dev/usb/usb_fdt_support.c?annotate=347974#l100

If it were to become a "generic" function, where would we put it?

manu added a comment.Jan 12 2020, 5:01 PM
In D23145#507266, @bz wrote:
In D23145#507260, @manu wrote:

The bindings (https://github.com/torvalds/linux/blob/master/Documentation/devicetree/bindings/net/ethernet-controller.yaml#L16) seems to indicate
that mac-address should be used if it exists and is prefered to local-mac-address.
Maybe change dwc_get_hwaddr_fdt to handle that and make it a generic helper ?

Should we support both as USB does here? https://svnweb.freebsd.org/base/head/sys/dev/usb/usb_fdt_support.c?annotate=347974#l100

The usb code looks correct. yes.

If it were to become a "generic" function, where would we put it?

No idea :)

kevans added a comment.EditedJan 12 2020, 5:19 PM
In D23145#507267, @manu wrote:
In D23145#507266, @bz wrote:

If it were to become a "generic" function, where would we put it?

No idea :)

I'd be tempted to stick such a thing in ^/sys/dev/fdt/fdt_common.c, since it's (edit: not) hooking much into ethernet stuff -- just determining a MAC address to use for a given node.

manu added a comment.Jan 12 2020, 5:28 PM
In D23145#507267, @manu wrote:
In D23145#507266, @bz wrote:

If it were to become a "generic" function, where would we put it?

No idea :)

I'd be tempted to stick such a thing in ^/sys/dev/fdt/fdt_common.c, since it's (edit: not) hooking much into ethernet stuff -- just determining a MAC address to use for a given node.

Yes that should be ok since we're only adding one net-related function, if we grew more net-related helpers we could always move those to dedicated file.

bz updated this revision to Diff 67464.Wed, Jan 29, 5:48 PM

Update the change factoring out the fdt logic to resemble the USB one.
Is this what you were thinking of?

(currently untested)

bz added a comment.Sun, Feb 16, 4:54 PM

Any comments on this? I'd love to get the change out of my tree soon..
I am also interested in "this is what we were thinking of" comments.

In D23145#520806, @bz wrote:

Any comments on this? I'd love to get the change out of my tree soon..
I am also interested in "this is what we were thinking of" comments.

I did wonder if the common fdt method should guarantee a MAC address and call ether_gen_addr() itself if it couldn't find an applicable fdt prop, maybe with an ENOENT to indicate that it was generated rather than found (0)- this would avoid drivers having to then call ether_gen_addr() themselves if it fails, which I kind of suspect might be the common case. I'm unsure, though.

bz added a comment.Mon, Feb 17, 9:07 AM
In D23145#520806, @bz wrote:

Any comments on this? I'd love to get the change out of my tree soon..
I am also interested in "this is what we were thinking of" comments.

I did wonder if the common fdt method should guarantee a MAC address and call ether_gen_addr() itself if it couldn't find an applicable fdt prop, maybe with an ENOENT to indicate that it was generated rather than found (0)- this would avoid drivers having to then call ether_gen_addr() themselves if it fails, which I kind of suspect might be the common case. I'm unsure, though.

The problem is that in a few cases, such as dwc, we read the HW address before we reset and that is before we allocate an ifp; without an ifp you cannot call ether_gen_addr().
We could restructure a lot of driver code to accommodate this; but the NIC drivers are essentially all following the same logic order.