Re: [PATCH 2/3] crypto: inside-secure - added support for rfc3686(ctr(aes))

[Antoine Tenart <antoine.tenart@bootlin.com>]

On Fri, Jul 26, 2019 at 02:29:48PM +0000, Pascal Van Leeuwen wrote:
> > From: linux-crypto-owner@vger.kernel.org <linux-crypto-owner@vger.kernel.org> On Behalf Of Antoine Tenart
> > On Fri, Jul 26, 2019 at 01:28:13PM +0000, Pascal Van Leeuwen wrote:
> > > > On Fri, Jul 05, 2019 at 08:49:23AM +0200, Pascal van Leeuwen wrote:
> > >
> > > > > @@ -62,9 +63,9 @@ static void safexcel_skcipher_token(struct safexcel_cipher_ctx *ctx, u8 *iv,
> > > > >  				    u32 length)
> > > > > -	if (ctx->mode == CONTEXT_CONTROL_CRYPTO_MODE_CBC) {
> > > > > +	if (ctx->mode != CONTEXT_CONTROL_CRYPTO_MODE_ECB) {
> > > >
> > > > I think it's better for maintenance and readability to have something
> > > > like:
> > > >
> > > >   if (ctx->mode == CONTEXT_CONTROL_CRYPTO_MODE_CBC ||
> > > >       ctx->mode == CONTEXT_CONTROL_CRYPTO_MODE_CTR_LOAD)
> > > >
> > > Not really. I *really* want to execute that for any mode other than ECB,
> > > ECB being the *only* mode that does not require an IV (which I know
> > > for a fact, being the architect and all :-).
> > > And I don't believe a long list of modes that *do* require an IV would
> > > be  more readable or easy to maintain than this single compare ...
> > 
> > That's where I disagree as you need extra knowledge to be aware of this.
> > Being explicit removes any possible question one may ask. But that's a
> > small point really :)
> > 
> Well, while we're disagreeing ... I disagree with your assertion that you
> would need more knowledge to know which modes do NOT need an IV
> than to know which modes DO need an IV. There's really no fundamental
> difference, it's two sides of the exact same coin ... you need that
> knowledge either way. 

The point is if you look for occurrences of, let's say
CONTEXT_CONTROL_CRYPTO_MODE_CBC, to see the code path it'll be way
easier if you have direct comparisons.

> 1) This code is executed for each individual cipher call, i.e. it's in the
> critical path. Having just 1 compare-and-branch there is better for
> performance than having many.

Not sure about what the impact really is.

> 2) Generally, all else being equal, having less code is easier to maintain
> than having more code. 

That really depends, having readable code is easier to maintain :)

> 4) If there is anything unclear about an otherwise fine code construct,
> then you clarify it by adding a comment, not by rewriting it to be inefficient
> and redundant ;-)

Fair point.

Thanks,
Antoine

-- 
Antoine Ténart, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com