Re: [PATCH v2 1/2] drivers: caam: add RNG software self-test

[Roland Hieber <r.hieber@pengutronix.de>]

Hi Sascha,

On Thu, Nov 29, 2018 at 02:42:07PM +0100, Roland Hieber wrote:
> +/*
> + * caam_rng_self_test() - Perform RNG self test
> + * Returns zero on success, and negative on error.
> + */
> +int caam_rng_self_test(struct device_d *dev, const u8 caam_era, const u8 rngvid, const u8 rngrev)
> +{
> +	int ret, i, desc_size = 0, result_size = 0, job_err = 0;
> +	const u32 *rng_st_dsc;
> +	const u8 *exp_result;
> +	u32 *desc;
> +	u8 *result;
> +
> +	pr_debug("got CAAM ERA %d, RNG Version ID %d, RNG revision %d\n", caam_era, rngvid, rngrev);
> +
> +	if (caam_era < 8 && rngvid == 4 && rngrev < 3) {
> +		// older affected i.MX chipsets have CAAM < 8 and have RNG4 < 4.3
> +		rng_st_dsc = rng_dsc1;
> +		desc_size = ARRAY_SIZE(rng_dsc1);
> +		exp_result = rng_result1;
> +		result_size = ARRAY_SIZE(rng_result1);
> +	} else if (caam_era >= 8 || (rngvid >= 4 && rngrev >= 3)) {
> +		// newer affected chipsets have CAAM >= 8 or RNG4 >= 4.3
> +		rng_st_dsc = rng_dsc2;
> +		desc_size = ARRAY_SIZE(rng_dsc2);
> +		exp_result = rng_result2;
> +		result_size = ARRAY_SIZE(rng_result2);
> +	} else {
> +		pr_err("Error: Invalid CAAM ERA (%d) or RNG Version ID (%d) or RNG revision (%d)\n",
> +				caam_era, rngvid, rngrev);
> +		return -EINVAL;
> +	}
> +
> +	result = dma_alloc(sizeof(*result) * result_size);
> +	if (!result) {
> +		return -ENOMEM;
> +	}
> +
> +	desc = dma_alloc(sizeof(*desc) * desc_size);
> +	if (!desc) {
> +		return -ENOMEM;
> +	}
> +
> +	construct_rng_self_test_jobdesc(desc, rng_st_dsc, result, desc_size);
> +
> +	dma_sync_single_for_device((unsigned long)desc,
> +			desc_size * sizeof(*desc), DMA_TO_DEVICE);
> +	dma_sync_single_for_device((unsigned long)result,
> +			result_size * sizeof(*result), DMA_FROM_DEVICE);
> +
> +	/* wait for job completion */
> +	ret = caam_jr_enqueue(dev, desc, rng_self_test_done, &job_err);
> +	if (ret) {
> +		pr_err("Error while running RNG self-test descriptor: %d %s\n",
> +				ret, strerror(ret));
> +		goto err;
> +	}
> +	if (job_err) {
> +		pr_err("Job Error:\n");
> +		caam_jr_strstatus(dev, job_err);
> +		goto err;
> +	}
> +
> +	dma_sync_single_for_cpu((unsigned long)result, result_size * sizeof(*result),
> +			DMA_FROM_DEVICE);
> +
> +	pr_vdebug("Result\n");
> +	for (i = 0; i < result_size; i++) {
> +		pr_vdebug("%02X\n", result[i]);
> +	}
> +
> +	pr_vdebug("Expected Result:\n");
> +	for (i = 0; i < result_size; i++) {
> +		pr_vdebug("%02X\n", exp_result[i]);
> +	}
> +
> +	if (memcmp(result, exp_result, sizeof(*result) * result_size) != 0) {
> +		pr_err("RNG self-test failed with unexpected result\n");
> +		ret = -ERANGE;
> +		goto err;
> +	}
> +
> +	pr_notice("RNG software self-test passed\n");

I noticed that you downgraded this to a pr_info() when applying the
patch, is there a reason?  pr_warn()s have the "WARNING:" prefix,
pr_notice()s have a "NOTICE:" prefix, but pr_info()s don't have any
prefix. I think this one should be a pr_notice because it overrides the
pr_warn("RNG self-test failure detected...") from HAB and tells the user
that this warning is no longer relevant. With pr_info(), I think it will
get lost between all the other infos.

 - Roland

> +	ret = 0;
> +
> +err:
> +	dma_free(desc);
> +	dma_free(result);
> +	return ret;
> +}
> diff --git a/drivers/crypto/caam/rng_self_test.h b/drivers/crypto/caam/rng_self_test.h
[...]

-- 
Roland Hieber                     | r.hieber@pengutronix.de     |
Pengutronix e.K.                  | https://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim | Phone: +49-5121-206917-5086 |
Amtsgericht Hildesheim, HRA 2686  | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox