| Message ID | E1ZnqkG-0005WH-TE@rmk-PC.arm.linux.org.uk (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Herbert Xu |
| Headers | show |
Hey Russell, On Sun, Oct 18, 2015 at 05:23:40PM +0100, Russell King wrote: > Rather than determining whether we're using a MD5 hash by looking at > the digest size, switch to a cleaner solution using a per-request flag > initialised by the method type. > > Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk> > --- > drivers/crypto/marvell/cesa.h | 1 + > drivers/crypto/marvell/hash.c | 17 +++++++++-------- > 2 files changed, 10 insertions(+), 8 deletions(-) > ... > diff --git a/drivers/crypto/marvell/hash.c b/drivers/crypto/marvell/hash.c > index f59faabcd34f..aa12274608ab 100644 > --- a/drivers/crypto/marvell/hash.c > +++ b/drivers/crypto/marvell/hash.c > @@ -351,7 +351,7 @@ static int mv_cesa_ahash_process(struct crypto_async_request *req, u32 status) > * Hardware's MD5 digest is in little endian format, but > * SHA in big endian format > */ > - if (digsize == MD5_DIGEST_SIZE) { > + if (creq->algo_le) { > __le32 *result = (void *)ahashreq->result; > > for (i = 0; i < digsize / 4; i++) > @@ -407,7 +407,7 @@ static const struct mv_cesa_req_ops mv_cesa_ahash_req_ops = { > }; > > static int mv_cesa_ahash_init(struct ahash_request *req, > - struct mv_cesa_op_ctx *tmpl) > + struct mv_cesa_op_ctx *tmpl, bool algo_le) nit: Would it make more sense the do bool algo_endian, and then ... > @@ -861,7 +862,7 @@ static int mv_cesa_md5_init(struct ahash_request *req) > > mv_cesa_set_op_cfg(&tmpl, CESA_SA_DESC_CFG_MACM_MD5); > > - mv_cesa_ahash_init(req, &tmpl); > + mv_cesa_ahash_init(req, &tmpl, true); mv_cesa_ahash_init(req, &tmpl, ALGO_ENDIAN_LITTLE); 'true' doesn't seem as obvious. But again, nit-picky. thx, Jason. -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Oct 19, 2015 at 03:04:51PM +0000, Jason Cooper wrote: > Hey Russell, > > On Sun, Oct 18, 2015 at 05:23:40PM +0100, Russell King wrote: > > static int mv_cesa_ahash_init(struct ahash_request *req, > > - struct mv_cesa_op_ctx *tmpl) > > + struct mv_cesa_op_ctx *tmpl, bool algo_le) > > nit: Would it make more sense the do bool algo_endian, and then ... I don't like "bool"s that don't self-document what they mean. What does "if (algo_endian)" mean? It's pretty clear what "if (algo_le)" means in the context of endianness, though I'll give you that "if (algo_little_endian)" would be a lot better. > > > @@ -861,7 +862,7 @@ static int mv_cesa_md5_init(struct ahash_request *req) > > > > mv_cesa_set_op_cfg(&tmpl, CESA_SA_DESC_CFG_MACM_MD5); > > > > - mv_cesa_ahash_init(req, &tmpl); > > + mv_cesa_ahash_init(req, &tmpl, true); > > mv_cesa_ahash_init(req, &tmpl, ALGO_ENDIAN_LITTLE); > > 'true' doesn't seem as obvious. But again, nit-picky. I did think about: enum { ALGO_LITTLE_ENDIAN, ALGO_BIG_ENDIAN, }; and passing an int algo_endian around, but that seemed to be like too much bloat... though if you want to insist, I could make that change.
On Mon, Oct 19, 2015 at 04:25:07PM +0100, Russell King - ARM Linux wrote: > On Mon, Oct 19, 2015 at 03:04:51PM +0000, Jason Cooper wrote: > > Hey Russell, > > > > On Sun, Oct 18, 2015 at 05:23:40PM +0100, Russell King wrote: > > > static int mv_cesa_ahash_init(struct ahash_request *req, > > > - struct mv_cesa_op_ctx *tmpl) > > > + struct mv_cesa_op_ctx *tmpl, bool algo_le) > > > > nit: Would it make more sense the do bool algo_endian, and then ... > > I don't like "bool"s that don't self-document what they mean. > What does "if (algo_endian)" mean? It's pretty clear what That's where I would go with an enum. "if (algo_endian == ALGO_ENDIAN_LITTLE) ..." > "if (algo_le)" means in the context of endianness, though I'll > give you that "if (algo_little_endian)" would be a lot better. Right, it's really a question of where do we want readability? I was focusing on the call site, since the context isn't there for newcomers to easily grok 'true'. > > > @@ -861,7 +862,7 @@ static int mv_cesa_md5_init(struct ahash_request *req) > > > > > > mv_cesa_set_op_cfg(&tmpl, CESA_SA_DESC_CFG_MACM_MD5); > > > > > > - mv_cesa_ahash_init(req, &tmpl); > > > + mv_cesa_ahash_init(req, &tmpl, true); > > > > mv_cesa_ahash_init(req, &tmpl, ALGO_ENDIAN_LITTLE); > > > > 'true' doesn't seem as obvious. But again, nit-picky. > > I did think about: > > enum { > ALGO_LITTLE_ENDIAN, > ALGO_BIG_ENDIAN, > }; > > and passing an int algo_endian around, but that seemed to be like too > much bloat... though if you want to insist, I could make that change. Like I said, it's a nit. Either a self-documenting bool, or an enum will work. thx, Jason. -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Oct 19, 2015 at 04:25:07PM +0100, Russell King - ARM Linux wrote: > > > > @@ -861,7 +862,7 @@ static int mv_cesa_md5_init(struct ahash_request *req) > > > > > > mv_cesa_set_op_cfg(&tmpl, CESA_SA_DESC_CFG_MACM_MD5); > > > > > > - mv_cesa_ahash_init(req, &tmpl); > > > + mv_cesa_ahash_init(req, &tmpl, true); > > > > mv_cesa_ahash_init(req, &tmpl, ALGO_ENDIAN_LITTLE); > > > > 'true' doesn't seem as obvious. But again, nit-picky. > > I did think about: > > enum { > ALGO_LITTLE_ENDIAN, > ALGO_BIG_ENDIAN, > }; > > and passing an int algo_endian around, but that seemed to be like too > much bloat... though if you want to insist, I could make that change. I think the patch is fine as it is. Thanks,
diff --git a/drivers/crypto/marvell/cesa.h b/drivers/crypto/marvell/cesa.h index e19302c9dec9..5d5b66ea2ceb 100644 --- a/drivers/crypto/marvell/cesa.h +++ b/drivers/crypto/marvell/cesa.h @@ -612,6 +612,7 @@ struct mv_cesa_ahash_req { u64 len; int src_nents; bool last_req; + bool algo_le; u32 state[8]; }; diff --git a/drivers/crypto/marvell/hash.c b/drivers/crypto/marvell/hash.c index f59faabcd34f..aa12274608ab 100644 --- a/drivers/crypto/marvell/hash.c +++ b/drivers/crypto/marvell/hash.c @@ -351,7 +351,7 @@ static int mv_cesa_ahash_process(struct crypto_async_request *req, u32 status) * Hardware's MD5 digest is in little endian format, but * SHA in big endian format */ - if (digsize == MD5_DIGEST_SIZE) { + if (creq->algo_le) { __le32 *result = (void *)ahashreq->result; for (i = 0; i < digsize / 4; i++) @@ -407,7 +407,7 @@ static const struct mv_cesa_req_ops mv_cesa_ahash_req_ops = { }; static int mv_cesa_ahash_init(struct ahash_request *req, - struct mv_cesa_op_ctx *tmpl) + struct mv_cesa_op_ctx *tmpl, bool algo_le) { struct mv_cesa_ahash_req *creq = ahash_request_ctx(req); @@ -421,6 +421,7 @@ static int mv_cesa_ahash_init(struct ahash_request *req, mv_cesa_set_mac_op_frag_len(tmpl, 0); creq->op_tmpl = *tmpl; creq->len = 0; + creq->algo_le = algo_le; return 0; } @@ -861,7 +862,7 @@ static int mv_cesa_md5_init(struct ahash_request *req) mv_cesa_set_op_cfg(&tmpl, CESA_SA_DESC_CFG_MACM_MD5); - mv_cesa_ahash_init(req, &tmpl); + mv_cesa_ahash_init(req, &tmpl, true); return 0; } @@ -924,7 +925,7 @@ static int mv_cesa_sha1_init(struct ahash_request *req) mv_cesa_set_op_cfg(&tmpl, CESA_SA_DESC_CFG_MACM_SHA1); - mv_cesa_ahash_init(req, &tmpl); + mv_cesa_ahash_init(req, &tmpl, false); return 0; } @@ -987,7 +988,7 @@ static int mv_cesa_sha256_init(struct ahash_request *req) mv_cesa_set_op_cfg(&tmpl, CESA_SA_DESC_CFG_MACM_SHA256); - mv_cesa_ahash_init(req, &tmpl); + mv_cesa_ahash_init(req, &tmpl, false); return 0; } @@ -1218,7 +1219,7 @@ static int mv_cesa_ahmac_md5_init(struct ahash_request *req) mv_cesa_set_op_cfg(&tmpl, CESA_SA_DESC_CFG_MACM_HMAC_MD5); memcpy(tmpl.ctx.hash.iv, ctx->iv, sizeof(ctx->iv)); - mv_cesa_ahash_init(req, &tmpl); + mv_cesa_ahash_init(req, &tmpl, true); return 0; } @@ -1288,7 +1289,7 @@ static int mv_cesa_ahmac_sha1_init(struct ahash_request *req) mv_cesa_set_op_cfg(&tmpl, CESA_SA_DESC_CFG_MACM_HMAC_SHA1); memcpy(tmpl.ctx.hash.iv, ctx->iv, sizeof(ctx->iv)); - mv_cesa_ahash_init(req, &tmpl); + mv_cesa_ahash_init(req, &tmpl, false); return 0; } @@ -1378,7 +1379,7 @@ static int mv_cesa_ahmac_sha256_init(struct ahash_request *req) mv_cesa_set_op_cfg(&tmpl, CESA_SA_DESC_CFG_MACM_HMAC_SHA256); memcpy(tmpl.ctx.hash.iv, ctx->iv, sizeof(ctx->iv)); - mv_cesa_ahash_init(req, &tmpl); + mv_cesa_ahash_init(req, &tmpl, false); return 0; }
Rather than determining whether we're using a MD5 hash by looking at the digest size, switch to a cleaner solution using a per-request flag initialised by the method type. Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk> --- drivers/crypto/marvell/cesa.h | 1 + drivers/crypto/marvell/hash.c | 17 +++++++++-------- 2 files changed, 10 insertions(+), 8 deletions(-)