Re: [PATCH 03/18] crypto: marvell: add flag to determine algorithm endianness

[Jason Cooper]

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 
> ---
>  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(, CESA_SA_DESC_CFG_MACM_MD5);
>  
> - mv_cesa_ahash_init(req, );
> + mv_cesa_ahash_init(req, , true);

mv_cesa_ahash_init(req, , 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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 03/18] crypto: marvell: add flag to determine algorithm endianness

[Russell King - ARM Linux]

On Mon, Oct 19, 2015 at 03:04:51PM +, 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(, CESA_SA_DESC_CFG_MACM_MD5);
> >  
> > -   mv_cesa_ahash_init(req, );
> > +   mv_cesa_ahash_init(req, , true);
> 
>   mv_cesa_ahash_init(req, , 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.

-- 
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 03/18] crypto: marvell: add flag to determine algorithm endianness

[Herbert Xu]

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(, CESA_SA_DESC_CFG_MACM_MD5);
> > >  
> > > - mv_cesa_ahash_init(req, );
> > > + mv_cesa_ahash_init(req, , true);
> > 
> > mv_cesa_ahash_init(req, , 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,
-- 
Email: Herbert Xu 
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 03/18] crypto: marvell: add flag to determine algorithm endianness

[Jason Cooper]

On Mon, Oct 19, 2015 at 04:25:07PM +0100, Russell King - ARM Linux wrote:
> On Mon, Oct 19, 2015 at 03:04:51PM +, 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(, CESA_SA_DESC_CFG_MACM_MD5);
> > >  
> > > - mv_cesa_ahash_init(req, );
> > > + mv_cesa_ahash_init(req, , true);
> > 
> > mv_cesa_ahash_init(req, , 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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html