Re: [U-Boot] [PATCH v2 4/5] lib: Import hexdump.c from Linux

[Alexey Brodkin]

On Wed, 2018-06-13 at 10:24 +0200, Mario Six wrote:
> Hi Alexey,
> 
> On Mon, Jun 4, 2018 at 6:05 PM, Alexey Brodkin
>  wrote:
> > Hi Mario,
> > 
> > On Wed, 2018-05-23 at 14:09 +0200, Mario Six wrote:
> > > Especially for commands, it is useful to be able to turn a hexadecimal
> > > string into its binary representation.
> > > 
> > > Hence, import the hex_to_bin, bin2hex, and hex2bin functions from the
> > > Linux kernel.
> > > 
> > > Signed-off-by: Mario Six 
> > > 
> > > ---
> > > 
> > > v1 -> v2:
> > > New in v2
> > 
> > Something is missing?
> > 
> > Note there was a similar discussion some time ago here:
> > https://urldefense.proofpoint.com/v2/url?u=https-3A__patchwork.ozlabs.org_patch_633733_=DwIBaQ=DPL6_X_6JkXFx7AXWqB0tg=lqdeeSSEes0GFDDl656eVi
> > XO7breS55ytWkhpk5R81I=mnw00As1T_NpnFNncTURuK4YkIAYD-Dj-VTNkRqquVY=HcgjkTO1GdtGqH9sYIjizH30AC3m_Xfa1F6n_Cy_qZY=,
> >  might worth checking.
> > 
> > If of any interest you may pick up my earlier patch and do
> > fix-ups mentioned by Tom:
> >  1. Move hexdump.h away from common.h
> >  2. Update existing users of print_hex_dump() in U-Boot
> > such that they don't use debug level (i.e. drop the first argument)
> > 
> > Or I may do the same re-spin sometime soon.
> > 
> 
> Thanks for your feedback! I saw that you posted a re-spin of your patch;
> Thank you, that's very helpful.

FWIW Tom just pulled that in, see
http://git.denx.de/?p=u-boot.git;a=commit;h=f8c987f8f127f867d96ca74bcd1fcb11d8265b67

> 
> > Still read-on for a couple of comments for your patch.
> > 
> > [snip]
> > 
> > >  /*
> > >   * min()/max()/clamp() macros that also do
> > >   * strict type-checking.. See the
> > > diff --git a/lib/Makefile b/lib/Makefile
> > > index d531ea54b31..0f6d744579f 100644
> > > --- a/lib/Makefile
> > > +++ b/lib/Makefile
> > > @@ -29,6 +29,7 @@ obj-$(CONFIG_FIT) += fdtdec_common.o
> > >  obj-$(CONFIG_TEST_FDTDEC) += fdtdec_test.o
> > >  obj-$(CONFIG_GZIP_COMPRESSED) += gzip.o
> > >  obj-$(CONFIG_GENERATE_SMBIOS_TABLE) += smbios.o
> > > +obj-y += hexdump.o
> > 
> > U-Boot might be used on targets with limited memory
> > so having ability to include hexdump or not might be
> > beneficial here. Especially in production builds why would you need hexdump?
> > 
> 
> Yes, it's probably better to have it deactivatable, true. But as for why
> production builds need hexdump: It's not so much the hexdump function, but the
> bin2hex function, which can be used in a number of U-Boot commands that read
> hexadecimal data. We use one such command to initialize a TPM on one of our
> boards, for example.

I spoke a bit too soon as I forgot what I did before :)
We don't disable building of entire hexdump.o instead in hexdump.c we
just put some code in #ifdefs that way you may move bin2hex() outside
#ifdef.

-Alexey
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] [PATCH v2 4/5] lib: Import hexdump.c from Linux

[Mario Six]

Hi Alexey,

On Mon, Jun 4, 2018 at 6:05 PM, Alexey Brodkin
 wrote:
> Hi Mario,
>
> On Wed, 2018-05-23 at 14:09 +0200, Mario Six wrote:
>> Especially for commands, it is useful to be able to turn a hexadecimal
>> string into its binary representation.
>>
>> Hence, import the hex_to_bin, bin2hex, and hex2bin functions from the
>> Linux kernel.
>>
>> Signed-off-by: Mario Six 
>>
>> ---
>>
>> v1 -> v2:
>> New in v2
>
> Something is missing?
>
> Note there was a similar discussion some time ago here:
> https://patchwork.ozlabs.org/patch/633733/, might worth checking.
>
> If of any interest you may pick up my earlier patch and do
> fix-ups mentioned by Tom:
>  1. Move hexdump.h away from common.h
>  2. Update existing users of print_hex_dump() in U-Boot
> such that they don't use debug level (i.e. drop the first argument)
>
> Or I may do the same re-spin sometime soon.
>

Thanks for your feedback! I saw that you posted a re-spin of your patch;
Thank you, that's very helpful.

> Still read-on for a couple of comments for your patch.
>
> [snip]
>
>>  /*
>>   * min()/max()/clamp() macros that also do
>>   * strict type-checking.. See the
>> diff --git a/lib/Makefile b/lib/Makefile
>> index d531ea54b31..0f6d744579f 100644
>> --- a/lib/Makefile
>> +++ b/lib/Makefile
>> @@ -29,6 +29,7 @@ obj-$(CONFIG_FIT) += fdtdec_common.o
>>  obj-$(CONFIG_TEST_FDTDEC) += fdtdec_test.o
>>  obj-$(CONFIG_GZIP_COMPRESSED) += gzip.o
>>  obj-$(CONFIG_GENERATE_SMBIOS_TABLE) += smbios.o
>> +obj-y += hexdump.o
>
> U-Boot might be used on targets with limited memory
> so having ability to include hexdump or not might be
> beneficial here. Especially in production builds why would you need hexdump?
>

Yes, it's probably better to have it deactivatable, true. But as for why
production builds need hexdump: It's not so much the hexdump function, but the
bin2hex function, which can be used in a number of U-Boot commands that read
hexadecimal data. We use one such command to initialize a TPM on one of our
boards, for example.

> [snip]
>
>> +#ifdef CONFIG_PRINTK
>
> Why PRINTK in U-Boot? It's purely kernel's thing.
>
>> +#if !defined(CONFIG_DYNAMIC_DEBUG)
>
> Ditto, CONFIG_DYNAMIC_DEBUG has nothing to do with U-Boot.
>

Those were copied verbatim from the kernel, I'm pretty sure I just got them by
mistake.

> -Alexey
>

Best regards,
Mario
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] [PATCH v2 4/5] lib: Import hexdump.c from Linux

[Alexey Brodkin]

Hi Mario,

On Wed, 2018-05-23 at 14:09 +0200, Mario Six wrote:
> Especially for commands, it is useful to be able to turn a hexadecimal
> string into its binary representation.
> 
> Hence, import the hex_to_bin, bin2hex, and hex2bin functions from the
> Linux kernel.
> 
> Signed-off-by: Mario Six 
> 
> ---
> 
> v1 -> v2:
> New in v2

Something is missing?

Note there was a similar discussion some time ago here:
https://patchwork.ozlabs.org/patch/633733/, might worth checking.

If of any interest you may pick up my earlier patch and do
fix-ups mentioned by Tom:
 1. Move hexdump.h away from common.h
 2. Update existing users of print_hex_dump() in U-Boot
such that they don't use debug level (i.e. drop the first argument)

Or I may do the same re-spin sometime soon.

Still read-on for a couple of comments for your patch.

[snip]

>  /*
>   * min()/max()/clamp() macros that also do
>   * strict type-checking.. See the
> diff --git a/lib/Makefile b/lib/Makefile
> index d531ea54b31..0f6d744579f 100644
> --- a/lib/Makefile
> +++ b/lib/Makefile
> @@ -29,6 +29,7 @@ obj-$(CONFIG_FIT) += fdtdec_common.o
>  obj-$(CONFIG_TEST_FDTDEC) += fdtdec_test.o
>  obj-$(CONFIG_GZIP_COMPRESSED) += gzip.o
>  obj-$(CONFIG_GENERATE_SMBIOS_TABLE) += smbios.o
> +obj-y += hexdump.o

U-Boot might be used on targets with limited memory
so having ability to include hexdump or not might be
beneficial here. Especially in production builds why would you need hexdump?

[snip]

> +#ifdef CONFIG_PRINTK

Why PRINTK in U-Boot? It's purely kernel's thing.

> +#if !defined(CONFIG_DYNAMIC_DEBUG)

Ditto, CONFIG_DYNAMIC_DEBUG has nothing to do with U-Boot.

-Alexey
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] [PATCH v2 4/5] lib: Import hexdump.c from Linux

[Mario Six]

Hi Simon,

On Wed, May 23, 2018 at 6:33 PM, Simon Glass  wrote:
> Hi Mario,
>
> On 23 May 2018 at 06:09, Mario Six  wrote:
>> Especially for commands, it is useful to be able to turn a hexadecimal
>> string into its binary representation.
>>
>> Hence, import the hex_to_bin, bin2hex, and hex2bin functions from the
>> Linux kernel.
>>
>> Signed-off-by: Mario Six 
>>
>> ---
>>
>> v1 -> v2:
>> New in v2
>>
>> ---
>>  include/linux/kernel.h |   4 +
>>  lib/Makefile   |   1 +
>>  lib/hexdump.c  | 321 
>> +
>>  3 files changed, 326 insertions(+)
>>  create mode 100644 lib/hexdump.c
>
> Does Linux have any tests for this code?
>

No, there are no tests for this. I'll write some for v3, then.

> Regards,
> Simon
>

Best regards,
Mario
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] [PATCH v2 4/5] lib: Import hexdump.c from Linux

[Simon Glass]

Hi Mario,

On 23 May 2018 at 06:09, Mario Six  wrote:
> Especially for commands, it is useful to be able to turn a hexadecimal
> string into its binary representation.
>
> Hence, import the hex_to_bin, bin2hex, and hex2bin functions from the
> Linux kernel.
>
> Signed-off-by: Mario Six 
>
> ---
>
> v1 -> v2:
> New in v2
>
> ---
>  include/linux/kernel.h |   4 +
>  lib/Makefile   |   1 +
>  lib/hexdump.c  | 321 
> +
>  3 files changed, 326 insertions(+)
>  create mode 100644 lib/hexdump.c

Does Linux have any tests for this code?

Regards,
Simon
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot