mail archive of the barebox mailing list
 help / color / mirror / Atom feed
From: Roland Hieber <r.hieber@pengutronix.de>
To: Sascha Hauer <s.hauer@pengutronix.de>
Cc: barebox@lists.infradead.org
Subject: Re: [PATCH 1/2] drivers: caam: add RNG software self-test
Date: Thu, 29 Nov 2018 11:17:25 +0100	[thread overview]
Message-ID: <20181127160654.7emsivocuj3hulq7@pengutronix.de> (raw)
In-Reply-To: <20181126090140.k443kixpzmcxhjim@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

      parent reply	other threads:[~2018-11-29 10:17 UTC|newest]

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-11-25 22:59 Roland Hieber
2018-11-25 22:59 ` [PATCH 2/2] i.MX: HABv4: always print HAB status at boot time Roland Hieber
2018-11-26  7:56   ` Denis OSTERLAND
2018-12-03 10:17   ` Denis OSTERLAND
2018-11-26  9:01 ` [PATCH 1/2] drivers: caam: add RNG software self-test Sascha Hauer
2018-11-26 10:53   ` Roland Hieber
2018-11-29 14:11     ` Roland Hieber
2018-12-03  7:45       ` Sascha Hauer
2018-12-03  9:41         ` Roland Hieber
2018-11-29 10:17   ` Roland Hieber [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20181127160654.7emsivocuj3hulq7@pengutronix.de \
    --to=r.hieber@pengutronix.de \
    --cc=barebox@lists.infradead.org \
    --cc=s.hauer@pengutronix.de \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox