[U-Boot] [PATCH] pxe: get ethaddr from the current device instead of env
From: Rob Herring rob.herring@calxeda.com
The env variable "ethaddr" may not be set, so get the address from the eth_device struct instead. This also enables pxe for secondary ethernet devices.
Signed-off-by: Rob Herring rob.herring@calxeda.com --- common/cmd_pxe.c | 31 ++++++++++++------------------- 1 files changed, 12 insertions(+), 19 deletions(-)
diff --git a/common/cmd_pxe.c b/common/cmd_pxe.c index 3efd700..f14ef89 100644 --- a/common/cmd_pxe.c +++ b/common/cmd_pxe.c @@ -56,38 +56,31 @@ static char *from_env(char *envvar) */ static int format_mac_pxe(char *outbuf, size_t outbuf_len) { - size_t ethaddr_len; - char *p, *ethaddr; - - ethaddr = from_env("ethaddr"); - - if (!ethaddr) - return -ENOENT; - - ethaddr_len = strlen(ethaddr); + char *p; + struct eth_device *dev;
/* * ethaddr_len + 4 gives room for "01-", ethaddr, and a NUL byte at * the end. */ - if (outbuf_len < ethaddr_len + 4) { - printf("outbuf is too small (%d < %d)\n", - outbuf_len, ethaddr_len + 4); - + if (outbuf_len < 21) { + printf("outbuf is too small (%d < 21)\n", outbuf_len); return -EINVAL; }
- strcpy(outbuf, "01-"); + eth_set_current(); + dev = eth_get_dev(); + if (!dev) + return -ENODEV;
- for (p = outbuf + 3; *ethaddr; ethaddr++, p++) { - if (*ethaddr == ':') + sprintf(outbuf, "01-%pM", dev->enetaddr); + for (p = outbuf + 3; *p; p++) { + if (*p == ':') *p = '-'; else - *p = tolower(*ethaddr); + *p = tolower(*p); }
- *p = '\0'; - return 1; }
On Monday 05 December 2011 19:04:33 Rob Herring wrote:
The env variable "ethaddr" may not be set, so get the address from the eth_device struct instead. This also enables pxe for secondary ethernet devices.
NAK: this won't work either ;). the API contract is that the env is the canonical storage, and eth_device->enetaddr is there only for the network device drivers themselves: they seed it during registration, and then use it when starting up as the common layer has taken care of syncing from the env.
what you probably want to do is update net/eth.c's eth_register() to see if dev->enetaddr is valid (is_valid_ether_addr), and if the env addr is not set for that device, and if not, set the env with it.
then you can assume in cmd_pxe.c that the env is setup. -mike
Dear Rob Herring,
In message 1323129873-8786-1-git-send-email-robherring2@gmail.com you wrote:
The env variable "ethaddr" may not be set, so get the address from the eth_device struct instead. This also enables pxe for secondary ethernet devices.
This is not correct. The "ethaddr" environment variable shall always take precedence. Feel free to _additionally_ test other places that store this information. If the "ethaddr" environment variable is not set, then this alternative value can be used as default. In this case a warning should be printed. If both settings exist and differ, the "ethaddr" environment variable shall be used, and a warning be printed, too.
Best regards,
Wolfgang Denk
From: Rob Herring rob.herring@calxeda.com
If the net driver has setup a valid ethernet address and an ethernet address is not set in the environment already, then set the environment variables from the net driver setting
This enables pxe booting on boards which don't set ethaddr env variable.
Signed-off-by: Rob Herring rob.herring@calxeda.com --- v2: - Re-wrote to always setup ethaddr env variables
doc/README.enetaddr | 4 +++- net/eth.c | 19 ++++++++++++++++--- 2 files changed, 19 insertions(+), 4 deletions(-)
diff --git a/doc/README.enetaddr b/doc/README.enetaddr index 2d8e24f..6c61817 100644 --- a/doc/README.enetaddr +++ b/doc/README.enetaddr @@ -32,7 +32,9 @@ Correct flow of setting up the MAC address (summarized):
1. Read from hardware in initialize() function 2. Read from environment in net/eth.c after initialize() -3. Give priority to the value in the environment if a conflict +3. Write value to environment if setup in struct eth_device->enetaddr by driver + initialize() function. Give priority to the value in the environment if a + conflict. 4. Program the address into hardware if the following conditions are met: a) The relevant driver has a 'write_addr' function b) The user hasn't set an 'ethmacskip' environment variable diff --git a/net/eth.c b/net/eth.c index 4280d6d..e3d8777 100644 --- a/net/eth.c +++ b/net/eth.c @@ -62,6 +62,15 @@ int eth_getenv_enetaddr_by_index(const char *base_name, int index, return eth_getenv_enetaddr(enetvar, enetaddr); }
+int eth_setenv_enetaddr_by_index(const char *base_name, int index, + uchar *enetaddr) +{ + char enetvar[32]; + sprintf(enetvar, index ? "%s%daddr" : "%saddr", base_name, index); + return eth_setenv_enetaddr(enetvar, enetaddr); +} + + static int eth_mac_skip(int index) { char enetvar[15]; @@ -190,9 +199,13 @@ int eth_write_hwaddr(struct eth_device *dev, const char *base_name, unsigned char env_enetaddr[6]; int ret = 0;
- if (!eth_getenv_enetaddr_by_index(base_name, eth_number, env_enetaddr)) - return -1; - + if (!eth_getenv_enetaddr_by_index(base_name, eth_number, env_enetaddr)) { + if (!is_valid_ether_addr(dev->enetaddr)) + return -1; + eth_setenv_enetaddr_by_index(base_name, eth_number, + dev->enetaddr); + memcpy(env_enetaddr, dev->enetaddr, 6); + } if (memcmp(env_enetaddr, "\0\0\0\0\0\0", 6)) { if (memcmp(dev->enetaddr, "\0\0\0\0\0\0", 6) && memcmp(dev->enetaddr, env_enetaddr, 6)) {
Dear Rob Herring,
In message 1323199478-21001-1-git-send-email-robherring2@gmail.com you wrote:
From: Rob Herring rob.herring@calxeda.com
If the net driver has setup a valid ethernet address and an ethernet address is not set in the environment already, then set the environment variables from the net driver setting
This enables pxe booting on boards which don't set ethaddr env variable.
Signed-off-by: Rob Herring rob.herring@calxeda.com
v2:
- Re-wrote to always setup ethaddr env variables
doc/README.enetaddr | 4 +++- net/eth.c | 19 ++++++++++++++++--- 2 files changed, 19 insertions(+), 4 deletions(-)
I asked before:
... If the "ethaddr" environment variable is not set, then this alternative value can be used as default. In this case a warning should be printed. If both settings exist and differ, the "ethaddr" environment variable shall be used, and a warning be printed, too.
I do not see any wanings in your patch?
Best regards,
Wolfgang Denk
From: Rob Herring rob.herring@calxeda.com
If the net driver has setup a valid ethernet address and an ethernet address is not set in the environment already, then set the environment variables from the net driver setting.
This enables pxe booting on boards which don't set ethaddr env variable.
Signed-off-by: Rob Herring rob.herring@calxeda.com --- Wolfgang,
This now prints a warning. If both env and device mac's are set, then the behavior is unchanged and the env setting is used.
v3: - print a warning if using mac address from the net device
v2: - Re-wrote to always setup ethaddr env variables
Rob
doc/README.enetaddr | 4 +++- net/eth.c | 21 ++++++++++++++++++--- 2 files changed, 21 insertions(+), 4 deletions(-)
diff --git a/doc/README.enetaddr b/doc/README.enetaddr index 2d8e24f..6c61817 100644 --- a/doc/README.enetaddr +++ b/doc/README.enetaddr @@ -32,7 +32,9 @@ Correct flow of setting up the MAC address (summarized):
1. Read from hardware in initialize() function 2. Read from environment in net/eth.c after initialize() -3. Give priority to the value in the environment if a conflict +3. Write value to environment if setup in struct eth_device->enetaddr by driver + initialize() function. Give priority to the value in the environment if a + conflict. 4. Program the address into hardware if the following conditions are met: a) The relevant driver has a 'write_addr' function b) The user hasn't set an 'ethmacskip' environment variable diff --git a/net/eth.c b/net/eth.c index b4b9b43..f75a944 100644 --- a/net/eth.c +++ b/net/eth.c @@ -62,6 +62,15 @@ int eth_getenv_enetaddr_by_index(const char *base_name, int index, return eth_getenv_enetaddr(enetvar, enetaddr); }
+int eth_setenv_enetaddr_by_index(const char *base_name, int index, + uchar *enetaddr) +{ + char enetvar[32]; + sprintf(enetvar, index ? "%s%daddr" : "%saddr", base_name, index); + return eth_setenv_enetaddr(enetvar, enetaddr); +} + + static int eth_mac_skip(int index) { char enetvar[15]; @@ -175,9 +184,15 @@ int eth_write_hwaddr(struct eth_device *dev, const char *base_name, unsigned char env_enetaddr[6]; int ret = 0;
- if (!eth_getenv_enetaddr_by_index(base_name, eth_number, env_enetaddr)) - return -1; - + if (!eth_getenv_enetaddr_by_index(base_name, eth_number, env_enetaddr)) { + if (!is_valid_ether_addr(dev->enetaddr)) + return -1; + eth_setenv_enetaddr_by_index(base_name, eth_number, + dev->enetaddr); + printf("\nWarning: %s using MAC address from net device\n", + dev->name); + return 0; + } if (memcmp(env_enetaddr, "\0\0\0\0\0\0", 6)) { if (memcmp(dev->enetaddr, "\0\0\0\0\0\0", 6) && memcmp(dev->enetaddr, env_enetaddr, 6)) {
Wolfgang, Mike,
On 02/01/2012 05:27 PM, Rob Herring wrote:
From: Rob Herring rob.herring@calxeda.com
If the net driver has setup a valid ethernet address and an ethernet address is not set in the environment already, then set the environment variables from the net driver setting.
This enables pxe booting on boards which don't set ethaddr env variable.
Signed-off-by: Rob Herring rob.herring@calxeda.com
Wolfgang,
This now prints a warning. If both env and device mac's are set, then the behavior is unchanged and the env setting is used.
v3:
- print a warning if using mac address from the net device
v2:
- Re-wrote to always setup ethaddr env variables
Any comments on this?
Rob
Rob
doc/README.enetaddr | 4 +++- net/eth.c | 21 ++++++++++++++++++--- 2 files changed, 21 insertions(+), 4 deletions(-)
diff --git a/doc/README.enetaddr b/doc/README.enetaddr index 2d8e24f..6c61817 100644 --- a/doc/README.enetaddr +++ b/doc/README.enetaddr @@ -32,7 +32,9 @@ Correct flow of setting up the MAC address (summarized):
- Read from hardware in initialize() function
- Read from environment in net/eth.c after initialize()
-3. Give priority to the value in the environment if a conflict +3. Write value to environment if setup in struct eth_device->enetaddr by driver
- initialize() function. Give priority to the value in the environment if a
- conflict.
- Program the address into hardware if the following conditions are met:
a) The relevant driver has a 'write_addr' function b) The user hasn't set an 'ethmacskip' environment variable diff --git a/net/eth.c b/net/eth.c index b4b9b43..f75a944 100644 --- a/net/eth.c +++ b/net/eth.c @@ -62,6 +62,15 @@ int eth_getenv_enetaddr_by_index(const char *base_name, int index, return eth_getenv_enetaddr(enetvar, enetaddr); }
+int eth_setenv_enetaddr_by_index(const char *base_name, int index,
uchar *enetaddr)+{
- char enetvar[32];
- sprintf(enetvar, index ? "%s%daddr" : "%saddr", base_name, index);
- return eth_setenv_enetaddr(enetvar, enetaddr);
+}
static int eth_mac_skip(int index) { char enetvar[15]; @@ -175,9 +184,15 @@ int eth_write_hwaddr(struct eth_device *dev, const char *base_name, unsigned char env_enetaddr[6]; int ret = 0;
- if (!eth_getenv_enetaddr_by_index(base_name, eth_number, env_enetaddr))
return -1;
- if (!eth_getenv_enetaddr_by_index(base_name, eth_number, env_enetaddr)) {
if (!is_valid_ether_addr(dev->enetaddr))return -1;eth_setenv_enetaddr_by_index(base_name, eth_number,dev->enetaddr);printf("\nWarning: %s using MAC address from net device\n",dev->name);return 0;- } if (memcmp(env_enetaddr, "\0\0\0\0\0\0", 6)) { if (memcmp(dev->enetaddr, "\0\0\0\0\0\0", 6) && memcmp(dev->enetaddr, env_enetaddr, 6)) {
Dear Rob Herring,
In message 1328138854-28612-1-git-send-email-robherring2@gmail.com you wrote:
--- a/doc/README.enetaddr +++ b/doc/README.enetaddr @@ -32,7 +32,9 @@ Correct flow of setting up the MAC address (summarized):
- Read from hardware in initialize() function
- Read from environment in net/eth.c after initialize()
-3. Give priority to the value in the environment if a conflict +3. Write value to environment if setup in struct eth_device->enetaddr by driver
- initialize() function. Give priority to the value in the environment if a
- conflict.
Sorry, but this description is not correct. You say here that the environment variable should always be written, but this is not the case. Only if it does not exist it shall be set. If it exists, it shall only be read, and in case of inconsistencies a warning shall be printed.
Best regards,
Wolfgang Denk
On 03/06/2012 01:30 PM, Wolfgang Denk wrote:
Dear Rob Herring,
In message 1328138854-28612-1-git-send-email-robherring2@gmail.com you wrote:
--- a/doc/README.enetaddr +++ b/doc/README.enetaddr @@ -32,7 +32,9 @@ Correct flow of setting up the MAC address (summarized):
- Read from hardware in initialize() function
- Read from environment in net/eth.c after initialize()
-3. Give priority to the value in the environment if a conflict +3. Write value to environment if setup in struct eth_device->enetaddr by driver
- initialize() function. Give priority to the value in the environment if a
- conflict.
Sorry, but this description is not correct. You say here that the environment variable should always be written, but this is not the case. Only if it does not exist it shall be set. If it exists, it shall only be read, and in case of inconsistencies a warning shall be printed.
How about this:
3. Always use the value in the environment if there is a conflict. If the environment variable is not set and the driver initialized struct eth_device->enetaddr, then print a warning and set the environment variable to initialized by the driver.
Rob
Best regards,
Wolfgang Denk
Dear Rob,
In message 4F566D05.5020809@gmail.com you wrote:
+3. Write value to environment if setup in struct eth_device->enetaddr by driver
- initialize() function. Give priority to the value in the environment if a
- conflict.
Sorry, but this description is not correct. You say here that the environment variable should always be written, but this is not the case. Only if it does not exist it shall be set. If it exists, it shall only be read, and in case of inconsistencies a warning shall be printed.
How about this:
- Always use the value in the environment if there is a conflict. If
the environment variable is not set and the driver initialized struct eth_device->enetaddr, then print a warning and set the environment variable to initialized by the driver.
I find you make it difficult to read without need by explaining it backwards.
The environment variable will be compared to the driver initialized struct eth_device->enetaddr. If they differ, a warning is printed, an the environment variable will be used unchanged.
If the environment variable is not set, it will be initialized from eth_device->enetaddr, and a warning will be printed.
Best regards,
Wolfgang Denk
From: Rob Herring rob.herring@calxeda.com
If the net driver has setup a valid ethernet address and an ethernet address is not set in the environment already, then set the environment variables from the net driver setting.
This enables pxe booting on boards which don't set ethaddr env variable.
Signed-off-by: Rob Herring rob.herring@calxeda.com --- v4: - rewrite of documentation from Wolfgang
v3: - print a warning if using mac address from the net device
v2: - Re-wrote to always setup ethaddr env variables
doc/README.enetaddr | 6 +++++- net/eth.c | 21 ++++++++++++++++++--- 2 files changed, 23 insertions(+), 4 deletions(-)
diff --git a/doc/README.enetaddr b/doc/README.enetaddr index 2d8e24f..24b25f7 100644 --- a/doc/README.enetaddr +++ b/doc/README.enetaddr @@ -32,7 +32,11 @@ Correct flow of setting up the MAC address (summarized):
1. Read from hardware in initialize() function 2. Read from environment in net/eth.c after initialize() -3. Give priority to the value in the environment if a conflict +3. The environment variable will be compared to the driver initialized + struct eth_device->enetaddr. If they differ, a warning is printed, and the + environment variable will be used unchanged. + If the environment variable is not set, it will be initialized from + eth_device->enetaddr, and a warning will be printed. 4. Program the address into hardware if the following conditions are met: a) The relevant driver has a 'write_addr' function b) The user hasn't set an 'ethmacskip' environment variable diff --git a/net/eth.c b/net/eth.c index f14767b..22cd37d 100644 --- a/net/eth.c +++ b/net/eth.c @@ -62,6 +62,15 @@ int eth_getenv_enetaddr_by_index(const char *base_name, int index, return eth_getenv_enetaddr(enetvar, enetaddr); }
+int eth_setenv_enetaddr_by_index(const char *base_name, int index, + uchar *enetaddr) +{ + char enetvar[32]; + sprintf(enetvar, index ? "%s%daddr" : "%saddr", base_name, index); + return eth_setenv_enetaddr(enetvar, enetaddr); +} + + static int eth_mac_skip(int index) { char enetvar[15]; @@ -172,9 +181,15 @@ int eth_write_hwaddr(struct eth_device *dev, const char *base_name, unsigned char env_enetaddr[6]; int ret = 0;
- if (!eth_getenv_enetaddr_by_index(base_name, eth_number, env_enetaddr)) - return -1; - + if (!eth_getenv_enetaddr_by_index(base_name, eth_number, env_enetaddr)) { + if (!is_valid_ether_addr(dev->enetaddr)) + return -1; + eth_setenv_enetaddr_by_index(base_name, eth_number, + dev->enetaddr); + printf("\nWarning: %s using MAC address from net device\n", + dev->name); + return 0; + } if (memcmp(env_enetaddr, "\0\0\0\0\0\0", 6)) { if (memcmp(dev->enetaddr, "\0\0\0\0\0\0", 6) && memcmp(dev->enetaddr, env_enetaddr, 6)) {
Hi Rob,
On Tue, Mar 6, 2012 at 9:03 PM, Rob Herring robherring2@gmail.com wrote:
From: Rob Herring rob.herring@calxeda.com
If the net driver has setup a valid ethernet address and an ethernet address is not set in the environment already, then set the environment variables from the net driver setting.
This enables pxe booting on boards which don't set ethaddr env variable.
Signed-off-by: Rob Herring rob.herring@calxeda.com
v4:
- rewrite of documentation from Wolfgang
v3:
- print a warning if using mac address from the net device
v2:
- Re-wrote to always setup ethaddr env variables
doc/README.enetaddr | 6 +++++- net/eth.c | 21 ++++++++++++++++++--- 2 files changed, 23 insertions(+), 4 deletions(-)
checkpatch.pl failures:
-------------------------------------
ERROR: trailing whitespace #48: FILE: doc/README.enetaddr:38: + If the environment variable is not set, it will be initialized from $
WARNING: line over 80 characters #80: FILE: net/eth.c:184: + if (!eth_getenv_enetaddr_by_index(base_name, eth_number, env_enetaddr)) {
total: 1 errors, 1 warnings, 45 lines checked
NOTE: whitespace errors detected, you may wish to use scripts/cleanpatch or scripts/cleanfile
NOTE: Ignored message types: COMPLEX_MACRO CONSIDER_KSTRTO MINMAX MULTISTATEMENT_MACRO_USE_DO_WHILE
U-Boot-v4-net-allow-setting-env-enetaddr-from-net-device-setting.patch has style problems, please review.
----------------------
Also, it seems that just because the enetaddr was read from dev (possibly from an eeprom or elsewhere) doesn't mean that the MAC doesn't need you to call write_hwaddr(). I think you shouldn't return 0 if you use the net device's addr, but rather should make the if (memcmp... into an else if.
-Joe
From: Rob Herring rob.herring@calxeda.com
If the net driver has setup a valid ethernet address and an ethernet address is not set in the environment already, then set the environment variables from the net driver setting.
This enables pxe booting on boards which don't set ethaddr env variable.
Signed-off-by: Rob Herring rob.herring@calxeda.com --- v5: - Based on Joe's comments and new commit 6937664426446b763 "net/eth.c: fix eth_write_hwaddr() to use dev->enetaddr as fall back", moved the setting of ethaddr and allow the driver write_hwaddr to be called. - fixed trailing whitespace in documentation
v4: - rewrite of documentation from Wolfgang
v3: - print a warning if using mac address from the net device
v2: - Re-wrote to always setup ethaddr env variables
doc/README.enetaddr | 6 +++++- net/eth.c | 14 ++++++++++++++ 2 files changed, 19 insertions(+), 1 deletion(-)
diff --git a/doc/README.enetaddr b/doc/README.enetaddr index 2d8e24f..1eaeaf9 100644 --- a/doc/README.enetaddr +++ b/doc/README.enetaddr @@ -32,7 +32,11 @@ Correct flow of setting up the MAC address (summarized):
1. Read from hardware in initialize() function 2. Read from environment in net/eth.c after initialize() -3. Give priority to the value in the environment if a conflict +3. The environment variable will be compared to the driver initialized + struct eth_device->enetaddr. If they differ, a warning is printed, and the + environment variable will be used unchanged. + If the environment variable is not set, it will be initialized from + eth_device->enetaddr, and a warning will be printed. 4. Program the address into hardware if the following conditions are met: a) The relevant driver has a 'write_addr' function b) The user hasn't set an 'ethmacskip' environment variable diff --git a/net/eth.c b/net/eth.c index 3eeb908..d4b94a4 100644 --- a/net/eth.c +++ b/net/eth.c @@ -62,6 +62,15 @@ int eth_getenv_enetaddr_by_index(const char *base_name, int index, return eth_getenv_enetaddr(enetvar, enetaddr); }
+int eth_setenv_enetaddr_by_index(const char *base_name, int index, + uchar *enetaddr) +{ + char enetvar[32]; + sprintf(enetvar, index ? "%s%daddr" : "%saddr", base_name, index); + return eth_setenv_enetaddr(enetvar, enetaddr); +} + + static int eth_mac_skip(int index) { char enetvar[15]; @@ -186,6 +195,11 @@ int eth_write_hwaddr(struct eth_device *dev, const char *base_name, }
memcpy(dev->enetaddr, env_enetaddr, 6); + } else if (is_valid_ether_addr(dev->enetaddr)) { + eth_setenv_enetaddr_by_index(base_name, eth_number, + dev->enetaddr); + printf("\nWarning: %s using MAC address from net device\n", + dev->name); }
if (dev->write_hwaddr &&
Hi Rob,
On Sat, Apr 14, 2012 at 11:06 PM, Rob Herring robherring2@gmail.com wrote:
From: Rob Herring rob.herring@calxeda.com
If the net driver has setup a valid ethernet address and an ethernet address is not set in the environment already, then set the environment variables from the net driver setting.
This enables pxe booting on boards which don't set ethaddr env variable.
Signed-off-by: Rob Herring rob.herring@calxeda.com
Applied to next, thanks.
-Joe
participants (4)
-
Joe Hershberger -
Mike Frysinger -
Rob Herring -
Wolfgang Denk