[PATCH] lib/test_hexdump: fix failure on big endian cpu

[PATCH] lib/test_hexdump: fix failure on big endian cpu

[Christophe Leroy]

On a big endian cpu, test_hexdump fails as follows. The logs show
that bytes are expected in reversed order.

[...]
[   16.643648] test_hexdump: Len: 24 buflen: 130 strlen: 97
[   16.648681] test_hexdump: Result: 97 'be32db7b 0a1893b2 70bac424 7d83349b a69c31ad 9c0face9                    .2.{....p..$}.4...1.....'
[   16.660951] test_hexdump: Expect: 97 '7bdb32be b293180a 24c4ba70 9b34837d ad319ca6 e9ac0f9c                    .2.{....p..$}.4...1.....'
[   16.673129] test_hexdump: Len: 8 buflen: 130 strlen: 77
[   16.678113] test_hexdump: Result: 77 'be32db7b0a1893b2                                                     .2.{....'
[   16.688660] test_hexdump: Expect: 77 'b293180a7bdb32be                                                     .2.{....'
[   16.699170] test_hexdump: Len: 6 buflen: 131 strlen: 87
[   16.704238] test_hexdump: Result: 87 'be32 db7b 0a18                                                                   .2.{..'
[   16.715511] test_hexdump: Expect: 87 '32be 7bdb 180a                                                                   .2.{..'
[   16.726864] test_hexdump: Len: 24 buflen: 131 strlen: 97
[   16.731902] test_hexdump: Result: 97 'be32db7b 0a1893b2 70bac424 7d83349b a69c31ad 9c0face9                    .2.{....p..$}.4...1.....'
[   16.744175] test_hexdump: Expect: 97 '7bdb32be b293180a 24c4ba70 9b34837d ad319ca6 e9ac0f9c                    .2.{....p..$}.4...1.....'
[   16.756379] test_hexdump: Len: 32 buflen: 131 strlen: 101
[   16.761507] test_hexdump: Result: 101 'be32db7b0a1893b2 70bac4247d83349b a69c31ad9c0face9 4cd1199943b1af0c  .2.{....p..$}.4...1.....L...C...'
[   16.774212] test_hexdump: Expect: 101 'b293180a7bdb32be 9b34837d24c4ba70 e9ac0f9cad319ca6 0cafb1439919d14c  .2.{....p..$}.4...1.....L...C...'
[   16.786763] test_hexdump: failed 801 out of 1184 tests

This patch fixes it.

Fixes: 64d1d77a44697 ("hexdump: introduce test suite")
Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
---
 lib/test_hexdump.c | 28 +++++++++++++++++++++++-----
 1 file changed, 23 insertions(+), 5 deletions(-)

diff --git a/lib/test_hexdump.c b/lib/test_hexdump.c
index 3f415d8101f3..626f580b4ff7 100644
--- a/lib/test_hexdump.c
+++ b/lib/test_hexdump.c
@@ -18,7 +18,7 @@ static const unsigned char data_b[] = {
 
 static const unsigned char data_a[] = ".2.{....p..$}.4...1.....L...C...";
 
-static const char * const test_data_1_le[] __initconst = {
+static const char * const test_data_1[] __initconst = {
 	"be", "32", "db", "7b", "0a", "18", "93", "b2",
 	"70", "ba", "c4", "24", "7d", "83", "34", "9b",
 	"a6", "9c", "31", "ad", "9c", "0f", "ac", "e9",
@@ -32,16 +32,33 @@ static const char * const test_data_2_le[] __initconst = {
 	"d14c", "9919", "b143", "0caf",
 };
 
+static const char * const test_data_2_be[] __initconst = {
+	"be32", "db7b", "0a18", "93b2",
+	"70ba", "c424", "7d83", "349b",
+	"a69c", "31ad", "9c0f", "ace9",
+	"4cd1", "1999", "43b1", "af0c",
+};
+
 static const char * const test_data_4_le[] __initconst = {
 	"7bdb32be", "b293180a", "24c4ba70", "9b34837d",
 	"ad319ca6", "e9ac0f9c", "9919d14c", "0cafb143",
 };
 
+static const char * const test_data_4_be[] __initconst = {
+	"be32db7b", "0a1893b2", "70bac424", "7d83349b",
+	"a69c31ad", "9c0face9", "4cd11999", "43b1af0c",
+};
+
 static const char * const test_data_8_le[] __initconst = {
 	"b293180a7bdb32be", "9b34837d24c4ba70",
 	"e9ac0f9cad319ca6", "0cafb1439919d14c",
 };
 
+static const char * const test_data_8_be[] __initconst = {
+	"be32db7b0a1893b2", "70bac4247d83349b",
+	"a69c31ad9c0face9", "4cd1199943b1af0c",
+};
+
 #define FILL_CHAR	'#'
 
 static unsigned total_tests __initdata;
@@ -56,6 +73,7 @@ static void __init test_hexdump_prepare_test(size_t len, int rowsize,
 	size_t l = len;
 	int gs = groupsize, rs = rowsize;
 	unsigned int i;
+	const bool is_be = IS_ENABLED(CONFIG_CPU_BIG_ENDIAN);
 
 	if (rs != 16 && rs != 32)
 		rs = 16;
@@ -67,13 +85,13 @@ static void __init test_hexdump_prepare_test(size_t len, int rowsize,
 		gs = 1;
 
 	if (gs == 8)
-		result = test_data_8_le;
+		result = is_be ? test_data_8_be : test_data_8_le;
 	else if (gs == 4)
-		result = test_data_4_le;
+		result = is_be ? test_data_4_be : test_data_4_le;
 	else if (gs == 2)
-		result = test_data_2_le;
+		result = is_be ? test_data_2_be : test_data_2_le;
 	else
-		result = test_data_1_le;
+		result = test_data_1;
 
 	/* hex dump */
 	p = test;
-- 
2.13.3

Re: [PATCH] lib/test_hexdump: fix failure on big endian cpu

[Michael Ellerman]

Christophe Leroy <christophe.leroy@c-s.fr> writes:
> diff --git a/lib/test_hexdump.c b/lib/test_hexdump.c
> index 3f415d8101f3..626f580b4ff7 100644
> --- a/lib/test_hexdump.c
> +++ b/lib/test_hexdump.c
> @@ -32,16 +32,33 @@ static const char * const test_data_2_le[] __initconst = {
>  	"d14c", "9919", "b143", "0caf",
>  };
>  
> +static const char * const test_data_2_be[] __initconst = {
> +	"be32", "db7b", "0a18", "93b2",
> +	"70ba", "c424", "7d83", "349b",
> +	"a69c", "31ad", "9c0f", "ace9",
> +	"4cd1", "1999", "43b1", "af0c",
> +};
> +
>  static const char * const test_data_4_le[] __initconst = {
>  	"7bdb32be", "b293180a", "24c4ba70", "9b34837d",
>  	"ad319ca6", "e9ac0f9c", "9919d14c", "0cafb143",
>  };
>  
> +static const char * const test_data_4_be[] __initconst = {
> +	"be32db7b", "0a1893b2", "70bac424", "7d83349b",
> +	"a69c31ad", "9c0face9", "4cd11999", "43b1af0c",
> +};
> +

Is there a reason we can't do it all at compile time?

eg:

static const char * const test_data_4[] __initconst = {
#ifdef CONFIG_CPU_LITTLE_ENDIAN
	"7bdb32be", "b293180a", "24c4ba70", "9b34837d",
	"ad319ca6", "e9ac0f9c", "9919d14c", "0cafb143",
#else
	"be32db7b", "0a1893b2", "70bac424", "7d83349b",
	"a69c31ad", "9c0face9", "4cd11999", "43b1af0c",
#endif
};


cheers

Re: [PATCH] lib/test_hexdump: fix failure on big endian cpu

[Christophe LEROY]

Le 08/08/2018 à 09:25, Michael Ellerman a écrit :
> Christophe Leroy <christophe.leroy@c-s.fr> writes:
>> diff --git a/lib/test_hexdump.c b/lib/test_hexdump.c
>> index 3f415d8101f3..626f580b4ff7 100644
>> --- a/lib/test_hexdump.c
>> +++ b/lib/test_hexdump.c
>> @@ -32,16 +32,33 @@ static const char * const test_data_2_le[] __initconst = {
>>   	"d14c", "9919", "b143", "0caf",
>>   };
>>   
>> +static const char * const test_data_2_be[] __initconst = {
>> +	"be32", "db7b", "0a18", "93b2",
>> +	"70ba", "c424", "7d83", "349b",
>> +	"a69c", "31ad", "9c0f", "ace9",
>> +	"4cd1", "1999", "43b1", "af0c",
>> +};
>> +
>>   static const char * const test_data_4_le[] __initconst = {
>>   	"7bdb32be", "b293180a", "24c4ba70", "9b34837d",
>>   	"ad319ca6", "e9ac0f9c", "9919d14c", "0cafb143",
>>   };
>>   
>> +static const char * const test_data_4_be[] __initconst = {
>> +	"be32db7b", "0a1893b2", "70bac424", "7d83349b",
>> +	"a69c31ad", "9c0face9", "4cd11999", "43b1af0c",
>> +};
>> +
> 
> Is there a reason we can't do it all at compile time?

Codyingstyle suggests to use IS_ENABLED() as much as possible.
I checked symbols inside resulting vmlinux, only the BE ones are there 
so it has
the same effect as an #ifdef

Extract from Documentation/process/codying-style.rst:

Within code, where possible, use the IS_ENABLED macro to convert a Kconfig
symbol into a C boolean expression, and use it in a normal C conditional:

.. code-block:: c

	if (IS_ENABLED(CONFIG_SOMETHING)) {
		...
	}

The compiler will constant-fold the conditional away, and include or exclude
the block of code just as with an #ifdef, so this will not add any runtime
overhead.  However, this approach still allows the C compiler to see the 
code
inside the block, and check it for correctness (syntax, types, symbol
references, etc).  Thus, you still have to use an #ifdef if the code 
inside the
block references symbols that will not exist if the condition is not met.

Christophe

> 
> eg:
> 
> static const char * const test_data_4[] __initconst = {
> #ifdef CONFIG_CPU_LITTLE_ENDIAN
> 	"7bdb32be", "b293180a", "24c4ba70", "9b34837d",
> 	"ad319ca6", "e9ac0f9c", "9919d14c", "0cafb143",
> #else
> 	"be32db7b", "0a1893b2", "70bac424", "7d83349b",
> 	"a69c31ad", "9c0face9", "4cd11999", "43b1af0c",
> #endif
> };
> 
> 
> cheers
> 

Re: [PATCH] lib/test_hexdump: fix failure on big endian cpu

[rashmica]

On 08/08/18 17:25, Michael Ellerman wrote:
> Christophe Leroy <christophe.leroy@c-s.fr> writes:
>> diff --git a/lib/test_hexdump.c b/lib/test_hexdump.c
>> index 3f415d8101f3..626f580b4ff7 100644
>> --- a/lib/test_hexdump.c
>> +++ b/lib/test_hexdump.c
>> @@ -32,16 +32,33 @@ static const char * const test_data_2_le[] __initconst = {
>>  	"d14c", "9919", "b143", "0caf",
>>  };
>>  
>> +static const char * const test_data_2_be[] __initconst = {
>> +	"be32", "db7b", "0a18", "93b2",
>> +	"70ba", "c424", "7d83", "349b",
>> +	"a69c", "31ad", "9c0f", "ace9",
>> +	"4cd1", "1999", "43b1", "af0c",
>> +};
>> +
>>  static const char * const test_data_4_le[] __initconst = {
>>  	"7bdb32be", "b293180a", "24c4ba70", "9b34837d",
>>  	"ad319ca6", "e9ac0f9c", "9919d14c", "0cafb143",
>>  };
>>  
>> +static const char * const test_data_4_be[] __initconst = {
>> +	"be32db7b", "0a1893b2", "70bac424", "7d83349b",
>> +	"a69c31ad", "9c0face9", "4cd11999", "43b1af0c",
>> +};
>> +
> Is there a reason we can't do it all at compile time?

mpe I sent a patch doing that awhile ago and you obviously didn't like
it because you never merged it :P
http://patchwork.ozlabs.org/patch/620405/ I prefer this version because
of the IS_ENABLED



> eg:
>
> static const char * const test_data_4[] __initconst = {
> #ifdef CONFIG_CPU_LITTLE_ENDIAN
> 	"7bdb32be", "b293180a", "24c4ba70", "9b34837d",
> 	"ad319ca6", "e9ac0f9c", "9919d14c", "0cafb143",
> #else
> 	"be32db7b", "0a1893b2", "70bac424", "7d83349b",
> 	"a69c31ad", "9c0face9", "4cd11999", "43b1af0c",
> #endif
> };
>
>
> cheers

Re: [PATCH] lib/test_hexdump: fix failure on big endian cpu

[Michael Ellerman]

rashmica <rashmicy@gmail.com> writes:
> On 08/08/18 17:25, Michael Ellerman wrote:
>> Christophe Leroy <christophe.leroy@c-s.fr> writes:
>>> diff --git a/lib/test_hexdump.c b/lib/test_hexdump.c
>>> index 3f415d8101f3..626f580b4ff7 100644
>>> --- a/lib/test_hexdump.c
>>> +++ b/lib/test_hexdump.c
>>> @@ -32,16 +32,33 @@ static const char * const test_data_2_le[] __initconst = {
>>>  	"d14c", "9919", "b143", "0caf",
>>>  };
>>>  
>>> +static const char * const test_data_2_be[] __initconst = {
>>> +	"be32", "db7b", "0a18", "93b2",
>>> +	"70ba", "c424", "7d83", "349b",
>>> +	"a69c", "31ad", "9c0f", "ace9",
>>> +	"4cd1", "1999", "43b1", "af0c",
>>> +};
>>> +
>>>  static const char * const test_data_4_le[] __initconst = {
>>>  	"7bdb32be", "b293180a", "24c4ba70", "9b34837d",
>>>  	"ad319ca6", "e9ac0f9c", "9919d14c", "0cafb143",
>>>  };
>>>  
>>> +static const char * const test_data_4_be[] __initconst = {
>>> +	"be32db7b", "0a1893b2", "70bac424", "7d83349b",
>>> +	"a69c31ad", "9c0face9", "4cd11999", "43b1af0c",
>>> +};
>>> +
>> Is there a reason we can't do it all at compile time?
>
> mpe I sent a patch doing that awhile ago and you obviously didn't like
> it because you never merged it :P

Sorry, I wasn't sure who should merge it, and never followed up.

cheers

Re: [PATCH] lib/test_hexdump: fix failure on big endian cpu

[Andy Shevchenko]

On Thu, 2018-08-09 at 16:18 +1000, Michael Ellerman wrote:
> rashmica <rashmicy@gmail.com> writes:
> > On 08/08/18 17:25, Michael Ellerman wrote:
> > > Christophe Leroy <christophe.leroy@c-s.fr> writes:
> > > > 
> > mpe I sent a patch doing that awhile ago and you obviously didn't
> > like
> > it because you never merged it :P

Hmm... I (as an author of the test case) never saw that patch.

> Sorry, I wasn't sure who should merge it, and never followed up.

lib/* most of the time under Andrew's responsibility, though since it's
orphaned in MAINTAINERS, anyone can push, though, I think, it's good to
notify Andrew.

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH] lib/test_hexdump: fix failure on big endian cpu

[Michael Ellerman]

Andy Shevchenko <andriy.shevchenko@linux.intel.com> writes:
> On Thu, 2018-08-09 at 16:18 +1000, Michael Ellerman wrote:
>> rashmica <rashmicy@gmail.com> writes:
>> > On 08/08/18 17:25, Michael Ellerman wrote:
>> > > Christophe Leroy <christophe.leroy@c-s.fr> writes:
>> > > > 
>> > mpe I sent a patch doing that awhile ago and you obviously didn't
>> > like
>> > it because you never merged it :P
>
> Hmm... I (as an author of the test case) never saw that patch.

I probably told Rashmica to send it to the linuxppc list and then I was
meant to follow up who should merge it. So my fault.

>> Sorry, I wasn't sure who should merge it, and never followed up.
>
> lib/* most of the time under Andrew's responsibility, though since it's
> orphaned in MAINTAINERS, anyone can push, though, I think, it's good to
> notify Andrew.

Yeah. I was going to apply it with your Ack because it seems pretty
uncontroversial, and there's nothing else in linux-next touching that
file so it should be conflict-free.

I guess I'll wait and see if Andrew would prefer to take it.

cheers

Re: [PATCH] lib/test_hexdump: fix failure on big endian cpu

[Andy Shevchenko]

On Wed, 2018-08-08 at 06:37 +0000, Christophe Leroy wrote:
> On a big endian cpu, test_hexdump fails as follows. The logs show
> that bytes are expected in reversed order.
> 
> [...]
> [   16.643648] test_hexdump: Len: 24 buflen: 130 strlen: 97
> [   16.648681] test_hexdump: Result: 97 'be32db7b 0a1893b2 70bac424
> 7d83349b a69c31ad
> 9c0face9                    .2.{....p..$}.4...1.....'
> [   16.660951] test_hexdump: Expect: 97 '7bdb32be b293180a 24c4ba70
> 9b34837d ad319ca6
> e9ac0f9c                    .2.{....p..$}.4...1.....'
> [   16.673129] test_hexdump: Len: 8 buflen: 130 strlen: 77
> [   16.678113] test_hexdump: Result: 77
> 'be32db7b0a1893b2                                                     
> .2.{....'
> [   16.688660] test_hexdump: Expect: 77
> 'b293180a7bdb32be                                                     
> .2.{....'
> [   16.699170] test_hexdump: Len: 6 buflen: 131 strlen: 87
> [   16.704238] test_hexdump: Result: 87 'be32 db7b
> 0a18                                                                  
>  .2.{..'
> [   16.715511] test_hexdump: Expect: 87 '32be 7bdb
> 180a                                                                  
>  .2.{..'
> [   16.726864] test_hexdump: Len: 24 buflen: 131 strlen: 97
> [   16.731902] test_hexdump: Result: 97 'be32db7b 0a1893b2 70bac424
> 7d83349b a69c31ad
> 9c0face9                    .2.{....p..$}.4...1.....'
> [   16.744175] test_hexdump: Expect: 97 '7bdb32be b293180a 24c4ba70
> 9b34837d ad319ca6
> e9ac0f9c                    .2.{....p..$}.4...1.....'
> [   16.756379] test_hexdump: Len: 32 buflen: 131 strlen: 101
> [   16.761507] test_hexdump: Result: 101 'be32db7b0a1893b2
> 70bac4247d83349b a69c31ad9c0face9
> 4cd1199943b1af0c  .2.{....p..$}.4...1.....L...C...'
> [   16.774212] test_hexdump: Expect: 101 'b293180a7bdb32be
> 9b34837d24c4ba70 e9ac0f9cad319ca6
> 0cafb1439919d14c  .2.{....p..$}.4...1.....L...C...'
> [   16.786763] test_hexdump: failed 801 out of 1184 tests
> 
> This patch fixes it.

I like this approach because in the future we might introduce something
to print be data on le cpu or otherwise and thus test data will be used
independently of cpu endianess.

Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Thanks for adding the BE support.

> 
> Fixes: 64d1d77a44697 ("hexdump: introduce test suite")
> Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
> ---
>  lib/test_hexdump.c | 28 +++++++++++++++++++++++-----
>  1 file changed, 23 insertions(+), 5 deletions(-)
> 
> diff --git a/lib/test_hexdump.c b/lib/test_hexdump.c
> index 3f415d8101f3..626f580b4ff7 100644
> --- a/lib/test_hexdump.c
> +++ b/lib/test_hexdump.c
> @@ -18,7 +18,7 @@ static const unsigned char data_b[] = {
>  
>  static const unsigned char data_a[] =
> ".2.{....p..$}.4...1.....L...C...";
>  
> -static const char * const test_data_1_le[] __initconst = {
> +static const char * const test_data_1[] __initconst = {
>  	"be", "32", "db", "7b", "0a", "18", "93", "b2",
>  	"70", "ba", "c4", "24", "7d", "83", "34", "9b",
>  	"a6", "9c", "31", "ad", "9c", "0f", "ac", "e9",
> @@ -32,16 +32,33 @@ static const char * const test_data_2_le[]
> __initconst = {
>  	"d14c", "9919", "b143", "0caf",
>  };
>  
> +static const char * const test_data_2_be[] __initconst = {
> +	"be32", "db7b", "0a18", "93b2",
> +	"70ba", "c424", "7d83", "349b",
> +	"a69c", "31ad", "9c0f", "ace9",
> +	"4cd1", "1999", "43b1", "af0c",
> +};
> +
>  static const char * const test_data_4_le[] __initconst = {
>  	"7bdb32be", "b293180a", "24c4ba70", "9b34837d",
>  	"ad319ca6", "e9ac0f9c", "9919d14c", "0cafb143",
>  };
>  
> +static const char * const test_data_4_be[] __initconst = {
> +	"be32db7b", "0a1893b2", "70bac424", "7d83349b",
> +	"a69c31ad", "9c0face9", "4cd11999", "43b1af0c",
> +};
> +
>  static const char * const test_data_8_le[] __initconst = {
>  	"b293180a7bdb32be", "9b34837d24c4ba70",
>  	"e9ac0f9cad319ca6", "0cafb1439919d14c",
>  };
>  
> +static const char * const test_data_8_be[] __initconst = {
> +	"be32db7b0a1893b2", "70bac4247d83349b",
> +	"a69c31ad9c0face9", "4cd1199943b1af0c",
> +};
> +
>  #define FILL_CHAR	'#'
>  
>  static unsigned total_tests __initdata;
> @@ -56,6 +73,7 @@ static void __init test_hexdump_prepare_test(size_t
> len, int rowsize,
>  	size_t l = len;
>  	int gs = groupsize, rs = rowsize;
>  	unsigned int i;
> +	const bool is_be = IS_ENABLED(CONFIG_CPU_BIG_ENDIAN);
>  
>  	if (rs != 16 && rs != 32)
>  		rs = 16;
> @@ -67,13 +85,13 @@ static void __init
> test_hexdump_prepare_test(size_t len, int rowsize,
>  		gs = 1;
>  
>  	if (gs == 8)
> -		result = test_data_8_le;
> +		result = is_be ? test_data_8_be : test_data_8_le;
>  	else if (gs == 4)
> -		result = test_data_4_le;
> +		result = is_be ? test_data_4_be : test_data_4_le;
>  	else if (gs == 2)
> -		result = test_data_2_le;
> +		result = is_be ? test_data_2_be : test_data_2_le;
>  	else
> -		result = test_data_1_le;
> +		result = test_data_1;
>  
>  	/* hex dump */
>  	p = test;

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy