[U-Boot] [PATCH] Ethernet: let user know if there is no valid ethernet address

[U-Boot] [PATCH] Ethernet: let user know if there is no valid ethernet address

[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 <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);
 	}

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

[U-Boot] [PATCH] Ethernet: let user know if there is no valid ethernet address

[Marek Vasut]

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

[U-Boot] [PATCH] Ethernet: let user know if there is no valid ethernet address

[Pavel Machek]

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
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[U-Boot] [PATCH] Ethernet: let user know if there is no valid ethernet address

[Marek Vasut]

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

[U-Boot] [PATCH] Ethernet: let user know if there is no valid ethernet address

[Pavel Machek]

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
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[U-Boot] [PATCH] Ethernet: let user know if there is no valid ethernet address

[Marek Vasut]

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

[U-Boot] [PATCH] Ethernet: let user know if there is no valid ethernet address

[Wolfgang Denk]

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

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

[U-Boot] [PATCH] Ethernet: let user know if there is no valid ethernet address

[Pavel Machek]

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
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[U-Boot] [PATCH] Ethernet: let user know if there is no valid ethernet address

[Wolfgang Denk]

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

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

[U-Boot] [PATCH] Ethernet: let user know if there is no valid ethernet address

[Pavel Machek]

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
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[U-Boot] [PATCH] Ethernet: let user know if there is no valid ethernet address

[Tom Rini]

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.

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

[U-Boot] [PATCH] Ethernet: let user know if there is no valid ethernet address

[Wolfgang Denk]

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

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

[U-Boot] [PATCHv2] Ethernet: let user know if there is no valid ethernet address

[Pavel Machek]

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

Signed-off-by: Pavel Machek <pavel@denx.de>

---

[U-Boot] [PATCH] Ethernet: let user know if there is no valid ethernet address

[Pavel Machek]

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
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html