| Message ID | 20161125153032.14617-2-oliver@schinagl.nl |
|---|---|
| State | Accepted |
| Commit | a40db6d |
| Delegated to: | Joe Hershberger |
| Headers | show |
This patch-series introduces methods to retrieve the MAC address from an onboard EEPROM. The series does a few small cleanups at the start, as either I ran into them while doing this series and fixed them along the way or actually depended on them. If you want to split the smaller ones off into a smaller series I understand, but again, I do somewhat depend on them. A manufacturer wants to produce boards and may even have MAC addresses for boards. Maintaining unique environments on a per-board basis however is horrible. Also this data should be very persistent, and not easily deletable by simply wiping the environment or device tree. Finally there are chips available on the market with a pre-programmed MAC address chips (proms) that a board manufacturer wants to use. Because of this, the MAC needs to be stored be able to read from such an 'external' source. The current idea of the eeprom layout, is to skip the first 8 bytes, so that other information can be stored there if needed, for example a header with some magic to identify the EEPROM. Or equivalent purposes. After those 8 bytes the MAC address follows the first macaddress. The macaddress is appended by a CRC8 byte and then padded to make for nice 8 bytes. Following the first macaddress one can store a second, or a third etc etc mac address. The CRC8 is optional (via a define) but is strongly recommended to have. It helps preventing user error and more importantly, checks if the bytes read are actually a user inserted address. E.g. only writing 1 macaddress into the eeprom but trying to consume 2. For the added tools, I explicitly did not use the wiser eth_validate_ethaddr_str(), eth_parse_enetaddr() and the ARP_HLEN define as it was quite painful (dependancies) to get these functions into the tools. I would suggest to merge as is, and if someone wants to improve these simple tools to use these functions to happily do so. These patches where tested on Olimex OLinuXino Lime1 (A10/A20), Lime2 (NAND and eMMC) and A20-OLinuXino-MICRO-4G variants and have been in use internally on our production systems since v2 of this patch set. To be able to replicate these tests the second series (which will be separate from this set) are needed. Left TODO: If U-Boot configures eth0 and eth1 it inserts these values into the environment. If the fdt then has an ethernet alias for ethernet0, with a MAC address for a different device, lets say eth2) things will not work at so well. I guess that leaves some discussion with a separated improvement patch as a reliable way is needed to match actual u-boot detected devices, with fdt described devices. E.g. is dev->name the same name to the fdt? Can we blindly match here? ======= Changes since v3: * Split off board specific stuff and only modify the generic functions * Make reading of an eeprom available to every board. By default this is unconfigure and thus should just fall through * Clean some minor bits up (ARP_HLEN) and use it more generically * Update the gen_ethaddr_crc as suggested by simon * Let the fixup_ethernet from fdt_common insert mac addresses to the environment for unconfigured devices. There is a small caveat here however as described in the TODO above. * Print the mac address that u-boot assigned to each device. Changes since v2: * Drop the id byte altogether and just mark it as reserved. The 'index' can be used to indicate the interface instead * Addopt the read_rom_hwaddr hooks * Renamed crc8 tool to gen_ethaddr_crc * Improved the layout EEPROM example * Made a function out of the hwaddress writing function in sunxi_emac so it can be re-used as the write_hwaddr net_ops hook. * No longer handle fdt parameters in board.c Changes since v1: * Do not CRC the id byte, move it just after the crc byte. One of the reasons I decided to move it after the crc8 was mostly due to mass generation of MAC + CRC combo's where the ID is still unknown. Also not crc-ing the ID means that it is much easier for a user to change it (via the u-boot i2c cmd line or from within linux) without having to worry about the crc. * Add a generator to convert a MAC address from the input to a MAC + CRC8 on the output.
Hi, On 25.11.2016 16:43, Olliver Schinagl wrote: > This patch-series introduces methods to retrieve the MAC address from an > onboard EEPROM. The series does a few small cleanups at the start, as > either > I ran into them while doing this series and fixed them along the way or > actually depended on them. If you want to split the smaller ones off into a > smaller series I understand, but again, I do somewhat depend on them. > > A manufacturer wants to produce boards and may even have MAC addresses for > boards. Maintaining unique environments on a per-board basis however is > horrible. Also this data should be very persistent, and not easily > deletable > by simply wiping the environment or device tree. Finally there are > chips available on the market with a pre-programmed MAC address chips > (proms) > that a board manufacturer wants to use. Because of this, the MAC needs > to be > stored be able to read from such an 'external' source. > > The current idea of the eeprom layout, is to skip the first 8 bytes, so > that > other information can be stored there if needed, for example a header > with some > magic to identify the EEPROM. Or equivalent purposes. > > After those 8 bytes the MAC address follows the first macaddress. The > macaddress > is appended by a CRC8 byte and then padded to make for nice 8 bytes. > Following > the first macaddress one can store a second, or a third etc etc mac > address. > > The CRC8 is optional (via a define) but is strongly recommended to have. It > helps preventing user error and more importantly, checks if the bytes > read are > actually a user inserted address. E.g. only writing 1 macaddress into > the eeprom > but trying to consume 2. > > For the added tools, I explicitly did not use the wiser > eth_validate_ethaddr_str(), eth_parse_enetaddr() and the ARP_HLEN define > as it > was quite painful (dependancies) to get these functions into the tools. > I would > suggest to merge as is, and if someone wants to improve these simple > tools to > use these functions to happily do so. > > These patches where tested on Olimex OLinuXino Lime1 (A10/A20), Lime2 (NAND > and eMMC) and A20-OLinuXino-MICRO-4G variants and have been in use > internally on our production systems since v2 of this patch set. To be > able to > replicate these tests the second series (which will be separate from > this set) > are needed. > > Left TODO: > If U-Boot configures eth0 and eth1 it inserts these values into the > environment. > If the fdt then has an ethernet alias for ethernet0, with a MAC address > for a > different device, lets say eth2) things will not work at so well. > I guess that leaves some discussion with a separated improvement patch as a > reliable way is needed to match actual u-boot detected devices, with fdt > described devices. E.g. is dev->name the same name to the fdt? Can we > blindly > match here? > > ======= > Changes since v3: > * Split off board specific stuff and only modify the generic functions > * Make reading of an eeprom available to every board. By default this is > unconfigure and thus should just fall through > * Clean some minor bits up (ARP_HLEN) and use it more generically > * Update the gen_ethaddr_crc as suggested by simon > * Let the fixup_ethernet from fdt_common insert mac addresses to the > environment > for unconfigured devices. There is a small caveat here however as > described > in the TODO above. > * Print the mac address that u-boot assigned to each device. > > Changes since v2: > * Drop the id byte altogether and just mark it as reserved. The 'index' > can be > used to indicate the interface instead > * Addopt the read_rom_hwaddr hooks > * Renamed crc8 tool to gen_ethaddr_crc > * Improved the layout EEPROM example > * Made a function out of the hwaddress writing function in sunxi_emac so it > can be re-used as the write_hwaddr net_ops hook. > * No longer handle fdt parameters in board.c > > Changes since v1: > * Do not CRC the id byte, move it just after the crc byte. > One of the reasons I decided to move it after the crc8 was mostly due to > mass > generation of MAC + CRC combo's where the ID is still unknown. Also not > crc-ing > the ID means that it is much easier for a user to change it (via the > u-boot i2c > cmd line or from within linux) without having to worry about the crc. > * Add a generator to convert a MAC address from the input to a MAC + > CRC8 on > the output. I have tested this on zcu102 with mac in eeprom and it is working fine with one mac. I expect you have tested it with more and code looks good in this sense. I would personally prefer to know where that mac address is coming from in output. Thanks, Michal
On Fri, Nov 25, 2016 at 9:30 AM, Olliver Schinagl <oliver@schinagl.nl> wrote: > Commit 674bb249825a ("net: cosmetic: Replace magic numbers in arp.c with > constants") introduced a nice define to replace the magic value 6 for > the ethernet hardware address. Replace more hardcoded instances of 6 > which really reference the ARP_HLEN (iow the MAC/Hardware/Ethernet > address). > > Signed-off-by: Olliver Schinagl <oliver@schinagl.nl> Acked-by: Joe Hershberger <joe.hershberger@ni.com>
Hi oliver@schinagl.nl, https://patchwork.ozlabs.org/patch/699274/ was applied to u-boot-net.git. Thanks! -Joe
diff --git a/common/fdt_support.c b/common/fdt_support.c index 0609470..b082662 100644 --- a/common/fdt_support.c +++ b/common/fdt_support.c @@ -471,7 +471,7 @@ void fdt_fixup_ethernet(void *fdt) char *tmp, *end; char mac[16]; const char *path; - unsigned char mac_addr[6]; + unsigned char mac_addr[ARP_HLEN]; int offset; if (fdt_path_offset(fdt, "/aliases") < 0) diff --git a/include/net.h b/include/net.h index 06320c6..8137cf3 100644 --- a/include/net.h +++ b/include/net.h @@ -38,6 +38,9 @@ #define PKTALIGN ARCH_DMA_MINALIGN +/* ARP hardware address length */ +#define ARP_HLEN 6 + /* IPv4 addresses are always 32 bits in size */ struct in_addr { __be32 s_addr; @@ -90,7 +93,7 @@ enum eth_state_t { */ struct eth_pdata { phys_addr_t iobase; - unsigned char enetaddr[6]; + unsigned char enetaddr[ARP_HLEN]; int phy_interface; int max_speed; }; @@ -161,7 +164,7 @@ void eth_halt_state_only(void); /* Set passive state */ #ifndef CONFIG_DM_ETH struct eth_device { char name[16]; - unsigned char enetaddr[6]; + unsigned char enetaddr[ARP_HLEN]; phys_addr_t iobase; int state; @@ -293,9 +296,9 @@ u32 ether_crc(size_t len, unsigned char const *p); */ struct ethernet_hdr { - u8 et_dest[6]; /* Destination node */ - u8 et_src[6]; /* Source node */ - u16 et_protlen; /* Protocol or length */ + u8 et_dest[ARP_HLEN]; /* Destination node */ + u8 et_src[ARP_HLEN]; /* Source node */ + u16 et_protlen; /* Protocol or length */ }; /* Ethernet header size */ @@ -304,16 +307,16 @@ struct ethernet_hdr { #define ETH_FCS_LEN 4 /* Octets in the FCS */ struct e802_hdr { - u8 et_dest[6]; /* Destination node */ - u8 et_src[6]; /* Source node */ - u16 et_protlen; /* Protocol or length */ - u8 et_dsap; /* 802 DSAP */ - u8 et_ssap; /* 802 SSAP */ - u8 et_ctl; /* 802 control */ - u8 et_snap1; /* SNAP */ + u8 et_dest[ARP_HLEN]; /* Destination node */ + u8 et_src[ARP_HLEN]; /* Source node */ + u16 et_protlen; /* Protocol or length */ + u8 et_dsap; /* 802 DSAP */ + u8 et_ssap; /* 802 SSAP */ + u8 et_ctl; /* 802 control */ + u8 et_snap1; /* SNAP */ u8 et_snap2; u8 et_snap3; - u16 et_prot; /* 802 protocol */ + u16 et_prot; /* 802 protocol */ }; /* 802 + SNAP + ethernet header size */ @@ -323,11 +326,11 @@ struct e802_hdr { * Virtual LAN Ethernet header */ struct vlan_ethernet_hdr { - u8 vet_dest[6]; /* Destination node */ - u8 vet_src[6]; /* Source node */ - u16 vet_vlan_type; /* PROT_VLAN */ - u16 vet_tag; /* TAG of VLAN */ - u16 vet_type; /* protocol type */ + u8 vet_dest[ARP_HLEN]; /* Destination node */ + u8 vet_src[ARP_HLEN]; /* Source node */ + u16 vet_vlan_type; /* PROT_VLAN */ + u16 vet_tag; /* TAG of VLAN */ + u16 vet_type; /* protocol type */ }; /* VLAN Ethernet header size */ @@ -398,7 +401,6 @@ struct arp_hdr { # define ARP_ETHER 1 /* Ethernet hardware address */ u16 ar_pro; /* Format of protocol address */ u8 ar_hln; /* Length of hardware address */ -# define ARP_HLEN 6 u8 ar_pln; /* Length of protocol address */ # define ARP_PLEN 4 u16 ar_op; /* Operation */ @@ -507,16 +509,16 @@ extern char net_nis_domain[32]; /* Our IS domain */ extern char net_hostname[32]; /* Our hostname */ extern char net_root_path[64]; /* Our root path */ /** END OF BOOTP EXTENTIONS **/ -extern u8 net_ethaddr[6]; /* Our ethernet address */ -extern u8 net_server_ethaddr[6]; /* Boot server enet address */ +extern u8 net_ethaddr[ARP_HLEN]; /* Our ethernet address */ +extern u8 net_server_ethaddr[ARP_HLEN]; /* Boot server enet address */ extern struct in_addr net_ip; /* Our IP addr (0 = unknown) */ extern struct in_addr net_server_ip; /* Server IP addr (0 = unknown) */ extern uchar *net_tx_packet; /* THE transmit packet */ extern uchar *net_rx_packets[PKTBUFSRX]; /* Receive packets */ extern uchar *net_rx_packet; /* Current receive packet */ extern int net_rx_packet_len; /* Current rx packet length */ -extern const u8 net_bcast_ethaddr[6]; /* Ethernet broadcast address */ -extern const u8 net_null_ethaddr[6]; +extern const u8 net_bcast_ethaddr[ARP_HLEN]; /* Ethernet broadcast address */ +extern const u8 net_null_ethaddr[ARP_HLEN]; #define VLAN_NONE 4095 /* untagged */ #define VLAN_IDMASK 0x0fff /* mask of valid vlan id */ @@ -555,9 +557,9 @@ extern ushort cdp_appliance_vlan; /* CDP returned appliance VLAN */ */ static inline int is_cdp_packet(const uchar *ethaddr) { - extern const u8 net_cdp_ethaddr[6]; + extern const u8 net_cdp_ethaddr[ARP_HLEN]; - return memcmp(ethaddr, net_cdp_ethaddr, 6) == 0; + return memcmp(ethaddr, net_cdp_ethaddr, ARP_HLEN) == 0; } #endif diff --git a/net/eth-uclass.c b/net/eth-uclass.c index a32961e..a9fc6bb 100644 --- a/net/eth-uclass.c +++ b/net/eth-uclass.c @@ -230,7 +230,7 @@ static int on_ethaddr(const char *name, const char *value, enum env_op op, eth_write_hwaddr(dev); break; case env_op_delete: - memset(pdata->enetaddr, 0, 6); + memset(pdata->enetaddr, 0, ARP_HLEN); } } @@ -458,7 +458,7 @@ static int eth_post_probe(struct udevice *dev) { struct eth_device_priv *priv = dev->uclass_priv; struct eth_pdata *pdata = dev->platdata; - unsigned char env_enetaddr[6]; + unsigned char env_enetaddr[ARP_HLEN]; #if defined(CONFIG_NEEDS_MANUAL_RELOC) struct eth_ops *ops = eth_get_ops(dev); @@ -497,7 +497,7 @@ static int eth_post_probe(struct udevice *dev) eth_getenv_enetaddr_by_index("eth", dev->seq, env_enetaddr); if (!is_zero_ethaddr(env_enetaddr)) { if (!is_zero_ethaddr(pdata->enetaddr) && - memcmp(pdata->enetaddr, env_enetaddr, 6)) { + memcmp(pdata->enetaddr, env_enetaddr, ARP_HLEN)) { printf("\nWarning: %s MAC addresses don't match:\n", dev->name); printf("Address in SROM is %pM\n", @@ -507,7 +507,7 @@ static int eth_post_probe(struct udevice *dev) } /* Override the ROM MAC address */ - memcpy(pdata->enetaddr, env_enetaddr, 6); + memcpy(pdata->enetaddr, env_enetaddr, ARP_HLEN); } else if (is_valid_ethaddr(pdata->enetaddr)) { eth_setenv_enetaddr_by_index("eth", dev->seq, pdata->enetaddr); printf("\nWarning: %s using MAC address from ROM\n", @@ -534,7 +534,7 @@ static int eth_pre_remove(struct udevice *dev) eth_get_ops(dev)->stop(dev); /* clear the MAC address */ - memset(pdata->enetaddr, 0, 6); + memset(pdata->enetaddr, 0, ARP_HLEN); return 0; } diff --git a/net/eth_legacy.c b/net/eth_legacy.c index d6d7cee..2b2c2de 100644 --- a/net/eth_legacy.c +++ b/net/eth_legacy.c @@ -120,7 +120,7 @@ static int on_ethaddr(const char *name, const char *value, enum env_op op, eth_parse_enetaddr(value, dev->enetaddr); break; case env_op_delete: - memset(dev->enetaddr, 0, 6); + memset(dev->enetaddr, 0, ARP_HLEN); } } dev = dev->next; @@ -133,14 +133,14 @@ U_BOOT_ENV_CALLBACK(ethaddr, on_ethaddr); int eth_write_hwaddr(struct eth_device *dev, const char *base_name, int eth_number) { - unsigned char env_enetaddr[6]; + unsigned char env_enetaddr[ARP_HLEN]; int ret = 0; eth_getenv_enetaddr_by_index(base_name, eth_number, env_enetaddr); if (!is_zero_ethaddr(env_enetaddr)) { if (!is_zero_ethaddr(dev->enetaddr) && - memcmp(dev->enetaddr, env_enetaddr, 6)) { + memcmp(dev->enetaddr, env_enetaddr, ARP_HLEN)) { printf("\nWarning: %s MAC addresses don't match:\n", dev->name); printf("Address in SROM is %pM\n", @@ -149,7 +149,7 @@ int eth_write_hwaddr(struct eth_device *dev, const char *base_name, env_enetaddr); } - memcpy(dev->enetaddr, env_enetaddr, 6); + memcpy(dev->enetaddr, env_enetaddr, ARP_HLEN); } else if (is_valid_ethaddr(dev->enetaddr)) { eth_setenv_enetaddr_by_index(base_name, eth_number, dev->enetaddr); @@ -298,7 +298,7 @@ int eth_initialize(void) */ int eth_mcast_join(struct in_addr mcast_ip, int join) { - u8 mcast_mac[6]; + u8 mcast_mac[ARP_HLEN]; if (!eth_current || !eth_current->mcast) return -1; mcast_mac[5] = htonl(mcast_ip.s_addr) & 0xff;
Commit 674bb249825a ("net: cosmetic: Replace magic numbers in arp.c with constants") introduced a nice define to replace the magic value 6 for the ethernet hardware address. Replace more hardcoded instances of 6 which really reference the ARP_HLEN (iow the MAC/Hardware/Ethernet address). Signed-off-by: Olliver Schinagl <oliver@schinagl.nl> --- common/fdt_support.c | 2 +- include/net.h | 52 +++++++++++++++++++++++++++------------------------- net/eth-uclass.c | 10 +++++----- net/eth_legacy.c | 10 +++++----- 4 files changed, 38 insertions(+), 36 deletions(-)