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

STALE2917d

8 messages, 4 authors, 2018-08-09 · open the first message on its own page

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

From: Christophe Leroy <hidden>
Date: 2018-08-08 06:37:09

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

From: Michael Ellerman <mpe@ellerman.id.au>
Date: 2018-08-08 07:25:35

Christophe Leroy [off-list ref] writes:

quoted hunk

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

From: Christophe LEROY <hidden>
Date: 2018-08-08 07:44:34


Le 08/08/2018 à 09:25, Michael Ellerman a écrit :

Christophe Leroy [off-list ref] writes:

quoted

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

From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Date: 2018-08-08 12:13:12

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.

quoted hunk

Fixes: 64d1d77a44697 ("hexdump: introduce test suite")
Signed-off-by: Christophe Leroy <redacted>
---
 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 [off-list ref]
Intel Finland Oy

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

From: rashmica <hidden>
Date: 2018-08-09 02:04:18


On 08/08/18 17:25, Michael Ellerman wrote:

Christophe Leroy [off-list ref] writes:

quoted

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

From: Michael Ellerman <mpe@ellerman.id.au>
Date: 2018-08-09 06:18:12

rashmica [off-list ref] writes:

On 08/08/18 17:25, Michael Ellerman wrote:

quoted

Christophe Leroy [off-list ref] writes:

quoted

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

From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Date: 2018-08-09 09:34:07

On Thu, 2018-08-09 at 16:18 +1000, Michael Ellerman wrote:

rashmica [off-list ref] writes:

quoted

On 08/08/18 17:25, Michael Ellerman wrote:

quoted

Christophe Leroy [off-list ref] writes:

quoted

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 [off-list ref]
Intel Finland Oy

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

From: Michael Ellerman <mpe@ellerman.id.au>
Date: 2018-08-09 12:33:44

Andy Shevchenko [off-list ref] writes:

On Thu, 2018-08-09 at 16:18 +1000, Michael Ellerman wrote:

quoted

rashmica [off-list ref] writes:

quoted

On 08/08/18 17:25, Michael Ellerman wrote:

quoted

Christophe Leroy [off-list ref] writes:

quoted

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.

quoted

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

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help