From mboxrd@z Thu Jan 1 00:00:00 1970 From: Boris Brezillon Subject: Re: [PATCH v3 1/2] crypto: marvell - Use an unique pool to copy results of requests Date: Tue, 4 Oct 2016 15:17:03 +0200 Message-ID: <20161004151703.515c9967@bbrezillon> References: <20161004125720.3347-1-romain.perier@free-electrons.com> <20161004125720.3347-2-romain.perier@free-electrons.com> Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Cc: Arnaud Ebalard , "David S. Miller" , Herbert Xu , Thomas Petazzoni , Jason Cooper , Andrew Lunn , Sebastian Hesselbarth , Gregory Clement , Nadav Haklai , Ofer Heifetz , linux-crypto@vger.kernel.org, linux-arm-kernel@lists.infradead.org To: Romain Perier Return-path: Received: from down.free-electrons.com ([37.187.137.238]:59630 "EHLO mail.free-electrons.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1753186AbcJDNRG (ORCPT ); Tue, 4 Oct 2016 09:17:06 -0400 In-Reply-To: <20161004125720.3347-2-romain.perier@free-electrons.com> Sender: linux-crypto-owner@vger.kernel.org List-ID: On Tue, 4 Oct 2016 14:57:19 +0200 Romain Perier wrote: > So far, we used a dedicated dma pool to copy the result of outer IV for > cipher requests. Instead of using a dma pool per outer data, we prefer > use the op dma pool that contains all part of the request from the SRAM. > Then, the outer data that is likely to be used by the 'complete' > operation, is copied later. In this way, any type of result can be > retrieved by DMA for cipher or ahash requests. > > Signed-off-by: Romain Perier > --- > > Changes in v3: > - Don't allocate a new op ctx for the last tdma descriptor. Instead > we point to the last op ctx in the tdma chain, and copy the context > of the current request to this location. > > Changes in v2: > - Use the dma pool "op" to retrieve outer data intead of introducing > a new one. > > drivers/crypto/marvell/cesa.c | 4 ---- > drivers/crypto/marvell/cesa.h | 5 ++--- > drivers/crypto/marvell/cipher.c | 8 +++++--- > drivers/crypto/marvell/tdma.c | 28 ++++++++++++++-------------- > 4 files changed, 21 insertions(+), 24 deletions(-) > > diff --git a/drivers/crypto/marvell/cesa.c b/drivers/crypto/marvell/cesa.c > index 37dadb2..6e7a5c7 100644 > --- a/drivers/crypto/marvell/cesa.c > +++ b/drivers/crypto/marvell/cesa.c > @@ -375,10 +375,6 @@ static int mv_cesa_dev_dma_init(struct mv_cesa_dev *cesa) > if (!dma->padding_pool) > return -ENOMEM; > > - dma->iv_pool = dmam_pool_create("cesa_iv", dev, 16, 1, 0); > - if (!dma->iv_pool) > - return -ENOMEM; > - > cesa->dma = dma; > > return 0; > diff --git a/drivers/crypto/marvell/cesa.h b/drivers/crypto/marvell/cesa.h > index e423d33..a768da7 100644 > --- a/drivers/crypto/marvell/cesa.h > +++ b/drivers/crypto/marvell/cesa.h > @@ -277,7 +277,7 @@ struct mv_cesa_op_ctx { > #define CESA_TDMA_DUMMY 0 > #define CESA_TDMA_DATA 1 > #define CESA_TDMA_OP 2 > -#define CESA_TDMA_IV 3 > +#define CESA_TDMA_RESULT 3 > > /** > * struct mv_cesa_tdma_desc - TDMA descriptor > @@ -393,7 +393,6 @@ struct mv_cesa_dev_dma { > struct dma_pool *op_pool; > struct dma_pool *cache_pool; > struct dma_pool *padding_pool; > - struct dma_pool *iv_pool; > }; > > /** > @@ -839,7 +838,7 @@ mv_cesa_tdma_desc_iter_init(struct mv_cesa_tdma_chain *chain) > memset(chain, 0, sizeof(*chain)); > } > > -int mv_cesa_dma_add_iv_op(struct mv_cesa_tdma_chain *chain, dma_addr_t src, > +int mv_cesa_dma_add_result_op(struct mv_cesa_tdma_chain *chain, dma_addr_t src, > u32 size, u32 flags, gfp_t gfp_flags); > > struct mv_cesa_op_ctx *mv_cesa_dma_add_op(struct mv_cesa_tdma_chain *chain, > diff --git a/drivers/crypto/marvell/cipher.c b/drivers/crypto/marvell/cipher.c > index d19dc96..098871a 100644 > --- a/drivers/crypto/marvell/cipher.c > +++ b/drivers/crypto/marvell/cipher.c > @@ -212,7 +212,8 @@ mv_cesa_ablkcipher_complete(struct crypto_async_request *req) > struct mv_cesa_req *basereq; > > basereq = &creq->base; > - memcpy(ablkreq->info, basereq->chain.last->data, ivsize); > + memcpy(ablkreq->info, basereq->chain.last->op->ctx.blkcipher.iv, > + ivsize); > } else { > memcpy_fromio(ablkreq->info, > engine->sram + CESA_SA_CRYPT_IV_SRAM_OFFSET, > @@ -373,8 +374,9 @@ static int mv_cesa_ablkcipher_dma_req_init(struct ablkcipher_request *req, > > /* Add output data for IV */ > ivsize = crypto_ablkcipher_ivsize(crypto_ablkcipher_reqtfm(req)); > - ret = mv_cesa_dma_add_iv_op(&basereq->chain, CESA_SA_CRYPT_IV_SRAM_OFFSET, > - ivsize, CESA_TDMA_SRC_IN_SRAM, flags); > + ret = mv_cesa_dma_add_result_op(&basereq->chain, CESA_SA_CFG_SRAM_OFFSET, > + CESA_SA_DATA_SRAM_OFFSET, > + CESA_TDMA_SRC_IN_SRAM, flags); > > if (ret) > goto err_free_tdma; > diff --git a/drivers/crypto/marvell/tdma.c b/drivers/crypto/marvell/tdma.c > index 9fd7a5f..991dc3f 100644 > --- a/drivers/crypto/marvell/tdma.c > +++ b/drivers/crypto/marvell/tdma.c > @@ -69,9 +69,6 @@ void mv_cesa_dma_cleanup(struct mv_cesa_req *dreq) > if (type == CESA_TDMA_OP) > dma_pool_free(cesa_dev->dma->op_pool, tdma->op, > le32_to_cpu(tdma->src)); > - else if (type == CESA_TDMA_IV) > - dma_pool_free(cesa_dev->dma->iv_pool, tdma->data, > - le32_to_cpu(tdma->dst)); > > tdma = tdma->next; > dma_pool_free(cesa_dev->dma->tdma_desc_pool, old_tdma, > @@ -209,29 +206,32 @@ mv_cesa_dma_add_desc(struct mv_cesa_tdma_chain *chain, gfp_t flags) > return new_tdma; > } > > -int mv_cesa_dma_add_iv_op(struct mv_cesa_tdma_chain *chain, dma_addr_t src, > +int mv_cesa_dma_add_result_op(struct mv_cesa_tdma_chain *chain, dma_addr_t src, > u32 size, u32 flags, gfp_t gfp_flags) > { > - > - struct mv_cesa_tdma_desc *tdma; > - u8 *iv; > - dma_addr_t dma_handle; > + struct mv_cesa_tdma_desc *tdma, *op_desc; > > tdma = mv_cesa_dma_add_desc(chain, gfp_flags); > if (IS_ERR(tdma)) > return PTR_ERR(tdma); > > - iv = dma_pool_alloc(cesa_dev->dma->iv_pool, gfp_flags, &dma_handle); > - if (!iv) > - return -ENOMEM; Can you add a comment explaining what you're doing here? /* We re-use an existing op_desc object to retrieve the context * and result instead of allocating a new one. * There is at least one object of this type in a CESA crypto * req, just pick the first one in the chain. */ Once this is addressed, you can add my Acked-by: Boris Brezillon > + for (op_desc = chain->first; op_desc; op_desc = op_desc->next) { > + u32 type = op_desc->flags & CESA_TDMA_TYPE_MSK; > + > + if (type == CESA_TDMA_OP) > + break; > + } > + > + if (!op_desc) > + return -EIO; > > tdma->byte_cnt = cpu_to_le32(size | BIT(31)); > tdma->src = src; > - tdma->dst = cpu_to_le32(dma_handle); > - tdma->data = iv; > + tdma->dst = op_desc->src; > + tdma->op = op_desc->op; > > flags &= (CESA_TDMA_DST_IN_SRAM | CESA_TDMA_SRC_IN_SRAM); > - tdma->flags = flags | CESA_TDMA_IV; > + tdma->flags = flags | CESA_TDMA_RESULT; > return 0; > } > From mboxrd@z Thu Jan 1 00:00:00 1970 From: boris.brezillon@free-electrons.com (Boris Brezillon) Date: Tue, 4 Oct 2016 15:17:03 +0200 Subject: [PATCH v3 1/2] crypto: marvell - Use an unique pool to copy results of requests In-Reply-To: <20161004125720.3347-2-romain.perier@free-electrons.com> References: <20161004125720.3347-1-romain.perier@free-electrons.com> <20161004125720.3347-2-romain.perier@free-electrons.com> Message-ID: <20161004151703.515c9967@bbrezillon> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On Tue, 4 Oct 2016 14:57:19 +0200 Romain Perier wrote: > So far, we used a dedicated dma pool to copy the result of outer IV for > cipher requests. Instead of using a dma pool per outer data, we prefer > use the op dma pool that contains all part of the request from the SRAM. > Then, the outer data that is likely to be used by the 'complete' > operation, is copied later. In this way, any type of result can be > retrieved by DMA for cipher or ahash requests. > > Signed-off-by: Romain Perier > --- > > Changes in v3: > - Don't allocate a new op ctx for the last tdma descriptor. Instead > we point to the last op ctx in the tdma chain, and copy the context > of the current request to this location. > > Changes in v2: > - Use the dma pool "op" to retrieve outer data intead of introducing > a new one. > > drivers/crypto/marvell/cesa.c | 4 ---- > drivers/crypto/marvell/cesa.h | 5 ++--- > drivers/crypto/marvell/cipher.c | 8 +++++--- > drivers/crypto/marvell/tdma.c | 28 ++++++++++++++-------------- > 4 files changed, 21 insertions(+), 24 deletions(-) > > diff --git a/drivers/crypto/marvell/cesa.c b/drivers/crypto/marvell/cesa.c > index 37dadb2..6e7a5c7 100644 > --- a/drivers/crypto/marvell/cesa.c > +++ b/drivers/crypto/marvell/cesa.c > @@ -375,10 +375,6 @@ static int mv_cesa_dev_dma_init(struct mv_cesa_dev *cesa) > if (!dma->padding_pool) > return -ENOMEM; > > - dma->iv_pool = dmam_pool_create("cesa_iv", dev, 16, 1, 0); > - if (!dma->iv_pool) > - return -ENOMEM; > - > cesa->dma = dma; > > return 0; > diff --git a/drivers/crypto/marvell/cesa.h b/drivers/crypto/marvell/cesa.h > index e423d33..a768da7 100644 > --- a/drivers/crypto/marvell/cesa.h > +++ b/drivers/crypto/marvell/cesa.h > @@ -277,7 +277,7 @@ struct mv_cesa_op_ctx { > #define CESA_TDMA_DUMMY 0 > #define CESA_TDMA_DATA 1 > #define CESA_TDMA_OP 2 > -#define CESA_TDMA_IV 3 > +#define CESA_TDMA_RESULT 3 > > /** > * struct mv_cesa_tdma_desc - TDMA descriptor > @@ -393,7 +393,6 @@ struct mv_cesa_dev_dma { > struct dma_pool *op_pool; > struct dma_pool *cache_pool; > struct dma_pool *padding_pool; > - struct dma_pool *iv_pool; > }; > > /** > @@ -839,7 +838,7 @@ mv_cesa_tdma_desc_iter_init(struct mv_cesa_tdma_chain *chain) > memset(chain, 0, sizeof(*chain)); > } > > -int mv_cesa_dma_add_iv_op(struct mv_cesa_tdma_chain *chain, dma_addr_t src, > +int mv_cesa_dma_add_result_op(struct mv_cesa_tdma_chain *chain, dma_addr_t src, > u32 size, u32 flags, gfp_t gfp_flags); > > struct mv_cesa_op_ctx *mv_cesa_dma_add_op(struct mv_cesa_tdma_chain *chain, > diff --git a/drivers/crypto/marvell/cipher.c b/drivers/crypto/marvell/cipher.c > index d19dc96..098871a 100644 > --- a/drivers/crypto/marvell/cipher.c > +++ b/drivers/crypto/marvell/cipher.c > @@ -212,7 +212,8 @@ mv_cesa_ablkcipher_complete(struct crypto_async_request *req) > struct mv_cesa_req *basereq; > > basereq = &creq->base; > - memcpy(ablkreq->info, basereq->chain.last->data, ivsize); > + memcpy(ablkreq->info, basereq->chain.last->op->ctx.blkcipher.iv, > + ivsize); > } else { > memcpy_fromio(ablkreq->info, > engine->sram + CESA_SA_CRYPT_IV_SRAM_OFFSET, > @@ -373,8 +374,9 @@ static int mv_cesa_ablkcipher_dma_req_init(struct ablkcipher_request *req, > > /* Add output data for IV */ > ivsize = crypto_ablkcipher_ivsize(crypto_ablkcipher_reqtfm(req)); > - ret = mv_cesa_dma_add_iv_op(&basereq->chain, CESA_SA_CRYPT_IV_SRAM_OFFSET, > - ivsize, CESA_TDMA_SRC_IN_SRAM, flags); > + ret = mv_cesa_dma_add_result_op(&basereq->chain, CESA_SA_CFG_SRAM_OFFSET, > + CESA_SA_DATA_SRAM_OFFSET, > + CESA_TDMA_SRC_IN_SRAM, flags); > > if (ret) > goto err_free_tdma; > diff --git a/drivers/crypto/marvell/tdma.c b/drivers/crypto/marvell/tdma.c > index 9fd7a5f..991dc3f 100644 > --- a/drivers/crypto/marvell/tdma.c > +++ b/drivers/crypto/marvell/tdma.c > @@ -69,9 +69,6 @@ void mv_cesa_dma_cleanup(struct mv_cesa_req *dreq) > if (type == CESA_TDMA_OP) > dma_pool_free(cesa_dev->dma->op_pool, tdma->op, > le32_to_cpu(tdma->src)); > - else if (type == CESA_TDMA_IV) > - dma_pool_free(cesa_dev->dma->iv_pool, tdma->data, > - le32_to_cpu(tdma->dst)); > > tdma = tdma->next; > dma_pool_free(cesa_dev->dma->tdma_desc_pool, old_tdma, > @@ -209,29 +206,32 @@ mv_cesa_dma_add_desc(struct mv_cesa_tdma_chain *chain, gfp_t flags) > return new_tdma; > } > > -int mv_cesa_dma_add_iv_op(struct mv_cesa_tdma_chain *chain, dma_addr_t src, > +int mv_cesa_dma_add_result_op(struct mv_cesa_tdma_chain *chain, dma_addr_t src, > u32 size, u32 flags, gfp_t gfp_flags) > { > - > - struct mv_cesa_tdma_desc *tdma; > - u8 *iv; > - dma_addr_t dma_handle; > + struct mv_cesa_tdma_desc *tdma, *op_desc; > > tdma = mv_cesa_dma_add_desc(chain, gfp_flags); > if (IS_ERR(tdma)) > return PTR_ERR(tdma); > > - iv = dma_pool_alloc(cesa_dev->dma->iv_pool, gfp_flags, &dma_handle); > - if (!iv) > - return -ENOMEM; Can you add a comment explaining what you're doing here? /* We re-use an existing op_desc object to retrieve the context * and result instead of allocating a new one. * There is at least one object of this type in a CESA crypto * req, just pick the first one in the chain. */ Once this is addressed, you can add my Acked-by: Boris Brezillon > + for (op_desc = chain->first; op_desc; op_desc = op_desc->next) { > + u32 type = op_desc->flags & CESA_TDMA_TYPE_MSK; > + > + if (type == CESA_TDMA_OP) > + break; > + } > + > + if (!op_desc) > + return -EIO; > > tdma->byte_cnt = cpu_to_le32(size | BIT(31)); > tdma->src = src; > - tdma->dst = cpu_to_le32(dma_handle); > - tdma->data = iv; > + tdma->dst = op_desc->src; > + tdma->op = op_desc->op; > > flags &= (CESA_TDMA_DST_IN_SRAM | CESA_TDMA_SRC_IN_SRAM); > - tdma->flags = flags | CESA_TDMA_IV; > + tdma->flags = flags | CESA_TDMA_RESULT; > return 0; > } >