linux-crypto.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
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 1/5] selftests/powerpc: Add header files for GZIP engine test
Date: Fri, 17 Apr 2020 15:05:32 +1000	[thread overview]
Message-ID: <87imhyitwj.fsf@mpe.ellerman.id.au> (raw)
In-Reply-To: <20200413155916.16900-2-rzinsly@linux.ibm.com>

Hi Raphael,

Some comments below ...

Raphael Moreira Zinsly <rzinsly@linux.ibm.com> writes:
> Add files to access the powerpc NX-GZIP engine in user space.
>
> Signed-off-by: Bulent Abali <abali@us.ibm.com>
> Signed-off-by: Raphael Moreira Zinsly <rzinsly@linux.ibm.com>
> ---
>  .../selftests/powerpc/nx-gzip/inc/crb.h       | 159 ++++++++++++++++++
>  .../selftests/powerpc/nx-gzip/inc/nx-gzip.h   |  27 +++
>  .../powerpc/nx-gzip/inc/nx-helpers.h          |  54 ++++++
>  .../selftests/powerpc/nx-gzip/inc/nx.h        |  38 +++++
>  4 files changed, 278 insertions(+)
>  create mode 100644 tools/testing/selftests/powerpc/nx-gzip/inc/crb.h
>  create mode 100644 tools/testing/selftests/powerpc/nx-gzip/inc/nx-gzip.h
>  create mode 100644 tools/testing/selftests/powerpc/nx-gzip/inc/nx-helpers.h
>  create mode 100644 tools/testing/selftests/powerpc/nx-gzip/inc/nx.h

It's standard to call the directory "include", can you rename it please?

> diff --git a/tools/testing/selftests/powerpc/nx-gzip/inc/crb.h b/tools/testing/selftests/powerpc/nx-gzip/inc/crb.h
> new file mode 100644
> index 000000000000..9056e3dc1831
> --- /dev/null
> +++ b/tools/testing/selftests/powerpc/nx-gzip/inc/crb.h
> @@ -0,0 +1,159 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +#ifndef __CRB_H
> +#define __CRB_H
> +#include <linux/types.h>
> +#include "nx.h"
> +
> +typedef unsigned char u8;
> +typedef unsigned int u32;
> +typedef unsigned long long u64;

The vast bulk of the code uses the stdint.h types, so it would be
preferable to either use those throughout or use the kernel types
throughout, eg. __u8, __u32, __u64, rather than defining your own here.


...
> diff --git a/tools/testing/selftests/powerpc/nx-gzip/inc/nx-gzip.h b/tools/testing/selftests/powerpc/nx-gzip/inc/nx-gzip.h
> new file mode 100644
> index 000000000000..75482c45574d
> --- /dev/null
> +++ b/tools/testing/selftests/powerpc/nx-gzip/inc/nx-gzip.h
> @@ -0,0 +1,27 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later
> + *
> + * Copyright 2020 IBM Corp.
> + *
> + */
> +
> +#ifndef _UAPI_MISC_VAS_H
> +#define _UAPI_MISC_VAS_H

That's the wrong include guard.

> +
> +#include <asm/ioctl.h>
> +
> +#define VAS_FLAGS_PIN_WINDOW	0x1
> +#define VAS_FLAGS_HIGH_PRI	0x2
> +
> +#define VAS_FTW_SETUP		_IOW('v', 1, struct vas_gzip_setup_attr)
> +#define VAS_842_TX_WIN_OPEN	_IOW('v', 2, struct vas_gzip_setup_attr)
> +#define VAS_GZIP_TX_WIN_OPEN	_IOW('v', 0x20, struct vas_gzip_setup_attr)
> +
> +struct vas_gzip_setup_attr {
> +	int32_t		version;
> +	int16_t		vas_id;
> +	int16_t		reserved1;
> +	int64_t		flags;
> +	int64_t		reserved2[6];
> +};

This doesn't match the kernel header.

In fact you should just be able to symlink the uapi header.

> +#endif /* _UAPI_MISC_VAS_H */
> diff --git a/tools/testing/selftests/powerpc/nx-gzip/inc/nx-helpers.h b/tools/testing/selftests/powerpc/nx-gzip/inc/nx-helpers.h
> new file mode 100644
> index 000000000000..e0d68914c941
> --- /dev/null
> +++ b/tools/testing/selftests/powerpc/nx-gzip/inc/nx-helpers.h
> @@ -0,0 +1,54 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +#include <sys/time.h>
> +#include <asm/byteorder.h>
> +#include <stdint.h>
> +#include <stdbool.h>
> +#include "crb.h"
> +
> +#define cpu_to_be32		__cpu_to_be32
> +#define cpu_to_be64		__cpu_to_be64
> +#define be32_to_cpu		__be32_to_cpu
> +#define be64_to_cpu		__be64_to_cpu
> +
> +/*
> + * Several helpers/macros below were copied from the tree
> + * (kernel.h, nx-842.h, nx-ftw.h, asm-compat.h etc)
> + */
> +
> +/* from kernel.h */
> +#define IS_ALIGNED(x, a)	(((x) & ((typeof(x))(a) - 1)) == 0)
> +#define __round_mask(x, y)	((__typeof__(x))((y)-1))
> +#define round_up(x, y)		((((x)-1) | __round_mask(x, y))+1)
> +#define round_down(x, y)	((x) & ~__round_mask(x, y))

Unused?

> +#define min_t(t, x, y)	((x) < (y) ? (x) : (y))

Unused?

> +/*
> + * Get/Set bit fields. (from nx-842.h)
> + */
> +#define GET_FIELD(m, v)         (((v) & (m)) >> MASK_LSH(m))
> +#define MASK_LSH(m)             (__builtin_ffsl(m) - 1)
> +#define SET_FIELD(m, v, val)    \
> +		(((v) & ~(m)) | ((((typeof(v))(val)) << MASK_LSH(m)) & (m)))

Unused?

> +
> +/* From asm-compat.h */
> +#define __stringify_in_c(...)	#__VA_ARGS__
> +#define stringify_in_c(...)	__stringify_in_c(__VA_ARGS__) " "
> +
> +#define	pr_debug
> +#define	pr_debug_ratelimited	printf
> +#define	pr_err			printf
> +#define	pr_err_ratelimited	printf
> +
> +#define WARN_ON_ONCE(x)		do {if (x) \
> +				printf("WARNING: %s:%d\n", __func__, __LINE__)\
> +				} while (0)

Unused?

> +extern void dump_buffer(char *msg, char *buf, int len);
> +extern void *alloc_aligned_mem(int len, int align, char *msg);
> +extern void get_payload(char *buf, int len);
> +extern void time_add(struct timeval *in, int seconds, struct timeval *out);
>
> +extern bool time_after(struct timeval *a, struct timeval *b);
> +extern long time_delta(struct timeval *a, struct timeval *b);
> +extern void dump_dde(struct data_descriptor_entry *dde, char *msg);
> +extern void copy_paste_crb_data(struct coprocessor_request_block *crb);

None of those externs appear to exist or be used.

> diff --git a/tools/testing/selftests/powerpc/nx-gzip/inc/nx.h b/tools/testing/selftests/powerpc/nx-gzip/inc/nx.h
> new file mode 100644
> index 000000000000..1ae8348b59d6
> --- /dev/null
> +++ b/tools/testing/selftests/powerpc/nx-gzip/inc/nx.h
> @@ -0,0 +1,38 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later
> + *
> + * Copyright 2020 IBM Corp.
> + *
> + */
> +#ifndef _NX_H
> +#define _NX_H
> +
> +#include <stdbool.h>
> +
> +#define	NX_FUNC_COMP_842	1
> +#define NX_FUNC_COMP_GZIP	2
> +
> +#ifndef __aligned
> +#define __aligned(x)	__attribute__((aligned(x)))
> +#endif
> +
> +struct nx842_func_args {
> +	bool use_crc;
> +	bool decompress;		/* true decompress; false compress */
> +	bool move_data;
> +	int timeout;			/* seconds */
> +};
> +
> +struct nxbuf_t {
> +	int len;
> +	char *buf;
> +};
> +
> +/* @function should be EFT (aka 842), GZIP etc */
> +extern void *nx_function_begin(int function, int pri);
> +
> +extern int nx_function(void *handle, struct nxbuf_t *in, struct nxbuf_t *out,
> +			void *arg);
> +
> +extern int nx_function_end(void *handle);

You don't need extern on function declarations in headers.

> +
> +#endif	/* _NX_H */
> -- 
> 2.21.0


cheers

  reply	other threads:[~2020-04-17  5:05 UTC|newest]

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-04-13 15:59 [PATCH V3 0/5] selftests/powerpc: Add NX-GZIP engine testcase Raphael Moreira Zinsly
2020-04-13 15:59 ` [PATCH V3 1/5] selftests/powerpc: Add header files for GZIP engine test Raphael Moreira Zinsly
2020-04-17  5:05   ` Michael Ellerman [this message]
2020-04-13 15:59 ` [PATCH V3 2/5] selftests/powerpc: Add header files for NX compresion/decompression Raphael Moreira Zinsly
2020-04-13 15:59 ` [PATCH V3 3/5] selftests/powerpc: Add NX-GZIP engine compress testcase Raphael Moreira Zinsly
2020-04-17  5:05   ` Michael Ellerman
2020-04-13 15:59 ` [PATCH V3 4/5] selftests/powerpc: Add NX-GZIP engine decompress testcase Raphael Moreira Zinsly
2020-04-13 15:59 ` [PATCH V3 5/5] selftests/powerpc: Add README for GZIP engine tests Raphael Moreira Zinsly

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=87imhyitwj.fsf@mpe.ellerman.id.au \
    --to=mpe@ellerman.id.au \
    --cc=abali@us.ibm.com \
    --cc=dja@axtens.net \
    --cc=haren@linux.ibm.com \
    --cc=herbert@gondor.apana.org.au \
    --cc=linux-crypto@vger.kernel.org \
    --cc=linuxppc-dev@lists.ozlabs.org \
    --cc=rzinsly@linux.ibm.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).