Re: [PATCH] crypto: atmel-sha204a - drop hwrng quality reduction for ATSHA204A

["Ard Biesheuvel" <ardb@kernel.org>]

Hi Thorsten,

On Mon, 27 Apr 2026, at 14:40, Thorsten Blum wrote:
> Commit 8006aff15516 ("crypto: atmel-sha204a - Set hwrng quality to
> lowest possible") reduced the hwrng quality to 1 based on a review by
> Bill Cox [1]. However, despite its title, the review only tested the
> ATSHA204, not the ATSHA204A.
>
> In the same thread, Atmel engineer Landon Cox wrote "this behavior has
> been eliminated entirely"[2] in the ATSHA204A and "this problem does not
> affect the ATECC108 or the ATECC108A (or the ATSHA204A)"[3].
>
> According to the official ATSHA204A datasheet [4], the device contains a
> high-quality hardware RNG that combines its output with an internal seed
> value stored in EEPROM or SRAM to generate random numbers. The device
> also implements all security functions using SHA-256, and the driver
> uses the chip's Random command in seed-update mode.
>
> Keep 'quality = 1' for ATSHA204, but drop the explicit hwrng quality
> reduction for ATSHA204A and fall back to the hwrng core default.
>

Interesting! Thanks for digging this up.

> [1] 
> https://www.metzdowd.com/pipermail/cryptography/2014-December/023858.html
> [2] 
> https://www.metzdowd.com/pipermail/cryptography/2014-December/023852.html
> [3] 
> https://www.metzdowd.com/pipermail/cryptography/2014-December/023886.html
> [4] 
> https://ww1.microchip.com/downloads/en/DeviceDoc/ATSHA204A-Data-Sheet-40002025A.pdf
>
> Fixes: 8006aff15516 ("crypto: atmel-sha204a - Set hwrng quality to 
> lowest possible")
> Cc: stable@vger.kernel.org
> Signed-off-by: Thorsten Blum <thorsten.blum@linux.dev>
> ---
>  drivers/crypto/atmel-sha204a.c | 40 ++++++++++++++++++----------------
>  1 file changed, 21 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/crypto/atmel-sha204a.c b/drivers/crypto/atmel-sha204a.c
> index dbb39ed0cea1..df69fb190e52 100644
> --- a/drivers/crypto/atmel-sha204a.c
> +++ b/drivers/crypto/atmel-sha204a.c
> @@ -19,6 +19,25 @@
>  #include <linux/workqueue.h>
>  #include "atmel-i2c.h"
> 
> +enum atmel_sha204a_variant {
> +	ATSHA204 = 1,
> +	ATSHA204A,
> +};
> +

I agree that setting quality to '1' is only appropriate for the ATSHA204
but this looks a bit clunky to me.

Can we retain the comment here that you deleted below, and add
something like

static const unsigned short atsha204_quality = 1;

> +static const struct of_device_id atmel_sha204a_dt_ids[] __maybe_unused = {
> +	{ .compatible = "atmel,atsha204",  .data = (void *)ATSHA204 },
> +	{ .compatible = "atmel,atsha204a", .data = (void *)ATSHA204A },
> +	{ /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, atmel_sha204a_dt_ids);
> +
> +static const struct i2c_device_id atmel_sha204a_id[] = {
> +	{ .name = "atsha204",  .driver_data = ATSHA204 },
> +	{ .name = "atsha204a", .driver_data = ATSHA204A },
> +	{ /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(i2c, atmel_sha204a_id);
> +

Then, move these back to the old location, and point .data /
.driver_data to &atsha204_quality for atsha204 only.


>  static void atmel_sha204a_rng_done(struct atmel_i2c_work_data *work_data,
>  				   void *areq, int status)
>  {
> @@ -171,11 +190,8 @@ static int atmel_sha204a_probe(struct i2c_client *client)
>  	i2c_priv->hwrng.name = dev_name(&client->dev);
>  	i2c_priv->hwrng.read = atmel_sha204a_rng_read;
> 
> -	/*
> -	 * According to review by Bill Cox [1], this HWRNG has very low 
> entropy.
> -	 * [1] 
> https://www.metzdowd.com/pipermail/cryptography/2014-December/023858.html
> -	 */
> -	i2c_priv->hwrng.quality = 1;
> +	if ((uintptr_t)i2c_get_match_data(client) == ATSHA204)
> +		i2c_priv->hwrng.quality = 1;
> 

Here you can override the field by dereferencing the match data if it
is non-NULL.

Alternatively, you could store the quality in the device_id structs
directly, but I think this is slightly more idiomatic.


>  	ret = devm_hwrng_register(&client->dev, &i2c_priv->hwrng);
>  	if (ret)
> @@ -202,20 +218,6 @@ static void atmel_sha204a_remove(struct i2c_client *client)
>  	kfree((void *)i2c_priv->hwrng.priv);
>  }
> 
> -static const struct of_device_id atmel_sha204a_dt_ids[] __maybe_unused = {
> -	{ .compatible = "atmel,atsha204", },
> -	{ .compatible = "atmel,atsha204a", },
> -	{ /* sentinel */ }
> -};
> -MODULE_DEVICE_TABLE(of, atmel_sha204a_dt_ids);
> -
> -static const struct i2c_device_id atmel_sha204a_id[] = {
> -	{ "atsha204" },
> -	{ "atsha204a" },
> -	{ /* sentinel */ }
> -};
> -MODULE_DEVICE_TABLE(i2c, atmel_sha204a_id);
> -
>  static struct i2c_driver atmel_sha204a_driver = {
>  	.probe			= atmel_sha204a_probe,
>  	.remove			= atmel_sha204a_remove,