Sure Easwar,
Thanks for the feedback. I will try to split the patches into
smaller, more manageable ones.
Warm regards,
PK
On Mon, Apr 1, 2024 at 10:04 PM Easwar Hariharan
<eahariha@xxxxxxxxxxxxxxxxxxx> wrote:
>
> On 4/1/2024 12:21 AM, Pavitrakumar Managutte wrote:
> > Hi Easwar,
> > My comments are embedded below. Also should I wait for more comments
> > from you or
> > should I go ahead and push v2 with the below fixes ?
> >
> > Warm regards,
> > PK
> >
>
> <snip>
>
> Please see more comments below, and go ahead and push a v2. If possible, split
> the series into more patches. I leave it to you to group them by file or logical
> functionality, so long as each individual commit compiles neatly so as to not break
> future git bisects.
>
>
> On 3/28/2024 11:26 AM, Pavitrakumar M wrote:
> > Signed-off-by: shwetar <shwetar@xxxxxxxxxxxxxxx>
> > Signed-off-by: Pavitrakumar M <pavitrakumarm@xxxxxxxxxxxxxxx>
> > Acked-by: Ruud Derwig <Ruud.Derwig@xxxxxxxxxxxx>
> > ---
> > drivers/crypto/dwc-spacc/spacc_aead.c | 1382 ++++++++++
> > drivers/crypto/dwc-spacc/spacc_ahash.c | 1183 ++++++++
> > drivers/crypto/dwc-spacc/spacc_core.c | 2917 ++++++++++++++++++++
> > drivers/crypto/dwc-spacc/spacc_core.h | 839 ++++++
> > drivers/crypto/dwc-spacc/spacc_device.c | 324 +++
> > drivers/crypto/dwc-spacc/spacc_device.h | 236 ++
> > drivers/crypto/dwc-spacc/spacc_hal.c | 365 +++
> > drivers/crypto/dwc-spacc/spacc_hal.h | 113 +
> > drivers/crypto/dwc-spacc/spacc_interrupt.c | 204 ++
> > drivers/crypto/dwc-spacc/spacc_manager.c | 670 +++++
> > drivers/crypto/dwc-spacc/spacc_skcipher.c | 754 +++++
> > 11 files changed, 8987 insertions(+)
> > create mode 100644 drivers/crypto/dwc-spacc/spacc_aead.c
> > create mode 100644 drivers/crypto/dwc-spacc/spacc_ahash.c
> > create mode 100644 drivers/crypto/dwc-spacc/spacc_core.c
> > create mode 100644 drivers/crypto/dwc-spacc/spacc_core.h
> > create mode 100644 drivers/crypto/dwc-spacc/spacc_device.c
> > create mode 100644 drivers/crypto/dwc-spacc/spacc_device.h
> > create mode 100644 drivers/crypto/dwc-spacc/spacc_hal.c
> > create mode 100644 drivers/crypto/dwc-spacc/spacc_hal.h
> > create mode 100644 drivers/crypto/dwc-spacc/spacc_interrupt.c
> > create mode 100644 drivers/crypto/dwc-spacc/spacc_manager.c
> > create mode 100644 drivers/crypto/dwc-spacc/spacc_skcipher.c
> > diff --git a/drivers/crypto/dwc-spacc/spacc_ahash.c b/drivers/crypto/dwc-spacc/spacc_ahash.c
> > new file mode 100644
> > index 000000000000..53c76ee16c53
> > --- /dev/null
> > +++ b/drivers/crypto/dwc-spacc/spacc_ahash.c
> > @@ -0,0 +1,1183 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +
>
> <snip>
>
> > +
> > +static int spacc_hash_init(struct ahash_request *req)
> > +{
> > + int x = 0, rc = 0;
> > + struct crypto_ahash *reqtfm = crypto_ahash_reqtfm(req);
> > + struct spacc_crypto_ctx *tctx = crypto_ahash_ctx(reqtfm);
> > + struct spacc_crypto_reqctx *ctx = ahash_request_ctx(req);
> > + const struct spacc_alg *salg = spacc_tfm_ahash(&reqtfm->base);
> > + struct spacc_priv *priv = dev_get_drvdata(tctx->dev);
> > +
> > +
> > + ctx->digest_buf = NULL;
> > +
> > + ctx->single_shot = 0;
> > + ctx->total_nents = 0;
> > + ctx->cur_part_pck = 0;
> > + ctx->final_part_pck = 0;
> > + ctx->rem_len = 0;
> > + ctx->rem_nents = 0;
> > + ctx->first_ppp_chunk = 1;
> > + ctx->small_pck = 1;
> > + tctx->ppp_sgl = NULL;
> > +
> > + if (tctx->handle < 0 || !tctx->ctx_valid) {
> > + priv = NULL;
> > + dev_dbg(tctx->dev, "%s: open SPAcc context\n", __func__);
> > +
> > + for (x = 0; x < ELP_CAPI_MAX_DEV && salg->dev[x]; x++) {
> > + priv = dev_get_drvdata(salg->dev[x]);
> > + tctx->dev = get_device(salg->dev[x]);
> > + if (spacc_isenabled(&priv->spacc, salg->mode->id, 0)) {
> > + tctx->handle = spacc_open(&priv->spacc,
> > + CRYPTO_MODE_NULL,
> > + salg->mode->id, -1, 0,
> > + spacc_digest_cb, reqtfm);
> > + }
> > +
> > + if (tctx->handle >= 0)
> > + break;
> > +
> > + put_device(salg->dev[x]);
> > + }
> > +
> > + if (tctx->handle < 0) {
> > + dev_dbg(salg->dev[0], "Failed to open SPAcc context\n");
> > + goto fallback;
> > + }
> > +
> > + rc = spacc_set_operation(&priv->spacc, tctx->handle,
> > + OP_ENCRYPT, ICV_HASH, IP_ICV_OFFSET,
> > + 0, 0, 0);
> > + if (rc < 0) {
> > + spacc_close(&priv->spacc, tctx->handle);
> > + dev_dbg(salg->dev[0], "Failed to open SPAcc context\n");
> > + tctx->handle = -1;
> > + put_device(tctx->dev);
> > + goto fallback;
> > + }
> > + tctx->ctx_valid = true;
> > + } else {
> > + ;/* do nothing */
> > + }
>
> Do we need this else?
>
> > +
> > + /* alloc ppp_sgl */
> > + tctx->ppp_sgl = kmalloc(sizeof(*(tctx->ppp_sgl)) * 2, GFP_KERNEL);
> > + if (!tctx->ppp_sgl)
> > + return -ENOMEM;
> > +
> > + sg_init_table(tctx->ppp_sgl, 2);
> > +
> > + return 0;
> > +fallback:
> > +
> > + ctx->fb.hash_req.base = req->base;
> > + ahash_request_set_tfm(&ctx->fb.hash_req, tctx->fb.hash);
> > +
> > + return crypto_ahash_init(&ctx->fb.hash_req);
> > +}
> > +
> > +static int spacc_hash_final_part_pck(struct ahash_request *req)
> > +{
> > + struct crypto_ahash *reqtfm = crypto_ahash_reqtfm(req);
> > + struct spacc_crypto_ctx *tctx = crypto_ahash_ctx(reqtfm);
> > + struct spacc_crypto_reqctx *ctx = ahash_request_ctx(req);
> > + struct spacc_priv *priv = dev_get_drvdata(tctx->dev);
> > +
> > + int rc;
> > +
> > + ctx->final_part_pck = 1;
> > +
> > + /* In all the final calls the data is same as prev update and
> > + * hence we can skip this init dma part and just enQ ddt
> > + * No use in calling initdata, just process remaining bytes in ppp_sgl
> > + * and be done with it.
> > + */
> > +
> > + rc = spacc_hash_init_dma(tctx->dev, req, 1);
> > +
> > + if (rc < 0)
> > + return -ENOMEM;
> > +
> > + if (rc == 0) {
> > + ;/* small packet */
> > + }
>
> Please cleanup these do-nothing comment code paths throughout.
>
> > +
> > + /* enqueue ddt for the remaining bytes of data, everything else
> > + * would have been processed already, req->nbytes need not be
> > + * processed
> > + * Since this will hit only for small pkts, hence the condition
> > + * ctx->rem_len-req->nbytes for the small pkt len
> > + */
> > + if (ctx->rem_len)
> > + rc = spacc_packet_enqueue_ddt(&priv->spacc,
> > + ctx->acb.new_handle, &ctx->src, &ctx->dst,
> > + tctx->ppp_sgl[0].length,
> > + 0, tctx->ppp_sgl[0].length, 0, 0, 0);
> > + else {
> > + /* zero msg handling */
> > + rc = spacc_packet_enqueue_ddt(&priv->spacc,
> > + ctx->acb.new_handle,
> > + &ctx->src, &ctx->dst, 0, 0, 0, 0, 0, 0);
> > + }
> > +
> > + if (rc < 0) {
> > + spacc_hash_cleanup_dma(tctx->dev, req);
> > + spacc_close(&priv->spacc, ctx->acb.new_handle);
> > +
> > + if (rc != -EBUSY) {
> > + dev_err(tctx->dev, "ERR: Failed to enqueue job: %d\n", rc);
> > + return rc;
> > + }
> > +
> > + if (!(req->base.flags & CRYPTO_TFM_REQ_MAY_BACKLOG))
> > + return -EBUSY;
> > + }
> > +
> > + return -EINPROGRESS;
> > +}
> > +
>
> <snip>
>
> Thanks,
> Easwar
