Ethernet: let user know if there is no valid ethernet address

PATCH New Pavel Machek 4 participants 14 messages Jul 11, 2014 – Jul 14, 2014 Networking

Share & tools

Pavel Machek

author

2014-07-11 09:42 UTC #0 lore raw

PATCH 1/1 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 <[email protected]>

net/eth.c +5 -3

index 99386e3..b72ae84 100644
@@ -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);
 	}

Signature · 2 lines

(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Marek Vasut

2014-07-11 09:46 UTC #1 lore raw

PATCH 1/1 Ethernet: let user know if there is no valid ethernet address

Quoted 12 lines · Pavel Machek

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 <[email protected]>
net/eth.c +0 -0
index 99386e3..b72ae84 100644
@@ -179,10 +179,12 @@ int eth_write_hwaddr(struct eth_device *dev, const
 char *base_name, dev->name);

net/eth.c int eth_write_hwaddr(struct eth_device *dev, const · line 186

Quoted 10 lines

net/eth.c +4 -3

 	}
 
-	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

Pavel Machek

author

2014-07-11 10:43 UTC #2 lore raw

PATCH 1/1 Ethernet: let user know if there is no valid ethernet address

Quoted 15 lines · Marek Vasut

Quoted 14 lines · Pavel Machek
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 <[email protected]>
net/eth.c +0 -0
index 99386e3..b72ae84 100644
@@ -179,10 +179,12 @@ int eth_write_hwaddr(struct eth_device *dev, const
 char *base_name, dev->name);
 	}
 

Quoted 10 lines

net/eth.c int eth_write_hwaddr(struct eth_device *dev, const char *base_name, · line 186
Quoted 8 lines
net/eth.c +4 -3
-	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 :-).

Quoted 2 lines

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

Signature · 2 lines

(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Marek Vasut

2014-07-11 10:46 UTC #3 lore raw

PATCH 1/1 Ethernet: let user know if there is no valid ethernet address

Quoted 25 lines · Pavel Machek

Quoted 24 lines · Marek Vasut
Quoted 23 lines · Pavel Machek
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 <[email protected]>
net/eth.c +4 -3
index 99386e3..b72ae84 100644
@@ -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);
 

Quoted 5 lines

Quoted 3 lines
net/eth.c int eth_write_hwaddr(struct eth_device *dev, const char *base_name, · line 186
Quoted 1 line
net/eth.c +0 -0
 			return -1;
You might as well return -EINVAL here while at it.
And change two things at once? Too scary :-).

Subsequent patch is welcome.

Quoted 6 lines

Quoted 2 lines
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

Pavel Machek

author

2014-07-11 10:52 UTC #4 lore raw

PATCH 1/1 Ethernet: let user know if there is no valid ethernet address

Hi!

Quoted 9 lines

Quoted 6 lines
Quoted 2 lines
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 <[email protected]> should be listed as socfpga maintainer?
Pavel

Signature · 2 lines

(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Marek Vasut

2014-07-11 11:50 UTC #5 lore raw

PATCH 1/1 Ethernet: let user know if there is no valid ethernet address

Quoted 12 lines · Pavel Machek

Hi!
Quoted 9 lines
Quoted 6 lines
Quoted 2 lines
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?

Quoted 6 lines

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 <[email protected]> should be listed as socfpga maintainer?

Also update doc/git-mailrc then .

Best regards,
Marek Vasut

Pavel Machek

author

2014-07-14 12:19 UTC #6 lore raw

PATCH 1/1 Ethernet: let user know if there is no valid ethernet address

Hi!

Quoted 10 lines

Quoted 10 lines
Quoted 9 lines
Quoted 6 lines
Quoted 2 lines
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?

Quoted 8 lines

Quoted 6 lines
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 <[email protected]> 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

Signature · 2 lines

(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Wolfgang Denk

2014-07-11 15:27 UTC #7 lore raw

PATCH 1/1 Ethernet: let user know if there is no valid ethernet address

In reply to Pavel Machek

Dear Pavel Machek,

Quoted 11 lines · In message <[email protected]> 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 <[email protected]>
net/eth.c +0 -0
index 99386e3..b72ae84 100644

net/eth.c int eth_write_hwaddr(struct eth_device *dev, const char *base_name, · line 185

Quoted 10 lines

net/eth.c +4 -3

@@ -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

Signature · 4 lines

DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
"Pull the wool over your own eyes!"                - J.R. "Bob" Dobbs

Pavel Machek

author

2014-07-11 16:32 UTC #8 lore raw

PATCH 1/1 Ethernet: let user know if there is no valid ethernet address

Quoted 17 lines · Wolfgang Denk

Dear Pavel Machek,
Quoted 14 lines · In message <[email protected]> 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 <[email protected]>
net/eth.c +0 -0
index 99386e3..b72ae84 100644
@@ -179,10 +179,12 @@ int eth_write_hwaddr(struct eth_device *dev, const char *base_name,
 			dev->name);
 	}
 

Quoted 11 lines

net/eth.c int eth_write_hwaddr(struct eth_device *dev, const char *base_name, · line 185
Quoted 7 lines
net/eth.c +4 -3
-	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.

Quoted 3 lines

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

Signature · 2 lines

(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Wolfgang Denk

2014-07-11 19:13 UTC #9 lore raw

PATCH 1/1 Ethernet: let user know if there is no valid ethernet address

Dear Pavel,

Quoted 5 lines · In message <[email protected]> you wrote:

Quoted 4 lines
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.

Quoted 8 lines

Well, it is what the code checks for.
Quoted 3 lines
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

Signature · 4 lines

DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
panic: kernel trap (ignored)

Pavel Machek

author

2014-07-11 20:30 UTC #10 lore raw

PATCH 1/1 Ethernet: let user know if there is no valid ethernet address

Quoted 14 lines · Wolfgang Denk

Dear Pavel,
Quoted 11 lines · In message <[email protected]> you wrote:
Quoted 4 lines
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.
Quoted 3 lines
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.

Quoted 6 lines

Quoted 2 lines
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

Signature · 2 lines

(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Tom Rini

2014-07-11 21:24 UTC #11 lore raw

PATCH 1/1 Ethernet: let user know if there is no valid ethernet address

Quoted 22 lines · Pavel Machek

Quoted 14 lines · Wolfgang Denk
Dear Pavel,
Quoted 11 lines · In message <[email protected]> you wrote:
Quoted 4 lines
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.
Quoted 3 lines
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.
Quoted 6 lines
Quoted 2 lines
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.

Quoted 6 lines

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.

Signature · 8 lines

Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20140711/a469f261/attachment.pgp>

Wolfgang Denk

2014-07-12 07:46 UTC #12 lore raw

PATCH 1/1 Ethernet: let user know if there is no valid ethernet address

In reply to Pavel Machek

Dear Pavel,

Quoted 9 lines · In message <[email protected]> you wrote:

Quoted 7 lines
Quoted 3 lines
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.

Quoted 4 lines

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

Signature · 5 lines

DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Plans break down. You cannot plan the future. Only presumptuous fools
plan. The wise man _steers_.        - Terry Pratchett, _Making_Money_

Pavel Machek

author

2014-07-13 08:27 UTC #13 lore raw

PATCH v2 1/1 Ethernet: let user know if there is no valid ethernet address

Improve error messages in case of invalid/unset ethernet addresses.

Signed-off-by Pavel Machek <[email protected]>

---