From: Stanimir Varbanov Subject: Re: [PATCH 5/9] crypto: qce: Adds sha and hmac transforms Date: Thu, 10 Apr 2014 17:40:36 +0300 Message-ID: <5346AD64.1000409@mm-sol.com> References: <1396541886-10966-1-git-send-email-svarbanov@mm-sol.com> <1396541886-10966-6-git-send-email-svarbanov@mm-sol.com> <20140409000931.GN9985@codeaurora.org> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Cc: Herbert Xu , "David S. Miller" , linux-kernel@vger.kernel.org, linux-crypto@vger.kernel.org, linux-arm-msm@vger.kernel.org To: Stephen Boyd Return-path: In-Reply-To: <20140409000931.GN9985@codeaurora.org> Sender: linux-arm-msm-owner@vger.kernel.org List-Id: linux-crypto.vger.kernel.org Hi Stephen, thanks for the comments. On 04/09/2014 03:09 AM, Stephen Boyd wrote: > On 04/03, Stanimir Varbanov wrote: >> +static void qce_ahash_dma_done(void *data) >> +{ >> + struct crypto_async_request *async_req = data; >> + struct ahash_request *req = ahash_request_cast(async_req); >> + struct crypto_ahash *ahash = crypto_ahash_reqtfm(req); >> + struct qce_sha_reqctx *rctx = ahash_request_ctx(req); >> + struct qce_alg_template *tmpl = to_ahash_tmpl(async_req->tfm); >> + struct qce_device *qce = tmpl->qce; >> + struct qce_result_dump *result = qce->dma.result_buf; >> + unsigned int digestsize = crypto_ahash_digestsize(ahash); >> + int error; >> + u32 status; >> + >> + qce_dma_terminate_all(&qce->dma); >> + >> + qce_unmapsg(qce->dev, req->src, rctx->src_nents, DMA_TO_DEVICE, >> + rctx->src_chained); >> + qce_unmapsg(qce->dev, &rctx->result_sg, 1, DMA_FROM_DEVICE, 0); >> + >> + memcpy(rctx->digest, result->auth_iv, digestsize); >> + if (req->result) >> + memcpy(req->result, result->auth_iv, digestsize); >> + >> + rctx->byte_count[0] = cpu_to_be32(result->auth_byte_count[0]); >> + rctx->byte_count[1] = cpu_to_be32(result->auth_byte_count[1]); > > Does rctx->byte_count need to be marked __be32? yes, makes sense. > >> + >> + error = qce_check_status(qce, &status); >> + if (error < 0) >> + dev_err(qce->dev, "ahash operation error (%x)\n", status); >> + >> + req->src = rctx->src; >> + req->nbytes = rctx->nbytes; >> + >> + rctx->last_blk = false; >> + rctx->first_blk = false; >> + >> + tmpl->async_req_done(tmpl->qce, error); >> +} >> + > [...] >> +static int qce_import_common(struct ahash_request *req, u64 in_count, >> + u32 *state, u8 *buffer, bool hmac) >> +{ >> + struct crypto_ahash *ahash = crypto_ahash_reqtfm(req); >> + struct qce_sha_reqctx *rctx = ahash_request_ctx(req); >> + u64 count = in_count; >> + unsigned int digestsize = crypto_ahash_digestsize(ahash); >> + unsigned int blocksize; >> + >> + blocksize = crypto_tfm_alg_blocksize(crypto_ahash_tfm(ahash)); >> + rctx->count = in_count; >> + memcpy(rctx->trailing_buf, buffer, blocksize); >> + >> + if (in_count <= blocksize) { >> + rctx->first_blk = 1; >> + } else { >> + rctx->first_blk = 0; >> + /* >> + * For HMAC, there is a hardware padding done when first block >> + * is set. Therefore the byte_count must be incremened by 64 >> + * after the first block operation. >> + */ >> + if (hmac) >> + count += SHA_PADDING; >> + } >> + >> + rctx->byte_count[0] = (u32)(count & ~SHA_PADDING_MASK); >> + rctx->byte_count[1] = (u32)(count >> 32); >> + qce_cpu_to_be32p_array((__be32 *)rctx->digest, (const u8 *)state, >> + digestsize); >> + rctx->trailing_buf_len = (unsigned int)(in_count & (blocksize - 1)); > > Is this a way to say > > (unsigned int)clamp_t(u64, in_count, blocksize - 1) > > ? In fact no, I think it should be: trailing_buf_len = in_count % blocksize. > >> + >> + return 0; >> +} >> + >> +static int qce_ahash_import(struct ahash_request *req, const void *in) >> +{ >> + struct qce_sha_reqctx *rctx = ahash_request_ctx(req); >> + u32 flags = rctx->flags; >> + bool hmac = IS_SHA_HMAC(flags); >> + int ret; >> + >> + if (IS_SHA1(flags) || IS_SHA1_HMAC(flags)) { >> + struct sha1_state *state = (struct sha1_state *)in; > > Unnecessary cast from void *. Nope, "in" is "const void *". Cast is needed to avoid compiler warnings. > >> + >> + ret = qce_import_common(req, state->count, state->state, >> + state->buffer, hmac); >> + } else if (IS_SHA256(flags) || IS_SHA256_HMAC(flags)) { >> + struct sha256_state *state = (struct sha256_state *)in; > > Ditto. > >> + >> + ret = qce_import_common(req, state->count, state->state, >> + state->buf, hmac); >> + } else { >> + ret = -EINVAL; >> + } >> + >> + return ret; >> +} >> + >> +static int qce_ahash_update(struct ahash_request *req) >> +{ >> + struct crypto_ahash *tfm = crypto_ahash_reqtfm(req); >> + struct qce_sha_reqctx *rctx = ahash_request_ctx(req); >> + struct qce_alg_template *tmpl = to_ahash_tmpl(req->base.tfm); >> + unsigned int total, len; >> + int nents; >> + struct scatterlist *sg_last; >> + u8 *buf; > >> + u32 pad_len; >> + u32 trailing_buf_len; >> + u32 nbytes; >> + u32 offset; >> + u32 bytes; > > size_t for these? Hm, probably yes, I thought to revise this function cause it looks more complicated than it should be. > >> + u8 *staging; >> + bool chained; >> + unsigned int blocksize; >> + >> + blocksize = crypto_tfm_alg_blocksize(crypto_ahash_tfm(tfm)); >> + rctx->count += req->nbytes; >> + >> + /* check for trailing buffer from previous updates and append it */ >> + total = req->nbytes + rctx->trailing_buf_len; >> + len = req->nbytes; > [...] >> + >> +struct qce_ahash_def { >> + u32 flags; > > unsigned long? sure, will do. > >> + const char *name; >> + const char *drv_name; >> + unsigned int digestsize; >> + unsigned int blocksize; >> + unsigned int statesize; >> + const __be32 *std_iv; >> +}; > [..] >> + >> +/* > > Nit: This isn't kernel doc notation OK. Will change to kernel doc in next version. > >> + * @flags: operation flags >> + * @src: request sg >> + * @src_chained: is source scatterlist chained >> + * @src_nents: source number of entries >> + * @nbytes: request number of bytes >> + * @byte_count: byte count >> + * @count: save count in states during update, import and export >> + * @first_blk: is it the first block >> + * @last_blk: is it the last block >> + * @trailing_buf: used during update, import and export >> + * @trailing_buf_len: lenght of the trailing buffer >> + * @staging_buf: buffer for internal use >> + * @digest: calculated digest >> + * @sg: used to chain sg lists >> + * @authkey: pointer to auth key in sha ctx >> + * @authklen: auth key length >> + * @result_sg: scatterlist used for result buffer >> + */ >> +struct qce_sha_reqctx { >> + u32 flags; > > unsigned long? sure, will do. > >> + struct scatterlist *src; >> + bool src_chained; >> + int src_nents; >> + unsigned int nbytes; >> + u32 byte_count[2]; >> + u64 count; >> + bool first_blk; > -- regards, Stan