drivers: crypto: hisilicon: add DH algorithm #6781
Conversation
| size_t key_size) | ||
| { | ||
| struct hpre_dh_msg msg = { }; | ||
| TEE_Result ret; |
There was a problem hiding this comment.
Initialize these two
| #define HPRE_DH_TOTAL_BUF_SIZE(key_bytes) ((key_bytes) * 4) | ||
| struct hpre_dh_msg { | ||
| uint8_t *x_p; /* X and p data in order */ | ||
| uintptr_t x_p_dma; |
There was a problem hiding this comment.
If this is an integer representing a virtual address, please use vaddr_t. If it's a physical address, use paddr_t. Same below.
| #include "hpre_main.h" | ||
|
|
||
| #include <initcall.h> | ||
| #include <hpre_main.h> |
There was a problem hiding this comment.
swap the 2 lines (alphabetical order).
It ould be nice this change comes in a dedicated commit (e.g. drivers: crypto: hisilicon: add missing header file inclusions).
There was a problem hiding this comment.
OK.I will seperate a dedicated commit after other comments solved.
| #define __TEE_HPRE_DH_H__ | ||
|
|
||
| #include <drvcrypt.h> | ||
| #include <drvcrypt_acipher.h> |
There was a problem hiding this comment.
include also types_ext.h (paddr_t), stdint.h (uintX_t) and tee_api_types.h (TEE_result).
There was a problem hiding this comment.
I think these head files has been already included, such as tee_api_types included in drvcrypt_acipher.h
There was a problem hiding this comment.
Strictly speaking, this header file requires uintX_t and paddr_t types for the structure it defines (struct hpre_dh_msg), not resources defined in other header files. Therefore, this header file should include the required header files. From a overall view, if we (hypothetically and quite unlikely :) modify drvcrypt.h, we should not have to modify this header (hpre_dh.h) to add header file inclusions it lacks.
Note that this header file does not need drvcrypt.h and drvcrypt_acipher.h at all.
I'm sorry such comments look annoying, I think they help your implementation to be easier to maintain in the long term.
There was a problem hiding this comment.
Thank you for your comments to make the code better！I will fix this.
yuzexiyzx
force-pushed
the
master
branch
4 times, most recently
from
1997c1c
to
fd3732f
Compare
|
|
||
| #include <hpre_dh.h> | ||
| #include <hpre_main.h> | ||
| #include <rng_support.h> |
There was a problem hiding this comment.
I would be better that source file include the header files they need. I doubt hisi_qm.h needs string.h (or malloc.h etc...), it is rather the source files that include hisi_qm.h that need string.h. Header files should not be used to include all potential header files source file need or not need. But that's only my opinion.
| #define __TEE_HPRE_DH_H__ | ||
|
|
||
| #include <drvcrypt.h> | ||
| #include <drvcrypt_acipher.h> |
There was a problem hiding this comment.
Strictly speaking, this header file requires uintX_t and paddr_t types for the structure it defines (struct hpre_dh_msg), not resources defined in other header files. Therefore, this header file should include the required header files. From a overall view, if we (hypothetically and quite unlikely :) modify drvcrypt.h, we should not have to modify this header (hpre_dh.h) to add header file inclusions it lacks.
Note that this header file does not need drvcrypt.h and drvcrypt_acipher.h at all.
I'm sorry such comments look annoying, I think they help your implementation to be easier to maintain in the long term.
yuzexiyzx
force-pushed
the
master
branch
3 times, most recently
from
ef35954
to
78db31d
Compare
|
@jenswi-linaro @etienne-lms All comments have been solved. |
|
|
||
| #include <hpre_dh.h> | ||
| #include <hpre_main.h> | ||
| #include <rng_support.h> |
There was a problem hiding this comment.
#include <malloc.h>
#include <string.h>
#include <string_ext.h>
#include <trace.h>| @@ -0,0 +1,33 @@ | |||
| /* SPDX-License-Identifier: BSD-2-Clause */ | |||
| /* | |||
| * Copyright (c) 2022, HiSilicon Technologies Co., Ltd. | |||
There was a problem hiding this comment.
drvcrypt.h and initcall.h is needed in dh.c. I think these head files is already included, such as trace.h is included in drvcrypt.h
There was a problem hiding this comment.
The general rule is to include all the header files you are using. For instance if you use traces then you include <trace.h> even if it is included by another header. The reason is because relying on indirect inclusions can break if the header files change.
yuzexiyzx
force-pushed
the
master
branch
3 times, most recently
from
25405e9
to
94059cb
Compare
|
|
add operation of DH algorithm, including alloc_keypair, gen_keypair and shared_secret Signed-off-by: yuzexi <yuzexi@hisilicon.com> Acked-by: Etienne Carriere <etienne.carriere@foss.st.com> Acked-by: Jens Wiklander <jens.wiklander@linaro.org>
|
@jforissier All comments have been solved. |
add operation of DH algorithm, including alloc_keypair, gen_keypair and shared_secret