| Message ID | f3112437f62c2f48300535510918e8be1dceacfb.1533610877.git.christophe.leroy@c-s.fr (mailing list archive) |
|---|---|
| State | Awaiting Upstream |
| Headers | show |
| Series | lib/test_hexdump: fix failure on big endian cpu | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/apply_patch | success | next/apply_patch Successfully applied |
| snowpatch_ozlabs/checkpatch | success | Test checkpatch on branch next |
| snowpatch_ozlabs/build-ppc64le | success | Test build-ppc64le on branch next |
| snowpatch_ozlabs/build-ppc64be | success | Test build-ppc64be on branch next |
| snowpatch_ozlabs/build-ppc64e | success | Test build-ppc64e on branch next |
| snowpatch_ozlabs/build-ppc32 | success | Test build-ppc32 on branch next |
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
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 >
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;
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
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
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> 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
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;
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(-)