From: Xu Zaibo <xuzaibo@huawei.com>
To: Marco Elver <elver@google.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: Wed, 4 Dec 2019 09:10:24 +0800 [thread overview]
Message-ID: <4c45461f-7c72-8f3c-4785-3a119383df0b@huawei.com> (raw)
In-Reply-To: <20191203120148.GA68157@google.com>
Hi,
On 2019/12/3 20:01, Marco Elver wrote:
> 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/
Okay, I will check, and send out a patch to fixed them, thanks.
>
> 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;
Yes, atomic_t is better.
>
>> +};
>> +
> [...]
>> 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>
Okay.
>
> [...]
>> +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)
yes.
>
>> + 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))
okay
>
>> + 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, ...)
Yes, thanks.
cheers,
Zaibo
.
>
next prev parent reply other threads:[~2019-12-04 1:10 UTC|newest]
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
2019-12-04 1:10 ` Xu Zaibo [this message]
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 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=4c45461f-7c72-8f3c-4785-3a119383df0b@huawei.com \
--to=xuzaibo@huawei.com \
--cc=davem@davemloft.net \
--cc=elver@google.com \
--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=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
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).