From: Oliver Schinagl <[email protected]>
I've tried to incoperate all requests/issues but as always could have possibly
missed some. I've talked to a few people, maxime mostly about the return vs
goto and he said it was up to me, and have choosen to stick with the goto for
the error handling.
Changes from v2:
* Removed the global pointer, we can change that when the need for external
access arises
* Fixed header inclusions
* Corrected if guards. There where some crude mistakes there
* Changed offset to an unsigned int so we don't have to worry about negatives
* Cleaned up variable declarations
* Changed ret value, ENXIO (No device/io) as that better matches a missing dt
* Made the loading informercial print version so it is somewhat usefull
Changes from v1:
* Renamed the sys-fs exported key to eeprom, since it really a read-only eeprom
* Removed mention of sun[67]i since we haven't tested those
* Fixed up mistakes in comments
* Removed PAGE_SIZE references, since this is a binary only driver
* Removed lookup table and calculate offsets better
* Use proper endianess
* Add the SID to seed the kernel entropy pool
* Rewrite probe to use platform_get_resource/devm_ioremap_resource instead
The Allwinner A-series of SoC's have efuses exposed via registers to read the
factory programmed e-fuses. These should in theory be programmable but this is
still to be confirmed. It does appear that these fuses are unique enough to be
used as serial numbers, RSA keys, generate MAC addresses from etc. If it turns
out to be user programmable, the use obviously increases. Allwinner did use the
fuses initially to determine the chip-type.
This driver supports all currently known chips based on datasheets and 'dumped'
drivers that we have so far, the dts is only implemented for known chips.
It has been tested on a Cubieboard 1
This is my very first driver so please try to be gentle
Oliver Schinagl (2):
Initial support for Allwinner's Security ID fuses
Add sunxi-sid to dts for sun4i and sun5i
arch/arm/boot/dts/sun4i-a10.dtsi | 5 ++
arch/arm/boot/dts/sun5i-a13.dtsi | 5 ++
drivers/misc/eeprom/Kconfig | 17 ++++
drivers/misc/eeprom/Makefile | 1 +
drivers/misc/eeprom/sunxi_sid.c | 167 +++++++++++++++++++++++++++++++++++++++
5 files changed, 195 insertions(+)
create mode 100644 drivers/misc/eeprom/sunxi_sid.c
--
1.8.1.5
From: Oliver Schinagl <[email protected]>
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)
Signed-off-by: Oliver Schinagl <[email protected]>
---
drivers/misc/eeprom/Kconfig | 17 ++++
drivers/misc/eeprom/Makefile | 1 +
drivers/misc/eeprom/sunxi_sid.c | 167 ++++++++++++++++++++++++++++++++++++++++
3 files changed, 185 insertions(+)
create mode 100644 drivers/misc/eeprom/sunxi_sid.c
diff --git a/drivers/misc/eeprom/Kconfig b/drivers/misc/eeprom/Kconfig
index 04f2e1f..c7bc6ed 100644
--- a/drivers/misc/eeprom/Kconfig
+++ b/drivers/misc/eeprom/Kconfig
@@ -96,4 +96,21 @@ config EEPROM_DIGSY_MTC_CFG
If unsure, say N.
+config EEPROM_SUNXI_SID
+ tristate "Allwinner sunxi security ID support"
+ depends on ARCH_SUNXI && SYSFS
+ help
+ This is a driver for the 'security ID' available on various Allwinner
+ devices. Currently supported are:
+ sun4i (A10)
+ sun5i (A13)
+
+ Due to the potential risks involved with changing e-fuses,
+ this driver is read-only
+
+ For more information visit http://linux-sunxi.org/SID
+
+ This driver can also be built as a module. If so, the module
+ will be called sunxi_sid.
+
endmenu
diff --git a/drivers/misc/eeprom/Makefile b/drivers/misc/eeprom/Makefile
index fc1e81d..9507aec 100644
--- a/drivers/misc/eeprom/Makefile
+++ b/drivers/misc/eeprom/Makefile
@@ -4,4 +4,5 @@ obj-$(CONFIG_EEPROM_LEGACY) += eeprom.o
obj-$(CONFIG_EEPROM_MAX6875) += max6875.o
obj-$(CONFIG_EEPROM_93CX6) += eeprom_93cx6.o
obj-$(CONFIG_EEPROM_93XX46) += eeprom_93xx46.o
+obj-$(CONFIG_EEPROM_SUNXI_SID) += sunxi_sid.o
obj-$(CONFIG_EEPROM_DIGSY_MTC_CFG) += digsy_mtc_eeprom.o
diff --git a/drivers/misc/eeprom/sunxi_sid.c b/drivers/misc/eeprom/sunxi_sid.c
new file mode 100644
index 0000000..f014e1b
--- /dev/null
+++ b/drivers/misc/eeprom/sunxi_sid.c
@@ -0,0 +1,167 @@
+/*
+ * Copyright (c) 2013 Oliver Schinagl
+ * http://www.linux-sunxi.org
+ *
+ * Oliver Schinagl <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * This driver exposes the Allwinner security ID, a 128 bit eeprom, in byte
+ * sized chunks.
+ */
+
+#include <linux/compiler.h>
+#include <linux/device.h>
+#include <linux/err.h>
+#include <linux/errno.h>
+#include <linux/export.h>
+#include <linux/fs.h>
+#include <linux/init.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/kobject.h>
+#include <linux/module.h>
+#include <linux/of_address.h>
+#include <linux/platform_device.h>
+#include <linux/random.h>
+#include <linux/stat.h>
+#include <linux/sysfs.h>
+#include <linux/types.h>
+
+#define DRV_NAME "sunxi-sid"
+#define DRV_VERSION "1.0"
+
+/* There are 4 32-bit keys */
+#define SID_KEYS 4
+/* and 4 byte sized keys per 32-bit key */
+#define SID_SIZE (SID_KEYS * 4)
+
+
+/* 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.
+ */
+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;
+
+ sid_key = ioread32be(sid_reg_base + round_down(offset, 4));
+ sid_key >>= (offset % 4) * 8;
+ sid_key &= 0xff;
+ /* fall through */
+
+exit:
+ return (u8)sid_key;
+}
+
+static ssize_t sid_read(struct file *fd, struct kobject *kobj,
+ struct bin_attribute *attr, char *buf,
+ loff_t pos, size_t size)
+{
+ int i;
+ struct platform_device *pdev;
+ void __iomem *sid_reg_base;
+
+ pdev = (struct platform_device *)to_platform_device(kobj_to_dev(kobj));
+ sid_reg_base = (void __iomem *)platform_get_drvdata(pdev);
+
+ 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);
+ }
+
+ return i;
+}
+
+static const struct of_device_id sunxi_sid_of_match[] = {
+ {
+ .compatible = "allwinner,sun4i-sid",
+ },
+ {/* sentinel */}
+};
+MODULE_DEVICE_TABLE(of, sunxi_sid_of_match);
+
+static const struct bin_attribute sid_bin_attr = {
+ .attr = {
+ .name = "eeprom",
+ .mode = S_IRUGO,
+ },
+ .size = SID_SIZE,
+ .read = sid_read,
+};
+
+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");
+
+ return 0;
+}
+
+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;
+ }
+
+ res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ sid_reg_base = devm_ioremap_resource(&pdev->dev, res);
+ if (IS_ERR(sid_reg_base)) {
+ ret = PTR_ERR(sid_reg_base);
+ goto exit;
+ }
+ platform_set_drvdata(pdev, sid_reg_base);
+
+ 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;
+ }
+
+ for (i = 0; i < SID_SIZE; i++)
+ entropy[i] = sunxi_sid_read_byte(sid_reg_base, i);
+ add_device_randomness(entropy, SID_SIZE);
+ dev_info(&pdev->dev, "sunxi SID ver %s loaded\n", DRV_VERSION);
+ ret = 0;
+ /* fall through */
+
+exit:
+ return ret;
+}
+
+static struct platform_driver sunxi_sid_driver = {
+ .probe = sunxi_sid_probe,
+ .remove = sunxi_sid_remove,
+ .driver = {
+ .name = DRV_NAME,
+ .owner = THIS_MODULE,
+ .of_match_table = sunxi_sid_of_match,
+ },
+};
+module_platform_driver(sunxi_sid_driver);
+
+
+MODULE_AUTHOR("Oliver Schinagl <[email protected]>");
+MODULE_DESCRIPTION("Allwinner sunxi security id driver");
+MODULE_VERSION(DRV_VERSION);
+MODULE_LICENSE("GPL");
--
1.8.1.5
From: Oliver Schinagl <[email protected]>
This patch shall add support for the sunxi-sid driver to the device table for
sun4i and sun5i.
Signed-off-by: Oliver Schinagl <[email protected]>
---
arch/arm/boot/dts/sun4i-a10.dtsi | 5 +++++
arch/arm/boot/dts/sun5i-a13.dtsi | 5 +++++
2 files changed, 10 insertions(+)
diff --git a/arch/arm/boot/dts/sun4i-a10.dtsi b/arch/arm/boot/dts/sun4i-a10.dtsi
index e7ef619..bc71d64 100644
--- a/arch/arm/boot/dts/sun4i-a10.dtsi
+++ b/arch/arm/boot/dts/sun4i-a10.dtsi
@@ -213,6 +213,11 @@
reg = <0x01c20c90 0x10>;
};
+ sid: eeprom@01c23800 {
+ compatible = "allwinner,sun4i-sid";
+ reg = <0x01c23800 0x10>;
+ };
+
uart0: serial@01c28000 {
compatible = "snps,dw-apb-uart";
reg = <0x01c28000 0x400>;
diff --git a/arch/arm/boot/dts/sun5i-a13.dtsi b/arch/arm/boot/dts/sun5i-a13.dtsi
index 8ba65c1..c80c81b 100644
--- a/arch/arm/boot/dts/sun5i-a13.dtsi
+++ b/arch/arm/boot/dts/sun5i-a13.dtsi
@@ -196,6 +196,11 @@
reg = <0x01c20c90 0x10>;
};
+ sid: eeprom@01c23800 {
+ compatible = "allwinner,sun4i-sid";
+ reg = <0x01c23800 0x10>;
+ };
+
uart1: serial@01c28400 {
compatible = "snps,dw-apb-uart";
reg = <0x01c28400 0x400>;
--
1.8.1.5
On Sat, Jun 15, 2013 at 2:16 AM, Oliver Schinagl
<[email protected]> wrote:
> From: Oliver Schinagl <[email protected]>
>
> 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?
> +#define SID_SIZE (SID_KEYS * 4)
> +
> +
Extra line.
> +/* 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?
> + */
> +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.
> + sid_key = ioread32be(sid_reg_base + round_down(offset, 4));
> + sid_key >>= (offset % 4) * 8;
> + sid_key &= 0xff;
Redundant 0xff.
> + /* fall through */
> +
> +exit:
> + return (u8)sid_key;
No need to have explicit casting here.
> + pdev = (struct platform_device *)to_platform_device(kobj_to_dev(kobj));
Ditto.
> + sid_reg_base = (void __iomem *)platform_get_drvdata(pdev);
Ditto.
> +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?
> +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.
--
With Best Regards,
Andy Shevchenko
On 06/15/13 04:14, Andy Shevchenko wrote:
> On Sat, Jun 15, 2013 at 2:16 AM, Oliver Schinagl
> <[email protected]> wrote:
>> From: Oliver Schinagl <[email protected]>
>>
>> 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
>
Hi,
Some comments inline.
On Saturday 15 of June 2013 01:16:20 Oliver Schinagl wrote:
> From: Oliver Schinagl <[email protected]>
>
> 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)
>
> Signed-off-by: Oliver Schinagl <[email protected]>
> ---
> drivers/misc/eeprom/Kconfig | 17 ++++
> drivers/misc/eeprom/Makefile | 1 +
> drivers/misc/eeprom/sunxi_sid.c | 167
> ++++++++++++++++++++++++++++++++++++++++ 3 files changed, 185
> insertions(+)
> create mode 100644 drivers/misc/eeprom/sunxi_sid.c
>
> diff --git a/drivers/misc/eeprom/Kconfig b/drivers/misc/eeprom/Kconfig
> index 04f2e1f..c7bc6ed 100644
> --- a/drivers/misc/eeprom/Kconfig
> +++ b/drivers/misc/eeprom/Kconfig
> @@ -96,4 +96,21 @@ config EEPROM_DIGSY_MTC_CFG
>
> If unsure, say N.
>
> +config EEPROM_SUNXI_SID
> + tristate "Allwinner sunxi security ID support"
> + depends on ARCH_SUNXI && SYSFS
> + help
> + This is a driver for the 'security ID' available on various
> Allwinner + devices. Currently supported are:
> + sun4i (A10)
> + sun5i (A13)
> +
> + Due to the potential risks involved with changing e-fuses,
> + this driver is read-only
> +
> + For more information visit http://linux-sunxi.org/SID
> +
> + This driver can also be built as a module. If so, the module
> + will be called sunxi_sid.
> +
> endmenu
> diff --git a/drivers/misc/eeprom/Makefile b/drivers/misc/eeprom/Makefile
> index fc1e81d..9507aec 100644
> --- a/drivers/misc/eeprom/Makefile
> +++ b/drivers/misc/eeprom/Makefile
> @@ -4,4 +4,5 @@ obj-$(CONFIG_EEPROM_LEGACY) += eeprom.o
> obj-$(CONFIG_EEPROM_MAX6875) += max6875.o
> obj-$(CONFIG_EEPROM_93CX6) += eeprom_93cx6.o
> obj-$(CONFIG_EEPROM_93XX46) += eeprom_93xx46.o
> +obj-$(CONFIG_EEPROM_SUNXI_SID) += sunxi_sid.o
> obj-$(CONFIG_EEPROM_DIGSY_MTC_CFG) += digsy_mtc_eeprom.o
> diff --git a/drivers/misc/eeprom/sunxi_sid.c
> b/drivers/misc/eeprom/sunxi_sid.c new file mode 100644
> index 0000000..f014e1b
> --- /dev/null
> +++ b/drivers/misc/eeprom/sunxi_sid.c
> @@ -0,0 +1,167 @@
> +/*
> + * Copyright (c) 2013 Oliver Schinagl
> + * http://www.linux-sunxi.org
> + *
> + * Oliver Schinagl <[email protected]>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published
> by + * the Free Software Foundation; either version 2 of the License,
> or + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * This driver exposes the Allwinner security ID, a 128 bit eeprom, in
> byte + * sized chunks.
> + */
> +
> +#include <linux/compiler.h>
> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/errno.h>
> +#include <linux/export.h>
> +#include <linux/fs.h>
> +#include <linux/init.h>
> +#include <linux/io.h>
> +#include <linux/kernel.h>
> +#include <linux/kobject.h>
> +#include <linux/module.h>
> +#include <linux/of_address.h>
> +#include <linux/platform_device.h>
> +#include <linux/random.h>
> +#include <linux/stat.h>
> +#include <linux/sysfs.h>
> +#include <linux/types.h>
> +
> +#define DRV_NAME "sunxi-sid"
> +#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.
> +
> +/* There are 4 32-bit keys */
> +#define SID_KEYS 4
> +/* and 4 byte sized keys per 32-bit key */
> +#define SID_SIZE (SID_KEYS * 4)
>From this definition it looks like there are just 4 32-bit keys, I don't
see those extra four byte-sized keys accounted by this size.
> +
> +
One _empty_ line is enough to separate definitions from 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.
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().
> + */
> +static u8 sunxi_sid_read_byte(const void __iomem *sid_reg_base,
> + const unsigned int offset)
> +{
> + u32 sid_key = 0;
u32 sid_key; ...
> +
> + if (offset >= SID_SIZE)
> + goto exit;
return 0; ...
> +
> + sid_key = ioread32be(sid_reg_base + round_down(offset, 4));
> + sid_key >>= (offset % 4) * 8;
> + sid_key &= 0xff;
> + /* fall through */
> +
> +exit:
...and now magically here you can remove two unneeded lines.
> + return (u8)sid_key;
Unnecessary casting.
> +}
> +
> +static ssize_t sid_read(struct file *fd, struct kobject *kobj,
> + struct bin_attribute *attr, char *buf,
> + loff_t pos, size_t size)
> +{
> + int i;
> + struct platform_device *pdev;
> + void __iomem *sid_reg_base;
> +
> + pdev = (struct platform_device
> *)to_platform_device(kobj_to_dev(kobj)); + sid_reg_base = (void
__iomem
> *)platform_get_drvdata(pdev);
> +
> + 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.
> + return i;
> +}
> +
> +static const struct of_device_id sunxi_sid_of_match[] = {
> + {
> + .compatible = "allwinner,sun4i-sid",
> + },
Above 3 lines could be put in single line.
> + {/* sentinel */}
> +};
> +MODULE_DEVICE_TABLE(of, sunxi_sid_of_match);
> +
> +static const struct bin_attribute sid_bin_attr = {
> + .attr = {
> + .name = "eeprom",
> + .mode = S_IRUGO,
> + },
> + .size = SID_SIZE,
> + .read = sid_read,
> +};
> +
> +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");
Please either make this dev_dbg or remove it completely, as it is not
something that users should be concerned about.
> + return 0;
> +}
> +
> +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;
> + }
What is this check for? You don't seem to need anything from dev.of_node
in this driver.
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + sid_reg_base = devm_ioremap_resource(&pdev->dev, res);
> + if (IS_ERR(sid_reg_base)) {
> + ret = PTR_ERR(sid_reg_base);
> + goto exit;
> + }
One of the points of having devm_ helpers was removing the need to use
error paths at functions end. You can save 2 lines of code by changing
this check to:
if (IS_ERR(sid_reg_base))
return PTR_ERR(sid_reg_base);
> + platform_set_drvdata(pdev, sid_reg_base);
> +
> + 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;
> + }
Same here.
> +
> + 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?
> + 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)...
> + dev_info(&pdev->dev, "sunxi SID ver %s loaded\n", DRV_VERSION);
Same comment as for the message in remove().
> + ret = 0;
> + /* fall through */
> +
> +exit:
> + return ret;
Above 4 non-empty lines can be replaced by a single
return 0;
> +}
> +
> +static struct platform_driver sunxi_sid_driver = {
> + .probe = sunxi_sid_probe,
> + .remove = sunxi_sid_remove,
> + .driver = {
> + .name = DRV_NAME,
> + .owner = THIS_MODULE,
> + .of_match_table = sunxi_sid_of_match,
> + },
> +};
> +module_platform_driver(sunxi_sid_driver);
> +
> +
One empty line is enough.
Best regards,
Tomasz
> +MODULE_AUTHOR("Oliver Schinagl <[email protected]>");
> +MODULE_DESCRIPTION("Allwinner sunxi security id driver");
> +MODULE_VERSION(DRV_VERSION);
> +MODULE_LICENSE("GPL");
On 15-06-13 12:28, Tomasz Figa wrote:
> Hi,
>
> Some comments inline.
Thank you
>
> On Saturday 15 of June 2013 01:16:20 Oliver Schinagl wrote:
>> From: Oliver Schinagl <[email protected]>
>>
>> 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)
>>
>> Signed-off-by: Oliver Schinagl <[email protected]>
>> ---
>> drivers/misc/eeprom/Kconfig | 17 ++++
>> drivers/misc/eeprom/Makefile | 1 +
>> drivers/misc/eeprom/sunxi_sid.c | 167
>> ++++++++++++++++++++++++++++++++++++++++ 3 files changed, 185
>> insertions(+)
>> create mode 100644 drivers/misc/eeprom/sunxi_sid.c
>>
>> diff --git a/drivers/misc/eeprom/Kconfig b/drivers/misc/eeprom/Kconfig
>> index 04f2e1f..c7bc6ed 100644
>> --- a/drivers/misc/eeprom/Kconfig
>> +++ b/drivers/misc/eeprom/Kconfig
>> @@ -96,4 +96,21 @@ config EEPROM_DIGSY_MTC_CFG
>>
>> If unsure, say N.
>>
>> +config EEPROM_SUNXI_SID
>> + tristate "Allwinner sunxi security ID support"
>> + depends on ARCH_SUNXI && SYSFS
>> + help
>> + This is a driver for the 'security ID' available on various
>> Allwinner + devices. Currently supported are:
>> + sun4i (A10)
>> + sun5i (A13)
>> +
>> + Due to the potential risks involved with changing e-fuses,
>> + this driver is read-only
>> +
>> + For more information visit http://linux-sunxi.org/SID
>> +
>> + This driver can also be built as a module. If so, the module
>> + will be called sunxi_sid.
>> +
>> endmenu
>> diff --git a/drivers/misc/eeprom/Makefile b/drivers/misc/eeprom/Makefile
>> index fc1e81d..9507aec 100644
>> --- a/drivers/misc/eeprom/Makefile
>> +++ b/drivers/misc/eeprom/Makefile
>> @@ -4,4 +4,5 @@ obj-$(CONFIG_EEPROM_LEGACY) += eeprom.o
>> obj-$(CONFIG_EEPROM_MAX6875) += max6875.o
>> obj-$(CONFIG_EEPROM_93CX6) += eeprom_93cx6.o
>> obj-$(CONFIG_EEPROM_93XX46) += eeprom_93xx46.o
>> +obj-$(CONFIG_EEPROM_SUNXI_SID) += sunxi_sid.o
>> obj-$(CONFIG_EEPROM_DIGSY_MTC_CFG) += digsy_mtc_eeprom.o
>> diff --git a/drivers/misc/eeprom/sunxi_sid.c
>> b/drivers/misc/eeprom/sunxi_sid.c new file mode 100644
>> index 0000000..f014e1b
>> --- /dev/null
>> +++ b/drivers/misc/eeprom/sunxi_sid.c
>> @@ -0,0 +1,167 @@
>> +/*
>> + * Copyright (c) 2013 Oliver Schinagl
>> + * http://www.linux-sunxi.org
>> + *
>> + * Oliver Schinagl <[email protected]>
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License as published
>> by + * the Free Software Foundation; either version 2 of the License,
>> or + * (at your option) any later version.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>> + * GNU General Public License for more details.
>> + *
>> + * This driver exposes the Allwinner security ID, a 128 bit eeprom, in
>> byte + * sized chunks.
>> + */
>> +
>> +#include <linux/compiler.h>
>> +#include <linux/device.h>
>> +#include <linux/err.h>
>> +#include <linux/errno.h>
>> +#include <linux/export.h>
>> +#include <linux/fs.h>
>> +#include <linux/init.h>
>> +#include <linux/io.h>
>> +#include <linux/kernel.h>
>> +#include <linux/kobject.h>
>> +#include <linux/module.h>
>> +#include <linux/of_address.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/random.h>
>> +#include <linux/stat.h>
>> +#include <linux/sysfs.h>
>> +#include <linux/types.h>
>> +
>> +#define DRV_NAME "sunxi-sid"
>> +#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?
>
>> +
>> +/* There are 4 32-bit keys */
>> +#define SID_KEYS 4
>> +/* and 4 byte sized keys per 32-bit key */
>> +#define SID_SIZE (SID_KEYS * 4)
>
> From this definition it looks like there are just 4 32-bit keys, I don't
> see those extra four byte-sized keys accounted by this size.
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.
>
>> +
>> +
>
> One _empty_ line is enough to separate definitions from code.
Okay, I find it personally a little clearer to separate them, but sure.
>
>> +/* 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.
>
>> + */
>> +static u8 sunxi_sid_read_byte(const void __iomem *sid_reg_base,
>> + const unsigned int offset)
>> +{
>> + u32 sid_key = 0;
>
> u32 sid_key; ...
Indeed, a left over from a previous version. It does no longer need to
be initted.
>
>> +
>> + 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 :)
>
>> +
>> + sid_key = ioread32be(sid_reg_base + round_down(offset, 4));
>> + sid_key >>= (offset % 4) * 8;
>> + sid_key &= 0xff;
>> + /* fall through */
>> +
>> +exit:
>
> ...and now magically here you can remove two unneeded lines.
>
>> + 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).
>
>> +}
>> +
>> +static ssize_t sid_read(struct file *fd, struct kobject *kobj,
>> + struct bin_attribute *attr, char *buf,
>> + loff_t pos, size_t size)
>> +{
>> + int i;
>> + struct platform_device *pdev;
>> + void __iomem *sid_reg_base;
>> +
>> + pdev = (struct platform_device
>> *)to_platform_device(kobj_to_dev(kobj)); + sid_reg_base = (void
> __iomem
>> *)platform_get_drvdata(pdev);
>> +
>> + 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.
>
>> + return i;
>> +}
>> +
>> +static const struct of_device_id sunxi_sid_of_match[] = {
>> + {
>> + .compatible = "allwinner,sun4i-sid",
>> + },
>
> Above 3 lines could be put in single line.
okay
>
>> + {/* sentinel */}
>> +};
>> +MODULE_DEVICE_TABLE(of, sunxi_sid_of_match);
>> +
>> +static const struct bin_attribute sid_bin_attr = {
>> + .attr = {
>> + .name = "eeprom",
>> + .mode = S_IRUGO,
>> + },
>> + .size = SID_SIZE,
>> + .read = sid_read,
>> +};
>> +
>> +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");
>
> Please either make this dev_dbg or remove it completely, as it is not
> something that users should be concerned about.
dev_dbg might be better for the remove, thanks
>
>> + return 0;
>> +}
>> +
>> +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;
>> + }
>
> 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?
>
>> +
>> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> + sid_reg_base = devm_ioremap_resource(&pdev->dev, res);
>> + if (IS_ERR(sid_reg_base)) {
>> + ret = PTR_ERR(sid_reg_base);
>> + goto exit;
>> + }
>
> One of the points of having devm_ helpers was removing the need to use
> error paths at functions end. You can save 2 lines of code by changing
> this check to:
>
> if (IS_ERR(sid_reg_base))
> return PTR_ERR(sid_reg_base);
Okay
>
>> + platform_set_drvdata(pdev, sid_reg_base);
>> +
>> + 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;
>> + }
>
> Same here.
Yeah, many returns, check
>
>> +
>> + 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.
>
>> + dev_info(&pdev->dev, "sunxi SID ver %s loaded\n", DRV_VERSION);
>
> Same comment as for the message in remove().
Though with the DRV_(A[BP]I_)VERSION this is purly informational, I
changed it to only show in the dev_dbg as requested.
>
>> + ret = 0;
>> + /* fall through */
>> +
>> +exit:
>> + return ret;
>
> Above 4 non-empty lines can be replaced by a single
>
> return 0;
>
>> +}
>> +
>> +static struct platform_driver sunxi_sid_driver = {
>> + .probe = sunxi_sid_probe,
>> + .remove = sunxi_sid_remove,
>> + .driver = {
>> + .name = DRV_NAME,
>> + .owner = THIS_MODULE,
>> + .of_match_table = sunxi_sid_of_match,
>> + },
>> +};
>> +module_platform_driver(sunxi_sid_driver);
>> +
>> +
>
> One empty line is enough.
Okay
>
> Best regards,
> Tomasz
>
>> +MODULE_AUTHOR("Oliver Schinagl <[email protected]>");
>> +MODULE_DESCRIPTION("Allwinner sunxi security id driver");
>> +MODULE_VERSION(DRV_VERSION);
>> +MODULE_LICENSE("GPL");
On Mon, Jun 17, 2013 at 12:36:47PM +0200, Oliver Schinagl wrote:
> On 15-06-13 12:28, Tomasz Figa wrote:
>> 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?
What is better to do is to export such things as properties, or
design the API in such a way that the length of the ID is reportable.
However, it's actually quite easy to do if you only care about the
number of bytes - you just arrange for the read() function to return
the number of bytes read. So in the case of 128 bits available, that's
16 bytes, so a read() of the sysfs attribute with a buffer of (say)
256 bytes should report only 16 bytes read.
If it were to become 256 bytes later, then the read() would return
32 bytes read. So there's no need for any new APIs to do this.
Also, this is over-complicated:
+ 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);
+ }
Maybe:
if (pos < 0 || pos >= SID_SIZE)
return 0;
if (size > SID_SIZE - pos)
size = SID_SIZE - pos;
for (i = 0; i < size; i++)
buf[i] = sunxi_sid_read_byte(sid_reg_base, pos + i);
return size;
On 17-06-13 13:25, Russell King - ARM Linux wrote:
> On Mon, Jun 17, 2013 at 12:36:47PM +0200, Oliver Schinagl wrote:
>> On 15-06-13 12:28, Tomasz Figa wrote:
>>> 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?
>
> What is better to do is to export such things as properties, or
> design the API in such a way that the length of the ID is reportable.
>
> However, it's actually quite easy to do if you only care about the
> number of bytes - you just arrange for the read() function to return
> the number of bytes read. So in the case of 128 bits available, that's
> 16 bytes, so a read() of the sysfs attribute with a buffer of (say)
> 256 bytes should report only 16 bytes read.
>
> If it were to become 256 bytes later, then the read() would return
> 32 bytes read. So there's no need for any new APIs to do this.
That makes sense for the sysfs bit and as the only user, I guess makes
the version information obsolete for now.
>
> Also, this is over-complicated:
>
> + 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);
> + }
>
> Maybe:
> if (pos < 0 || pos >= SID_SIZE)
> return 0;
> if (size > SID_SIZE - pos)
> size = SID_SIZE - pos;
>
> for (i = 0; i < size; i++)
> buf[i] = sunxi_sid_read_byte(sid_reg_base, pos + i);
>
> return size;
>
I do like your approach, but takes a second to read ;) How is it less
complicated though? It's more LOC i suppose. I do appreciate that we
only perform the read function when our size is correct, thus making the
for loop only execute the minimally required code. While in this driver
is insignificant and not important, I am a proponent of it.
Consider it changed.
Will wait a bit for Thomaz to optionally reply and then send yet a
nother version ;)
Thanks for your time,
Oliver
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.
> >>+/* 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.
> >>+
> >>+ 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.
> >>+ 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.
> >>+ 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.
You need this check when you actually need some more informations from
the DT, the value of an additionnal property for example.
Maxime
--
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
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 :)
>
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 <[email protected]>
> >>
> >> 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)
> >>
> >> Signed-off-by: Oliver Schinagl <[email protected]>
> >> ---
> >>
> >> drivers/misc/eeprom/Kconfig | 17 ++++
> >> drivers/misc/eeprom/Makefile | 1 +
> >> drivers/misc/eeprom/sunxi_sid.c | 167
> >>
> >> ++++++++++++++++++++++++++++++++++++++++ 3 files changed, 185
> >> insertions(+)
> >>
> >> create mode 100644 drivers/misc/eeprom/sunxi_sid.c
> >>
> >> diff --git a/drivers/misc/eeprom/Kconfig b/drivers/misc/eeprom/Kconfig
> >> index 04f2e1f..c7bc6ed 100644
> >> --- a/drivers/misc/eeprom/Kconfig
> >> +++ b/drivers/misc/eeprom/Kconfig
> >> @@ -96,4 +96,21 @@ config EEPROM_DIGSY_MTC_CFG
> >>
> >> If unsure, say N.
> >>
> >> +config EEPROM_SUNXI_SID
> >> + tristate "Allwinner sunxi security ID support"
> >> + depends on ARCH_SUNXI && SYSFS
> >> + help
> >> + This is a driver for the 'security ID' available on various
> >> Allwinner + devices. Currently supported are:
> >> + sun4i (A10)
> >> + sun5i (A13)
> >> +
> >> + Due to the potential risks involved with changing e-fuses,
> >> + this driver is read-only
> >> +
> >> + For more information visit http://linux-sunxi.org/SID
> >> +
> >> + This driver can also be built as a module. If so, the module
> >> + will be called sunxi_sid.
> >> +
> >>
> >> endmenu
> >>
> >> diff --git a/drivers/misc/eeprom/Makefile b/drivers/misc/eeprom/Makefile
> >> index fc1e81d..9507aec 100644
> >> --- a/drivers/misc/eeprom/Makefile
> >> +++ b/drivers/misc/eeprom/Makefile
> >> @@ -4,4 +4,5 @@ obj-$(CONFIG_EEPROM_LEGACY) += eeprom.o
> >>
> >> obj-$(CONFIG_EEPROM_MAX6875) += max6875.o
> >> obj-$(CONFIG_EEPROM_93CX6) += eeprom_93cx6.o
> >> obj-$(CONFIG_EEPROM_93XX46) += eeprom_93xx46.o
> >>
> >> +obj-$(CONFIG_EEPROM_SUNXI_SID) += sunxi_sid.o
> >>
> >> obj-$(CONFIG_EEPROM_DIGSY_MTC_CFG) += digsy_mtc_eeprom.o
> >>
> >> diff --git a/drivers/misc/eeprom/sunxi_sid.c
> >> b/drivers/misc/eeprom/sunxi_sid.c new file mode 100644
> >> index 0000000..f014e1b
> >> --- /dev/null
> >> +++ b/drivers/misc/eeprom/sunxi_sid.c
> >> @@ -0,0 +1,167 @@
> >> +/*
> >> + * Copyright (c) 2013 Oliver Schinagl
> >> + * http://www.linux-sunxi.org
> >> + *
> >> + * Oliver Schinagl <[email protected]>
> >> + *
> >> + * This program is free software; you can redistribute it and/or modify
> >> + * it under the terms of the GNU General Public License as published
> >> by + * the Free Software Foundation; either version 2 of the License,
> >> or + * (at your option) any later version.
> >> + *
> >> + * This program is distributed in the hope that it will be useful,
> >> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> >> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> >> + * GNU General Public License for more details.
> >> + *
> >> + * This driver exposes the Allwinner security ID, a 128 bit eeprom, in
> >> byte + * sized chunks.
> >> + */
> >> +
> >> +#include <linux/compiler.h>
> >> +#include <linux/device.h>
> >> +#include <linux/err.h>
> >> +#include <linux/errno.h>
> >> +#include <linux/export.h>
> >> +#include <linux/fs.h>
> >> +#include <linux/init.h>
> >> +#include <linux/io.h>
> >> +#include <linux/kernel.h>
> >> +#include <linux/kobject.h>
> >> +#include <linux/module.h>
> >> +#include <linux/of_address.h>
> >> +#include <linux/platform_device.h>
> >> +#include <linux/random.h>
> >> +#include <linux/stat.h>
> >> +#include <linux/sysfs.h>
> >> +#include <linux/types.h>
> >> +
> >> +#define DRV_NAME "sunxi-sid"
> >> +#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?
>
> >> +
> >> +/* There are 4 32-bit keys */
> >> +#define SID_KEYS 4
> >> +/* and 4 byte sized keys per 32-bit key */
> >> +#define SID_SIZE (SID_KEYS * 4)
> >>
> > From this definition it looks like there are just 4 32-bit keys, I don't
> >
> > see those extra four byte-sized keys accounted by this size.
>
> 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)
> >> +
> >> +
> >
> > One _empty_ line is enough to separate definitions from code.
>
> Okay, I find it personally a little clearer to separate them, but sure.
>
> >> +/* 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.
OK. If it's not to be read too often then it's fine.
> >> + */
> >> +static u8 sunxi_sid_read_byte(const void __iomem *sid_reg_base,
> >> + const unsigned int offset)
> >> +{
> >> + u32 sid_key = 0;
> >>
> > u32 sid_key; ...
>
> Indeed, a left over from a previous version. It does no longer need to
> be initted.
>
> >> +
> >> + 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.
> >> +
> >> + sid_key = ioread32be(sid_reg_base + round_down(offset, 4));
> >> + sid_key >>= (offset % 4) * 8;
> >> + sid_key &= 0xff;
> >> + /* fall through */
> >> +
> >
> >> +exit:
> > ...and now magically here you can remove two unneeded lines.
> >
> >> + 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).
>
> >> +}
> >> +
> >> +static ssize_t sid_read(struct file *fd, struct kobject *kobj,
> >> + struct bin_attribute *attr, char *buf,
> >> + loff_t pos, size_t size)
> >> +{
> >> + int i;
> >> + struct platform_device *pdev;
> >> + void __iomem *sid_reg_base;
> >> +
> >> + pdev = (struct platform_device
> >> *)to_platform_device(kobj_to_dev(kobj)); + sid_reg_base = (void
> >
> > __iomem
> >
> >> *)platform_get_drvdata(pdev);
> >> +
> >> + 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.
>
> >> + return i;
> >> +}
> >> +
> >> +static const struct of_device_id sunxi_sid_of_match[] = {
> >> + {
> >> + .compatible = "allwinner,sun4i-sid",
> >> + },
> >
> > Above 3 lines could be put in single line.
>
> okay
>
> >> + {/* sentinel */}
> >> +};
> >> +MODULE_DEVICE_TABLE(of, sunxi_sid_of_match);
> >> +
> >> +static const struct bin_attribute sid_bin_attr = {
> >> + .attr = {
> >> + .name = "eeprom",
> >> + .mode = S_IRUGO,
> >> + },
> >> + .size = SID_SIZE,
> >> + .read = sid_read,
> >> +};
> >> +
> >> +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");
> >
> > Please either make this dev_dbg or remove it completely, as it is not
> > something that users should be concerned about.
>
> dev_dbg might be better for the remove, thanks
>
> >> + return 0;
> >> +}
> >> +
> >> +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;
> >> + }
> >
> > 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?
When you call platform_get_resource(), it looks at platform_device you pass to
it and takes all resources from an array that is generated early at bootup (in
case when DT is available). It will return an error pointer if there is no
resource satisfying your requirements in this array.
In addition devm_ioremap_resource() checks the resource pointer, so if it's an
error pointer it will print a message and return an error as well.
> >> +
> >> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> >> + sid_reg_base = devm_ioremap_resource(&pdev->dev, res);
> >> + if (IS_ERR(sid_reg_base)) {
> >> + ret = PTR_ERR(sid_reg_base);
> >> + goto exit;
> >> + }
> >
> > One of the points of having devm_ helpers was removing the need to use
> > error paths at functions end. You can save 2 lines of code by changing
> >
> > this check to:
> > if (IS_ERR(sid_reg_base))
> >
> > return PTR_ERR(sid_reg_base);
>
> Okay
>
> >> + platform_set_drvdata(pdev, sid_reg_base);
> >> +
> >> + 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;
> >> + }
> >
> > Same here.
>
> Yeah, many returns, check
>
> >> +
> >> + 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
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
(little endianness assumed)
Best regards,
Tomasz
(for rest of comments I think it's enough said in Russell's and Maxime's
replies)
> >> + dev_info(&pdev->dev, "sunxi SID ver %s loaded\n", DRV_VERSION);
> >
> > Same comment as for the message in remove().
>
> Though with the DRV_(A[BP]I_)VERSION this is purly informational, I
> changed it to only show in the dev_dbg as requested.
>
> >> + ret = 0;
> >> + /* fall through */
> >> +
> >> +exit:
> >> + return ret;
> >
> > Above 4 non-empty lines can be replaced by a single
> >
> > return 0;
> >
> >> +}
> >> +
> >> +static struct platform_driver sunxi_sid_driver = {
> >> + .probe = sunxi_sid_probe,
> >> + .remove = sunxi_sid_remove,
> >> + .driver = {
> >> + .name = DRV_NAME,
> >> + .owner = THIS_MODULE,
> >> + .of_match_table = sunxi_sid_of_match,
> >> + },
> >> +};
> >> +module_platform_driver(sunxi_sid_driver);
> >> +
> >> +
> >
> > One empty line is enough.
>
> Okay
>
> > Best regards,
> > Tomasz
> >
> >> +MODULE_AUTHOR("Oliver Schinagl <[email protected]>");
> >> +MODULE_DESCRIPTION("Allwinner sunxi security id driver");
> >> +MODULE_VERSION(DRV_VERSION);
> >> +MODULE_LICENSE("GPL");
>
> _______________________________________________
> linux-arm-kernel mailing list
> [email protected]
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
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 <[email protected]>
>>>>
>>>> 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
On Monday 17 of June 2013 15:10:47 Oliver Schinagl wrote:
> 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 <[email protected]>
> >>>>
> >>>> 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?
The second argument of add_randomness is number of bytes, not number of
elements in array, as far as I can tell.
Best regards,
Tomasz
> 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
On 17-06-13 15:23, Tomasz Figa wrote:
> On Monday 17 of June 2013 15:10:47 Oliver Schinagl wrote:
>> 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:
<snip>
>> 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?
>
> The second argument of add_randomness is number of bytes, not number of
> elements in array, as far as I can tell.
Oh wow, very good catch. I had to dig a little into the source where it
said 'nbytes'. I guess the function prototype would have been nicer if it
said nbytes instead of size.
Anyway, with using an array of u8 this is nicely handled.
> Best regards,
> Tomasz
>
oliver