[U-Boot] Inconsistency between $filesize and commands which accept numeric params.

[U-Boot] Inconsistency between $filesize and commands which accept numeric params.

[Ian Campbell]

It seems there is some inconsistency wrt number base between commands
which set $filesize in the env and the commands which might consume
them.

e.g.
        
        sun7i# load scsi 0 $fdt_addr_r dtbs/$fdtfile
        21639 bytes read in 191 ms (110.4 KiB/s)
        sun7i# printenv filesize
        filesize=5487
        
So filesize is in hex, but without an 0x prefix. But:
        
        sun7i# fdt addr $fdt_addr_r 0x10000 
        sun7i# fdt set /chosen foo <$filesize>
        sun7i# fdt print /chosen foo          
        foo = <0x0000156f>
        
IOW the parameter to fdt set has been interpreted as a decimal.

If $filesize happens to contains digits [a-f] then:

        sun7i# load scsi 0 $fdt_addr_r dtbs/sun6i-a31-app4-evb1.dtb
        16563 bytes read in 478 ms (33.2 KiB/s)
        sun7i# printenv filesize
        filesize=40b3
        sun7i# fdt set /chosen foo <$filesize>                     
        Sorry, I could not convert "b3>"
        
Obviously I can hack around this with:
        sun7i# fdt set /chosen foo <0x$filesize>

But that seems a bit of an odd thing to have to do (not to mention
potentially fragile).

There seem to be other commands, e.g. mmc write which interpret their
parameters as hex by default, although in that case it does also DTRT
with an 0x prefix.

So I'm not sure if the bug is that setenv_hex doesn't include the 0x, or
that fdt set interprets things as decimal by default instead of hex. Or
maybe there is no bug at all?

Just changing the setenv_hex to include the 0x seemed at first glance
like a good idea, but I'm not really sure.

Thanks,
Ian.

PS my usecase is to implement the fdt based multiboot protocol which Xen
uses in order to load boot-modules, see e.g.
http://wiki.xen.org/wiki/Xen_ARM_with_Virtualization_Extensions/Allwinner#Boot_script
http://wiki.xen.org/wiki/Xen_ARM_with_Virtualization_Extensions/Multiboot
(we use something like this on all u-boot based platforms, Allwinner is
just an example).

Ian.

[U-Boot] Inconsistency between $filesize and commands which accept numeric params.

[Wolfgang Denk]

Dear Ian,

In message <1414677435.2064.34.camel@hellion.org.uk> you wrote:
> It seems there is some inconsistency wrt number base between commands
> which set $filesize in the env and the commands which might consume
> them.

That would be bugs, then.

>         
>         sun7i# load scsi 0 $fdt_addr_r dtbs/$fdtfile
>         21639 bytes read in 191 ms (110.4 KiB/s)
>         sun7i# printenv filesize
>         filesize=5487
>         
> So filesize is in hex, but without an 0x prefix. But:

This is normal.  U-Boot uses hex input base by default.  All commands
should take hex input; the only inglorious exception is the "sleep"
command which takes decimal; numbers as arguments.

>         sun7i# fdt addr $fdt_addr_r 0x10000 
>         sun7i# fdt set /chosen foo <$filesize>
>         sun7i# fdt print /chosen foo          
>         foo = <0x0000156f>
>         
> IOW the parameter to fdt set has been interpreted as a decimal.

That's a bug.

> So I'm not sure if the bug is that setenv_hex doesn't include the 0x, or

No 0x prefix should be needed anywhere.

> that fdt set interprets things as decimal by default instead of hex. Or
> maybe there is no bug at all?

The bug is in fdt set, then.


Thanks for pointing out!

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
Prediction is very difficult, especially of the future.  - Niels Bohr

[U-Boot] Inconsistency between $filesize and commands which accept numeric params.

[Ian Campbell]

On Thu, 2014-10-30 at 16:21 +0100, Wolfgang Denk wrote:
> Dear Ian,
> 
> In message <1414677435.2064.34.camel@hellion.org.uk> you wrote:
> > It seems there is some inconsistency wrt number base between commands
> > which set $filesize in the env and the commands which might consume
> > them.
> 
> That would be bugs, then.
> 
> >         
> >         sun7i# load scsi 0 $fdt_addr_r dtbs/$fdtfile
> >         21639 bytes read in 191 ms (110.4 KiB/s)
> >         sun7i# printenv filesize
> >         filesize=5487
> >         
> > So filesize is in hex, but without an 0x prefix. But:
> 
> This is normal.  U-Boot uses hex input base by default.  All commands
> should take hex input; the only inglorious exception is the "sleep"
> command which takes decimal; numbers as arguments.
> 
> >         sun7i# fdt addr $fdt_addr_r 0x10000 
> >         sun7i# fdt set /chosen foo <$filesize>
> >         sun7i# fdt print /chosen foo          
> >         foo = <0x0000156f>
> >         
> > IOW the parameter to fdt set has been interpreted as a decimal.
> 
> That's a bug.
> 
> > So I'm not sure if the bug is that setenv_hex doesn't include the 0x, or
> 
> No 0x prefix should be needed anywhere.
> 
> > that fdt set interprets things as decimal by default instead of hex. Or
> > maybe there is no bug at all?
> 
> The bug is in fdt set, then.
> 
> 
> Thanks for pointing out!

Thanks for pointing me in the right direction. CCing the folks who
get_maintainers.pl tells me might be involved with common/cmd_fdt.c

Looks like the function which does this is fdt_parse_prop, which is
documented with:
/*
 * Parse the user's input, partially heuristic.  Valid formats:
 * <0x00112233 4 05>	- an array of cells.  Numbers follow standard
 *			C conventions.
 * [00 11 22 .. nn] - byte stream
 * "string"	- If the the value doesn't start with "<" or "[", it is
 *			treated as a string.  Note that the quotes are
 *			stripped by the parser before we get the string.

which is inconsistent with the "U-Boot uses hex input base by default"
mantra.

Ian.

[U-Boot] Inconsistency between $filesize and commands which accept numeric params

[Tom Rini]

On Thu, Oct 30, 2014 at 03:32:58PM +0000, Ian Campbell wrote:
> On Thu, 2014-10-30 at 16:21 +0100, Wolfgang Denk wrote:
> > Dear Ian,
> > 
> > In message <1414677435.2064.34.camel@hellion.org.uk> you wrote:
> > > It seems there is some inconsistency wrt number base between commands
> > > which set $filesize in the env and the commands which might consume
> > > them.
> > 
> > That would be bugs, then.
> > 
> > >         
> > >         sun7i# load scsi 0 $fdt_addr_r dtbs/$fdtfile
> > >         21639 bytes read in 191 ms (110.4 KiB/s)
> > >         sun7i# printenv filesize
> > >         filesize=5487
> > >         
> > > So filesize is in hex, but without an 0x prefix. But:
> > 
> > This is normal.  U-Boot uses hex input base by default.  All commands
> > should take hex input; the only inglorious exception is the "sleep"
> > command which takes decimal; numbers as arguments.
> > 
> > >         sun7i# fdt addr $fdt_addr_r 0x10000 
> > >         sun7i# fdt set /chosen foo <$filesize>
> > >         sun7i# fdt print /chosen foo          
> > >         foo = <0x0000156f>
> > >         
> > > IOW the parameter to fdt set has been interpreted as a decimal.
> > 
> > That's a bug.
> > 
> > > So I'm not sure if the bug is that setenv_hex doesn't include the 0x, or
> > 
> > No 0x prefix should be needed anywhere.
> > 
> > > that fdt set interprets things as decimal by default instead of hex. Or
> > > maybe there is no bug at all?
> > 
> > The bug is in fdt set, then.
> > 
> > 
> > Thanks for pointing out!
> 
> Thanks for pointing me in the right direction. CCing the folks who
> get_maintainers.pl tells me might be involved with common/cmd_fdt.c
> 
> Looks like the function which does this is fdt_parse_prop, which is
> documented with:
> /*
>  * Parse the user's input, partially heuristic.  Valid formats:
>  * <0x00112233 4 05>	- an array of cells.  Numbers follow standard
>  *			C conventions.
>  * [00 11 22 .. nn] - byte stream
>  * "string"	- If the the value doesn't start with "<" or "[", it is
>  *			treated as a string.  Note that the quotes are
>  *			stripped by the parser before we get the string.
> 
> which is inconsistent with the "U-Boot uses hex input base by default"
> mantra.

Because this is FDT centric and not U-Boot centric, yes?  Since <decimal
numbers> is a valid thing to do in an entry here.  I think the right
answer is the more hacky sounding one of when you script using 'fdt set'
you need to be mindful of the constraints on input that are imposed upon
us by FDT.  <hex> is invalid but <0xhex> is what is expected.

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

[U-Boot] Inconsistency between $filesize and commands which accept numeric params

[Ian Campbell]

On Tue, 2014-11-04 at 10:44 -0500, Tom Rini wrote:
> On Thu, Oct 30, 2014 at 03:32:58PM +0000, Ian Campbell wrote:
> > On Thu, 2014-10-30 at 16:21 +0100, Wolfgang Denk wrote:
> > > Dear Ian,
> > > 
> > > In message <1414677435.2064.34.camel@hellion.org.uk> you wrote:
> > > > It seems there is some inconsistency wrt number base between commands
> > > > which set $filesize in the env and the commands which might consume
> > > > them.
> > > 
> > > That would be bugs, then.
> > > 
> > > >         
> > > >         sun7i# load scsi 0 $fdt_addr_r dtbs/$fdtfile
> > > >         21639 bytes read in 191 ms (110.4 KiB/s)
> > > >         sun7i# printenv filesize
> > > >         filesize=5487
> > > >         
> > > > So filesize is in hex, but without an 0x prefix. But:
> > > 
> > > This is normal.  U-Boot uses hex input base by default.  All commands
> > > should take hex input; the only inglorious exception is the "sleep"
> > > command which takes decimal; numbers as arguments.
> > > 
> > > >         sun7i# fdt addr $fdt_addr_r 0x10000 
> > > >         sun7i# fdt set /chosen foo <$filesize>
> > > >         sun7i# fdt print /chosen foo          
> > > >         foo = <0x0000156f>
> > > >         
> > > > IOW the parameter to fdt set has been interpreted as a decimal.
> > > 
> > > That's a bug.
> > > 
> > > > So I'm not sure if the bug is that setenv_hex doesn't include the 0x, or
> > > 
> > > No 0x prefix should be needed anywhere.
> > > 
> > > > that fdt set interprets things as decimal by default instead of hex. Or
> > > > maybe there is no bug at all?
> > > 
> > > The bug is in fdt set, then.
> > > 
> > > 
> > > Thanks for pointing out!
> > 
> > Thanks for pointing me in the right direction. CCing the folks who
> > get_maintainers.pl tells me might be involved with common/cmd_fdt.c
> > 
> > Looks like the function which does this is fdt_parse_prop, which is
> > documented with:
> > /*
> >  * Parse the user's input, partially heuristic.  Valid formats:
> >  * <0x00112233 4 05>	- an array of cells.  Numbers follow standard
> >  *			C conventions.
> >  * [00 11 22 .. nn] - byte stream
> >  * "string"	- If the the value doesn't start with "<" or "[", it is
> >  *			treated as a string.  Note that the quotes are
> >  *			stripped by the parser before we get the string.
> > 
> > which is inconsistent with the "U-Boot uses hex input base by default"
> > mantra.
> 
> Because this is FDT centric and not U-Boot centric, yes?

I guess?

>   Since <decimal
> numbers> is a valid thing to do in an entry here.  I think the right
> answer is the more hacky sounding one of when you script using 'fdt set'
> you need to be mindful of the constraints on input that are imposed upon
> us by FDT.  <hex> is invalid but <0xhex> is what is expected.

The 0x prefix hack/trick is what I'm actually doing, but the maintainer
of our test system wanted some assurances that u-boot wasn't going to
change under our feet if someone fixed a bug.

Sounds like you are saying this is all working as designed, and there is
no bug, but I'll let Wolfgang and you thrash that out ;-).

Ian.

[U-Boot] Inconsistency between $filesize and commands which accept numeric params

[Wolfgang Denk]

Dear Ian,

In message <1415116135.11486.51.camel@hellion.org.uk> you wrote:
>
> > > /*
> > >  * Parse the user's input, partially heuristic.  Valid formats:
> > >  * <0x00112233 4 05>	- an array of cells.  Numbers follow standard
> > >  *			C conventions.
> > >  * [00 11 22 .. nn] - byte stream
> > >  * "string"	- If the the value doesn't start with "<" or "[", it is
> > >  *			treated as a string.  Note that the quotes are
> > >  *			stripped by the parser before we get the string.
> > > 
> > > which is inconsistent with the "U-Boot uses hex input base by default"
> > > mantra.
> > 
> > Because this is FDT centric and not U-Boot centric, yes?
> 
> I guess?

Well, we probably should not use this FDT centric decoding function
directly on arguments passed from the U-Boot command line.

> >   Since <decimal
> > numbers> is a valid thing to do in an entry here.  I think the right
> > answer is the more hacky sounding one of when you script using 'fdt set'
> > you need to be mindful of the constraints on input that are imposed upon
> > us by FDT.  <hex> is invalid but <0xhex> is what is expected.
> 
> The 0x prefix hack/trick is what I'm actually doing, but the maintainer
> of our test system wanted some assurances that u-boot wasn't going to
> change under our feet if someone fixed a bug.

I think this is a hack that is bound to break.  Today you use
$filesize, tomorrow you may find you need to perform some operations
on this (say, rounding up in some way), and you may end up with a
variable that already contains a "0x" prefix, and your code will fail
beause 0x0x... gets misinterpreted.

> Sounds like you are saying this is all working as designed, and there is
> no bug, but I'll let Wolfgang and you thrash that out ;-).

Tom: this is a bug ;-)

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
The rule on staying alive as a program manager is to give 'em a  num-
ber or give 'em a date, but never give 'em both at once.

[U-Boot] Inconsistency between $filesize and commands which accept numeric params

[Tom Rini]

On Tue, Nov 04, 2014 at 09:53:02PM +0100, Wolfgang Denk wrote:
> Dear Ian,
> 
> In message <1415116135.11486.51.camel@hellion.org.uk> you wrote:
> >
> > > > /*
> > > >  * Parse the user's input, partially heuristic.  Valid formats:
> > > >  * <0x00112233 4 05>	- an array of cells.  Numbers follow standard
> > > >  *			C conventions.
> > > >  * [00 11 22 .. nn] - byte stream
> > > >  * "string"	- If the the value doesn't start with "<" or "[", it is
> > > >  *			treated as a string.  Note that the quotes are
> > > >  *			stripped by the parser before we get the string.
> > > > 
> > > > which is inconsistent with the "U-Boot uses hex input base by default"
> > > > mantra.
> > > 
> > > Because this is FDT centric and not U-Boot centric, yes?
> > 
> > I guess?
> 
> Well, we probably should not use this FDT centric decoding function
> directly on arguments passed from the U-Boot command line.

No, look again at what we're doing.  We're not calling a U-Boot function
to do U-Boot things.  We're calling a U-Boot function to set a DT
property.  U-Boot needs to obey DT conventions here.

I can see an argument that we could try and make the heuristics smarter
to add '0x' to obvious hex strings, but that'll just cause crazier
errors when 0x1000 is passed in and seen as 1000 decimal.

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

[U-Boot] Inconsistency between $filesize and commands which accept numeric params

[Wolfgang Denk]

Dear Tom,

In message <20141104205800.GM15133@bill-the-cat> you wrote:
> 
> > Well, we probably should not use this FDT centric decoding function
> > directly on arguments passed from the U-Boot command line.
> 
> No, look again at what we're doing.  We're not calling a U-Boot function
> to do U-Boot things.  We're calling a U-Boot function to set a DT
> property.  U-Boot needs to obey DT conventions here.

Well, we are running an U-Boot command.  And U-Boot documentation says
that we use hex inpout base (with the only unlucky exception of the
sleep command).  So this _command_ should also assume hex input.

> I can see an argument that we could try and make the heuristics smarter
> to add '0x' to obvious hex strings, but that'll just cause crazier
> errors when 0x1000 is passed in and seen as 1000 decimal.

Right.  So before calling the fdt code, the U-Boot command should
normalize the number.

Hm.... is there a way to determine in advance where we are passing a
number as argument [see "common/cmd_fdt.c" lines 260ff] ?

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
"What if" is a trademark of Hewlett Packard, so stop using it in your
sentences without permission, or risk being sued.

[U-Boot] Inconsistency between $filesize and commands which accept numeric params

[Tom Rini]

On Tue, Nov 04, 2014 at 10:20:46PM +0100, Wolfgang Denk wrote:
> Dear Tom,
> 
> In message <20141104205800.GM15133@bill-the-cat> you wrote:
> > 
> > > Well, we probably should not use this FDT centric decoding function
> > > directly on arguments passed from the U-Boot command line.
> > 
> > No, look again at what we're doing.  We're not calling a U-Boot function
> > to do U-Boot things.  We're calling a U-Boot function to set a DT
> > property.  U-Boot needs to obey DT conventions here.
> 
> Well, we are running an U-Boot command.  And U-Boot documentation says
> that we use hex inpout base (with the only unlucky exception of the
> sleep command).  So this _command_ should also assume hex input.

... when we can make assumptions about the input, yes.

> > I can see an argument that we could try and make the heuristics smarter
> > to add '0x' to obvious hex strings, but that'll just cause crazier
> > errors when 0x1000 is passed in and seen as 1000 decimal.
> 
> Right.  So before calling the fdt code, the U-Boot command should
> normalize the number.
> 
> Hm.... is there a way to determine in advance where we are passing a
> number as argument [see "common/cmd_fdt.c" lines 260ff] ?

The problem is that 'fdt set ... <decimal>' is valid and 'fdt set ...
<0xhex>' is valid (as it 'fdt set ... string'). What I'm saying is that
we should think of it as 'fdt set <input to DT>' and within <input to
DT> we assume DT conventions, not U-Boot conventions.

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

[U-Boot] Inconsistency between $filesize and commands which accept numeric params

[Wolfgang Denk]

Dear Tom,

In message <20141104220213.GN15133@bill-the-cat> you wrote:
> 
> > Hm.... is there a way to determine in advance where we are passing a
> > number as argument [see "common/cmd_fdt.c" lines 260ff] ?
> 
> The problem is that 'fdt set ... <decimal>' is valid and 'fdt set ...
> <0xhex>' is valid (as it 'fdt set ... string'). What I'm saying is that
> we should think of it as 'fdt set <input to DT>' and within <input to
> DT> we assume DT conventions, not U-Boot conventions.

Yes, I fully understand what you mean.  I just wonder if there is a
user-friendly way to prevent such errors.

I think we should at least document this behaviour - both in the help
message for this command, and in the manual.

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
A good aphorism is too hard for the tooth of time, and  is  not  worn
away  by  all  the  centuries,  although  it serves as food for every
epoch.                                  - Friedrich Wilhelm Nietzsche
                          _Miscellaneous Maxims and Opinions_ no. 168

[U-Boot] Inconsistency between $filesize and commands which accept numeric params

[Tom Rini]

On Tue, Nov 04, 2014 at 11:11:40PM +0100, Wolfgang Denk wrote:
> Dear Tom,
> 
> In message <20141104220213.GN15133@bill-the-cat> you wrote:
> > 
> > > Hm.... is there a way to determine in advance where we are passing a
> > > number as argument [see "common/cmd_fdt.c" lines 260ff] ?
> > 
> > The problem is that 'fdt set ... <decimal>' is valid and 'fdt set ...
> > <0xhex>' is valid (as it 'fdt set ... string'). What I'm saying is that
> > we should think of it as 'fdt set <input to DT>' and within <input to
> > DT> we assume DT conventions, not U-Boot conventions.
> 
> Yes, I fully understand what you mean.  I just wonder if there is a
> user-friendly way to prevent such errors.
> 
> I think we should at least document this behaviour - both in the help
> message for this command, and in the manual.

I think this is a documentation (readme and command help) only thing.

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