From: Oliver Schinagl <[email protected]>
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 | 172 +++++++++++++++++++++++++++++++++++++++
5 files changed, 200 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. From the name however it is
highly likly that this name is the programming voltage, required to
write these fuses.
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 (A13)
Signed-off-by: Oliver Schinagl <[email protected]>
---
drivers/misc/eeprom/Kconfig | 17 ++++
drivers/misc/eeprom/Makefile | 1 +
drivers/misc/eeprom/sunxi_sid.c | 172 ++++++++++++++++++++++++++++++++++++++++
3 files changed, 190 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..4ec86d5
--- /dev/null
+++ b/drivers/misc/eeprom/sunxi_sid.c
@@ -0,0 +1,172 @@
+/*
+ * 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 <asm/io.h>
+#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/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)
+
+static void __iomem *p_sid_reg_base;
+
+/* 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 a simpler.
+ */
+u8 sunxi_sid_read_byte(const int offset)
+{
+ u32 sid_key;
+ u8 ret;
+
+ ret = 0;
+
+ if (likely((SID_SIZE))) {
+ sid_key = ioread32be(p_sid_reg_base + round_down(offset, 4));
+ sid_key >>= (offset % 4) * 8;
+ ret = sid_key & 0xff;
+ }
+
+ return ret;
+}
+
+static ssize_t sid_read(struct file *fd, struct kobject *kobj,
+ struct bin_attribute *attr, char *buf,
+ loff_t pos, size_t size)
+{
+ ssize_t ret;
+ int i;
+
+ ret = -EPERM;
+
+ if ((likely(size > 0)) && ((size + pos) <= SID_SIZE)) {
+ for (i = 0; i < size; i++)
+ buf[i] = sunxi_sid_read_byte(pos + i);
+ ret = (ssize_t)size;
+ } else {
+ ret = 0;
+ }
+
+ return ret;
+}
+
+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 successfully.\n");
+
+ return 0;
+}
+
+static int __init sunxi_sid_probe(struct platform_device *pdev)
+{
+ int entropy[SID_SIZE], i, ret;
+ struct device *dev;
+ struct resource *res;
+ void __iomem *sid_reg_base;
+
+ dev = &pdev->dev;
+ if (unlikely(!pdev->dev.of_node)) {
+ dev_err(dev, "No devicetree data available\n");
+ ret = -EFAULT;
+ 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)) {
+ dev_err(dev, "Unable to obtain resource\n");
+ ret = PTR_ERR(sid_reg_base);
+ goto exit;
+ }
+ platform_set_drvdata(pdev, sid_reg_base);
+ p_sid_reg_base = sid_reg_base;
+
+ ret = device_create_bin_file(dev, &sid_bin_attr);
+ if (unlikely(ret)) {
+ dev_err(dev, "Unable to create sysfs bin entry\n");
+ goto exit;
+ }
+
+ for (i = 0; i < SID_SIZE; i++)
+ entropy[i] = sunxi_sid_read_byte(i);
+ add_device_randomness(entropy, SID_SIZE);
+
+ dev_info(dev, "Sunxi SID driver loaded successfully.\n");
+
+ return 0;
+
+
+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 should 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 Sun, Jun 02, 2013 at 04:58:49PM +0200, Oliver Schinagl wrote:
> +#include <asm/io.h>
We have an include file called linux/io.h. Please use linux/*.h files which
include asm/*.h files in preference to directly using asm/*.h.
In fact, no driver should include an asm/*.h header.
> +#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/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)
> +
> +static void __iomem *p_sid_reg_base;
> +
> +/* 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 a simpler.
> + */
> +u8 sunxi_sid_read_byte(const int offset)
> +{
> + u32 sid_key;
> + u8 ret;
> +
> + ret = 0;
> +
> + if (likely((SID_SIZE))) {
> + sid_key = ioread32be(p_sid_reg_base + round_down(offset, 4));
> + sid_key >>= (offset % 4) * 8;
> + ret = sid_key & 0xff;
> + }
What happens if you unbind the device in sysfs and then try and use
this function?
> +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 successfully.\n");
Maybe you want to set p_sid_reg_base to NULL here?
> +
> + return 0;
> +}
> +
> +static int __init sunxi_sid_probe(struct platform_device *pdev)
> +{
> + int entropy[SID_SIZE], i, ret;
> + struct device *dev;
> + struct resource *res;
> + void __iomem *sid_reg_base;
> +
> + dev = &pdev->dev;
> + if (unlikely(!pdev->dev.of_node)) {
> + dev_err(dev, "No devicetree data available\n");
> + ret = -EFAULT;
> + 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)) {
> + dev_err(dev, "Unable to obtain resource\n");
> + ret = PTR_ERR(sid_reg_base);
> + goto exit;
> + }
> + platform_set_drvdata(pdev, sid_reg_base);
> + p_sid_reg_base = sid_reg_base;
So what happens if you have two of these devices? Maybe you want to check
whether p_sid_reg_base is already set?
> +
> + ret = device_create_bin_file(dev, &sid_bin_attr);
> + if (unlikely(ret)) {
> + dev_err(dev, "Unable to create sysfs bin entry\n");
> + goto exit;
> + }
> +
> + for (i = 0; i < SID_SIZE; i++)
> + entropy[i] = sunxi_sid_read_byte(i);
> + add_device_randomness(entropy, SID_SIZE);
> +
> + dev_info(dev, "Sunxi SID driver loaded successfully.\n");
Do we really need to report that the driver "loaded successfully" ?
Do we need lots of lines in the kernel log telling us simply that
random driver X was built into the kernel or the module was loaded?
On 06/02/13 17:09, Russell King - ARM Linux wrote:
> On Sun, Jun 02, 2013 at 04:58:49PM +0200, Oliver Schinagl wrote:
>> +#include <asm/io.h>
>
> We have an include file called linux/io.h. Please use linux/*.h files which
> include asm/*.h files in preference to directly using asm/*.h.
>
> In fact, no driver should include an asm/*.h header.
And I didn't have that there, but it kept refusing to find ioread32be.
Of course, now I change it back to linux/io.h (which I had, I swear) and
all works swell.
Concider it changed.
>
>> +#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/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)
>> +
>> +static void __iomem *p_sid_reg_base;
>> +
>> +/* 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 a simpler.
>> + */
>> +u8 sunxi_sid_read_byte(const int offset)
>> +{
>> + u32 sid_key;
>> + u8 ret;
>> +
>> + ret = 0;
>> +
>> + if (likely((SID_SIZE))) {
>> + sid_key = ioread32be(p_sid_reg_base + round_down(offset, 4));
>> + sid_key >>= (offset % 4) * 8;
>> + ret = sid_key & 0xff;
>> + }
>
> What happens if you unbind the device in sysfs and then try and use
> this function?
>
>> +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 successfully.\n");
>
> Maybe you want to set p_sid_reg_base to NULL here?
>
>> +
>> + return 0;
>> +}
>> +
>> +static int __init sunxi_sid_probe(struct platform_device *pdev)
>> +{
>> + int entropy[SID_SIZE], i, ret;
>> + struct device *dev;
>> + struct resource *res;
>> + void __iomem *sid_reg_base;
>> +
>> + dev = &pdev->dev;
>> + if (unlikely(!pdev->dev.of_node)) {
>> + dev_err(dev, "No devicetree data available\n");
>> + ret = -EFAULT;
>> + 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)) {
>> + dev_err(dev, "Unable to obtain resource\n");
>> + ret = PTR_ERR(sid_reg_base);
>> + goto exit;
>> + }
>> + platform_set_drvdata(pdev, sid_reg_base);
>> + p_sid_reg_base = sid_reg_base;
>
> So what happens if you have two of these devices? Maybe you want to check
> whether p_sid_reg_base is already set?
>
>> +
>> + ret = device_create_bin_file(dev, &sid_bin_attr);
>> + if (unlikely(ret)) {
>> + dev_err(dev, "Unable to create sysfs bin entry\n");
>> + goto exit;
>> + }
>> +
>> + for (i = 0; i < SID_SIZE; i++)
>> + entropy[i] = sunxi_sid_read_byte(i);
>> + add_device_randomness(entropy, SID_SIZE);
>> +
>> + dev_info(dev, "Sunxi SID driver loaded successfully.\n");
>
> Do we really need to report that the driver "loaded successfully" ?
> Do we need lots of lines in the kernel log telling us simply that
> random driver X was built into the kernel or the module was loaded?
>
On Sun, Jun 2, 2013 at 5:58 PM, 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. From the name however it is
> highly likly that this name is the programming voltage, required to
> write these fuses.
> 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 (A13)
Few commets below.
> +++ b/drivers/misc/eeprom/sunxi_sid.c
> @@ -0,0 +1,172 @@
> +#define DRV_NAME "sunxi-sid"
> +#define DRV_VERSION "1.0"
> +static void __iomem *p_sid_reg_base;
So, why it's global?
> +/* 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 a simpler.
> + */
> +u8 sunxi_sid_read_byte(const int offset)
> +{
> + u32 sid_key;
> + u8 ret;
> +
> + ret = 0;
ret is redundant variable in this function.
> + if (likely((SID_SIZE))) {
Extra braces.
Use antipattern here.
> + sid_key = ioread32be(p_sid_reg_base + round_down(offset, 4));
> + sid_key >>= (offset % 4) * 8;
> + ret = sid_key & 0xff;
No need to do & 0xff, since return type is byte.
> +static ssize_t sid_read(struct file *fd, struct kobject *kobj,
> + struct bin_attribute *attr, char *buf,
> + loff_t pos, size_t size)
> +{
> + ssize_t ret;
> + int i;
> +
> + ret = -EPERM;
When will it happen?
Moreover, ret is redundant.
> +
> + if ((likely(size > 0)) && ((size + pos) <= SID_SIZE)) {
Extra braces in second part of condition.
Use antipattern.
> +static int __init sunxi_sid_probe(struct platform_device *pdev)
> +{
> + int entropy[SID_SIZE], i, ret;
Usually ret variable is located at the end of definition block.
Moreover, there is no relationship between those three. It means one
line per variable.
> + struct device *dev;
> + struct resource *res;
> + void __iomem *sid_reg_base;
> +
> + dev = &pdev->dev;
Please, be consistent, somewhere you still use &pdev->dev.
I recomend to use &pdev->dev everywhere in probe(), since we don't
know if the device will be probed successfully.
> + if (unlikely(!pdev->dev.of_node)) {
> + dev_err(dev, "No devicetree data available\n");
> + ret = -EFAULT;
> + goto exit;
Plain return here and in entire function where it applies.
> + }
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + sid_reg_base = devm_ioremap_resource(&pdev->dev, res);
> + if (IS_ERR(sid_reg_base)) {
> + dev_err(dev, "Unable to obtain resource\n");
Redundant message. You have not to duplicate this.
> + ret = PTR_ERR(sid_reg_base);
> + goto exit;
> + }
> + platform_set_drvdata(pdev, sid_reg_base);
> + p_sid_reg_base = sid_reg_base;
> +
> + ret = device_create_bin_file(dev, &sid_bin_attr);
> + if (unlikely(ret)) {
Any benifit of (un)likely in probe()?
> +
> +
> +exit:
> + return ret;
Remove those two and empty lines.
--
With Best Regards,
Andy Shevchenko
On 06/06/13 21:16, Andy Shevchenko wrote:
Thank you andy for your review, I do have a few questions/comments if
you don't mind.
> On Sun, Jun 2, 2013 at 5:58 PM, Oliver Schinagl <[email protected]> wrote:
>> From: Oliver Schinagl <[email protected]>
<snip>
>> + if (likely((SID_SIZE))) {
>
> Extra braces.
> Use antipattern here.
While I accidentally dropped the pointer here, sorry for the confusion,
what is antipattern? I have asked around and nobody really knew.
Wikipedia mentions it as a software development thing, but you make it
sound like it is some sort of tool?
<snip>
>> + if (unlikely(!pdev->dev.of_node)) {
>> + dev_err(dev, "No devicetree data available\n");
>> + ret = -EFAULT;
>> + goto exit;
>
> Plain return here and in entire function where it applies.
Why? I know there's conflicting preferences here. The general consensus
seems, don't return mid function if you don't absolutely have to. Yet,
you make it sound, just return wherever. I take it that really is just a
preference? I think i see both constructs throughout the kernel. So one
review prefers the one method, the next the other?
<snip>
>> +
>> + ret = device_create_bin_file(dev, &sid_bin_attr);
>> + if (unlikely(ret)) {
>
> Any benifit of (un)likely in probe()?
Does it hurt however in any way though? It's just a compiler
optimization isn't it.
<snip>
>
> --
> With Best Regards,
> Andy Shevchenko
>
Thank you for your time, it is much appreciated :)
Oliver
On Tue, Jun 11, 2013 at 12:43 AM, Oliver Schinagl
<[email protected]> wrote:
> On 06/06/13 21:16, Andy Shevchenko wrote:
>> On Sun, Jun 2, 2013 at 5:58 PM, Oliver Schinagl <[email protected]>
>> wrote:
>>> From: Oliver Schinagl <[email protected]>
>>> + if (likely((SID_SIZE))) {
>>
>> Extra braces.
>> Use antipattern here.
>
> While I accidentally dropped the pointer here, sorry for the confusion, what
> is antipattern? I have asked around and nobody really knew.
In this case instead of doing...
if (likely(condition)) {
do_smth();
}
do_very_few_ops();
return;
...better to do
if(unlikely(!condition)) {
do_very_few_ops();
return;
}
do_smth();
return;
It takes more lines of code, but increases readability a lot.
>>> + if (unlikely(!pdev->dev.of_node)) {
>>> + dev_err(dev, "No devicetree data available\n");
>>> + ret = -EFAULT;
>>> + goto exit;
>>
>>
>> Plain return here and in entire function where it applies.
>
> Why? I know there's conflicting preferences here. The general consensus
> seems, don't return mid function if you don't absolutely have to. Yet, you
> make it sound, just return wherever. I take it that really is just a
> preference? I think i see both constructs throughout the kernel. So one
> review prefers the one method, the next the other?
Usually it makes sense when you have to free resources or do something
like that. You have plain return statement under exit label.
>>> + ret = device_create_bin_file(dev, &sid_bin_attr);
>>> + if (unlikely(ret)) {
>>
>> Any benifit of (un)likely in probe()?
>
> Does it hurt however in any way though? It's just a compiler optimization
> isn't it.
It hurts readability. probe() function is usually doesn't require
fastest execution. Moreover, [1] tells us "You should use it only in
cases when the likeliest branch is very very very likely, or when the
unlikeliest branch is very very very unlikely."
There also an article [2] about cache issues. Bad usage of the
likely/unlikely macros might lead to performance degradation (cache
misses). You have to think about those macros really carefully.
[1] http://kernelnewbies.org/FAQ/LikelyUnlikely
[2] http://dslab.lzu.edu.cn:8080/docs/publications/NicholasMcGuire.pdf
--
With Best Regards,
Andy Shevchenko
Hello all,
While waiting/working in comments I realized/was reminded that there
where a few things I hadn't addressed. I'll try to go over all the
missing bits to be sure to answer all questions before submitting the
hopefully final version. This because I wouldn't want anybody to think
I'm ungrateful, because I am very much so.
> On 05/17/13 23:18, Maxime Ripard wrote:
>> +
>> +struct sid_priv {
>> + void __iomem *sid_base;
>> +};
>> +
>> +struct sid_priv *p;
>
> What's the point of having a structure here? And why putting a global
> value, !static, with a generic name, while you could have an
> instance-specific one?
and related (with the struct killed and the mem* renamed)
> On 02-06-13 17:09, Russell King - ARM Linux wrote:
> So what happens if you have two of these devices? Maybe you want to check
> whether p_sid_reg_base is already set?
I had to think a while about it, as my first answer was 'i don't know'.
But yeah you can't without some smart trick that I don't yet know.
> On 06-06-13 21:16, Andy Shevchenko wrote:
>> +static void __iomem *p_sid_reg_base;
> So, why it's global?
>
The idea behind the global pointer is, to have sunxi_sid_read_byte() be
usable without anything other then a memory offset/address of what byte
you want to read. The idea behind that is that you can use it from
within the kernel directly. I'll admit it's a future-proofing thing,
it's not exported now as there is no user for it yet. I talked to Maxime
about it and we (he) decided it's best to remove it for now, as we can
always add it back later when we find a user for it.
Having two SID's however is almost impossible too, since you get 1 per
SoC, and only 1 SoC per kernel? I added a guard in .probe() which only
loads the driver if the pointer is NULL.
> On 02-06-13 17:09, Russell King - ARM Linux wrote:
>> + if (likely((SID_SIZE))) {
>> + sid_key = ioread32be(p_sid_reg_base + round_down(offset, 4));
>> + sid_key >>= (offset % 4) * 8;
>> + ret = sid_key & 0xff;
>> + }
>
> What happens if you unbind the device in sysfs and then try and use
> this function?
>
It goes kaboom. Again, i had no idea naturally. I think what you meant
was, if you unbind and p_sid_reg_base points to 'somewhere' and we just
randomly read bytes. I added p_sid_reg_base to the guard above to at
least check if it's not NULL.
> On 02-06-13 17:09, Russell King - ARM Linux wrote:
>> +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 successfully.\n");
>
> Maybe you want to set p_sid_reg_base to NULL here?
>
Yeah, that made even more sense to the above thing and added it. When we
unbind or unload the driver, we point to NULL and a) can check for it
above and don't have nasty things dangling around. I think it was save
to assume, that when the driver is unloaded/unbound, the devm_set_data()
private data gets cleaned and the pointer set to NULL? I think that's
what Maxime/Emilio said, by using the devm stuff, those should get
cleaned up properly.
> On 02-06-13 17:09, Russell King - ARM Linux wrote:
>> + platform_set_drvdata(pdev, sid_reg_base);
>> + p_sid_reg_base = sid_reg_base;
>
> So what happens if you have two of these devices? Maybe you want to check
> whether p_sid_reg_base is already set?
>
Even that hint didn't trigger me as to why this could cause problems.
Userspace guy here, every driver has its own memory space right? So I've
added a guard that we only load this driver once and only if
p_sid_reg_base is NULL.
> On 02-06-13 17:09, Russell King - ARM Linux wrote:
>> + dev_info(dev, "Sunxi SID driver loaded successfully.\n");
>
> Do we really need to report that the driver "loaded successfully" ?
> Do we need lots of lines in the kernel log telling us simply that
> random driver X was built into the kernel or the module was loaded?
Don't we do that already though? I made the string more informative by
adding version information, but yeah I see tons of 'all is ok' rolling
by, isn't that a good thing? Isn't that only printed if we actually have
a SID onboard, so the user knows 'sid is available'. I know I often
scroll through dmesg to see if things got brought up nicely.
> On 06-06-13 21:16, Andy Shevchenko wrote:
>> + if (likely((SID_SIZE))) {
> Extra braces.
> Use antipattern here.
While this is a really small function, I guess if that's the normal
course of things; why not.
> On 06-06-13 21:16, Andy Shevchenko wrote:
>> + ret = sid_key & 0xff;
> No need to do & 0xff, since return type is byte.
>
With ret removed, I assume having return (u8)sid_key; is okay? Does that
typecast only return the last byte? Or do we still want & 0xff in the
return added for clarity?
> On 06-06-13 21:16, Andy Shevchenko wrote
>> +static int __init sunxi_sid_probe(struct platform_device *pdev)
>> +{
>> + int entropy[SID_SIZE], i, ret;
>
> Usually ret variable is located at the end of definition block.
> Moreover, there is no relationship between those three. It means one
> line per variable.
>
That's a good rule to group variables with, I should have known. Done.
> On 06-06-13 21:16, Andy Shevchenko wrote:
>> + dev = &pdev->dev;
> Please, be consistent, somewhere you still use &pdev->dev.
> I recomend to use &pdev->dev everywhere in probe(), since we don't
> know if the device will be probed successfully.
>
I would have thought that 'dev' would that it would be the same and valid,
but yes, I should have been consistent throughout. What bothers me here,
is that I actually managed to overlook it.