Setting MAC address from nv variable broken in barebox 2017.05.0?

Setting MAC address from nv variable broken in barebox 2017.05.0?

[Ian Abbott]

Hi,

I'm not sure if this is a bug or whether I'm doing something wrong.  In 
barebox 2017.04.0 and earlier, I stored the Ethernet MAC address in a 
non-volatile ('nv') variable dev.eth0.macaddr=xx:xx:xx:xx:xx:xx and that 
got propagated to 'global' and the eth0 device on boot:

barebox@xxxx:/ nv
   allow_color: true
   autoboot_timeout: 3
   dev.eth0.ethaddr: xx:xx:xx:xx:xx:xx
   user: none

barebox@xxxx:/ global
* allow_color: true
* autoboot_timeout: 3
   boot.default: xxxx
   boot.watchdog_timeout: 0
   bootm.appendroot: 0
   bootm.image:
   bootm.image.loadaddr:
   bootm.initrd:
   bootm.initrd.loadaddr:
   bootm.oftree:
   bootm.verbose: 0
   bootm.verify: hash
* dev.eth0.ethaddr: xx:xx:xx:xx:xx:xx
   dhcp.bootfile:
   dhcp.client_id:
   dhcp.client_uuid:
   dhcp.oftree_file:
   dhcp.rootpath:
   dhcp.tftp_server_name:
   dhcp.user_class:
   dhcp.vendor_id:
   editcmd: sedit
   hostname: generic
   linux.bootargs.base:
   linux.bootargs.console:
   linux.bootargs.dyn.ip:
   linux.bootargs.dyn.root:
   linux.rootnfsopts: v3,tcp
   loglevel: 4
   model: xxxx
   system.reset: unknown
* user: none
   version: 2017.04.0

barebox@xxxx:/ devinfo eth0
Parameters:
   ethaddr: xx:xx:xx:xx:xx:xx
   gateway: 0.0.0.0
   ipaddr: 0.0.0.0
   linux.bootargs:
   netmask: 0.0.0.0
   serverip: 0.0.0.0

However, in 2017.05.0, my 'dev.eth0.ethaddr' variable is no longer being 
propagated to global on boot, and as a consequence, is no longer 
propagated to eth0 (note that 'dev.eth0.ethaddr' is the only nv variable 
that I set manually when setting up a new board):

barebox@xxxx:/ global
* allow_color: true
* autoboot_timeout: 3
   boot.default: xxxx
   boot.watchdog_timeout: 0
   bootm.appendroot: 0
   bootm.image:
   bootm.image.loadaddr:
   bootm.initrd:
   bootm.initrd.loadaddr:
   bootm.oftree:
   bootm.verbose: 0
   bootm.verify: hash ("none", "hash", "signature", "available")
   dhcp.bootfile:
   dhcp.client_id:
   dhcp.client_uuid:
   dhcp.oftree_file:
   dhcp.rootpath:
   dhcp.tftp_server_name:
   dhcp.user_class:
   dhcp.vendor_id:
   editcmd: sedit
   hostname: generic
   linux.bootargs.base:
   linux.bootargs.console:
   linux.bootargs.dyn.ip:
   linux.bootargs.dyn.root:
   linux.rootnfsopts: v3,tcp
   loglevel: 4
   model: xxxx
   of_partition_binding: new ("new", "legacy", "donttouch")
   system.reset: unknown ("unknown", "POR", "RST", "WDG", "WKE", "JTAG", 
"THERM", "EXT")
* user: none
   version: 2017.05.0

barebox@xxxx:/ devinfo eth0
Parameters:
   ethaddr: 00:00:00:00:00:00
   gateway: 0.0.0.0
   ipaddr: 0.0.0.0
   linux.bootargs:
   netmask: 0.0.0.0
   serverip: 0.0.0.0

I can work around this using a script in /env/init/ to set the global 
from the nv variable, or change /env/network/eth0 to set the MAC address 
from the nv variable.

Is this the proper behaviour or a bug?  Could it be related to commit 
35d8e858bea17ec4796069c9c27fd0b134125eaf ("nv: Do not create globalvars 
from nvvars")?

-- 
-=( Ian Abbott @ MEV Ltd.    E-mail: <abbotti@mev.co.uk> )=-
-=(                          Web: http://www.mev.co.uk/  )=-

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: Setting MAC address from nv variable broken in barebox 2017.05.0?

[Sascha Hauer]

Hi Ian,

On Mon, May 08, 2017 at 02:39:19PM +0100, Ian Abbott wrote:
> Hi,
> 
> I'm not sure if this is a bug or whether I'm doing something wrong.  In
> barebox 2017.04.0 and earlier, I stored the Ethernet MAC address in a
> non-volatile ('nv') variable dev.eth0.macaddr=xx:xx:xx:xx:xx:xx and that got
> propagated to 'global' and the eth0 device on boot:

Yes this is broken. Sorry for breaking it and thank you for reporting it
;)
I just sent out a fix, you're on Cc. Please let me know if this works.

Sascha


-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: Setting MAC address from nv variable broken in barebox 2017.05.0?

[Ian Abbott]

On 08/05/17 14:39, Ian Abbott wrote:
> Hi,
>
> I'm not sure if this is a bug or whether I'm doing something wrong.  In
> barebox 2017.04.0 and earlier, I stored the Ethernet MAC address in a
> non-volatile ('nv') variable dev.eth0.macaddr=xx:xx:xx:xx:xx:xx and that
> got propagated to 'global' and the eth0 device on boot:
>
> barebox@xxxx:/ nv
>   allow_color: true
>   autoboot_timeout: 3
>   dev.eth0.ethaddr: xx:xx:xx:xx:xx:xx
>   user: none
[snip]
> However, in 2017.05.0, my 'dev.eth0.ethaddr' variable is no longer being
> propagated to global on boot, and as a consequence, is no longer
> propagated to eth0 (note that 'dev.eth0.ethaddr' is the only nv variable
> that I set manually when setting up a new board):
>
> barebox@xxxx:/ devinfo eth0
> Parameters:
>   ethaddr: 00:00:00:00:00:00
>   gateway: 0.0.0.0
>   ipaddr: 0.0.0.0
>   linux.bootargs:
>   netmask: 0.0.0.0
>   serverip: 0.0.0.0
>
[snip]
> Is this the proper behaviour or a bug?  Could it be related to commit
> 35d8e858bea17ec4796069c9c27fd0b134125eaf ("nv: Do not create globalvars
> from nvvars")?

As a related follow-up, this code in globalvar_add_simple() looks a bit 
strange:

	if (value)
		dev_set_param(&global_device, name, value);

	globalvar_nv_sync(name);

If you set a new global variable with a specified value and there is an 
existing nv variable of the same name, the global variable will be set 
to the value of the nv variable, rather than the specified value:

barebox@xxxx:/ nv -r quux
barebox@xxxx:/ global -r quux
barebox@xxxx:/ nv quux=foo
barebox@xxxx:/ global quux=bar
barebox@xxxx:/ echo ${global.quux}
foo

-- 
-=( Ian Abbott @ MEV Ltd.    E-mail: <abbotti@mev.co.uk> )=-
-=(                          Web: http://www.mev.co.uk/  )=-

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: Setting MAC address from nv variable broken in barebox 2017.05.0?

[Ian Abbott]

On 08/05/17 15:45, Ian Abbott wrote:
> As a related follow-up, this code in globalvar_add_simple() looks a bit
> strange:
>
>     if (value)
>         dev_set_param(&global_device, name, value);
>
>     globalvar_nv_sync(name);
>
> If you set a new global variable with a specified value and there is an
> existing nv variable of the same name, the global variable will be set
> to the value of the nv variable, rather than the specified value:
>
> barebox@xxxx:/ nv -r quux
> barebox@xxxx:/ global -r quux
> barebox@xxxx:/ nv quux=foo
> barebox@xxxx:/ global quux=bar
> barebox@xxxx:/ echo ${global.quux}
> foo

That was with 2017.05.0.  I'll check the behaviour with Sascha's patch 
shortly.

-- 
-=( Ian Abbott @ MEV Ltd.    E-mail: <abbotti@mev.co.uk> )=-
-=(                          Web: http://www.mev.co.uk/  )=-

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: Setting MAC address from nv variable broken in barebox 2017.05.0?

[Ian Abbott]

On 08/05/17 15:34, Sascha Hauer wrote:
> Hi Ian,
>
> On Mon, May 08, 2017 at 02:39:19PM +0100, Ian Abbott wrote:
>> Hi,
>>
>> I'm not sure if this is a bug or whether I'm doing something wrong.  In
>> barebox 2017.04.0 and earlier, I stored the Ethernet MAC address in a
>> non-volatile ('nv') variable dev.eth0.macaddr=xx:xx:xx:xx:xx:xx and that got
>> propagated to 'global' and the eth0 device on boot:
>
> Yes this is broken. Sorry for breaking it and thank you for reporting it
> ;)
> I just sent out a fix, you're on Cc. Please let me know if this works.
>
> Sascha

Hi Sacha,

I tried it.  I had to modify the patch slightly to apply it directly to 
2017-05-0 due to other changes on master (I think mostly due to commit
0071bacb4c7cab21c9fab8540f5aa9922a270a85 ("param: remove unnecessary 
device_d * argument")).

The global.dev.eth0.ethaddr variable is not created, but the 
nv.dev.eth0.ethaddr variable does now get applied to the eth0 device.  I 
think this is as expected.

The other problem I mentioned about setting global variables is still 
present.  That is:

barebox@xxxx:/ nv -r quux
barebox@xxxx:/ global -r quux
barebox@xxxx:/ nv quux=foo
barebox@xxxx:/ global quux=bar
barebox@xxxx:/ echo ${global.quux}
foo
barebox@xxxx:/ global quux=i_really_mean_it
barebox@xxxx:/ echo ${global.quux}
foo

-- 
-=( Ian Abbott @ MEV Ltd.    E-mail: <abbotti@mev.co.uk> )=-
-=(                          Web: http://www.mev.co.uk/  )=-

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: Setting MAC address from nv variable broken in barebox 2017.05.0?

[Sascha Hauer]

On Mon, May 08, 2017 at 03:45:49PM +0100, Ian Abbott wrote:
> On 08/05/17 14:39, Ian Abbott wrote:
> > Hi,
> > 
> > I'm not sure if this is a bug or whether I'm doing something wrong.  In
> > barebox 2017.04.0 and earlier, I stored the Ethernet MAC address in a
> > non-volatile ('nv') variable dev.eth0.macaddr=xx:xx:xx:xx:xx:xx and that
> > got propagated to 'global' and the eth0 device on boot:
> > 
> > barebox@xxxx:/ nv
> >   allow_color: true
> >   autoboot_timeout: 3
> >   dev.eth0.ethaddr: xx:xx:xx:xx:xx:xx
> >   user: none
> [snip]
> > However, in 2017.05.0, my 'dev.eth0.ethaddr' variable is no longer being
> > propagated to global on boot, and as a consequence, is no longer
> > propagated to eth0 (note that 'dev.eth0.ethaddr' is the only nv variable
> > that I set manually when setting up a new board):
> > 
> > barebox@xxxx:/ devinfo eth0
> > Parameters:
> >   ethaddr: 00:00:00:00:00:00
> >   gateway: 0.0.0.0
> >   ipaddr: 0.0.0.0
> >   linux.bootargs:
> >   netmask: 0.0.0.0
> >   serverip: 0.0.0.0
> > 
> [snip]
> > Is this the proper behaviour or a bug?  Could it be related to commit
> > 35d8e858bea17ec4796069c9c27fd0b134125eaf ("nv: Do not create globalvars
> > from nvvars")?
> 
> As a related follow-up, this code in globalvar_add_simple() looks a bit
> strange:
> 
> 	if (value)
> 		dev_set_param(&global_device, name, value);
> 
> 	globalvar_nv_sync(name);

Ok, I think this should just be the other way round. Fixed in a patch I
just sent.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: Setting MAC address from nv variable broken in barebox 2017.05.0?

[Ian Abbott]

On 08/05/17 19:12, Sascha Hauer wrote:
> On Mon, May 08, 2017 at 03:45:49PM +0100, Ian Abbott wrote:
>> On 08/05/17 14:39, Ian Abbott wrote:
>>> Hi,
>>>
>>> I'm not sure if this is a bug or whether I'm doing something wrong.  In
>>> barebox 2017.04.0 and earlier, I stored the Ethernet MAC address in a
>>> non-volatile ('nv') variable dev.eth0.macaddr=xx:xx:xx:xx:xx:xx and that
>>> got propagated to 'global' and the eth0 device on boot:
>>>
>>> barebox@xxxx:/ nv
>>>   allow_color: true
>>>   autoboot_timeout: 3
>>>   dev.eth0.ethaddr: xx:xx:xx:xx:xx:xx
>>>   user: none
>> [snip]
>>> However, in 2017.05.0, my 'dev.eth0.ethaddr' variable is no longer being
>>> propagated to global on boot, and as a consequence, is no longer
>>> propagated to eth0 (note that 'dev.eth0.ethaddr' is the only nv variable
>>> that I set manually when setting up a new board):
>>>
>>> barebox@xxxx:/ devinfo eth0
>>> Parameters:
>>>   ethaddr: 00:00:00:00:00:00
>>>   gateway: 0.0.0.0
>>>   ipaddr: 0.0.0.0
>>>   linux.bootargs:
>>>   netmask: 0.0.0.0
>>>   serverip: 0.0.0.0
>>>
>> [snip]
>>> Is this the proper behaviour or a bug?  Could it be related to commit
>>> 35d8e858bea17ec4796069c9c27fd0b134125eaf ("nv: Do not create globalvars
>>> from nvvars")?
>>
>> As a related follow-up, this code in globalvar_add_simple() looks a bit
>> strange:
>>
>> 	if (value)
>> 		dev_set_param(&global_device, name, value);
>>
>> 	globalvar_nv_sync(name);
>
> Ok, I think this should just be the other way round. Fixed in a patch I
> just sent.

Yes, I thought that might be the case about it being the wrong way 
round.  I tested your patch and it works.

While on the subject, what is the desired behaviour when removing an nv 
variable?  Currently, removing an nv variable also deletes a global with 
the same name, even if the global has changed value in the meantime:

barebox@xxxx:/ global -r quux
barebox@xxxx:/ nv -r quux
barebox@xxxx:/ nv quux=foo
barebox@xxxx:/ global quux=bar
barebox@xxxx:/ nv -r quux
barebox@xxxx:/ global
   ... list of globals but 'quux' is missing ...

In 2017-04-0, the global variable remained, although its value was cleared.

-- 
-=( Ian Abbott @ MEV Ltd.    E-mail: <abbotti@mev.co.uk> )=-
-=(                          Web: http://www.mev.co.uk/  )=-

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: Setting MAC address from nv variable broken in barebox 2017.05.0?

[Sascha Hauer]

On Tue, May 09, 2017 at 11:02:12AM +0100, Ian Abbott wrote:
> On 08/05/17 19:12, Sascha Hauer wrote:
> > On Mon, May 08, 2017 at 03:45:49PM +0100, Ian Abbott wrote:
> > > On 08/05/17 14:39, Ian Abbott wrote:
> > > > Hi,
> > > > 
> > > > I'm not sure if this is a bug or whether I'm doing something wrong.  In
> > > > barebox 2017.04.0 and earlier, I stored the Ethernet MAC address in a
> > > > non-volatile ('nv') variable dev.eth0.macaddr=xx:xx:xx:xx:xx:xx and that
> > > > got propagated to 'global' and the eth0 device on boot:
> > > > 
> > > > barebox@xxxx:/ nv
> > > >   allow_color: true
> > > >   autoboot_timeout: 3
> > > >   dev.eth0.ethaddr: xx:xx:xx:xx:xx:xx
> > > >   user: none
> > > [snip]
> > > > However, in 2017.05.0, my 'dev.eth0.ethaddr' variable is no longer being
> > > > propagated to global on boot, and as a consequence, is no longer
> > > > propagated to eth0 (note that 'dev.eth0.ethaddr' is the only nv variable
> > > > that I set manually when setting up a new board):
> > > > 
> > > > barebox@xxxx:/ devinfo eth0
> > > > Parameters:
> > > >   ethaddr: 00:00:00:00:00:00
> > > >   gateway: 0.0.0.0
> > > >   ipaddr: 0.0.0.0
> > > >   linux.bootargs:
> > > >   netmask: 0.0.0.0
> > > >   serverip: 0.0.0.0
> > > > 
> > > [snip]
> > > > Is this the proper behaviour or a bug?  Could it be related to commit
> > > > 35d8e858bea17ec4796069c9c27fd0b134125eaf ("nv: Do not create globalvars
> > > > from nvvars")?
> > > 
> > > As a related follow-up, this code in globalvar_add_simple() looks a bit
> > > strange:
> > > 
> > > 	if (value)
> > > 		dev_set_param(&global_device, name, value);
> > > 
> > > 	globalvar_nv_sync(name);
> > 
> > Ok, I think this should just be the other way round. Fixed in a patch I
> > just sent.
> 
> Yes, I thought that might be the case about it being the wrong way round.  I
> tested your patch and it works.
> 
> While on the subject, what is the desired behaviour when removing an nv
> variable?  Currently, removing an nv variable also deletes a global with the
> same name, even if the global has changed value in the meantime:
> 
> barebox@xxxx:/ global -r quux
> barebox@xxxx:/ nv -r quux
> barebox@xxxx:/ nv quux=foo
> barebox@xxxx:/ global quux=bar
> barebox@xxxx:/ nv -r quux
> barebox@xxxx:/ global
>   ... list of globals but 'quux' is missing ...
> 
> In 2017-04-0, the global variable remained, although its value was cleared.

Are you sure the global variable is removed? I can't reproduce this
here. Here the globalvar is cleared as it used to be. However, this
behaviour is not very nice and could be changed with the following
patch.

Sacha

-----------------------8<-----------------------------------

From 45a00b7e6518856fab24afd4b883a6f9c18f9b49 Mon Sep 17 00:00:00 2001
From: Sascha Hauer <s.hauer@pengutronix.de>
Date: Thu, 11 May 2017 08:11:39 +0200
Subject: [PATCH] nv: When a nv var is removed, do not clear the globalvar

When a nv var is removed, do not clear the corresponding global var.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 common/globalvar.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/common/globalvar.c b/common/globalvar.c
index c528b24062..4922346130 100644
--- a/common/globalvar.c
+++ b/common/globalvar.c
@@ -164,12 +164,15 @@ static int nvvar_device_set(const char *name, const char *val)
 	return 0;
 }
 
-static int nv_set(struct param_d *p, const char *val)
+static int nv_set(struct param_d *p, const char *_val)
 {
 	struct param_d *g;
 	int ret;
+	const char *val;
 
-	if (!val)
+	if (_val)
+		val = _val;
+	else
 		val = "";
 
 	ret = nvvar_device_set(p->name, val);
@@ -177,7 +180,7 @@ static int nv_set(struct param_d *p, const char *val)
 		return ret;
 
 	g = get_param_by_name(&global_device, p->name);
-	if (g) {
+	if (g && _val) {
 		ret = dev_set_param(&global_device, p->name, val);
 		if (ret)
 			return ret;
-- 
2.11.0

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: Setting MAC address from nv variable broken in barebox 2017.05.0?

[Ian Abbott]

On 11/05/17 07:17, Sascha Hauer wrote:
> On Tue, May 09, 2017 at 11:02:12AM +0100, Ian Abbott wrote:
>> While on the subject, what is the desired behaviour when removing an nv
>> variable?  Currently, removing an nv variable also deletes a global with the
>> same name, even if the global has changed value in the meantime:
>>
>> barebox@xxxx:/ global -r quux
>> barebox@xxxx:/ nv -r quux
>> barebox@xxxx:/ nv quux=foo
>> barebox@xxxx:/ global quux=bar
>> barebox@xxxx:/ nv -r quux
>> barebox@xxxx:/ global
>>   ... list of globals but 'quux' is missing ...
>>
>> In 2017-04-0, the global variable remained, although its value was cleared.
>
> Are you sure the global variable is removed? I can't reproduce this
> here. Here the globalvar is cleared as it used to be. However, this
> behaviour is not very nice and could be changed with the following
> patch.

Sorry, my mistake.  You are correct, the global var remains but is cleared.

-- 
-=( Ian Abbott @ MEV Ltd.    E-mail: <abbotti@mev.co.uk> )=-
-=(                          Web: http://www.mev.co.uk/  )=-

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox