Re: [PATCH 1/2] Initial support for Allwinner's Security ID fuses

[Oliver Schinagl]

On 17-06-13 13:51, Maxime Ripard wrote:
> On Mon, Jun 17, 2013 at 12:36:47PM +0200, Oliver Schinagl wrote:
> > On 15-06-13 12:28, Tomasz Figa wrote:
> > > > +#define DRV_VERSION "1.0"
> > >
> > > What is this version thingy?
> > >
> > > Is there a versioning scheme defined for this driver? Do you expect it to
> > > be changed every modification of this driver?
> > >
> > > I don't see any point of having such thing in a project with a version
> > > control system, where you have all change history.
> > Well we export something to userspace, while trivial there is the
> > possibility it changes over time. Say A40 which outputs 256 bits
> > instead of the current 128 bits. That would validate a bump in
> > version number. It's purely so the user can be aware of differences
> > in the driver. So maybe DRV_A[BP]I_VERSION would be better?
>
> Except that in that case, the userspace API won't change. You'll still
> call read on the same file in sysfs. The only thing that will change
> will be the number of bytes returned by your read function, which is
> (or should be) totally fine.
Aye, russel pointed this out as well, and I agree.
> > > > +/* We read the entire key, but only return the requested byte. This is
> > > > of + * course slower then it could be and uses 4 times more reads as
> > > > needed but + * keeps code simpler.
> > >
> > > I have no idea how often this is going to be read, but since the whole sid
> > > is really small (16 bytes), maybe it would be better to simply read the ID
> > > in probe to a buffer and then just memcpy from it in read().
> > The sid will be read once (well all 16 bytes) during probe and after
> > that only on user request. Right now we don't have a user (yet)
> > other then the sysfs entry.
> >
> > In future, this key can be used to read the MAC (low reads) or AES
> > keys for example (also low reads).
> >
> > Initially I had such an approach, but Maxime recommended against
> > having the value cached.
> >
> > As I wrote to andy, the only 'more efficient' way would be to store
> > the previously read key and see on the next read if its the same, So
> > best case, we could save 4 reads. I think it makes the code
> > unnecessarily complex for something that is read so little.
>
> I asked Oliver that because I felt like it could still be updated by the
> user, and sysfs should report that.
>
> And since it's not like it would be used extensively and very often,
> it's not a big deal anyway.
Actually it might still be possible to change the sid. We have figured 
out how we possible can program it (the 2.5 EFUSE_VDD pin, which olimex 
actually mapped out on the board) but requires someone to actually take 
the plunge and test it. So in theory, it can be changed still.
> > > > +
> > > > +	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 :)
>
> Yet, you don't have a single exit point neither, you have several of
> them. You can say that you have a single return statement in your code,
> which is true, yet you have several times a jump to this location, so we
> can definitely say that you actually have several exit points. Please
> also refer to the chapter 7 of the Documentation/CodingStyle file.
You are right of course :)
> > > > +	return (u8)sid_key;
> > >
> > > Unnecessary casting.
> > Unnecessary because of the &= 0xff above, or because you can put a
> > 32bit int in an 8bit int without worries? (we only want the last
> > byte).
>
> The latter. The & 0xff only filter out non-relevant informations from
> your 32-bits integer in that case, that's all, it doesn't do any
> casting.
So for clarity not leave the cast? Will it hurt? Will the compiler not 
do the cast anyway?
> > > > +	for (i = 0; i < size; i++) {
> > > > +		if ((pos + i) >= SID_SIZE || (pos < 0))
> > > > +			break;
> > > > +		buf[i] = sunxi_sid_read_byte(sid_reg_base, pos + i);
> > > > +	}
> > >
> > > This could be greatly simplified if you just read the whole sid to memory
> > > in probe and memcpy from it here.
> > But can't because we don't want to cache it.
>
> And we can't simply use memcpy, since we will need to do some endianness
> conversions. The data stored in the SID are big-endian, while we're
> running most likely in little endian mode.
>
> > > > +	if (!pdev->dev.of_node) {
> > > > +		dev_err(&pdev->dev, "No devicetree data available\n");
> > > > +		ret = -ENXIO;
> > > > +		goto exit;
> > > > +	}
> > >
> > > What is this check for? You don't seem to need anything from dev.of_node
> > > in this driver.
> > My understanding was, that when using the device tree, we check if
> > the device tree is atleast available. And we use
> > platform_get_resource, doesn't that get the data from the device
> > tree?
>
> If there's no device tree, you won't be probed in the first place. And
> resources get filled by the kernel from the device tree automatically at
> boot, so you're safe to assume that the resources are there when you get
> probed.
Ahh, assumption, the mother.
But I put my trust in you ;)

I'll remove it
> You need this check when you actually need some more informations from
> the DT, the value of an additional property for example.
I'll learn about that soon enough ;)
> Maxime
Thanks again maxime :)
>

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