Re: [PATCH v4 1/1] crypto: caam/rng: Add support for PRNG

["Horia Geantă" <horia.geanta@nxp.com>]

On 4/29/2022 11:45 AM, Meenakshi Aggarwal wrote:
> From: Meenakshi Aggarwal <meenakshi.aggarwal@nxp.com>
> 
> Add support for random number generation using PRNG
> mode of CAAM and expose the interface through crypto API.
> 
> According to the RM, the HW implementation of the DRBG follows
> NIST SP 800-90A specification for DRBG_Hash SHA-256 function
> 
> Signed-off-by: Meenakshi Aggarwal <meenakshi.aggarwal@nxp.com>

There are a few nitpicks below, if you can address them great,
either way here's my
Reviewed-by: Horia Geantă <horia.geanta@nxp.com>

> + * Length of used descriptors, see caam_init_desc()
caam_init_desc() does not exist in this file.

> + */
> +#define CAAM_PRNG_DESC_LEN (CAAM_CMD_SZ +				\
> +			    CAAM_CMD_SZ +				\
> +			    CAAM_CMD_SZ + CAAM_PTR_SZ_MAX)
The reseed descriptor length is now smaller.
Memmory allocated by kmalloc() will probably be the same,
but code would be more readable.
If you want to use the same length for both descriptors,
I suggest renaming the define to CAAM_PRNG_MAX_DESC_LEN.

> +static u32 *caam_init_reseed_desc(u32 *desc)
> +{
> +	init_job_desc(desc, 0);	/* + 1 cmd_sz */
> +	/* Generate random bytes: + 1 cmd_sz */
This is a "reseed" operation, not "generate".

> +	append_operation(desc, OP_TYPE_CLASS1_ALG | OP_ALG_ALGSEL_RNG |
> +			OP_ALG_AS_FINALIZE);
> +
[...]
> +static int caam_prng_generate(struct crypto_rng *tfm,
> +			     const u8 *src, unsigned int slen,
> +			     u8 *dst, unsigned int dlen)
> +{
[...]
> +	ret = caam_jr_enqueue(jrdev,
> +			      caam_init_prng_desc(desc, dst_dma, dlen),
> +			      caam_prng_done, &ctx);
> +
> +	if (ret == -EINPROGRESS) {
> +		wait_for_completion(&ctx.done);
> +		ret = ctx.err;
> +	}
> +
> +	dma_unmap_single(jrdev, dst_dma, dlen, DMA_FROM_DEVICE);
> +
> +	memcpy(dst, buf, dlen);
memcpy() should be performed only when the HW executed the operation
successfully.

Thanks,
Horia