Linux-Crypto Archive on lore.kernel.org
 help / color / Atom feed
From: Marco Elver <elver@google.com>
To: Zaibo Xu <xuzaibo@huawei.com>
Cc: herbert@gondor.apana.org.au, davem@davemloft.net,
	linux-crypto@vger.kernel.org, jonathan.cameron@huawei.com,
	wangzhou1@hisilicon.com, linuxarm@huawei.com,
	fanghao11@huawei.com, yekai13@huawei.com, zhangwei375@huawei.com,
	forest.zhouchang@huawei.com
Subject: Re: [PATCH v3 1/5] crypto: hisilicon - add HiSilicon SEC V2 driver
Date: Tue, 3 Dec 2019 13:01:48 +0100
Message-ID: <20191203120148.GA68157@google.com> (raw)
In-Reply-To: <1573643468-1812-2-git-send-email-xuzaibo@huawei.com>

Avoid using __sync builtins and instead prefer the kernel's own
facilities:

See comments below for suggestions, preserving the assumed memory
ordering requirements (but please double-check). By using atomic_t
instead of __sync, you'd also avoid any data races due to plain
concurrent accesses.

Reported in: https://lore.kernel.org/linux-crypto/CANpmjNM2b26Oo6k-4EqfrJf1sBj3WoFf-NQnwsLr3EW9B=G8kw@mail.gmail.com/

On Wed, 13 Nov 2019, Zaibo Xu wrote:

> SEC driver provides PCIe hardware device initiation with
> AES, SM4, and 3DES skcipher algorithms registered to Crypto.
> It uses Hisilicon QM as interface to CPU.
> 
> Signed-off-by: Zaibo Xu <xuzaibo@huawei.com>
> Signed-off-by: Longfang Liu <liulongfang@huawei.com>
> ---
>  drivers/crypto/hisilicon/Kconfig           |  16 +
>  drivers/crypto/hisilicon/Makefile          |   1 +
>  drivers/crypto/hisilicon/sec2/Makefile     |   2 +
>  drivers/crypto/hisilicon/sec2/sec.h        | 132 +++++
>  drivers/crypto/hisilicon/sec2/sec_crypto.c | 886 +++++++++++++++++++++++++++++
>  drivers/crypto/hisilicon/sec2/sec_crypto.h | 198 +++++++
>  drivers/crypto/hisilicon/sec2/sec_main.c   | 640 +++++++++++++++++++++
>  7 files changed, 1875 insertions(+)
>  create mode 100644 drivers/crypto/hisilicon/sec2/Makefile
>  create mode 100644 drivers/crypto/hisilicon/sec2/sec.h
>  create mode 100644 drivers/crypto/hisilicon/sec2/sec_crypto.c
>  create mode 100644 drivers/crypto/hisilicon/sec2/sec_crypto.h
>  create mode 100644 drivers/crypto/hisilicon/sec2/sec_main.c
[...]
> diff --git a/drivers/crypto/hisilicon/sec2/sec.h b/drivers/crypto/hisilicon/sec2/sec.h
> new file mode 100644
> index 0000000..443b6c5
> --- /dev/null
> +++ b/drivers/crypto/hisilicon/sec2/sec.h
> @@ -0,0 +1,132 @@
[...]
> +
> +/* SEC request of Crypto */
> +struct sec_req {
> +	struct sec_sqe sec_sqe;
> +	struct sec_ctx *ctx;
> +	struct sec_qp_ctx *qp_ctx;
> +
> +	/* Cipher supported only at present */
> +	struct sec_cipher_req c_req;
> +	int err_type;
> +	int req_id;
> +
> +	/* Status of the SEC request */
> +	int fake_busy;

This could be

	atomic_t fake_busy;

> +};
> +
[...]
> diff --git a/drivers/crypto/hisilicon/sec2/sec_crypto.c b/drivers/crypto/hisilicon/sec2/sec_crypto.c
> new file mode 100644
> index 0000000..23092a9
> --- /dev/null
> +++ b/drivers/crypto/hisilicon/sec2/sec_crypto.c
> @@ -0,0 +1,886 @@

Add

	#include <linux/atomic.h>

[...]
> +static int sec_bd_send(struct sec_ctx *ctx, struct sec_req *req)
> +{
> +	struct sec_qp_ctx *qp_ctx = req->qp_ctx;
> +	int ret;
> +
> +	mutex_lock(&qp_ctx->req_lock);
> +	ret = hisi_qp_send(qp_ctx->qp, &req->sec_sqe);
> +	mutex_unlock(&qp_ctx->req_lock);
> +
> +	if (ret == -EBUSY)
> +		return -ENOBUFS;
> +
> +	if (!ret) {
> +		if (req->fake_busy)

This could be:

	atomic_read(&req->fake_busy)

> +			ret = -EBUSY;
> +		else
> +			ret = -EINPROGRESS;
> +	}
> +
> +	return ret;
> +}
[...]
> +static void sec_skcipher_callback(struct sec_ctx *ctx, struct sec_req *req)
> +{
> +	struct skcipher_request *sk_req = req->c_req.sk_req;
> +	struct sec_qp_ctx *qp_ctx = req->qp_ctx;
> +
> +	atomic_dec(&qp_ctx->pending_reqs);
> +	sec_free_req_id(req);
> +
> +	/* IV output at encrypto of CBC mode */
> +	if (ctx->c_ctx.c_mode == SEC_CMODE_CBC && req->c_req.encrypt)
> +		sec_update_iv(req);
> +
> +	if (__sync_bool_compare_and_swap(&req->fake_busy, 1, 0))

This could be:

	int expect_val = 1;
	...
	if (atomic_try_cmpxchg_relaxed(&req->fake_busy, &expect_val, 0))

> +		sk_req->base.complete(&sk_req->base, -EINPROGRESS);
> +
> +	sk_req->base.complete(&sk_req->base, req->err_type);
> +}
> +
> +static void sec_request_uninit(struct sec_ctx *ctx, struct sec_req *req)
> +{
> +	struct sec_qp_ctx *qp_ctx = req->qp_ctx;
> +
> +	atomic_dec(&qp_ctx->pending_reqs);
> +	sec_free_req_id(req);
> +	sec_put_queue_id(ctx, req);
> +}
> +
> +static int sec_request_init(struct sec_ctx *ctx, struct sec_req *req)
> +{
> +	struct sec_qp_ctx *qp_ctx;
> +	int issue_id, ret;
> +
> +	/* To load balance */
> +	issue_id = sec_get_queue_id(ctx, req);
> +	qp_ctx = &ctx->qp_ctx[issue_id];
> +
> +	req->req_id = sec_alloc_req_id(req, qp_ctx);
> +	if (req->req_id < 0) {
> +		sec_put_queue_id(ctx, req);
> +		return req->req_id;
> +	}
> +
> +	if (ctx->fake_req_limit <= atomic_inc_return(&qp_ctx->pending_reqs))
> +		req->fake_busy = 1;
> +	else
> +		req->fake_busy = 0;

These could be:

	atomic_set(&req->fake_busy, ...)

> +
> +	ret = ctx->req_op->get_res(ctx, req);
> +	if (ret) {
> +		atomic_dec(&qp_ctx->pending_reqs);
> +		sec_request_uninit(ctx, req);
> +		dev_err(SEC_CTX_DEV(ctx), "get resources failed!\n");
> +	}
> +
> +	return ret;
> +}

Thanks,
-- Marco

  reply index

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-11-13 11:11 [PATCH v3 0/5] crypto: hisilicon - add HiSilicon SEC V2 support Zaibo Xu
2019-11-13 11:11 ` [PATCH v3 1/5] crypto: hisilicon - add HiSilicon SEC V2 driver Zaibo Xu
2019-12-03 12:01   ` Marco Elver [this message]
2019-12-04  1:10     ` Xu Zaibo
2019-11-13 11:11 ` [PATCH v3 2/5] crypto: hisilicon - add SRIOV for HiSilicon SEC Zaibo Xu
2019-11-13 11:11 ` [PATCH v3 3/5] Documentation: add DebugFS doc " Zaibo Xu
2019-11-13 11:11 ` [PATCH v3 4/5] crypto: hisilicon - add DebugFS " Zaibo Xu
2019-12-03 12:12   ` Marco Elver
2019-12-04  1:12     ` Xu Zaibo
2019-11-13 11:11 ` [PATCH v3 5/5] MAINTAINERS: Add maintainer for HiSilicon SEC V2 driver Zaibo Xu
2019-11-20 12:19 ` [PATCH v3 0/5] crypto: hisilicon - add HiSilicon SEC V2 support Xu Zaibo
2019-11-22 11:03 ` Herbert Xu

Reply instructions:

You may reply publically 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=20191203120148.GA68157@google.com \
    --to=elver@google.com \
    --cc=davem@davemloft.net \
    --cc=fanghao11@huawei.com \
    --cc=forest.zhouchang@huawei.com \
    --cc=herbert@gondor.apana.org.au \
    --cc=jonathan.cameron@huawei.com \
    --cc=linux-crypto@vger.kernel.org \
    --cc=linuxarm@huawei.com \
    --cc=wangzhou1@hisilicon.com \
    --cc=xuzaibo@huawei.com \
    --cc=yekai13@huawei.com \
    --cc=zhangwei375@huawei.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

Linux-Crypto Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-crypto/0 linux-crypto/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-crypto linux-crypto/ https://lore.kernel.org/linux-crypto \
		linux-crypto@vger.kernel.org
	public-inbox-index linux-crypto

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-crypto


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git