Re: [PATCH v7 2/2] crypto: s5p-sss: Add HASH support for Exynos

[Vladimir Zapolskiy]

Hi Kamil,

I'll just answer to your question, all the comments from you are accepted,
please send a new version with the minor fixes, hopefully the change will
be included into v4.15-rc.

On 10/24/2017 02:27 PM, Kamil Konieczny wrote:
> Hi Vladimir,
> 
> Thank you for review, I will apply almost all of your remarks,
> see answers below.
> 
> On 22.10.2017 12:18, Vladimir Zapolskiy wrote:
>> Hi Kamil,
>>
>> thank you for updates, I have just a few more comments.
>>

[snip]

>>> +/**
>>> + * s5p_hash_import - import hash state
>>> + * @req:	AHASH request
>>> + * @in:		buffer with state to be imported from
>>> + */
>>> +static int s5p_hash_import(struct ahash_request *req, const void *in)
>>> +{
>>> +	struct s5p_hash_reqctx *ctx = ahash_request_ctx(req);
>>> +	struct crypto_ahash *tfm = crypto_ahash_reqtfm(req);
>>> +	struct s5p_hash_ctx *tctx = crypto_ahash_ctx(tfm);
>>> +	const struct s5p_hash_reqctx *ctx_in = in;
>>> +
>>> +	memcpy(ctx, in, sizeof(*ctx) + BUFLEN);
>>> +	if (ctx_in->bufcnt < 0 || ctx_in->bufcnt > BUFLEN) {
>>
>> Now "ctx_in->bufcnt < 0" condition can be removed, moreover it
>> will eliminate a warning reproted by the compiler:
>>
>> drivers/crypto/s5p-sss.c:1748:21: warning: comparison of unsigned expression < 0 is always false [-Wtype-limits]
>>   if (ctx_in->bufcnt < 0 || ctx_in->bufcnt > BUFLEN) {
>>                      ^
> 
> You are right, I will drop first condition. BTW what compiler options are you using ?
> This one was not reported by my compilation process.
> 

Regularly I specify 'make C=1 W=1' options to build a kernel with changes,
some of the new reported warnings are false positives, but in general it
makes sense to check the output to catch potential issues like this one.

--
With best wishes,
Vladimir