[U-Boot] [PATCH] Ethernet: let user know if there is no valid ethernet address
When there's no ethernet address available, u-boot currently prints "could not set ethernet address", but fails to mention that there's no address it could set. Make it a bit less confusing.
Signed-off-by: Pavel Machek pavel@denx.de
diff --git a/net/eth.c b/net/eth.c index 99386e3..b72ae84 100644 --- a/net/eth.c +++ b/net/eth.c @@ -179,10 +179,12 @@ int eth_write_hwaddr(struct eth_device *dev, const char *base_name, dev->name); }
- if (dev->write_hwaddr && - !eth_mac_skip(eth_number)) { - if (!is_valid_ether_addr(dev->enetaddr)) + if (dev->write_hwaddr && !eth_mac_skip(eth_number)) { + if (!is_valid_ether_addr(dev->enetaddr)) { + printf("\nError: %s ethernet address not valid: %pM\n", + dev->name, dev->enetaddr); return -1; + }
ret = dev->write_hwaddr(dev); }
On Friday, July 11, 2014 at 11:42:13 AM, Pavel Machek wrote:
When there's no ethernet address available, u-boot currently prints "could not set ethernet address", but fails to mention that there's no address it could set. Make it a bit less confusing.
Signed-off-by: Pavel Machek pavel@denx.de
diff --git a/net/eth.c b/net/eth.c index 99386e3..b72ae84 100644 --- a/net/eth.c +++ b/net/eth.c @@ -179,10 +179,12 @@ int eth_write_hwaddr(struct eth_device *dev, const char *base_name, dev->name); }
- if (dev->write_hwaddr &&
!eth_mac_skip(eth_number)) {if (!is_valid_ether_addr(dev->enetaddr))
- if (dev->write_hwaddr && !eth_mac_skip(eth_number)) {
if (!is_valid_ether_addr(dev->enetaddr)) {printf("\nError: %s ethernet address not valid: %pM\n",dev->name, dev->enetaddr); return -1;
You might as well return -EINVAL here while at it.
btw. I think I should really NAK you a patch or two, just so you can continue complaining to me about how noone likes you here ;-)
Best regards, Marek Vasut
On Fri 2014-07-11 11:46:56, Marek Vasut wrote:
On Friday, July 11, 2014 at 11:42:13 AM, Pavel Machek wrote:
When there's no ethernet address available, u-boot currently prints "could not set ethernet address", but fails to mention that there's no address it could set. Make it a bit less confusing.
Signed-off-by: Pavel Machek pavel@denx.de
diff --git a/net/eth.c b/net/eth.c index 99386e3..b72ae84 100644 --- a/net/eth.c +++ b/net/eth.c @@ -179,10 +179,12 @@ int eth_write_hwaddr(struct eth_device *dev, const char *base_name, dev->name); }
- if (dev->write_hwaddr &&
!eth_mac_skip(eth_number)) {if (!is_valid_ether_addr(dev->enetaddr))
- if (dev->write_hwaddr && !eth_mac_skip(eth_number)) {
if (!is_valid_ether_addr(dev->enetaddr)) {printf("\nError: %s ethernet address not valid: %pM\n",dev->name, dev->enetaddr); return -1;You might as well return -EINVAL here while at it.
And change two things at once? Too scary :-).
btw. I think I should really NAK you a patch or two, just so you can continue complaining to me about how noone likes you here ;-)
I'm not saying noone likes me. Everybody loves me, especially my horse when I bring carrots. Thanks for the reviews. But I still did not get any "applied" or "looks good, will apply" responses...
Best regards,
Pavel
On Friday, July 11, 2014 at 12:43:47 PM, Pavel Machek wrote:
On Fri 2014-07-11 11:46:56, Marek Vasut wrote:
On Friday, July 11, 2014 at 11:42:13 AM, Pavel Machek wrote:
When there's no ethernet address available, u-boot currently prints "could not set ethernet address", but fails to mention that there's no address it could set. Make it a bit less confusing.
Signed-off-by: Pavel Machek pavel@denx.de
diff --git a/net/eth.c b/net/eth.c index 99386e3..b72ae84 100644 --- a/net/eth.c +++ b/net/eth.c @@ -179,10 +179,12 @@ int eth_write_hwaddr(struct eth_device *dev, const char *base_name, dev->name);
}
- if (dev->write_hwaddr &&
!eth_mac_skip(eth_number)) {if (!is_valid_ether_addr(dev->enetaddr))
if (dev->write_hwaddr && !eth_mac_skip(eth_number)) {
if (!is_valid_ether_addr(dev->enetaddr)) {printf("\nError: %s ethernet address not valid: %pM\n",dev->name, dev->enetaddr); return -1;You might as well return -EINVAL here while at it.
And change two things at once? Too scary :-).
Subsequent patch is welcome.
btw. I think I should really NAK you a patch or two, just so you can continue complaining to me about how noone likes you here ;-)
I'm not saying noone likes me. Everybody loves me, especially my horse when I bring carrots. Thanks for the reviews. But I still did not get any "applied" or "looks good, will apply" responses...
Do you CC those http://www.denx.de/wiki/U-Boot/Custodians people on patches belonging to their domain?
Best regards, Marek Vasut
Hi!
btw. I think I should really NAK you a patch or two, just so you can continue complaining to me about how noone likes you here ;-)
I'm not saying noone likes me. Everybody loves me, especially my horse when I bring carrots. Thanks for the reviews. But I still did not get any "applied" or "looks good, will apply" responses...
Do you CC those http://www.denx.de/wiki/U-Boot/Custodians people on patches belonging to their domain?
Aha, that's where the MAINTAINERS file hides :-). I believe I did pretty good job, except Joe Hershberger for the trivial mdelay() patch.
I guess Chin Liang See clsee@altera.com should be listed as socfpga maintainer? Pavel
On Friday, July 11, 2014 at 12:52:29 PM, Pavel Machek wrote:
Hi!
btw. I think I should really NAK you a patch or two, just so you can continue complaining to me about how noone likes you here ;-)
I'm not saying noone likes me. Everybody loves me, especially my horse when I bring carrots. Thanks for the reviews. But I still did not get any "applied" or "looks good, will apply" responses...
Do you CC those http://www.denx.de/wiki/U-Boot/Custodians people on patches belonging to their domain?
Aha, that's where the MAINTAINERS file hides :-). I believe I did pretty good job, except Joe Hershberger for the trivial mdelay() patch.
I guess Chin Liang See clsee@altera.com should be listed as socfpga maintainer?
Also update doc/git-mailrc then .
Best regards, Marek Vasut
Hi!
btw. I think I should really NAK you a patch or two, just so you can continue complaining to me about how noone likes you here ;-)
I'm not saying noone likes me. Everybody loves me, especially my horse when I bring carrots. Thanks for the reviews. But I still did not get any "applied" or "looks good, will apply" responses...
Do you CC those http://www.denx.de/wiki/U-Boot/Custodians people on patches belonging to their domain?
Aha, that's where the MAINTAINERS file hides :-). I believe I did pretty good job, except Joe Hershberger for the trivial mdelay() patch.
I guess Chin Liang See clsee@altera.com should be listed as socfpga maintainer?
Also update doc/git-mailrc then .
Actually, I'd like to hear confirmation from Chin Liang See that it is okay with him... Pavel
Dear Pavel Machek,
In message 20140711094213.GA4385@amd.pavel.ucw.cz you wrote:
When there's no ethernet address available, u-boot currently prints "could not set ethernet address", but fails to mention that there's no address it could set. Make it a bit less confusing.
Signed-off-by: Pavel Machek pavel@denx.de
diff --git a/net/eth.c b/net/eth.c index 99386e3..b72ae84 100644 --- a/net/eth.c +++ b/net/eth.c @@ -179,10 +179,12 @@ int eth_write_hwaddr(struct eth_device *dev, const char *base_name, dev->name); }
- if (dev->write_hwaddr &&
!eth_mac_skip(eth_number)) {if (!is_valid_ether_addr(dev->enetaddr))
- if (dev->write_hwaddr && !eth_mac_skip(eth_number)) {
if (!is_valid_ether_addr(dev->enetaddr)) {printf("\nError: %s ethernet address not valid: %pM\n",dev->name, dev->enetaddr);
Sorry, but this is not really helpful. "Not set" and "not valid" are different things. "Not valid" might be confusing when none is set at all.
Also, if I understand correctly, we will now have _two_ error messages ("ethernet address not valid" followed by "could not set ethernet address")? That's not so nice either.
Best regards,
Wolfgang Denk
On Fri 2014-07-11 17:27:38, Wolfgang Denk wrote:
Dear Pavel Machek,
In message 20140711094213.GA4385@amd.pavel.ucw.cz you wrote:
When there's no ethernet address available, u-boot currently prints "could not set ethernet address", but fails to mention that there's no address it could set. Make it a bit less confusing.
Signed-off-by: Pavel Machek pavel@denx.de
diff --git a/net/eth.c b/net/eth.c index 99386e3..b72ae84 100644 --- a/net/eth.c +++ b/net/eth.c @@ -179,10 +179,12 @@ int eth_write_hwaddr(struct eth_device *dev, const char *base_name, dev->name); }
- if (dev->write_hwaddr &&
!eth_mac_skip(eth_number)) {if (!is_valid_ether_addr(dev->enetaddr))
- if (dev->write_hwaddr && !eth_mac_skip(eth_number)) {
if (!is_valid_ether_addr(dev->enetaddr)) {printf("\nError: %s ethernet address not valid: %pM\n",dev->name, dev->enetaddr);Sorry, but this is not really helpful. "Not set" and "not valid" are different things. "Not valid" might be confusing when none is set at all.
Well, it is what the code checks for.
Also, if I understand correctly, we will now have _two_ error messages ("ethernet address not valid" followed by "could not set ethernet address")? That's not so nice either.
Ok, would it be acceptable to change 'count not set' message to 'could not set or invalid address' and print the address as well?
Best regards, Pavel
Dear Pavel,
In message 20140711163247.GA14873@amd.pavel.ucw.cz you wrote:
Sorry, but this is not really helpful. "Not set" and "not valid" are different things. "Not valid" might be confusing when none is set at all.
Well, it is what the code checks for.
Also, if I understand correctly, we will now have _two_ error messages ("ethernet address not valid" followed by "could not set ethernet address")? That's not so nice either.
Ok, would it be acceptable to change 'count not set' message to 'could not set or invalid address' and print the address as well?
Print the address - if it is not set? I think we should provide useful error messages. Either the address has not been set, then we should say so, or it is invalid, then we should say that.
Best regards,
Wolfgang Denk
On Fri 2014-07-11 21:13:22, Wolfgang Denk wrote:
Dear Pavel,
In message 20140711163247.GA14873@amd.pavel.ucw.cz you wrote:
Sorry, but this is not really helpful. "Not set" and "not valid" are different things. "Not valid" might be confusing when none is set at all.
Well, it is what the code checks for.
Also, if I understand correctly, we will now have _two_ error messages ("ethernet address not valid" followed by "could not set ethernet address")? That's not so nice either.
Ok, would it be acceptable to change 'count not set' message to 'could not set or invalid address' and print the address as well?
Print the address - if it is not set? I think we should provide useful error messages. Either the address has not been set, then we should say so, or it is invalid, then we should say that.
Well, it may be unset (00:00:...) or it may be invalid (b2:a3:...).
I think I do not understand you correctly. Yes, we should provide useful error messages, and current one is untrue and confusing.
Can you suggest a patch or messages you'd like to see?
Thanks, Pavel
On Fri, Jul 11, 2014 at 10:30:07PM +0200, Pavel Machek wrote:
On Fri 2014-07-11 21:13:22, Wolfgang Denk wrote:
Dear Pavel,
In message 20140711163247.GA14873@amd.pavel.ucw.cz you wrote:
Sorry, but this is not really helpful. "Not set" and "not valid" are different things. "Not valid" might be confusing when none is set at all.
Well, it is what the code checks for.
Also, if I understand correctly, we will now have _two_ error messages ("ethernet address not valid" followed by "could not set ethernet address")? That's not so nice either.
Ok, would it be acceptable to change 'count not set' message to 'could not set or invalid address' and print the address as well?
Print the address - if it is not set? I think we should provide useful error messages. Either the address has not been set, then we should say so, or it is invalid, then we should say that.
Well, it may be unset (00:00:...) or it may be invalid (b2:a3:...).
I think I do not understand you correctly. Yes, we should provide useful error messages, and current one is untrue and confusing.
Can you suggest a patch or messages you'd like to see?
Yes, say "unset" when it is unset and "invalid" when set but not valid.
Dear Pavel,
In message 20140711203007.GA1298@amd.pavel.ucw.cz you wrote:
Ok, would it be acceptable to change 'count not set' message to 'could not set or invalid address' and print the address as well?
Print the address - if it is not set? I think we should provide useful error messages. Either the address has not been set, then we should say so, or it is invalid, then we should say that.
Well, it may be unset (00:00:...) or it may be invalid (b2:a3:...).
The address being not set at all, and it being set to 00:00:00:00:00:00, are two totally different cases, In the former, it's, well, not set, and in the latter it is set to an invalid value.
I think I do not understand you correctly. Yes, we should provide useful error messages, and current one is untrue and confusing.
Can you suggest a patch or messages you'd like to see?
If not set at all:
"ERROR: eth?addr not set"
If set to an illegal value:
"ERROR: eth?addr=XX:XX:XX:XX:XX:XX illegal value"
Best regards,
Wolfgang Denk
Improve error messages in case of invalid/unset ethernet addresses.
Signed-off-by: Pavel Machek pavel@denx.de
---
From v1: distinguish between unset/invalid, avoid two error messages.
Well, it may be unset (00:00:...) or it may be invalid (b2:a3:...).
The address being not set at all, and it being set to 00:00:00:00:00:00, are two totally different cases, In the former, it's, well, not set, and in the latter it is set to an invalid value.
Unfortunately, that's not how the code works now. it uses 00:00: as a marker that address is not set. (And I don't think completely redoing the code for sake of error message is sane.)
--- a/net/eth.c +++ b/net/eth.c @@ -10,6 +10,7 @@ #include <net.h> #include <miiphy.h> #include <phy.h> +#include <asm/errno.h>
void eth_parse_enetaddr(const char *addr, uchar *enetaddr) { @@ -152,6 +153,11 @@ static void eth_current_changed(void) setenv("ethact", NULL); }
+int eth_address_set(unsigned char *addr) +{ + return memcmp(addr, "\0\0\0\0\0\0", 6); +} + int eth_write_hwaddr(struct eth_device *dev, const char *base_name, int eth_number) { @@ -160,8 +166,8 @@ int eth_write_hwaddr(struct eth_device *dev, const char *base_name,
eth_getenv_enetaddr_by_index(base_name, eth_number, env_enetaddr);
- if (memcmp(env_enetaddr, "\0\0\0\0\0\0", 6)) { - if (memcmp(dev->enetaddr, "\0\0\0\0\0\0", 6) && + if (eth_address_set(env_enetaddr)) { + if (eth_address_set(dev->enetaddr) && memcmp(dev->enetaddr, env_enetaddr, 6)) { printf("\nWarning: %s MAC addresses don't match:\n", dev->name); @@ -177,14 +183,22 @@ int eth_write_hwaddr(struct eth_device *dev, const char *base_name, dev->enetaddr); printf("\nWarning: %s using MAC address from net device\n", dev->name); + } else if (!(eth_address_set(dev->enetaddr))) { + printf("\nError: %s address not set.\n", + dev->name); + return -EINVAL; }
- if (dev->write_hwaddr && - !eth_mac_skip(eth_number)) { - if (!is_valid_ether_addr(dev->enetaddr)) - return -1; + if (dev->write_hwaddr && !eth_mac_skip(eth_number)) { + if (!is_valid_ether_addr(dev->enetaddr)) { + printf("\nError: %s address %pM illegal value\n", + dev->name, dev->enetaddr); + return -EINVAL; + }
ret = dev->write_hwaddr(dev); + if (ret) + printf("\nWarning: %s failed to set MAC address\n", dev->name); }
return ret; @@ -303,8 +317,7 @@ int eth_initialize(bd_t *bis) puts("\nWarning: eth device name has a space!" "\n");
- if (eth_write_hwaddr(dev, "eth", dev->index)) - puts("\nWarning: failed to set MAC address\n"); + eth_write_hwaddr(dev, "eth", dev->index);
dev = dev->next; num_devices++;
participants (4)
-
Marek Vasut -
Pavel Machek -
Tom Rini -
Wolfgang Denk