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

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

On Mon, Nov 26, 2018 at 10:01:40AM +0100, Sascha Hauer wrote:
> On Sun, Nov 25, 2018 at 11:59:51PM +0100, Roland Hieber wrote:
[...]
> > +	if (caam_era < 8 && rngvid == 4 && rngrev < 3) {
> > +		rng_st_dsc = rng_dsc1;
> > +		desc_size = DSC1SIZE;
> > +		exp_result = rng_result1;
> > +	} else if (rngvid != 3) {
> > +		rng_st_dsc = rng_dsc2;
> > +		desc_size = DSC2SIZE;
> > +		exp_result = 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;
> 
> Is this test really correct? Basically it says "We accept everything
> except rngvid == 3".

I asked back to Utkarsh Gupta at NXP, the original author of this
self-test. His reply was:

    (If condition) Legacy i.MX chipsets which are effected with this
    issue contain CAAM version below 8 and have RNG VID=4 and RNG
    REV <3.

    (else if condition) Current i.MX chipsets which are effected with
    this issue contain CAAM version greater than or equal to 8 and have
    RNG4.3 or above.

and he also agreed that the code could be more clear here.
If you do the math, the else-if clause only fires if

        !(caam_era < 8 && rngvid == 4 && rngrev < 3) && (rngvid != 3)
<=> [using De Morgan's laws]
        (caam_era >= 8 || rngvid != 4 || rngrev >= 3) && (rngvid != 3)

Note that (rngvid != 3) is always true because rngvid can only be 2 or
4. But he also mentions that the patch was meant to be run only on the
affected chips anyway, which rules out the cases rngvid == 2:

	(caam_era >= 8 || false || rngrev >= 3)
<=> 	(caam_era >= 8 || rngrev >= 3)

which is kind of what his formulation in natural language says.

I will use a clearer else-if condition instead in v2.

[...]
> > diff --git a/drivers/hab/habv4.c b/drivers/hab/habv4.c
> > index 28fd42ecd7..e64f34870e 100644
> > --- a/drivers/hab/habv4.c
> > +++ b/drivers/hab/habv4.c
> > @@ -387,6 +387,27 @@ static void habv4_display_event(uint8_t *data, uint32_t len)
> >  	habv4_display_event_record((struct hab_event_record *)data);
> >  }
> >  
> > +/* Some chips with HAB >= 4.2.3 have an incorrect implementation of the RNG
> > + * self-test in ROM code. In this case, an HAB event is generated, and a
> > + * software self-test should be run. This variable is set to @c true by
> > + * habv4_get_status() when this occurs. */
> > +bool habv4_need_rng_software_self_test = false;
> > +EXPORT_SYMBOL(habv4_need_rng_software_self_test);
> > +
> > +#define RNG_FAIL_EVENT_SIZE 36
> 
> ARRAY_SIZE is your friend.
> 
> > +static uint8_t habv4_known_rng_fail_events[][RNG_FAIL_EVENT_SIZE] = {
> > +	{ 0xdb, 0x00, 0x24, 0x42,  0x69, 0x30, 0xe1, 0x1d,
> > +	  0x00, 0x80, 0x00, 0x02,  0x40, 0x00, 0x36, 0x06,
> > +	  0x55, 0x55, 0x00, 0x03,  0x00, 0x00, 0x00, 0x00,
> > +	  0x00, 0x00, 0x00, 0x00,  0x00, 0x00, 0x00, 0x00,
> > +	  0x00, 0x00, 0x00, 0x01 },
> > +	{ 0xdb, 0x00, 0x24, 0x42,  0x69, 0x30, 0xe1, 0x1d,
> > +	  0x00, 0x04, 0x00, 0x02,  0x40, 0x00, 0x36, 0x06,
> > +	  0x55, 0x55, 0x00, 0x03,  0x00, 0x00, 0x00, 0x00,
> > +	  0x00, 0x00, 0x00, 0x00,  0x00, 0x00, 0x00, 0x00,
> > +	  0x00, 0x00, 0x00, 0x01 },
> > +};

No, ARRAY_SIZE won't work here:

drivers/hab/habv4.c:399:16: error: array type has incomplete element type 'uint8_t[] {aka unsigned char[]}'
 static uint8_t habv4_known_rng_fail_events[][] = {
                ^~~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/hab/habv4.c:399:16: note: declaration of 'habv4_known_rng_fail_events' as multidimensional array must have bounds for all dimensions except the first
 
 - Roland

-- 
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