Re: [PATCH v3] crypto: s5p-sss: Add HASH support for Exynos

[Kamil Konieczny]

On 03.10.2017 21:30, Krzysztof Kozlowski wrote:
> On Tue, Oct 03, 2017 at 04:57:43PM +0200, Kamil Konieczny wrote:
>  
>>>> [...]
>>>> +static struct ahash_alg algs_sha256[] = {
>>>> +{
>>>> +	.init		= s5p_hash_init,
>>>> +	.update		= s5p_hash_update,
>>>> +	.final		= s5p_hash_final,
>>>> +	.finup		= s5p_hash_finup,
>>>> +	.digest		= s5p_hash_digest,
>>>> +	.halg.digestsize	= SHA256_DIGEST_SIZE,
>>>> +	.halg.base	= {
>>>> +		.cra_name		= "sha256",
>>>> +		.cra_driver_name	= "exynos-sha256",
>>>> +		.cra_priority		= 100,
>>>> +		.cra_flags		= CRYPTO_ALG_TYPE_AHASH |
>>>> +					  CRYPTO_ALG_KERN_DRIVER_ONLY |
>>>> +					  CRYPTO_ALG_ASYNC |
>>>> +					  CRYPTO_ALG_NEED_FALLBACK,
>>>> +		.cra_blocksize		= HASH_BLOCK_SIZE,
>>>> +		.cra_ctxsize		= sizeof(struct s5p_hash_ctx),
>>>> +		.cra_alignmask		= SSS_DMA_ALIGN_MASK,
>>>> +		.cra_module		= THIS_MODULE,
>>>> +		.cra_init		= s5p_hash_cra_init,
>>>> +		.cra_exit		= s5p_hash_cra_exit,
>>>> +	}
>>>> +}
>>>> +
>>>> +};
>>>> +
>>>> +static struct sss_hash_algs_info exynos_hash_algs_info[] = {
>>>
>>> You have warnings in your code. Please be sure that all compiler,
>>> Smatch, Sparse, checkpatch and coccicheck warnings are fixed.
>>>
>>> ../drivers/crypto/s5p-sss.c:1896:34: warning: âexynos_hash_algs_infoâ defined but not used [-Wunused-variable]
>>>  static struct sss_hash_algs_info exynos_hash_algs_info[] = {
>>>
>>> Probably this should be __maybe_unused.
>>
>> You are right, I did not check this with EXYNOS_HASH disabled, I will
>> rewrite it.
>>
>>> Also this should be const. I do not understand why you have to add one
>>> more static variable (which sticks the driver to only one instance...)
>>> and then modify it during runtime. Everything should be stored in device
>>> state container (s5p_aes_dev) - directly or through some other pointers.
>>
>> There is .registered field which is incremented with each algo register.
>> I can move assignes to fields .import, .export and .statesize into struct.
>> When I tried add const, I got compiler warn:
>> drivers/crypto/s5p-sss.c: In function âs5p_aes_removeâ:
>> drivers/crypto/s5p-sss.c:2397:6: warning: passing argument 1 of âcrypto_unregister_ahashâ discards âconstâ qualifier from pointer target type [-Wdiscarded-qualifiers]
>>       &hash_algs_i[i].algs_list[j]);
>> so it was not designed to be const (in crypto framework).
>> In AES code the alg struct is also static:
>> static struct crypto_alg algs[] = {
> 
> The crypto_alg and ahash_alg must indeed stay non-const but
> sss_hash_algs_info is different. You do not pass it to crypto-core.

It was again from omap driver, which has descriptions for omap2, omap4 and omap5,
but here I over-complicated this. I will just remove it and simplify register
and deregister hash algs.

> 
>> What you mean by 'stick the driver to only one instance' ? In Exynos 4412 there
>> is only one SecSS block, in some other Exynos there is SlimSS, but it is not
>> the same (it has lower capabilities and other io addresses), so there should not
>> be two s5p_aes_dev drivers loaded at the same time. 
> 
> Current driver matches hardware in one-to-one so indeed there cannot be
> two s5p_aes_dev devices. However this might change thus almost every
> driver tries to follow the pattern of state-container passed to device
> (e.g. platform_set_drvdata()). With this approach the code is nicely
> encapsulated and usually much easier to review. Globals (or file-scope
> variables) usually makes code more difficult to maintain.
> 
> In this driver this is not entirely possible as some crypto-functions do
> not allow passing driver-supplied opaque pointer. But except this case,
> everywhere else the driver should follow common convention - do not use
> static variables.

-- 
Best regards,
Kamil Konieczny
Samsung R&D Institute Poland