From: George Cherian Subject: Re: [PATCH v3 2/3] drivers: crypto: Add the Virtual Function driver for CPT Date: Fri, 6 Jan 2017 12:33:55 +0530 Message-ID: <586F415B.7030207@caviumnetworks.com> References: <1482321373-407-1-git-send-email-george.cherian@cavium.com> <1482321373-407-3-git-send-email-george.cherian@cavium.com> <20161221140155.GB21051@Red> Mime-Version: 1.0 Content-Type: text/plain; charset="windows-1252"; format=flowed Content-Transfer-Encoding: 7bit Cc: , , , , To: Corentin Labbe , Return-path: Received: from mail-sn1nam02on0068.outbound.protection.outlook.com ([104.47.36.68]:40468 "EHLO NAM02-SN1-obe.outbound.protection.outlook.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1750839AbdAFHFK (ORCPT ); Fri, 6 Jan 2017 02:05:10 -0500 In-Reply-To: <20161221140155.GB21051@Red> Sender: linux-crypto-owner@vger.kernel.org List-ID: 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@cavium.com wrote: >> From: George Cherian >> >> 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 >