Re: [PATCH v3 2/3] drivers: crypto: Add the Virtual Function driver for CPT
From: George Cherian
Date: Fri Jan 06 2017 - 02:06:25 EST
Hi Corentin,
On 12/21/2016 07:31 PM, Corentin Labbe wrote:
Hello
I have some comment inline
On Wed, Dec 21, 2016 at 11:56:12AM +0000, george.cherian@xxxxxxxxxx wrote:
From: George Cherian <george.cherian@xxxxxxxxxx>
Enable the CPT VF driver. CPT is the cryptographic Accelaration Unit
typo acceleration
will fix
[...]
+static inline void update_input_data(struct cpt_request_info *req_info,
+ struct scatterlist *inp_sg,
+ u32 nbytes, u32 *argcnt)
+{
+ req_info->req.dlen += nbytes;
+
+ while (nbytes) {
+ u32 len = min(nbytes, inp_sg->length);
+ u8 *ptr = page_address(sg_page(inp_sg)) + inp_sg->offset;
You could use sg_virt instead.
Thanks for pointing it out, Yes will replace with sg_virt.
But do you have tested your accelerator with user space data (via cryptodev/AF_ALG) ?
No I have tested only using in kernel applications, Not used
cryptodev/AF_ALG
In my memory, you better use kmap() instead of this direct memory address.
[...]
+static inline u32 cvm_enc_dec(struct ablkcipher_request *req, u32 enc,
+ u32 cipher_type)
+{
+ struct crypto_ablkcipher *tfm = crypto_ablkcipher_reqtfm(req);
+ struct cvm_enc_ctx *ctx = crypto_ablkcipher_ctx(tfm);
+ u32 key_type = AES_128_BIT;
+ struct cvm_req_ctx *rctx = ablkcipher_request_ctx(req);
+ u32 enc_iv_len = crypto_ablkcipher_ivsize(tfm);
+ struct fc_context *fctx = &rctx->fctx;
+ struct cpt_request_info *req_info = &rctx->cpt_req;
+ void *cdev = NULL;
+ u32 status = -1;
Doable but dangerous
Furthermore, cptvf_do_request return int so why use u32 ?
will fix it.
[...]
+void cvm_enc_dec_exit(struct crypto_tfm *tfm)
+{
+ return;
+}
So you could remove all reference to this function
okay
[...]
+static inline int cav_register_algs(void)
+{
+ int err = 0;
+
+ err = crypto_register_algs(algs, ARRAY_SIZE(algs));
+ if (err) {
+ pr_err("Error in aes module init %d\n", err);
+ return -1;
This is not a standard error code
okay
[...]
diff --git a/drivers/crypto/cavium/cpt/cptvf_algs.h b/drivers/crypto/cavium/cpt/cptvf_algs.h
new file mode 100644
index 0000000..fcb287b
--- /dev/null
+++ b/drivers/crypto/cavium/cpt/cptvf_algs.h
[...]
+
+u32 cptvf_do_request(void *cptvf, struct cpt_request_info *req);
latter this function is set "return int"
[...]
+static int cptvf_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
+{
+ struct device *dev = &pdev->dev;
+ struct cpt_vf *cptvf;
+ int err;
+
+ cptvf = devm_kzalloc(dev, sizeof(struct cpt_vf), GFP_KERNEL);
use sizeof(*cptvf) and checkpatch
okay
[...]
+static int setup_sgio_components(struct cpt_vf *cptvf, struct buf_ptr *list,
+ int buf_count, u8 *buffer)
+{
+ int ret = 0, i, j;
+ int components;
+ struct sglist_component *sg_ptr = NULL;
+ struct pci_dev *pdev = cptvf->pdev;
+
+ if (unlikely(!list)) {
+ pr_err("Input List pointer is NULL\n");
+ ret = -EFAULT;
+ return ret;
You could directly return -EFAULT and use dev_err()
okay
+ }
+
+ for (i = 0; i < buf_count; i++) {
+ if (likely(list[i].vptr)) {
+ list[i].dma_addr = dma_map_single(&pdev->dev,
+ list[i].vptr,
+ list[i].size,
+ DMA_BIDIRECTIONAL);
+ if (unlikely(dma_mapping_error(&pdev->dev,
+ list[i].dma_addr))) {
+ pr_err("DMA map kernel buffer failed for component: %d\n",
+ i);
Use dev_err
[...]
+ u16 g_sz_bytes = 0, s_sz_bytes = 0;
+ int ret = 0;
+ struct pci_dev *pdev = cptvf->pdev;
+
+ if (req->incnt > MAX_SG_IN_CNT || req->outcnt > MAX_SG_OUT_CNT) {
+ pr_err("Requestes SG components are higher than supported\n");
typo request and use dev_err
In all files you have some pr_x that could be better use as dev_x
okay
+ ret = -EINVAL;
+ goto scatter_gather_clean;
+ }
+
+ /* Setup gather (input) components */
+ g_sz_bytes = ((req->incnt + 3) / 4) * sizeof(struct sglist_component);
+ info->gather_components = kzalloc((g_sz_bytes), GFP_KERNEL);
unnecessary parenthesis
+ if (!info->gather_components) {
+ ret = -ENOMEM;
+ goto scatter_gather_clean;
+ }
+
+ ret = setup_sgio_components(cptvf, req->in,
+ req->incnt,
+ info->gather_components);
+ if (ret) {
+ pr_err("Failed to setup gather list\n");
+ ret = -EFAULT;
+ goto scatter_gather_clean;
+ }
+
+ /* Setup scatter (output) components */
+ s_sz_bytes = ((req->outcnt + 3) / 4) * sizeof(struct sglist_component);
+ info->scatter_components = kzalloc((s_sz_bytes), GFP_KERNEL);
again
+ if (!info->scatter_components) {
+ ret = -ENOMEM;
+ goto scatter_gather_clean;
+ }
+
+ ret = setup_sgio_components(cptvf, req->out,
+ req->outcnt,
+ info->scatter_components);
+ if (ret) {
+ pr_err("Failed to setup gather list\n");
+ ret = -EFAULT;
+ goto scatter_gather_clean;
double space
okay
+ }
+
+ /* Create and initialize DPTR */
+ info->dlen = g_sz_bytes + s_sz_bytes + SG_LIST_HDR_SIZE;
+ info->in_buffer = kzalloc((info->dlen), GFP_KERNEL);
double parenthesis
I will stop here, you have lots of that in all your alloc
okay
[...]
+
+ ret = send_cpt_command(cptvf, &cptinst, queue);
+ spin_unlock_bh(&pqueue->lock);
+ if (unlikely(ret)) {
+ spin_unlock_bh(&pqueue->lock);
Double unlock
Yes will fix it.
[...]
diff --git a/drivers/crypto/cavium/cpt/request_manager.h b/drivers/crypto/cavium/cpt/request_manager.h
new file mode 100644
index 0000000..df6c306
--- /dev/null
+++ b/drivers/crypto/cavium/cpt/request_manager.h
@@ -0,0 +1,147 @@
+/*
+ * Copyright (C) 2016 Cavium, Inc.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of version 2 of the GNU General Public License
+ * as published by the Free Software Foundation.
+ */
+
+#ifndef __REQUEST_MANGER_H
+#define __REQUEST_MANGER_H
typo manager
okay
Thanks
Regards
Corentin Labbe