| Message ID | 1368797744-13737-2-git-send-email-oliver+list@schinagl.nl (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Friday 17 May 2013 15:35:43 Oliver Schinagl wrote: > +static struct bin_attribute sid_bin_attr = { > + .attr = { > + .name = "key", > + .mode = S_IRUGO, > + }, > + .size = SUNXI_SID_SIZE, > + .read = sid_read, > +}; I believe all the other drivers in drivers/misc/eeprom use "eeprom" as the name for the attribute, so using "key" here is a bit inconsistent. Can you change that? Arnd
On 05/17/13 15:45, Arnd Bergmann wrote: > On Friday 17 May 2013 15:35:43 Oliver Schinagl wrote: >> +static struct bin_attribute sid_bin_attr = { >> + .attr = { >> + .name = "key", >> + .mode = S_IRUGO, >> + }, >> + .size = SUNXI_SID_SIZE, >> + .read = sid_read, >> +}; > > I believe all the other drivers in drivers/misc/eeprom use "eeprom" > as the name for the attribute, so using "key" here is a bit inconsistent. > > Can you change that? Changed, will wait for more feedback and then use that in the final version. Should there also be a symlink elsewhere in /sys? Just curious is all. > > Arnd >
Hi Oliver, Le 17/05/2013 15:35, Oliver Schinagl a écrit : > From: Oliver Schinagl <oliver@schinagl.nl> > > Allwinner has electric fuses (efuse) on their line of chips. This driver > reads those fuses and exports them as a sysfs node. Also a symbol is exported > for in-kernel useage. > > While initially these fuses are used to somewhat determin the chipID, these > appear to be writeable by the user and thus can be used for other purpouses. > For example storing a 128 bit root key, a unique serial number, which could > then even be used as a MAC address. > > Because writing to e-fuses can be potentially dangerous, and are certainly > not as often writable (if at all) as flash memory, these shouldn't be easily > changeable, hence only a read-only mode. An offline tool to write the fuses > is in the works. > > Currently supported are the following known chips: > Allwinner sun4i (A10) > Allwinner sun5i (A10s A13) > Allwinner sun6i (A31, A31s) > Allwinner sun7i (A20) Since I don't think those patches have been tested on sun6i/sun7i, and that there's not even kernel support for those, maybe it's not worth mentionning them? > > Signed-off-by: Oliver Schinagl <oliver@schinagl.nl> > --- > drivers/misc/eeprom/Kconfig | 19 ++++ > drivers/misc/eeprom/Makefile | 1 + > drivers/misc/eeprom/sunxi_sid.c | 218 ++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 238 insertions(+) > create mode 100644 drivers/misc/eeprom/sunxi_sid.c > > diff --git a/drivers/misc/eeprom/Kconfig b/drivers/misc/eeprom/Kconfig > index 04f2e1f..c9ddda5 100644 > --- a/drivers/misc/eeprom/Kconfig > +++ b/drivers/misc/eeprom/Kconfig > @@ -96,4 +96,23 @@ 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 (A10s, A12, A13) > + sun6i (A31) > + sun7i (A20) Same things here. > + > + 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..953f137 > --- /dev/null > +++ b/drivers/misc/eeprom/sunxi_sid.c > @@ -0,0 +1,218 @@ > +/* > + * Copyright (c) 2013 Oliver Schinagl > + * http://www.linux-sunxi.org > + * > + * Oliver Schinagl <oliver@schinagl.nl> > + * > + * 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 chunks > + * of 8 bytes. 16 bytes or 8 bits? because 8 bytes != 128 bits. > + */ > + > +#include <linux/compiler.h> > +#include <linux/device.h> > +#include <linux/errno.h> > +#include <linux/export.h> > +#include <linux/fs.h> > +#include <linux/init.h> > +#include <linux/io.h> > +#include <linux/kobject.h> > +#include <linux/module.h> > +#include <linux/of_address.h> > +#include <linux/platform_device.h> > +#include <linux/stat.h> > +#include <linux/sysfs.h> > +#include <linux/types.h> > + > + > +#define DRV_NAME "sunxi-sid" > +#define DRV_VERSION "1.0" > + > +/* Register offsets */ > +#define SUNXI_SID_KEY0 0x00 > +#define SUNXI_SID_KEY1 0x04 > +#define SUNXI_SID_KEY2 0x08 > +#define SUNXI_SID_KEY3 0x0c > + > +/* There are 4 32-bit keys */ > +#define SUNXI_SID_KEYS 4 > +/* and 4 32-bit keys per 32-bit key */ The comment is wrong here. > +#define SUNXI_SID_SIZE (SUNXI_SID_KEYS * 4) > + > +#if (SUNXI_SID_SIZE > PAGE_SIZE) > +#error "SUNXI_SID_SIZE is larger then the target's PAGE_SIZE, ENOMEM." > +#endif Hmmmm, I don't follow you here, what's the relation between your driver and PAGE_SIZE? > + > +static u8 keys_lut[] = { > + SUNXI_SID_KEY0, > + SUNXI_SID_KEY1, > + SUNXI_SID_KEY2, > + SUNXI_SID_KEY3, > +}; > + > +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? struct file has a private_data field, use it. > + > + > +/* We read the entire key, using a look up table. Returned is only the > + * requested byte. This is of course slower then it could be and uses 4 times > + * more reads as needed but keeps code a little simpler. > + */ > +u8 sunxi_sid_read_byte(const int key) > +{ > + u32 sid_key; > + u8 ret; > + > + ret = 0; > + if (likely((key <= SUNXI_SID_SIZE))) { > + sid_key = ioread32(p->sid_base + keys_lut[key >> 2]); > + switch (key % 4) { > + case 0: > + ret = (sid_key >> 24) & 0xff; > + break; > + case 1: > + ret = (sid_key >> 16) & 0xff; > + break; > + case 2: > + ret = (sid_key >> 8) & 0xff; > + break; > + case 3: > + ret = sid_key & 0xff; > + break; > + } > + } Come on, you can do better. This lookup table is useless. Also, why the first key is the one with the MSBs? I'd expect that the key 0 is the one holding the LSBs. > + > + 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; > + struct device *dev; > + struct sid_priv *priv; > + int i; > + > + ret = -EPERM; > + dev = kobj_to_dev(kobj); > + priv = dev_get_drvdata(dev); > + > + if ((likely(size > 0)) && ((size + pos) <= SUNXI_SID_SIZE)) { > + for (i = 0; i < size; i++) { > + buf[i] = sunxi_sid_read_byte(pos + i); > + } > + if (i < PAGE_SIZE) { > + buf[i] = '\0'; > + ret = (ssize_t)size; > + } else { > + ret = -ENOMEM; > + } Hmmmm, what? Why returning \0 here? It's not a string, it's binary data. What's the relation with PAGE_SIZE again? Just return the number of bytes read, that's it. > + } else { > + buf[0] = '\0'; > + ret = 0; > + } > + > + return ret; > +} > + > +static struct of_device_id sid_of_match[] = { > + { > + .compatible = "allwinner,sun4i-sid", > + }, > + {/* sentinel */} > +}; > +MODULE_DEVICE_TABLE(of, sid_of_match); > + > +static struct bin_attribute sid_bin_attr = { > + .attr = { > + .name = "key", > + .mode = S_IRUGO, > + }, > + .size = SUNXI_SID_SIZE, > + .read = sid_read, > +}; > + > +static int sid_remove(struct platform_device *pdev) > +{ > + struct device *dev = &pdev->dev; > + struct sid_priv *priv; > + > + priv = dev_get_drvdata(dev); > + device_remove_bin_file(dev, &sid_bin_attr); > + iounmap(priv->sid_base); > + devm_kfree(dev, priv); > + return 0; > +} > + > +static int __init sid_probe(struct platform_device *pdev) > +{ > + int ret; > + struct device *dev = &pdev->dev; > + struct sid_priv *priv; > + > + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL); > + p = priv; > + > + dev_set_drvdata(dev, priv); > + > + if (!priv) { > + dev_err(dev, "Unable to allocate device private data\n"); > + ret = -ENOMEM; > + goto exit; > + } Isn't it a bit weird to check for the memory allocation after using the variable. Also, you don't really need the dev_err, since if the kernel fails to allocate some memory, it will tell you anyway. > + priv->sid_base = of_iomap(dev->of_node, 0); > + if (!priv->sid_base) { > + dev_err(dev, "Unable to map memory region\n"); > + ret = -ENOMEM; > + goto exit_free; > + } > + > + ret = device_create_bin_file(dev, &sid_bin_attr); > + if (ret) { > + dev_err(dev, "Unable to create sysfs bin entry\n"); > + goto exit_unmap; > + } Hmmm, maybe it's not worth all these gotos just for an iounmap, I'd probably return right away, but that's your call. > + dev_info(dev, "Sunxi security ID driver loaded successfully.\n"); > + > + return 0; > + > + > +exit_unmap: > + iounmap(priv->sid_base); > +exit_free: > + devm_kfree(dev, priv); > +exit: > + return ret; > +} > + > +static struct platform_driver sid_driver = { > + .probe = sid_probe, > + .remove = sid_remove, > + .driver = { > + .name = DRV_NAME, > + .owner = THIS_MODULE, > + .of_match_table = sid_of_match, > + }, > +}; > +module_platform_driver(sid_driver); > + > + > +MODULE_AUTHOR("Oliver Schinagl <oliver@schinagl.nl>"); > +MODULE_DESCRIPTION("Allwinner sunxi security id driver"); > +MODULE_VERSION(DRV_VERSION); > +MODULE_LICENSE("GPL"); > Thanks for this driver! Maxime
On 05/17/13 23:18, Maxime Ripard wrote: > Hi Oliver, > > Le 17/05/2013 15:35, Oliver Schinagl a écrit : >> From: Oliver Schinagl <oliver@schinagl.nl> >> >> Allwinner has electric fuses (efuse) on their line of chips. This driver >> reads those fuses and exports them as a sysfs node. Also a symbol is exported >> for in-kernel useage. >> >> While initially these fuses are used to somewhat determin the chipID, these >> appear to be writeable by the user and thus can be used for other purpouses. >> For example storing a 128 bit root key, a unique serial number, which could >> then even be used as a MAC address. >> >> Because writing to e-fuses can be potentially dangerous, and are certainly >> not as often writable (if at all) as flash memory, these shouldn't be easily >> changeable, hence only a read-only mode. An offline tool to write the fuses >> is in the works. >> >> Currently supported are the following known chips: >> Allwinner sun4i (A10) >> Allwinner sun5i (A10s A13) >> Allwinner sun6i (A31, A31s) >> Allwinner sun7i (A20) > > Since I don't think those patches have been tested on sun6i/sun7i, and > that there's not even kernel support for those, maybe it's not worth > mentionning them? A31 is out in the wild, but haven't seen that functionality in, I have seen the register named and defined, just not used, so that could go until confirmed. A20 has the same feature as we can see in the dumped sources. [0] > >> >> Signed-off-by: Oliver Schinagl <oliver@schinagl.nl> >> --- >> drivers/misc/eeprom/Kconfig | 19 ++++ >> drivers/misc/eeprom/Makefile | 1 + >> drivers/misc/eeprom/sunxi_sid.c | 218 ++++++++++++++++++++++++++++++++++++++++ >> 3 files changed, 238 insertions(+) >> create mode 100644 drivers/misc/eeprom/sunxi_sid.c >> >> diff --git a/drivers/misc/eeprom/Kconfig b/drivers/misc/eeprom/Kconfig >> index 04f2e1f..c9ddda5 100644 >> --- a/drivers/misc/eeprom/Kconfig >> +++ b/drivers/misc/eeprom/Kconfig >> @@ -96,4 +96,23 @@ 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 (A10s, A12, A13) >> + sun6i (A31) >> + sun7i (A20) > > Same things here. > >> + >> + 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..953f137 >> --- /dev/null >> +++ b/drivers/misc/eeprom/sunxi_sid.c >> @@ -0,0 +1,218 @@ >> +/* >> + * Copyright (c) 2013 Oliver Schinagl >> + * http://www.linux-sunxi.org >> + * >> + * Oliver Schinagl <oliver@schinagl.nl> >> + * >> + * 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 chunks >> + * of 8 bytes. > > 16 bytes or 8 bits? because 8 bytes != 128 bits. fixed, it was late when I wrote that :( It is indeed 8 bits, e.g. byte sized chunks. > >> + */ >> + >> +#include <linux/compiler.h> >> +#include <linux/device.h> >> +#include <linux/errno.h> >> +#include <linux/export.h> >> +#include <linux/fs.h> >> +#include <linux/init.h> >> +#include <linux/io.h> >> +#include <linux/kobject.h> >> +#include <linux/module.h> >> +#include <linux/of_address.h> >> +#include <linux/platform_device.h> >> +#include <linux/stat.h> >> +#include <linux/sysfs.h> >> +#include <linux/types.h> >> + >> + >> +#define DRV_NAME "sunxi-sid" >> +#define DRV_VERSION "1.0" >> + >> +/* Register offsets */ >> +#define SUNXI_SID_KEY0 0x00 >> +#define SUNXI_SID_KEY1 0x04 >> +#define SUNXI_SID_KEY2 0x08 >> +#define SUNXI_SID_KEY3 0x0c >> + >> +/* There are 4 32-bit keys */ >> +#define SUNXI_SID_KEYS 4 >> +/* and 4 32-bit keys per 32-bit key */ > > The comment is wrong here. Same as above, corrected. > >> +#define SUNXI_SID_SIZE (SUNXI_SID_KEYS * 4) >> + >> +#if (SUNXI_SID_SIZE > PAGE_SIZE) >> +#error "SUNXI_SID_SIZE is larger then the target's PAGE_SIZE, ENOMEM." >> +#endif > > Hmmmm, I don't follow you here, what's the relation between your driver > and PAGE_SIZE? Nothing, I thought there was, but isn't. removed. > >> + >> +static u8 keys_lut[] = { >> + SUNXI_SID_KEY0, >> + SUNXI_SID_KEY1, >> + SUNXI_SID_KEY2, >> + SUNXI_SID_KEY3, >> +}; >> + >> +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? Forgot the static keyword, and the struct kept on shrinking until only the base address was left. I guess memory wise it shouldn't make a difference, but the compiler also doesn't agree, so gone it is :) > > struct file has a private_data field, use it. But since we don't 'open' it, we can't use that, as we talked on IRC. > >> + >> + >> +/* We read the entire key, using a look up table. Returned is only the >> + * requested byte. This is of course slower then it could be and uses 4 times >> + * more reads as needed but keeps code a little simpler. >> + */ >> +u8 sunxi_sid_read_byte(const int key) >> +{ >> + u32 sid_key; >> + u8 ret; >> + >> + ret = 0; >> + if (likely((key <= SUNXI_SID_SIZE))) { >> + sid_key = ioread32(p->sid_base + keys_lut[key >> 2]); >> + switch (key % 4) { >> + case 0: >> + ret = (sid_key >> 24) & 0xff; >> + break; >> + case 1: >> + ret = (sid_key >> 16) & 0xff; >> + break; >> + case 2: >> + ret = (sid_key >> 8) & 0xff; >> + break; >> + case 3: >> + ret = sid_key & 0xff; >> + break; >> + } >> + } > > Come on, you can do better. This lookup table is useless. I didn't want to depend on the fixed layout of memory, but consider it removed. > > Also, why the first key is the one with the MSBs? > I'd expect that the key 0 is the one holding the LSBs. Strangely enough, they have swapped the MSB and LSB bytes. I double checked it with u-boot and yep, swapped. Though in the end, if we write stuff there and we read stuff from there, order doesn't matter? So what do we prefer. Have it so that it makes sense and ignore how u-boot reads it, or correct it and be consistent? > >> + >> + 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; >> + struct device *dev; >> + struct sid_priv *priv; >> + int i; >> + >> + ret = -EPERM; >> + dev = kobj_to_dev(kobj); >> + priv = dev_get_drvdata(dev); >> + >> + if ((likely(size > 0)) && ((size + pos) <= SUNXI_SID_SIZE)) { >> + for (i = 0; i < size; i++) { >> + buf[i] = sunxi_sid_read_byte(pos + i); >> + } >> + if (i < PAGE_SIZE) { >> + buf[i] = '\0'; >> + ret = (ssize_t)size; >> + } else { >> + ret = -ENOMEM; >> + } > > Hmmmm, what? Why returning \0 here? It's not a string, it's binary data. > What's the relation with PAGE_SIZE again? I thought that buf is at most PAGE_SIZE large [1] "sysfs allocates a buffer of size (PAGE_SIZE) and passes it to the method." > > Just return the number of bytes read, that's it. Yes, had forgotten that this was of course the binary read function and not the text version. So yeah, in the binary case, gone. > >> + } else { >> + buf[0] = '\0'; >> + ret = 0; >> + } >> + >> + return ret; >> +} >> + >> +static struct of_device_id sid_of_match[] = { >> + { >> + .compatible = "allwinner,sun4i-sid", >> + }, >> + {/* sentinel */} >> +}; >> +MODULE_DEVICE_TABLE(of, sid_of_match); >> + >> +static struct bin_attribute sid_bin_attr = { >> + .attr = { >> + .name = "key", >> + .mode = S_IRUGO, >> + }, >> + .size = SUNXI_SID_SIZE, >> + .read = sid_read, >> +}; >> + >> +static int sid_remove(struct platform_device *pdev) >> +{ >> + struct device *dev = &pdev->dev; >> + struct sid_priv *priv; >> + >> + priv = dev_get_drvdata(dev); >> + device_remove_bin_file(dev, &sid_bin_attr); >> + iounmap(priv->sid_base); >> + devm_kfree(dev, priv); >> + return 0; >> +} >> + >> +static int __init sid_probe(struct platform_device *pdev) >> +{ >> + int ret; >> + struct device *dev = &pdev->dev; >> + struct sid_priv *priv; >> + >> + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL); >> + p = priv; >> + >> + dev_set_drvdata(dev, priv); >> + >> + if (!priv) { >> + dev_err(dev, "Unable to allocate device private data\n"); >> + ret = -ENOMEM; >> + goto exit; >> + } > > Isn't it a bit weird to check for the memory allocation after using the Yes, more 'oops'. Re-arranged. > variable. Also, you don't really need the dev_err, since if the kernel > fails to allocate some memory, it will tell you anyway. I had it there mostly for consistency, the other 2 did the same check. Initially I had 5 functions here, but you corrected me on that :) > >> + priv->sid_base = of_iomap(dev->of_node, 0); >> + if (!priv->sid_base) { >> + dev_err(dev, "Unable to map memory region\n"); >> + ret = -ENOMEM; >> + goto exit_free; >> + } >> + >> + ret = device_create_bin_file(dev, &sid_bin_attr); >> + if (ret) { >> + dev_err(dev, "Unable to create sysfs bin entry\n"); >> + goto exit_unmap; >> + } > > Hmmm, maybe it's not worth all these gotos just for an iounmap, I'd > probably return right away, but that's your call. I saw a lot of drivers do the goto: dance in init/probe/exit/remove functions, I don't think the overhead here would be so bad? > >> + dev_info(dev, "Sunxi security ID driver loaded successfully.\n"); >> + >> + return 0; >> + >> + >> +exit_unmap: >> + iounmap(priv->sid_base); >> +exit_free: >> + devm_kfree(dev, priv); >> +exit: >> + return ret; >> +} >> + >> +static struct platform_driver sid_driver = { >> + .probe = sid_probe, >> + .remove = sid_remove, >> + .driver = { >> + .name = DRV_NAME, >> + .owner = THIS_MODULE, >> + .of_match_table = sid_of_match, >> + }, >> +}; >> +module_platform_driver(sid_driver); >> + >> + >> +MODULE_AUTHOR("Oliver Schinagl <oliver@schinagl.nl>"); >> +MODULE_DESCRIPTION("Allwinner sunxi security id driver"); >> +MODULE_VERSION(DRV_VERSION); >> +MODULE_LICENSE("GPL"); >> > > Thanks for this driver! > Maxime > You are welcome! :D While awaiting more feedback, i've pushed the current version to my github [2]. Oliver [0] https://github.com/amery/linux-allwinner/blob/lichee-3.3/sun7i-dev/arch/arm/mach-sun7i/security_id.c#L91 [1] https://www.kernel.org/doc/Documentation/filesystems/sysfs.txt [2] https://github.com/oliv3r/linux/blob/wip/sunxi-security-id/drivers/misc/eeprom/sunxi_sid.c
On Sat, May 18, 2013 at 07:19:43PM +0200, Oliver Schinagl wrote: > On 05/17/13 23:18, Maxime Ripard wrote: > >Hi Oliver, > > > >Le 17/05/2013 15:35, Oliver Schinagl a écrit : > >>From: Oliver Schinagl <oliver@schinagl.nl> > >> > >>Allwinner has electric fuses (efuse) on their line of chips. This driver > >>reads those fuses and exports them as a sysfs node. Also a symbol is exported > >>for in-kernel useage. > >> > >>While initially these fuses are used to somewhat determin the chipID, these > >>appear to be writeable by the user and thus can be used for other purpouses. > >>For example storing a 128 bit root key, a unique serial number, which could > >>then even be used as a MAC address. > >> > >>Because writing to e-fuses can be potentially dangerous, and are certainly > >>not as often writable (if at all) as flash memory, these shouldn't be easily > >>changeable, hence only a read-only mode. An offline tool to write the fuses > >>is in the works. > >> > >>Currently supported are the following known chips: > >>Allwinner sun4i (A10) > >>Allwinner sun5i (A10s A13) > >>Allwinner sun6i (A31, A31s) > >>Allwinner sun7i (A20) > > > >Since I don't think those patches have been tested on sun6i/sun7i, and > >that there's not even kernel support for those, maybe it's not worth > >mentionning them? > A31 is out in the wild, but haven't seen that functionality in, I > have seen the register named and defined, just not used, so that > could go until confirmed. > > A20 has the same feature as we can see in the dumped sources. [0] Yet, we can't even boot a mainline kernel, and this list will probably need some update when new SoCs come out. Anyway, it's not a big deal. > > > > > >> > >>Signed-off-by: Oliver Schinagl <oliver@schinagl.nl> > >>--- > >> drivers/misc/eeprom/Kconfig | 19 ++++ > >> drivers/misc/eeprom/Makefile | 1 + > >> drivers/misc/eeprom/sunxi_sid.c | 218 ++++++++++++++++++++++++++++++++++++++++ > >> 3 files changed, 238 insertions(+) > >> create mode 100644 drivers/misc/eeprom/sunxi_sid.c > >> > >>diff --git a/drivers/misc/eeprom/Kconfig b/drivers/misc/eeprom/Kconfig > >>index 04f2e1f..c9ddda5 100644 > >>--- a/drivers/misc/eeprom/Kconfig > >>+++ b/drivers/misc/eeprom/Kconfig > >>@@ -96,4 +96,23 @@ 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 (A10s, A12, A13) > >>+ sun6i (A31) > >>+ sun7i (A20) > > > >Same things here. > > > >>+ > >>+ 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..953f137 > >>--- /dev/null > >>+++ b/drivers/misc/eeprom/sunxi_sid.c > >>@@ -0,0 +1,218 @@ > >>+/* > >>+ * Copyright (c) 2013 Oliver Schinagl > >>+ * http://www.linux-sunxi.org > >>+ * > >>+ * Oliver Schinagl <oliver@schinagl.nl> > >>+ * > >>+ * 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 chunks > >>+ * of 8 bytes. > > > >16 bytes or 8 bits? because 8 bytes != 128 bits. > fixed, it was late when I wrote that :( It is indeed 8 bits, e.g. > byte sized chunks. > > > > >>+ */ > >>+ > >>+#include <linux/compiler.h> > >>+#include <linux/device.h> > >>+#include <linux/errno.h> > >>+#include <linux/export.h> > >>+#include <linux/fs.h> > >>+#include <linux/init.h> > >>+#include <linux/io.h> > >>+#include <linux/kobject.h> > >>+#include <linux/module.h> > >>+#include <linux/of_address.h> > >>+#include <linux/platform_device.h> > >>+#include <linux/stat.h> > >>+#include <linux/sysfs.h> > >>+#include <linux/types.h> > >>+ > >>+ > >>+#define DRV_NAME "sunxi-sid" > >>+#define DRV_VERSION "1.0" > >>+ > >>+/* Register offsets */ > >>+#define SUNXI_SID_KEY0 0x00 > >>+#define SUNXI_SID_KEY1 0x04 > >>+#define SUNXI_SID_KEY2 0x08 > >>+#define SUNXI_SID_KEY3 0x0c > >>+ > >>+/* There are 4 32-bit keys */ > >>+#define SUNXI_SID_KEYS 4 > >>+/* and 4 32-bit keys per 32-bit key */ > > > >The comment is wrong here. > Same as above, corrected. > > > > >>+#define SUNXI_SID_SIZE (SUNXI_SID_KEYS * 4) > >>+ > >>+#if (SUNXI_SID_SIZE > PAGE_SIZE) > >>+#error "SUNXI_SID_SIZE is larger then the target's PAGE_SIZE, ENOMEM." > >>+#endif > > > >Hmmmm, I don't follow you here, what's the relation between your driver > >and PAGE_SIZE? > Nothing, I thought there was, but isn't. removed. > > > >>+ > >>+static u8 keys_lut[] = { > >>+ SUNXI_SID_KEY0, > >>+ SUNXI_SID_KEY1, > >>+ SUNXI_SID_KEY2, > >>+ SUNXI_SID_KEY3, > >>+}; > >>+ > >>+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? > Forgot the static keyword, and the struct kept on shrinking until > only the base address was left. I guess memory wise it shouldn't > make a difference, but the compiler also doesn't agree, so gone it > is :) > > > >struct file has a private_data field, use it. > But since we don't 'open' it, we can't use that, as we talked on IRC. Then, use the passed kobject to retrieve its parent device structure, and from there, get the drvdata to get back on your feet. Actually, you already seem to do that and I overlooked that part in my previous review. So just use the priv structure you have defined in your read function and that is useless right now. > > > >>+ > >>+ > >>+/* We read the entire key, using a look up table. Returned is only the > >>+ * requested byte. This is of course slower then it could be and uses 4 times > >>+ * more reads as needed but keeps code a little simpler. > >>+ */ > >>+u8 sunxi_sid_read_byte(const int key) > >>+{ > >>+ u32 sid_key; > >>+ u8 ret; > >>+ > >>+ ret = 0; > >>+ if (likely((key <= SUNXI_SID_SIZE))) { > >>+ sid_key = ioread32(p->sid_base + keys_lut[key >> 2]); > >>+ switch (key % 4) { > >>+ case 0: > >>+ ret = (sid_key >> 24) & 0xff; > >>+ break; > >>+ case 1: > >>+ ret = (sid_key >> 16) & 0xff; > >>+ break; > >>+ case 2: > >>+ ret = (sid_key >> 8) & 0xff; > >>+ break; > >>+ case 3: > >>+ ret = sid_key & 0xff; > >>+ break; > >>+ } > >>+ } > > > >Come on, you can do better. This lookup table is useless. > I didn't want to depend on the fixed layout of memory, but consider > it removed. > > > >Also, why the first key is the one with the MSBs? > >I'd expect that the key 0 is the one holding the LSBs. > Strangely enough, they have swapped the MSB and LSB bytes. I double > checked it with u-boot and yep, swapped. Though in the end, if we > write stuff there and we read stuff from there, order doesn't > matter? So what do we prefer. Have it so that it makes sense and > ignore how u-boot reads it, or correct it and be consistent? Which u-boot are you talking about? I'd really prefer to have a sane and logical behaviour rather than a broken one, even though it used to be like that in the Allwinner-provided kernel. > > > >>+ > >>+ 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; > >>+ struct device *dev; > >>+ struct sid_priv *priv; > >>+ int i; > >>+ > >>+ ret = -EPERM; > >>+ dev = kobj_to_dev(kobj); > >>+ priv = dev_get_drvdata(dev); > >>+ > >>+ if ((likely(size > 0)) && ((size + pos) <= SUNXI_SID_SIZE)) { > >>+ for (i = 0; i < size; i++) { > >>+ buf[i] = sunxi_sid_read_byte(pos + i); > >>+ } > >>+ if (i < PAGE_SIZE) { > >>+ buf[i] = '\0'; > >>+ ret = (ssize_t)size; > >>+ } else { > >>+ ret = -ENOMEM; > >>+ } > > > >Hmmmm, what? Why returning \0 here? It's not a string, it's binary data. > >What's the relation with PAGE_SIZE again? > I thought that buf is at most PAGE_SIZE large [1] "sysfs allocates a > buffer of size (PAGE_SIZE) and passes it to the method." And you already check the size passed. > > > >Just return the number of bytes read, that's it. > Yes, had forgotten that this was of course the binary read function > and not the text version. So yeah, in the binary case, gone. > > > >>+ } else { > >>+ buf[0] = '\0'; > >>+ ret = 0; > >>+ } > >>+ > >>+ return ret; > >>+} > >>+ > >>+static struct of_device_id sid_of_match[] = { > >>+ { > >>+ .compatible = "allwinner,sun4i-sid", > >>+ }, > >>+ {/* sentinel */} > >>+}; > >>+MODULE_DEVICE_TABLE(of, sid_of_match); > >>+ > >>+static struct bin_attribute sid_bin_attr = { > >>+ .attr = { > >>+ .name = "key", > >>+ .mode = S_IRUGO, > >>+ }, > >>+ .size = SUNXI_SID_SIZE, > >>+ .read = sid_read, > >>+}; > >>+ > >>+static int sid_remove(struct platform_device *pdev) > >>+{ > >>+ struct device *dev = &pdev->dev; > >>+ struct sid_priv *priv; > >>+ > >>+ priv = dev_get_drvdata(dev); > >>+ device_remove_bin_file(dev, &sid_bin_attr); > >>+ iounmap(priv->sid_base); > >>+ devm_kfree(dev, priv); > >>+ return 0; > >>+} > >>+ > >>+static int __init sid_probe(struct platform_device *pdev) > >>+{ > >>+ int ret; > >>+ struct device *dev = &pdev->dev; > >>+ struct sid_priv *priv; > >>+ > >>+ priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL); > >>+ p = priv; > >>+ > >>+ dev_set_drvdata(dev, priv); > >>+ > >>+ if (!priv) { > >>+ dev_err(dev, "Unable to allocate device private data\n"); > >>+ ret = -ENOMEM; > >>+ goto exit; > >>+ } > > > >Isn't it a bit weird to check for the memory allocation after using the > Yes, more 'oops'. Re-arranged. > >variable. Also, you don't really need the dev_err, since if the kernel > >fails to allocate some memory, it will tell you anyway. > I had it there mostly for consistency, the other 2 did the same > check. Initially I had 5 functions here, but you corrected me on > that :) > > > > >>+ priv->sid_base = of_iomap(dev->of_node, 0); > >>+ if (!priv->sid_base) { > >>+ dev_err(dev, "Unable to map memory region\n"); > >>+ ret = -ENOMEM; > >>+ goto exit_free; > >>+ } > >>+ > >>+ ret = device_create_bin_file(dev, &sid_bin_attr); > >>+ if (ret) { > >>+ dev_err(dev, "Unable to create sysfs bin entry\n"); > >>+ goto exit_unmap; > >>+ } > > > >Hmmm, maybe it's not worth all these gotos just for an iounmap, I'd > >probably return right away, but that's your call. > I saw a lot of drivers do the goto: dance in init/probe/exit/remove > functions, I don't think the overhead here would be so bad? It's not really that it's bad, but using return directly would take less code overall, but that's not a big deal, if you prefer to keep it that way, I'm fine with it. > > > >>+ dev_info(dev, "Sunxi security ID driver loaded successfully.\n"); > >>+ > >>+ return 0; > >>+ > >>+ > >>+exit_unmap: > >>+ iounmap(priv->sid_base); > >>+exit_free: > >>+ devm_kfree(dev, priv); > >>+exit: > >>+ return ret; > >>+} > >>+ > >>+static struct platform_driver sid_driver = { > >>+ .probe = sid_probe, > >>+ .remove = sid_remove, > >>+ .driver = { > >>+ .name = DRV_NAME, > >>+ .owner = THIS_MODULE, > >>+ .of_match_table = sid_of_match, > >>+ }, > >>+}; > >>+module_platform_driver(sid_driver); > >>+ > >>+ > >>+MODULE_AUTHOR("Oliver Schinagl <oliver@schinagl.nl>"); > >>+MODULE_DESCRIPTION("Allwinner sunxi security id driver"); > >>+MODULE_VERSION(DRV_VERSION); > >>+MODULE_LICENSE("GPL"); > >> > > > >Thanks for this driver! > >Maxime > > > You are welcome! :D While awaiting more feedback, i've pushed the > current version to my github [2]. > > Oliver > > [0] https://github.com/amery/linux-allwinner/blob/lichee-3.3/sun7i-dev/arch/arm/mach-sun7i/security_id.c#L91 > [1] https://www.kernel.org/doc/Documentation/filesystems/sysfs.txt > [2] https://github.com/oliv3r/linux/blob/wip/sunxi-security-id/drivers/misc/eeprom/sunxi_sid.c
On Fri, May 17, 2013 at 3:35 PM, Oliver Schinagl <oliver+list@schinagl.nl> wrote: (...) > While initially these fuses are used to somewhat determin the chipID, these > appear to be writeable by the user and thus can be used for other purpouses. > For example storing a 128 bit root key, a unique serial number, which could > then even be used as a MAC address. (...) Then follows some code to read out the keys from sysfs I guess.. > +static int __init sid_probe(struct platform_device *pdev) It's really simple to actually make the kernel use this to seed the entropy pool. #include <linux/random.h> add_device_randomness(u8 *, num); If you know after probe that you can read out a number of bytes of device-unique data, I think you should add those bytes to the entropy pool like this. Yours, Linus Walleij
On 05/23/13 09:56, Linus Walleij wrote: > On Fri, May 17, 2013 at 3:35 PM, Oliver Schinagl > <oliver+list@schinagl.nl> wrote: > > (...) >> While initially these fuses are used to somewhat determin the chipID, these >> appear to be writeable by the user and thus can be used for other purpouses. >> For example storing a 128 bit root key, a unique serial number, which could >> then even be used as a MAC address. > (...) > Then follows some code to read out the keys from sysfs I guess.. >> +static int __init sid_probe(struct platform_device *pdev) > > It's really simple to actually make the kernel use this to seed the > entropy pool. > > #include <linux/random.h> > add_device_randomness(u8 *, num); > > If you know after probe that you can read out a number of bytes > of device-unique data, I think you should add those bytes to the > entropy pool like this. While that is a great idea, we can't guarantee device uniqueness. We've already seen some chips that where 'forgotten' to program and default set to all 0. I guess that doesn't have to be a bad thing. Then, i'm not sure if the driver is the best for this to be loaded? Maxime, what do you think? Personally I would feel more in having this in the mach-sunxi/core.c bit, but then again, this is currently a module and wouldn't be useful to have there. Maxime is far more knowledgeable to answer that. It should probably be noted, that the sunxi series have a hardware crypto engine, with hardware random seed generator, one for a later project. > > Yours, > Linus Walleij >
On Thu, May 23, 2013 at 10:10 AM, Oliver Schinagl <oliver+list@schinagl.nl> wrote: > On 05/23/13 09:56, Linus Walleij wrote: >> >> On Fri, May 17, 2013 at 3:35 PM, Oliver Schinagl >> <oliver+list@schinagl.nl> wrote: >> >> (...) >>> >>> While initially these fuses are used to somewhat determin the chipID, >>> these >>> appear to be writeable by the user and thus can be used for other >>> purpouses. >>> For example storing a 128 bit root key, a unique serial number, which >>> could >>> then even be used as a MAC address. >> >> (...) >> Then follows some code to read out the keys from sysfs I guess.. >>> >>> +static int __init sid_probe(struct platform_device *pdev) >> >> >> It's really simple to actually make the kernel use this to seed the >> entropy pool. >> >> #include <linux/random.h> >> add_device_randomness(u8 *, num); >> >> If you know after probe that you can read out a number of bytes >> of device-unique data, I think you should add those bytes to the >> entropy pool like this. > > While that is a great idea, we can't guarantee device uniqueness. We've > already seen some chips that where 'forgotten' to program and default set to > all 0. I guess that doesn't have to be a bad thing. Ted can confirm but AFAIK that is not a problem. This device-unique numer is just one of the things mixed into the pool, if it's on some devices just an array of zeroes it does not make things worse, but in the cases when there is some uniqueness in it make things better. > It should probably be noted, that the sunxi series have a hardware crypto > engine, with hardware random seed generator, one for a later project. That will anyway be augmented with the contents of the entropy pool rather than returned to random clients right off if I know the recent changes to random code right. Yours, Linus Walleij
On Thu, May 23, 2013 at 10:10:17AM +0200, Oliver Schinagl wrote: > Then, i'm not sure if the driver is the best for this to be loaded? > Maxime, what do you think? Personally I would feel more in having > this in the mach-sunxi/core.c bit, but then again, this is currently > a module and wouldn't be useful to have there. Maxime is far more > knowledgeable to answer that. Hmmm, I don't understand what you mean here. Could you explain what you have in mind? Thanks, Maxime
On 05/23/13 16:58, Maxime Ripard wrote: > On Thu, May 23, 2013 at 10:10:17AM +0200, Oliver Schinagl wrote: >> Then, i'm not sure if the driver is the best for this to be loaded? >> Maxime, what do you think? Personally I would feel more in having >> this in the mach-sunxi/core.c bit, but then again, this is currently >> a module and wouldn't be useful to have there. Maxime is far more >> knowledgeable to answer that. > > Hmmm, I don't understand what you mean here. Could you explain what you > have in mind? I've thought about it a little, and don't think core.c is a good spot, since the module will have to be loaded, or available there. And that's really early. So I guess, during probe, controlled by a parameter perhaps, load all 16 bytes into the random pool as Linus suggested? > > Thanks, > Maxime >
On Thu, May 23, 2013 at 05:05:21PM +0200, Oliver Schinagl wrote: > On 05/23/13 16:58, Maxime Ripard wrote: > >On Thu, May 23, 2013 at 10:10:17AM +0200, Oliver Schinagl wrote: > >>Then, i'm not sure if the driver is the best for this to be loaded? > >>Maxime, what do you think? Personally I would feel more in having > >>this in the mach-sunxi/core.c bit, but then again, this is currently > >>a module and wouldn't be useful to have there. Maxime is far more > >>knowledgeable to answer that. > > > >Hmmm, I don't understand what you mean here. Could you explain what you > >have in mind? > I've thought about it a little, and don't think core.c is a good > spot, since the module will have to be loaded, or available there. > And that's really early. > > So I guess, during probe, controlled by a parameter perhaps, load > all 16 bytes into the random pool as Linus suggested? Yeah, though I don't really know what the parameter would be useful for, but yes, do it in the driver's probe.
On 05/18/13 19:19, Oliver Schinagl wrote: <snip> >>> + >>> + >>> +/* We read the entire key, using a look up table. Returned is only the >>> + * requested byte. This is of course slower then it could be and uses 4 times >>> + * more reads as needed but keeps code a little simpler. >>> + */ >>> +u8 sunxi_sid_read_byte(const int key) >>> +{ >>> + u32 sid_key; >>> + u8 ret; >>> + >>> + ret = 0; >>> + if (likely((key <= SUNXI_SID_SIZE))) { >>> + sid_key = ioread32(p->sid_base + keys_lut[key >> 2]); >>> + switch (key % 4) { >>> + case 0: >>> + ret = (sid_key >> 24) & 0xff; >>> + break; >>> + case 1: >>> + ret = (sid_key >> 16) & 0xff; >>> + break; >>> + case 2: >>> + ret = (sid_key >> 8) & 0xff; >>> + break; >>> + case 3: >>> + ret = sid_key & 0xff; >>> + break; >>> + } >>> + } >> >> Come on, you can do better. This lookup table is useless. > I didn't want to depend on the fixed layout of memory, but consider it > removed. But i'm not smart enough :p We can either use the look up table (which does have benefits as its potentially more future proof), or do some ((key >> 2) << 2) to 'drop' the LSB's that we want to ignore (unless there's some smarter way). Personally, I think the LUT is a little cleaner and more readable, but I guess if you look at poor efficiency, the lut costs some memory, the left/right shift cost an additional >> 2 ... what you prefer. >> >> Also, why the first key is the one with the MSBs? >> I'd expect that the key 0 is the one holding the LSBs. > Strangely enough, they have swapped the MSB and LSB bytes. I double > checked it with u-boot and yep, swapped. Though in the end, if we write > stuff there and we read stuff from there, order doesn't matter? So what > do we prefer. Have it so that it makes sense and ignore how u-boot reads > it, or correct it and be consistent? > You had me confused and I was looking at this for a little while. Bit-ordering does not change, Byte endianness is a different story of course. As it is now, I decided to use Big endianess. So now a 32bit key looks like: 0x162367c7 and if we read one byte at a time, we get 0x16, 0x23, 0x67 and 0xc7. I made a comment that data is read as Big endian. If it is important, for eeprom data, to be stored little endian, I'll obviously change it per request.
Hi Oliver, On Fri, May 24, 2013 at 11:50:38PM +0200, Oliver Schinagl wrote: > On 05/18/13 19:19, Oliver Schinagl wrote: > <snip> > >>>+ > >>>+ > >>>+/* We read the entire key, using a look up table. Returned is only the > >>>+ * requested byte. This is of course slower then it could be and uses 4 times > >>>+ * more reads as needed but keeps code a little simpler. > >>>+ */ > >>>+u8 sunxi_sid_read_byte(const int key) > >>>+{ > >>>+ u32 sid_key; > >>>+ u8 ret; > >>>+ > >>>+ ret = 0; > >>>+ if (likely((key <= SUNXI_SID_SIZE))) { > >>>+ sid_key = ioread32(p->sid_base + keys_lut[key >> 2]); > >>>+ switch (key % 4) { > >>>+ case 0: > >>>+ ret = (sid_key >> 24) & 0xff; > >>>+ break; > >>>+ case 1: > >>>+ ret = (sid_key >> 16) & 0xff; > >>>+ break; > >>>+ case 2: > >>>+ ret = (sid_key >> 8) & 0xff; > >>>+ break; > >>>+ case 3: > >>>+ ret = sid_key & 0xff; > >>>+ break; > >>>+ } > >>>+ } > >> > >>Come on, you can do better. This lookup table is useless. > >I didn't want to depend on the fixed layout of memory, but consider it > >removed. > But i'm not smart enough :p > > We can either use the look up table (which does have benefits as its > potentially more future proof), or do some ((key >> 2) << 2) to > 'drop' the LSB's that we want to ignore (unless there's some smarter > way). > > Personally, I think the LUT is a little cleaner and more readable, > but I guess if you look at poor efficiency, the lut costs some > memory, the left/right shift cost an additional >> 2 ... what you > prefer. What about: val = ioread32be(base + (key / 4)); val >>= (key % 4) * 8; return val & 0xff; No lookup table, no weird swich statement, and you get the endianness conversion only when you need it. Maxime
On 05/25/13 14:22, Maxime Ripard wrote: > Hi Oliver, > > On Fri, May 24, 2013 at 11:50:38PM +0200, Oliver Schinagl wrote: >> On 05/18/13 19:19, Oliver Schinagl wrote: >> <snip> >>>>> + >>>>> + >>>>> +/* We read the entire key, using a look up table. Returned is only the >>>>> + * requested byte. This is of course slower then it could be and uses 4 times >>>>> + * more reads as needed but keeps code a little simpler. >>>>> + */ >>>>> +u8 sunxi_sid_read_byte(const int key) >>>>> +{ >>>>> + u32 sid_key; >>>>> + u8 ret; >>>>> + >>>>> + ret = 0; >>>>> + if (likely((key <= SUNXI_SID_SIZE))) { >>>>> + sid_key = ioread32(p->sid_base + keys_lut[key >> 2]); >>>>> + switch (key % 4) { >>>>> + case 0: >>>>> + ret = (sid_key >> 24) & 0xff; >>>>> + break; >>>>> + case 1: >>>>> + ret = (sid_key >> 16) & 0xff; >>>>> + break; >>>>> + case 2: >>>>> + ret = (sid_key >> 8) & 0xff; >>>>> + break; >>>>> + case 3: >>>>> + ret = sid_key & 0xff; >>>>> + break; >>>>> + } >>>>> + } >>>> >>>> Come on, you can do better. This lookup table is useless. >>> I didn't want to depend on the fixed layout of memory, but consider it >>> removed. >> But i'm not smart enough :p >> >> We can either use the look up table (which does have benefits as its >> potentially more future proof), or do some ((key >> 2) << 2) to >> 'drop' the LSB's that we want to ignore (unless there's some smarter >> way). >> >> Personally, I think the LUT is a little cleaner and more readable, >> but I guess if you look at poor efficiency, the lut costs some >> memory, the left/right shift cost an additional >> 2 ... what you >> prefer. > > What about: > > val = ioread32be(base + (key / 4)); > val >>= (key % 4) * 8; > return val & 0xff; > > No lookup table, no weird swich statement, and you get the endianness > conversion only when you need it. Ok I think I like the Endianess, ioread32be does the right thing then? I'll read up on that. As for key / 4; how will that work without the lut? Lets take byte 14 (out of the available 16). Byte 14 (0x0e) is located in SID_KEY3, so base + 0x0c. We need to write a whole 32bit word to keep with alignment, the registers go wakko if you do unaligned reads. So we need to read the entire 32 bits, then find our byte. key / 4 for 14 yields 0x03. So we have base + 0x03, which is not what we want. We want base + 0x0c, which is either ((key >> 2) << 2)) Or, ((key / 4) * 4)) which to me, are both equally confusing. So we either use the look up table, which is a little cleaner and is a bit more future proof if either a) there's more 'eeprom like' storage which can be combined herein or b) 'a' next version has non-continuing regions. Granted neither is something to worry about right now. Turl already mentioned the calculated shift, instead of the switch. I agree to also like it better and have already rewritten that bit. If I made a really stupid thinking mistake or my math is somehow wrong, feel free to point it out :) I don't mind manning up to my mistakes :) > > Maxime >
On Sat, May 25, 2013 at 09:25:51PM +0200, Oliver Schinagl wrote: > On 05/25/13 14:22, Maxime Ripard wrote: > >What about: > > > >val = ioread32be(base + (key / 4)); > >val >>= (key % 4) * 8; > >return val & 0xff; > > > >No lookup table, no weird swich statement, and you get the endianness > >conversion only when you need it. > Ok I think I like the Endianess, ioread32be does the right thing > then? I'll read up on that. > As for key / 4; how will that work without the lut? > > Lets take byte 14 (out of the available 16). Byte 14 (0x0e) is > located in SID_KEY3, so base + 0x0c. We need to write a whole 32bit > word to keep with alignment, the registers go wakko if you do > unaligned reads. So we need to read the entire 32 bits, then find > our byte. > > key / 4 for 14 yields 0x03. So we have base + 0x03, which is not > what we want. We want base + 0x0c, which is either ((key >> 2) << > 2)) Or, ((key / 4) * 4)) which to me, are both equally confusing. The statement you make that 3 isn't the index you want depends on your pointer type so it might be what you want, or might not. If it's still not what you want, you can always use round_down(key, 4). > So we either use the look up table, which is a little cleaner and is a > bit more future proof if either a) there's more 'eeprom like' > storage which can be combined herein or b) 'a' next version has > non-continuing regions. Granted neither is something to worry about > right now. I don't see how it's cleaner (you have three indirections to get the value that is actually used) or future proof (you have to extend this lookup table every time you have a slightly larger size). So I'm sorry but this lookup table holding only the index times 4 is a non-sense, could we please stop arguing about this?
diff --git a/drivers/misc/eeprom/Kconfig b/drivers/misc/eeprom/Kconfig index 04f2e1f..c9ddda5 100644 --- a/drivers/misc/eeprom/Kconfig +++ b/drivers/misc/eeprom/Kconfig @@ -96,4 +96,23 @@ 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 (A10s, A12, A13) + sun6i (A31) + sun7i (A20) + + 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..953f137 --- /dev/null +++ b/drivers/misc/eeprom/sunxi_sid.c @@ -0,0 +1,218 @@ +/* + * Copyright (c) 2013 Oliver Schinagl + * http://www.linux-sunxi.org + * + * Oliver Schinagl <oliver@schinagl.nl> + * + * 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 chunks + * of 8 bytes. + */ + +#include <linux/compiler.h> +#include <linux/device.h> +#include <linux/errno.h> +#include <linux/export.h> +#include <linux/fs.h> +#include <linux/init.h> +#include <linux/io.h> +#include <linux/kobject.h> +#include <linux/module.h> +#include <linux/of_address.h> +#include <linux/platform_device.h> +#include <linux/stat.h> +#include <linux/sysfs.h> +#include <linux/types.h> + + +#define DRV_NAME "sunxi-sid" +#define DRV_VERSION "1.0" + +/* Register offsets */ +#define SUNXI_SID_KEY0 0x00 +#define SUNXI_SID_KEY1 0x04 +#define SUNXI_SID_KEY2 0x08 +#define SUNXI_SID_KEY3 0x0c + +/* There are 4 32-bit keys */ +#define SUNXI_SID_KEYS 4 +/* and 4 32-bit keys per 32-bit key */ +#define SUNXI_SID_SIZE (SUNXI_SID_KEYS * 4) + +#if (SUNXI_SID_SIZE > PAGE_SIZE) +#error "SUNXI_SID_SIZE is larger then the target's PAGE_SIZE, ENOMEM." +#endif + +static u8 keys_lut[] = { + SUNXI_SID_KEY0, + SUNXI_SID_KEY1, + SUNXI_SID_KEY2, + SUNXI_SID_KEY3, +}; + +struct sid_priv { + void __iomem *sid_base; +}; + +struct sid_priv *p; + + +/* We read the entire key, using a look up table. Returned is only the + * requested byte. This is of course slower then it could be and uses 4 times + * more reads as needed but keeps code a little simpler. + */ +u8 sunxi_sid_read_byte(const int key) +{ + u32 sid_key; + u8 ret; + + ret = 0; + if (likely((key <= SUNXI_SID_SIZE))) { + sid_key = ioread32(p->sid_base + keys_lut[key >> 2]); + switch (key % 4) { + case 0: + ret = (sid_key >> 24) & 0xff; + break; + case 1: + ret = (sid_key >> 16) & 0xff; + break; + case 2: + ret = (sid_key >> 8) & 0xff; + break; + case 3: + ret = sid_key & 0xff; + break; + } + } + + 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; + struct device *dev; + struct sid_priv *priv; + int i; + + ret = -EPERM; + dev = kobj_to_dev(kobj); + priv = dev_get_drvdata(dev); + + if ((likely(size > 0)) && ((size + pos) <= SUNXI_SID_SIZE)) { + for (i = 0; i < size; i++) { + buf[i] = sunxi_sid_read_byte(pos + i); + } + if (i < PAGE_SIZE) { + buf[i] = '\0'; + ret = (ssize_t)size; + } else { + ret = -ENOMEM; + } + } else { + buf[0] = '\0'; + ret = 0; + } + + return ret; +} + +static struct of_device_id sid_of_match[] = { + { + .compatible = "allwinner,sun4i-sid", + }, + {/* sentinel */} +}; +MODULE_DEVICE_TABLE(of, sid_of_match); + +static struct bin_attribute sid_bin_attr = { + .attr = { + .name = "key", + .mode = S_IRUGO, + }, + .size = SUNXI_SID_SIZE, + .read = sid_read, +}; + +static int sid_remove(struct platform_device *pdev) +{ + struct device *dev = &pdev->dev; + struct sid_priv *priv; + + priv = dev_get_drvdata(dev); + device_remove_bin_file(dev, &sid_bin_attr); + iounmap(priv->sid_base); + devm_kfree(dev, priv); + return 0; +} + +static int __init sid_probe(struct platform_device *pdev) +{ + int ret; + struct device *dev = &pdev->dev; + struct sid_priv *priv; + + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL); + p = priv; + + dev_set_drvdata(dev, priv); + + if (!priv) { + dev_err(dev, "Unable to allocate device private data\n"); + ret = -ENOMEM; + goto exit; + } + + priv->sid_base = of_iomap(dev->of_node, 0); + if (!priv->sid_base) { + dev_err(dev, "Unable to map memory region\n"); + ret = -ENOMEM; + goto exit_free; + } + + ret = device_create_bin_file(dev, &sid_bin_attr); + if (ret) { + dev_err(dev, "Unable to create sysfs bin entry\n"); + goto exit_unmap; + } + + dev_info(dev, "Sunxi security ID driver loaded successfully.\n"); + + return 0; + + +exit_unmap: + iounmap(priv->sid_base); +exit_free: + devm_kfree(dev, priv); +exit: + return ret; +} + +static struct platform_driver sid_driver = { + .probe = sid_probe, + .remove = sid_remove, + .driver = { + .name = DRV_NAME, + .owner = THIS_MODULE, + .of_match_table = sid_of_match, + }, +}; +module_platform_driver(sid_driver); + + +MODULE_AUTHOR("Oliver Schinagl <oliver@schinagl.nl>"); +MODULE_DESCRIPTION("Allwinner sunxi security id driver"); +MODULE_VERSION(DRV_VERSION); +MODULE_LICENSE("GPL");