Received-SPF: pass (google.com: domain of linux-crypto-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
From:   Michael Ellerman <mpe@ellerman.id.au>
To:     Raphael Moreira Zinsly <rzinsly@linux.ibm.com>,
        linuxppc-dev@lists.ozlabs.org, linux-crypto@vger.kernel.org,
        dja@axtens.net
Cc:     rzinsly@linux.ibm.com, herbert@gondor.apana.org.au,
        haren@linux.ibm.com, abali@us.ibm.com
Subject: Re: [PATCH V3 3/5] selftests/powerpc: Add NX-GZIP engine compress testcase
In-Reply-To: <20200413155916.16900-4-rzinsly@linux.ibm.com>
References: <20200413155916.16900-1-rzinsly@linux.ibm.com> <20200413155916.16900-4-rzinsly@linux.ibm.com>
Date:   Fri, 17 Apr 2020 15:05:35 +1000
Message-ID: <87h7xiitwg.fsf@mpe.ellerman.id.au>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable
Sender: linux-crypto-owner@vger.kernel.org
Precedence: bulk

Hi Raphael,

Some comments below ...

Raphael Moreira Zinsly <rzinsly@linux.ibm.com> writes:
> Add a compression testcase for the powerpc NX-GZIP engine.
>
> Signed-off-by: Bulent Abali <abali@us.ibm.com>
> Signed-off-by: Raphael Moreira Zinsly <rzinsly@linux.ibm.com>
> ---
>  .../selftests/powerpc/nx-gzip/Makefile        |  21 +
>  .../selftests/powerpc/nx-gzip/gzfht_test.c    | 432 ++++++++++++++++++
>  .../selftests/powerpc/nx-gzip/gzip_vas.c      | 316 +++++++++++++
>  3 files changed, 769 insertions(+)
>  create mode 100644 tools/testing/selftests/powerpc/nx-gzip/Makefile
>  create mode 100644 tools/testing/selftests/powerpc/nx-gzip/gzfht_test.c
>  create mode 100644 tools/testing/selftests/powerpc/nx-gzip/gzip_vas.c

You haven't added this to tools/testing/selftests/powerpc/Makefile,
which means it's not actually built as part of the selftests build.
Which makes it not really a selftest.

We need:

diff --git a/tools/testing/selftests/powerpc/Makefile b/tools/testing/selft=
ests/powerpc/Makefile
index 644770c3b754..0830e63818c1 100644
--- a/tools/testing/selftests/powerpc/Makefile
+++ b/tools/testing/selftests/powerpc/Makefile
@@ -19,6 +19,7 @@ SUB_DIRS =3D alignment		\
 	   copyloops		\
 	   dscr			\
 	   mm			\
+	   nx-gzip		\
 	   pmu			\
 	   signal		\
 	   primitives		\


> diff --git a/tools/testing/selftests/powerpc/nx-gzip/Makefile b/tools/tes=
ting/selftests/powerpc/nx-gzip/Makefile
> new file mode 100644
> index 000000000000..ab903f63bbbd
> --- /dev/null
> +++ b/tools/testing/selftests/powerpc/nx-gzip/Makefile
> @@ -0,0 +1,21 @@
> +CC =3D gcc
> +CFLAGS =3D -O3
> +INC =3D ./inc
> +SRC =3D gzfht_test.c
> +OBJ =3D $(SRC:.c=3D.o)
> +TESTS =3D gzfht_test
> +EXTRA_SOURCES =3D gzip_vas.c
> +
> +all:	$(TESTS)
> +
> +$(OBJ): %.o: %.c
> +	$(CC) $(CFLAGS) -I$(INC) -c $<
> +
> +$(TESTS): $(OBJ)
> +	$(CC) $(CFLAGS) -I$(INC) -o $@ $@.o $(EXTRA_SOURCES)
> +
> +run_tests: $(TESTS)
> +	./gzfht_test gzip_vas.c
> +
> +clean:
> +	rm -f $(TESTS) *.o *~ *.gz

We have an existing system for Makefiles for selftests.

Also the test programs need to be able to just run standalone with no
arguments in order for the selftest machinery to run them correctly.

The patch below should integrate the tests properly and add a wrapper
script to run the tests.

However I'm still getting some warnings from the build:

  gunz_test.c: In function =E2=80=98decompress_file=E2=80=99:
  gunz_test.c:914:12: error: =E2=80=98total_out=E2=80=99 may be used uninit=
ialized in this function [-Werror=3Dmaybe-uninitialized]
    914 |  total_out =3D total_out + tpbc;
        |  ~~~~~~~~~~^~~~~~~~~~~~~~~~~~
  gunz_test.c:287:11: error: =E2=80=98last_comp_ratio=E2=80=99 may be used =
uninitialized in this function [-Werror=3Dmaybe-uninitialized]
    287 |  uint64_t last_comp_ratio; /* 1000 max */
        |           ^~~~~~~~~~~~~~~
  gunz_test.c:519:8: error: =E2=80=98outf=E2=80=99 may be used uninitialize=
d in this function [-Werror=3Dmaybe-uninitialized]
    519 |    n =3D fwrite(fifo_out, 1, write_sz, outf);
        |        ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  gunz_test.c:68:20: error: =E2=80=98inpf=E2=80=99 may be used uninitialize=
d in this function [-Werror=3Dmaybe-uninitialized]
     68 | #define GETINPC(X) fgetc(X)
        |                    ^~~~~
  gunz_test.c:274:8: note: =E2=80=98inpf=E2=80=99 was declared here
    274 |  FILE *inpf;
        |        ^~~~
  cc1: all warnings being treated as errors
  make[2]: *** [../../lib.mk:142: /home/michael/build/adhoc/kselftest/power=
pc/nx-gzip/gunz_test] Error 1


Please send a fix for those. If you just need to initialise them to 0 or
NULL at the beginning of the function that's fine by me.

cheers


diff --git a/tools/testing/selftests/powerpc/nx-gzip/Makefile b/tools/testi=
ng/selftests/powerpc/nx-gzip/Makefile
index 82abc19a49a0..387ad3857c9d 100644
--- a/tools/testing/selftests/powerpc/nx-gzip/Makefile
+++ b/tools/testing/selftests/powerpc/nx-gzip/Makefile
@@ -1,22 +1,8 @@
-CC =3D gcc
-CFLAGS =3D -O3
-INC =3D ./inc
-SRC =3D gzfht_test.c gunz_test.c
-OBJ =3D $(SRC:.c=3D.o)
-TESTS =3D gzfht_test gunz_test
-EXTRA_SOURCES =3D gzip_vas.c
+CFLAGS +=3D -O3 -m64 -I./inc
=20
-all:   $(TESTS)
+TEST_GEN_FILES :=3D gzfht_test gunz_test
+TEST_PROGS :=3D nx-gzip-test.sh
=20
-$(OBJ): %.o: %.c
-       $(CC) $(CFLAGS) -I$(INC) -c $<
+include ../../lib.mk
=20
-$(TESTS): $(OBJ)
-       $(CC) $(CFLAGS) -I$(INC) -o $@ $@.o $(EXTRA_SOURCES)
-
-run_tests: $(TESTS)
-       ./gzfht_test gzip_vas.c
-       ./gunz_test gzip_vas.c.nx.gz
-
-clean:
-       rm -f $(TESTS) *.o *~ *.gz *.gunzip
+$(TEST_GEN_FILES): gzip_vas.c
diff --git a/tools/testing/selftests/powerpc/nx-gzip/gunz_test.c b/tools/te=
sting/selftests/powerpc/nx-gzip/gunz_test.c
index 94cb79616225..8196cf56df7a 100644
--- a/tools/testing/selftests/powerpc/nx-gzip/gunz_test.c
+++ b/tools/testing/selftests/powerpc/nx-gzip/gunz_test.c
@@ -36,6 +36,9 @@
  * vas:      virtual accelerator switch; the user mode interface
  */
=20
+#define _ISOC11_SOURCE // For aligned_alloc()
+#define _DEFAULT_SOURCE        // For endian.h
+
 #include <stdio.h>
 #include <stdlib.h>
 #include <string.h>
@@ -242,7 +245,6 @@ static int nx_touch_pages_dde(struct nx_dde_t *ddep, lo=
ng buf_sz, long page_sz,
 static int nx_submit_job(struct nx_dde_t *src, struct nx_dde_t *dst,
                         struct nx_gzip_crb_cpb_t *cmdp, void *handle)
 {
-       int cc;
        uint64_t csbaddr;
=20
        memset((void *)&cmdp->crb.csb, 0, sizeof(cmdp->crb.csb));
diff --git a/tools/testing/selftests/powerpc/nx-gzip/gzfht_test.c b/tools/t=
esting/selftests/powerpc/nx-gzip/gzfht_test.c
index e60f743e2c6b..a3d90b2b1591 100644
--- a/tools/testing/selftests/powerpc/nx-gzip/gzfht_test.c
+++ b/tools/testing/selftests/powerpc/nx-gzip/gzfht_test.c
@@ -41,6 +41,8 @@
  * vas:      virtual accelerator switch; the user mode interface
  */
=20
+#define _ISOC11_SOURCE // For aligned_alloc()
+#define _DEFAULT_SOURCE        // For endian.h
=20
 #include <stdio.h>
 #include <stdlib.h>
@@ -75,7 +77,6 @@ static int compress_fht_sample(char *src, uint32_t srclen=
, char *dst,
                                uint32_t dstlen, int with_count,
                                struct nx_gzip_crb_cpb_t *cmdp, void *handl=
e)
 {
-       int cc;
        uint32_t fc;
=20
        assert(!!cmdp);
diff --git a/tools/testing/selftests/powerpc/nx-gzip/nx-gzip-test.sh b/tool=
s/testing/selftests/powerpc/nx-gzip/nx-gzip-test.sh
new file mode 100755
index 000000000000..896428ea7800
--- /dev/null
+++ b/tools/testing/selftests/powerpc/nx-gzip/nx-gzip-test.sh
@@ -0,0 +1,45 @@
+#!/bin/bash
+
+if [[ ! -w /dev/crypto/nx-gzip ]]; then
+    echo "Can't access /dev/crypto/nx-gzip, skipping"
+    echo "skip: $0"
+    exit 4
+fi
+
+set -e
+
+function cleanup
+{
+    rm -f nx-tempfile*
+}
+
+trap cleanup EXIT
+
+function test_sizes
+{
+    local n=3D$1
+    local fname=3D"nx-tempfile.$n"
+
+    for size in 4K 64K 1M 64M
+    do
+        echo "Testing $size ($n) ..."
+        dd if=3D/dev/urandom of=3D$fname bs=3D$size count=3D1
+        ./gzfht_test $fname
+        ./gunz_test ${fname}.nx.gz
+    done
+}
+
+echo "Doing basic test of different sizes ..."
+test_sizes 0
+
+echo "Running tests in parallel ..."
+for i in {1..16}
+do
+       test_sizes $i &
+done
+
+wait
+
+echo "OK"
+
+exit 0