Re: [PATCH 1/2] Initial support for Allwinner's Security ID fuses
From: Oliver Schinagl
Date: Mon Jun 17 2013 - 09:16:10 EST
On 17-06-13 14:51, Tomasz Figa wrote:
On Monday 17 of June 2013 12:36:47 Oliver Schinagl wrote:
On 15-06-13 12:28, Tomasz Figa wrote:
Hi,
Some comments inline.
Thank you
You're welcome. :)
On Saturday 15 of June 2013 01:16:20 Oliver Schinagl wrote:
From: Oliver Schinagl <oliver@xxxxxxxxxxx>
Allwinner has electric fuses (efuse) on their line of chips. This driver
reads those fuses, seeds the kernel entropy and exports them as a sysfs
node.
<snip>
I will change the comment, 'and 4 byte sized keys per SID' is probably
better
The array is 128 bits split into 32 bit words. Each 32 bit word consists
of 8 bits (1 byte).
So 4 * 4 = 16 bytes (SID_SIZE), is 128 bits.
What about:
/* There are 4 keys. */
#define SID_KEYS 4
/* Each key is 4 byte long (32-bit). */
#define SID_SIZE (SID_KEYS * 4)
I'll ommit the 'long (32-bit)' part but yeah that's probably enough.
<snip>
+
+ if (offset >= SID_SIZE)
+ goto exit;
return 0; ...
I did say in the changelog I opted for goto over return. But since
everybody keeps preferring returns (I personally like 'one single exit
point much more' I have already changed it all over to many returns, who
am I to argue :)
Well, single exit points makes sense (and is much nicer) when you have
something to do before exiting, take error paths as an example. But jumping
just to return makes no sense, because when reading the code you must scroll
down to the label to check what actually happens.
But functions shouldn't be so large :p But that is the first reasonable
reason I can live with :)
<snip>
+
+ for (i = 0; i < SID_SIZE; i++)
+ entropy[i] = sunxi_sid_read_byte(sid_reg_base, i);
You seem to read bytes into an array of ints. Your entropy data will
always have most significant 24-bits cleared. Is this behavior correct?
Yes, though I changed it so that entropy is an array of u8's, since
that's what sunxi_sid_read_byte returns.
+ add_device_randomness(entropy, SID_SIZE);
Now I'm pretty sure that above is not the correct behavior. You are adding
here first 16 bytes (=SID_SIZE) of entropy[], while it is an array of 16
ints (=4*SID_SIZE)...
Well technically, doesn't to compiler see that entropy is never larger
then 8 bits and thus uses only 8 bits? uint8_atleast or something. But
yeah, it's better to use the specified size to not waste 24 empty bits.
I mean, the loop fills the array with SID_SIZE ints, each with 3 zero bytes and
1 byte of actual data, so you get:
S0 0x00 0x00 0x00 S1 0x00 0x00 0x00 ... S15 0x00 0x00 0x00
Ok, I get that
but by calling add_device_randomness() with SID_SIZE as size argument, you add
only 16 first bytes of data from the array:
S0 0x00 0x00 0x00 S1 0x00 0x00 0x00 ... S3 0x00 0x00 0x00
That bit I'm not quite sure I understand:
We have an array of ints, { 0x00000000, 0x00000000, 0x00000000 .... }
We read 1 byte and copy it to the array (x16) (say sid = 0xdeadbeef...)
{ 0x000000de, 0x000000ad, 0x000000be, ... }
Now we pass this array to add_randomness(array, 16). So add_randomness
sees 16 ints in an array. So while there will be a lot of extra zero's,
there still be 16 elements copied/processed, no?
Otherwise, how does add_randomness() know it's dealing with bytes or
ints? it just see's the pointer to an int array that is 16 long? Or what
am I overlooking?
I did already change the array to be u8 big so it is only to help me
understand.
(little endianness assumed)
Best regards,
Tomasz
(for rest of comments I think it's enough said in Russell's and Maxime's
replies)
Yes it has :) thanks to all of you
Oliver
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/