Message-ID: <51BC353D.5010001@schinagl.nl>
Date:	Sat, 15 Jun 2013 11:34:53 +0200
From:	Oliver Schinagl <oliver+list@...inagl.nl>
To:	Andy Shevchenko <andy.shevchenko@...il.com>
CC:	Arnd Bergmann <arnd@...db.de>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	"maxime.ripard" <maxime.ripard@...e-electrons.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	linux-arm Mailing List <linux-arm-kernel@...ts.infradead.org>,
	Russell King <linux@....linux.org.uk>,
	Linus Walleij <linus.walleij@...aro.org>,
	Oliver Schinagl <oliver@...inagl.nl>
Subject: Re: [PATCH 1/2] Initial support for Allwinner's Security ID fuses

On 06/15/13 04:14, Andy Shevchenko wrote:
> On Sat, Jun 15, 2013 at 2:16 AM, Oliver Schinagl
> <oliver+list@...inagl.nl> wrote:
>> From: Oliver Schinagl <oliver@...inagl.nl>
>>
>> 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.
>>
>> These fuses are most likly to be programmed at the factory, encoding
>> things like Chip ID, some sort of serial number etc and appear to be
>> reasonable unique.
>> While in theory, these should be writeable by the user, it will probably
>> be inconvinient to do so. Allwinner recommends that a certain input pin,
>> labeled 'efuse_vddq', be connected to GND. To write these fuses, 2.5 V
>> needs to be applied to this pin.
>>
>> Even so, they can still be used to generate a board-unique mac from, board
>> unique RSA key and seed the kernel RNG.
>>
>> Currently supported are the following known chips:
>> Allwinner sun4i (A10)
>> Allwinner sun5i (A10s, A13)
>
> Few comments below.
>
>> +++ b/drivers/misc/eeprom/sunxi_sid.c
>
>> +#include <linux/compiler.h>
>
> Are you sure this has to be explicitly mentioned?
Well we use __iomem from it, so I think yes
>
>> +#define SID_SIZE (SID_KEYS * 4)
>> +
>> +
>
> Extra line.
to seperate defines/includes from the code?
>
>> +/* 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.
>
> May be better to rewrite this logic and save CPU and I/O resources?
Well we can only read 32 bits at a time and we only want to return 8 
bits. The only think I can think of, is read 32 bits and store those 
statically to the function and store with an extra int the location. 
Then check against the location and if it's the same use the int. Not 
sure all that is worth it.
>
>> + */
>> +static u8 sunxi_sid_read_byte(const void __iomem *sid_reg_base,
>> +                             const unsigned int offset)
>> +{
>> +       u32 sid_key = 0;
>> +
>> +       if (offset >= SID_SIZE)
>> +               goto exit;
>
> Just return here.
Well as said before, return vs goto; i choose goto ;)
>
>> +       sid_key = ioread32be(sid_reg_base + round_down(offset, 4));
>> +       sid_key >>= (offset % 4) * 8;
>> +       sid_key &= 0xff;
>
> Redundant 0xff.
Yes, but does clarify the intention, which helps in readability?
>
>> +       /* fall through */
>> +
>> +exit:
>> +       return (u8)sid_key;
>
> No need to have explicit casting here.
Also here, it makes the intention clear, that we are only interested in 
the last 8 bits. The compiler should optimize this and the above bit 
away so it's only for readability.
>
>> +       pdev = (struct platform_device *)to_platform_device(kobj_to_dev(kobj));
>
> Ditto.
Yes, I just looked more closy to container_of, and the 'const 
typeof(((type *)0)->member) takes care of the typecast, does it not?
>
>> +       sid_reg_base = (void __iomem *)platform_get_drvdata(pdev);
>
> Ditto.
Not sure on this one, since technically, it returns only void * (always) 
and we had a void '__iomem *' pointer. for clarity and completness I 
would keep it?
>
>> +static int sunxi_sid_remove(struct platform_device *pdev)
>> +{
>> +       device_remove_bin_file(&pdev->dev, &sid_bin_attr);
>> +       dev_info(&pdev->dev, "sunxi SID driver unloaded\n");
>
> Often this is useless message. In what case this is crucial?
well it's dev_info, so it is only information to the user.
>
>> +static int __init sunxi_sid_probe(struct platform_device *pdev)
>> +{
>> +       int entropy[SID_SIZE], i;
>> +       struct resource *res;
>> +       void __iomem *sid_reg_base;
>> +       int ret;
>> +
>> +       if (!pdev->dev.of_node) {
>> +               dev_err(&pdev->dev, "No devicetree data available\n");
>> +               ret = -ENXIO;
>> +               goto exit;
>
> You have only return, use it. It's common practice in the .probe() function.
>
>> +       if (IS_ERR(sid_reg_base)) {
>> +               ret = PTR_ERR(sid_reg_base);
>> +               goto exit;
>
> Ditto.
>
>> +       ret = device_create_bin_file(&pdev->dev, &sid_bin_attr);
>> +       if (ret) {
>> +               dev_err(&pdev->dev, "Unable to create sysfs bin entry\n");
>> +               goto exit;
>
> Ditto.
>
>> +       dev_info(&pdev->dev, "sunxi SID ver %s loaded\n", DRV_VERSION);
>> +       ret = 0;
>> +       /* fall through */
>
> Ditto.
>
>> +
>> +exit:
>> +       return ret;
>
> Useless lines.
>
>> +module_platform_driver(sunxi_sid_driver);
>> +
>> +
>
> Extra line.
Seperate the code from the trailing macro's
>
>
> --
> With Best Regards,
> Andy Shevchenko
>

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists