| Message ID | 1371502778-15849-2-git-send-email-oliver+list@schinagl.nl (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Oliver, On Monday 17 of June 2013 22:59:37 Oliver Schinagl wrote: > From: Oliver Schinagl <oliver@schinagl.nl> > > Allwinner has electric fuses (efuse) on their line of chips. This driver > reads those fuses, seeds the kernel entropy and exports them as a sysfs > node. > > These fuses are most likly to be programmed at the factory, encoding > things like Chip ID, some sort of serial number etc and appear to be > reasonable unique. > While in theory, these should be writeable by the user, it will probably > be inconvinient to do so. Allwinner recommends that a certain input > pin, labeled 'efuse_vddq', be connected to GND. To write these fuses, > 2.5 V needs to be applied to this pin. > > Even so, they can still be used to generate a board-unique mac from, > board unique RSA key and seed the kernel RNG. > > Currently supported are the following known chips: > Allwinner sun4i (A10) > Allwinner sun5i (A10s, A13) > > Signed-off-by: Oliver Schinagl <oliver@schinagl.nl> > --- > drivers/misc/eeprom/Kconfig | 17 +++++ > drivers/misc/eeprom/Makefile | 1 + > drivers/misc/eeprom/sunxi_sid.c | 147 > ++++++++++++++++++++++++++++++++++++++++ 3 files changed, 165 > insertions(+) > create mode 100644 drivers/misc/eeprom/sunxi_sid.c Looks good to me. Have my Reviewed-by: Tomasz Figa <tomasz.figa@gmail.com> Best regards, Tomasz > 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..6a16c19 > --- /dev/null > +++ b/drivers/misc/eeprom/sunxi_sid.c > @@ -0,0 +1,147 @@ > +/* > + * 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 > byte + * sized chunks. > + */ > + > +#include <linux/compiler.h> > +#include <linux/device.h> > +#include <linux/err.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/platform_device.h> > +#include <linux/random.h> > +#include <linux/stat.h> > +#include <linux/sysfs.h> > +#include <linux/types.h> > + > +#define DRV_NAME "sunxi-sid" > + > +/* There are 4 32-bit keys */ > +#define SID_KEYS 4 > +/* Each key is 4 bytes long */ > +#define SID_SIZE (SID_KEYS * 4) > + > +/* We read the entire key, due to a 32 bit read alignment requirement. > Since we + * want to return the requested byte, this resuls in somewhat > slower code and + * uses 4 times more reads as needed but keeps code > simpler. Since the SID is + * only very rarly probed, this is not > really an issue. > + */ > +static u8 sunxi_sid_read_byte(const void __iomem *sid_reg_base, > + const unsigned int offset) > +{ > + u32 sid_key; > + > + if (offset >= SID_SIZE) > + return 0; > + > + sid_key = ioread32be(sid_reg_base + round_down(offset, 4)); > + sid_key >>= (offset % 4) * 8; > + > + return sid_key; /* Only return 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 = to_platform_device(kobj_to_dev(kobj)); > + sid_reg_base = (void __iomem *)platform_get_drvdata(pdev); > + > + 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 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_dbg(&pdev->dev, "%s driver unloaded\n", DRV_NAME); > + > + return 0; > +} > + > +static int __init sunxi_sid_probe(struct platform_device *pdev) > +{ > + u8 entropy[SID_SIZE]; > + unsigned int i; > + struct resource *res; > + void __iomem *sid_reg_base; > + int ret; > + > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + sid_reg_base = devm_ioremap_resource(&pdev->dev, res); > + 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) > + return ret; > + > + for (i = 0; i < SID_SIZE; i++) > + entropy[i] = sunxi_sid_read_byte(sid_reg_base, i); > + add_device_randomness(entropy, SID_SIZE); > + dev_dbg(&pdev->dev, "%s loaded\n", DRV_NAME); > + > + 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); > + > +MODULE_AUTHOR("Oliver Schinagl <oliver@schinagl.nl>"); > +MODULE_DESCRIPTION("Allwinner sunxi security id driver"); > +MODULE_LICENSE("GPL");
On Mon, Jun 17, 2013 at 10:59:37PM +0200, Oliver Schinagl wrote: > From: Oliver Schinagl <oliver@schinagl.nl> > > Allwinner has electric fuses (efuse) on their line of chips. This driver > reads those fuses, seeds the kernel entropy and exports them as a sysfs node. > > These fuses are most likly to be programmed at the factory, encoding > things like Chip ID, some sort of serial number etc and appear to be > reasonable unique. > While in theory, these should be writeable by the user, it will probably > be inconvinient to do so. Allwinner recommends that a certain input pin, > labeled 'efuse_vddq', be connected to GND. To write these fuses, 2.5 V > needs to be applied to this pin. > > Even so, they can still be used to generate a board-unique mac from, board > unique RSA key and seed the kernel RNG. > > Currently supported are the following known chips: > Allwinner sun4i (A10) > Allwinner sun5i (A10s, A13) > > Signed-off-by: Oliver Schinagl <oliver@schinagl.nl> > --- > drivers/misc/eeprom/Kconfig | 17 +++++ > drivers/misc/eeprom/Makefile | 1 + > drivers/misc/eeprom/sunxi_sid.c | 147 ++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 165 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..6a16c19 > --- /dev/null > +++ b/drivers/misc/eeprom/sunxi_sid.c > @@ -0,0 +1,147 @@ > +/* > + * 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 byte > + * sized chunks. > + */ > + > +#include <linux/compiler.h> > +#include <linux/device.h> > +#include <linux/err.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/platform_device.h> > +#include <linux/random.h> > +#include <linux/stat.h> > +#include <linux/sysfs.h> > +#include <linux/types.h> > + > +#define DRV_NAME "sunxi-sid" > + > +/* There are 4 32-bit keys */ > +#define SID_KEYS 4 > +/* Each key is 4 bytes long */ > +#define SID_SIZE (SID_KEYS * 4) > + > +/* We read the entire key, due to a 32 bit read alignment requirement. Since we > + * want to return the requested byte, this resuls in somewhat slower code and > + * uses 4 times more reads as needed but keeps code simpler. Since the SID is > + * only very rarly probed, this is not really an issue. > + */ > +static u8 sunxi_sid_read_byte(const void __iomem *sid_reg_base, > + const unsigned int offset) > +{ > + u32 sid_key; > + > + if (offset >= SID_SIZE) > + return 0; > + > + sid_key = ioread32be(sid_reg_base + round_down(offset, 4)); > + sid_key >>= (offset % 4) * 8; > + > + return sid_key; /* Only return 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 = to_platform_device(kobj_to_dev(kobj)); > + sid_reg_base = (void __iomem *)platform_get_drvdata(pdev); > + > + 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 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_dbg(&pdev->dev, "%s driver unloaded\n", DRV_NAME); > + > + return 0; > +} > + > +static int __init sunxi_sid_probe(struct platform_device *pdev) > +{ > + u8 entropy[SID_SIZE]; > + unsigned int i; > + struct resource *res; > + void __iomem *sid_reg_base; > + int ret; > + > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + sid_reg_base = devm_ioremap_resource(&pdev->dev, res); > + 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) > + return ret; You just raced with userspace, having the file show up after the device was announced to users that it was there. Please use the proper device file api to add default attributes to prevent this from happening. Bonus is it ends up making your driver smaller and simpler :) > + for (i = 0; i < SID_SIZE; i++) > + entropy[i] = sunxi_sid_read_byte(sid_reg_base, i); > + add_device_randomness(entropy, SID_SIZE); Really? I don't mind it but is this something that is ok to add to the pool? Will it be different on different machines with this device? Or will it always be the same on all systems with this device? thanks, greg k-h
mån 2013-06-17 klockan 15:58 -0700 skrev Greg KH: > Really? I don't mind it but is this something that is ok to add to the > pool? Will it be different on different machines with this device? Or > will it always be the same on all systems with this device? The data is unique per CPU, so it's different on every machine and is why it was suggested to have it added to the pool. Regards Henrik
On Mon, Jun 17, 2013 at 11:59 PM, Oliver Schinagl <oliver+list@schinagl.nl> wrote: > From: Oliver Schinagl <oliver@schinagl.nl> > > Allwinner has electric fuses (efuse) on their line of chips. This driver > reads those fuses, seeds the kernel entropy and exports them as a sysfs node. > > These fuses are most likly to be programmed at the factory, encoding > things like Chip ID, some sort of serial number etc and appear to be > reasonable unique. > While in theory, these should be writeable by the user, it will probably > be inconvinient to do so. Allwinner recommends that a certain input pin, > labeled 'efuse_vddq', be connected to GND. To write these fuses, 2.5 V > needs to be applied to this pin. > > Even so, they can still be used to generate a board-unique mac from, board > unique RSA key and seed the kernel RNG. > > +++ b/drivers/misc/eeprom/sunxi_sid.c > @@ -0,0 +1,147 @@ > +#include <linux/compiler.h> I don't think you have to use this header explicitly. > +#define DRV_NAME "sunxi-sid" > + if (size > (SID_SIZE - pos)) Useless internal braces. > +static int sunxi_sid_remove(struct platform_device *pdev) > +{ > + device_remove_bin_file(&pdev->dev, &sid_bin_attr); > + dev_dbg(&pdev->dev, "%s driver unloaded\n", DRV_NAME); It's useless to use DRV_NAME in conjunction with dev_* macros. dev_* will print driver name as a prefix. -- With Best Regards, Andy Shevchenko
Hi Greg, On Mon, Jun 17, 2013 at 03:58:47PM -0700, Greg KH wrote: > On Mon, Jun 17, 2013 at 10:59:37PM +0200, Oliver Schinagl wrote: [..] > > +static int __init sunxi_sid_probe(struct platform_device *pdev) > > +{ > > + u8 entropy[SID_SIZE]; > > + unsigned int i; > > + struct resource *res; > > + void __iomem *sid_reg_base; > > + int ret; > > + > > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > > + sid_reg_base = devm_ioremap_resource(&pdev->dev, res); > > + 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) > > + return ret; > > You just raced with userspace, having the file show up after the device > was announced to users that it was there. Please use the proper device > file api to add default attributes to prevent this from happening. Sorry if the question looks dumb, but what kind of race can we generate here? The device_create_bin_file is the last call that we make (if we except the entropy stuff, but it doesn't really matter here), so after we created the file, we have everything properly initialised so that our functions can be called, right? And another dumb question for you, what is the "proper device file API" you are referring to ? :) Thanks! Maxime
On Mon, Jun 24, 2013 at 11:29:42AM +0200, Maxime Ripard wrote: > Hi Greg, > > On Mon, Jun 17, 2013 at 03:58:47PM -0700, Greg KH wrote: > > On Mon, Jun 17, 2013 at 10:59:37PM +0200, Oliver Schinagl wrote: > > [..] > > > > +static int __init sunxi_sid_probe(struct platform_device *pdev) > > > +{ > > > + u8 entropy[SID_SIZE]; > > > + unsigned int i; > > > + struct resource *res; > > > + void __iomem *sid_reg_base; > > > + int ret; > > > + > > > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > > > + sid_reg_base = devm_ioremap_resource(&pdev->dev, res); > > > + 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) > > > + return ret; > > > > You just raced with userspace, having the file show up after the device > > was announced to users that it was there. Please use the proper device > > file api to add default attributes to prevent this from happening. > > Sorry if the question looks dumb, but what kind of race can we generate > here? Userspace gets told about the device from the driver core, udev runs and reads all of the attributes, then your probe function comes along and adds a new attribute. Userspace will then not know about it at all. > The device_create_bin_file is the last call that we make (if we except > the entropy stuff, but it doesn't really matter here), so after we > created the file, we have everything properly initialised so that our > functions can be called, right? > > And another dumb question for you, what is the "proper device file API" > you are referring to ? :) Please read Documentation/driver_model/device.txt and see the section on Attributes for what to do. If you have specific questions after reading that, please let me know. thanks, greg k-h
Hey Greg, On 06/24/13 18:04, Greg KH wrote: > On Mon, Jun 24, 2013 at 11:29:42AM +0200, Maxime Ripard wrote: >> Hi Greg, >> >> On Mon, Jun 17, 2013 at 03:58:47PM -0700, Greg KH wrote: >>> On Mon, Jun 17, 2013 at 10:59:37PM +0200, Oliver Schinagl wrote: >> >> [..] >> >>>> +static int __init sunxi_sid_probe(struct platform_device *pdev) >>>> +{ >>>> + u8 entropy[SID_SIZE]; >>>> + unsigned int i; >>>> + struct resource *res; >>>> + void __iomem *sid_reg_base; >>>> + int ret; >>>> + >>>> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); >>>> + sid_reg_base = devm_ioremap_resource(&pdev->dev, res); >>>> + 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) >>>> + return ret; >>> >>> You just raced with userspace, having the file show up after the device >>> was announced to users that it was there. Please use the proper device >>> file api to add default attributes to prevent this from happening. >> >> Sorry if the question looks dumb, but what kind of race can we generate >> here? > > Userspace gets told about the device from the driver core, udev runs and > reads all of the attributes, then your probe function comes along and > adds a new attribute. Userspace will then not know about it at all. > >> The device_create_bin_file is the last call that we make (if we except >> the entropy stuff, but it doesn't really matter here), so after we >> created the file, we have everything properly initialised so that our >> functions can be called, right? >> >> And another dumb question for you, what is the "proper device file API" >> you are referring to ? :) > > Please read Documentation/driver_model/device.txt and see the section on > Attributes for what to do. If you have specific questions after reading > that, please let me know. Since Maxime kinda asked for me, I hope you don't mind me following up. That doc doesn't mention the binary interface at all. Initially I had both devices up, the 'read' device as a textual representation and added the binary one later. Maxime and I decided the binary one made more sense, as the only textual user would be a human and they don't poke that entry that often. So what default way exists for binary files or how would that be solved? Oliver > > thanks, > > greg k-h >
On Mon, Jun 24, 2013 at 07:11:35PM +0200, Oliver Schinagl wrote: > Hey Greg, > On 06/24/13 18:04, Greg KH wrote: > >On Mon, Jun 24, 2013 at 11:29:42AM +0200, Maxime Ripard wrote: > >>Hi Greg, > >> > >>On Mon, Jun 17, 2013 at 03:58:47PM -0700, Greg KH wrote: > >>>On Mon, Jun 17, 2013 at 10:59:37PM +0200, Oliver Schinagl wrote: > >> > >>[..] > >> > >>>>+static int __init sunxi_sid_probe(struct platform_device *pdev) > >>>>+{ > >>>>+ u8 entropy[SID_SIZE]; > >>>>+ unsigned int i; > >>>>+ struct resource *res; > >>>>+ void __iomem *sid_reg_base; > >>>>+ int ret; > >>>>+ > >>>>+ res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > >>>>+ sid_reg_base = devm_ioremap_resource(&pdev->dev, res); > >>>>+ 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) > >>>>+ return ret; > >>> > >>>You just raced with userspace, having the file show up after the device > >>>was announced to users that it was there. Please use the proper device > >>>file api to add default attributes to prevent this from happening. > >> > >>Sorry if the question looks dumb, but what kind of race can we generate > >>here? > > > >Userspace gets told about the device from the driver core, udev runs and > >reads all of the attributes, then your probe function comes along and > >adds a new attribute. Userspace will then not know about it at all. > > > >>The device_create_bin_file is the last call that we make (if we except > >>the entropy stuff, but it doesn't really matter here), so after we > >>created the file, we have everything properly initialised so that our > >>functions can be called, right? > >> > >>And another dumb question for you, what is the "proper device file API" > >>you are referring to ? :) > > > >Please read Documentation/driver_model/device.txt and see the section on > >Attributes for what to do. If you have specific questions after reading > >that, please let me know. > Since Maxime kinda asked for me, I hope you don't mind me following up. > > That doc doesn't mention the binary interface at all. Initially I > had both devices up, the 'read' device as a textual representation > and added the binary one later. Maxime and I decided the binary one > made more sense, as the only textual user would be a human and they > don't poke that entry that often. > > So what default way exists for binary files or how would that be solved? The same interface should work just fine for binary files, have you tried it? thanks, greg k-h
On Mon, Jun 24, 2013 at 09:04:40AM -0700, Greg KH wrote: > On Mon, Jun 24, 2013 at 11:29:42AM +0200, Maxime Ripard wrote: > > Hi Greg, > > > > On Mon, Jun 17, 2013 at 03:58:47PM -0700, Greg KH wrote: > > > On Mon, Jun 17, 2013 at 10:59:37PM +0200, Oliver Schinagl wrote: > > > > [..] > > > > > > +static int __init sunxi_sid_probe(struct platform_device *pdev) > > > > +{ > > > > + u8 entropy[SID_SIZE]; > > > > + unsigned int i; > > > > + struct resource *res; > > > > + void __iomem *sid_reg_base; > > > > + int ret; > > > > + > > > > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > > > > + sid_reg_base = devm_ioremap_resource(&pdev->dev, res); > > > > + 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) > > > > + return ret; > > > > > > You just raced with userspace, having the file show up after the device > > > was announced to users that it was there. Please use the proper device > > > file api to add default attributes to prevent this from happening. > > > > Sorry if the question looks dumb, but what kind of race can we generate > > here? > > Userspace gets told about the device from the driver core, udev runs and > reads all of the attributes, then your probe function comes along and > adds a new attribute. Userspace will then not know about it at all. Hmm, I see. Thanks for the explanations! Maxime
On 06/24/13 20:15, Greg KH wrote: > On Mon, Jun 24, 2013 at 07:11:35PM +0200, Oliver Schinagl wrote: >> Hey Greg, >> On 06/24/13 18:04, Greg KH wrote: >>> On Mon, Jun 24, 2013 at 11:29:42AM +0200, Maxime Ripard wrote: >>>> Hi Greg, >>>> >>>> On Mon, Jun 17, 2013 at 03:58:47PM -0700, Greg KH wrote: >>>>> On Mon, Jun 17, 2013 at 10:59:37PM +0200, Oliver Schinagl wrote: >>>> >>>> [..] >>>> >>>>>> +static int __init sunxi_sid_probe(struct platform_device *pdev) >>>>>> +{ >>>>>> + u8 entropy[SID_SIZE]; >>>>>> + unsigned int i; >>>>>> + struct resource *res; >>>>>> + void __iomem *sid_reg_base; >>>>>> + int ret; >>>>>> + >>>>>> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); >>>>>> + sid_reg_base = devm_ioremap_resource(&pdev->dev, res); >>>>>> + 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) >>>>>> + return ret; >>>>> >>>>> You just raced with userspace, having the file show up after the device >>>>> was announced to users that it was there. Please use the proper device >>>>> file api to add default attributes to prevent this from happening. >>>> >>>> Sorry if the question looks dumb, but what kind of race can we generate >>>> here? >>> >>> Userspace gets told about the device from the driver core, udev runs and >>> reads all of the attributes, then your probe function comes along and >>> adds a new attribute. Userspace will then not know about it at all. >>> >>>> The device_create_bin_file is the last call that we make (if we except >>>> the entropy stuff, but it doesn't really matter here), so after we >>>> created the file, we have everything properly initialised so that our >>>> functions can be called, right? >>>> >>>> And another dumb question for you, what is the "proper device file API" >>>> you are referring to ? :) >>> >>> Please read Documentation/driver_model/device.txt and see the section on >>> Attributes for what to do. If you have specific questions after reading >>> that, please let me know. >> Since Maxime kinda asked for me, I hope you don't mind me following up. >> >> That doc doesn't mention the binary interface at all. Initially I >> had both devices up, the 'read' device as a textual representation >> and added the binary one later. Maxime and I decided the binary one >> made more sense, as the only textual user would be a human and they >> don't poke that entry that often. >> >> So what default way exists for binary files or how would that be solved? > > The same interface should work just fine for binary files, have you > tried it? I'll just take the plunge and make myself look stupid ;) I tried to change things around, used DEVICE_ATTR(eeprom, S_IRUGO, sid_read, NULL); So far so good I'd hope. Of course now I'll have to change the function's parameters from static ssize_t sid_read(struct file *fd, struct kobject *kobj, struct bin_attribute *attr, char *buf, loff_t pos, size_t size) to static ssize_t sid_read(struct device *dev, struct device_attribute *attr, char *buf) But now, I'm missing things like 'pos' and 'size', both which determine the requested bytes. True, in this specific driver we are talking about 'only' 16 bytes, but what if it weren't but a few MiB and in sysfs we want to read some random byte, will we have to put the entire blok into the buffer? So sorry for not understanding, but ... I don't understand :) Oliver > > thanks, > > greg k-h >
On Mon, Jun 24, 2013 at 11:21:16PM +0200, Oliver Schinagl wrote: > On 06/24/13 20:15, Greg KH wrote: > >On Mon, Jun 24, 2013 at 07:11:35PM +0200, Oliver Schinagl wrote: > >>Hey Greg, > >>On 06/24/13 18:04, Greg KH wrote: > >>>On Mon, Jun 24, 2013 at 11:29:42AM +0200, Maxime Ripard wrote: > >>>>Hi Greg, > >>>> > >>>>On Mon, Jun 17, 2013 at 03:58:47PM -0700, Greg KH wrote: > >>>>>On Mon, Jun 17, 2013 at 10:59:37PM +0200, Oliver Schinagl wrote: > >>>> > >>>>[..] > >>>> > >>>>>>+static int __init sunxi_sid_probe(struct platform_device *pdev) > >>>>>>+{ > >>>>>>+ u8 entropy[SID_SIZE]; > >>>>>>+ unsigned int i; > >>>>>>+ struct resource *res; > >>>>>>+ void __iomem *sid_reg_base; > >>>>>>+ int ret; > >>>>>>+ > >>>>>>+ res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > >>>>>>+ sid_reg_base = devm_ioremap_resource(&pdev->dev, res); > >>>>>>+ 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) > >>>>>>+ return ret; > >>>>> > >>>>>You just raced with userspace, having the file show up after the device > >>>>>was announced to users that it was there. Please use the proper device > >>>>>file api to add default attributes to prevent this from happening. > >>>> > >>>>Sorry if the question looks dumb, but what kind of race can we generate > >>>>here? > >>> > >>>Userspace gets told about the device from the driver core, udev runs and > >>>reads all of the attributes, then your probe function comes along and > >>>adds a new attribute. Userspace will then not know about it at all. > >>> > >>>>The device_create_bin_file is the last call that we make (if we except > >>>>the entropy stuff, but it doesn't really matter here), so after we > >>>>created the file, we have everything properly initialised so that our > >>>>functions can be called, right? > >>>> > >>>>And another dumb question for you, what is the "proper device file API" > >>>>you are referring to ? :) > >>> > >>>Please read Documentation/driver_model/device.txt and see the section on > >>>Attributes for what to do. If you have specific questions after reading > >>>that, please let me know. > >>Since Maxime kinda asked for me, I hope you don't mind me following up. > >> > >>That doc doesn't mention the binary interface at all. Initially I > >>had both devices up, the 'read' device as a textual representation > >>and added the binary one later. Maxime and I decided the binary one > >>made more sense, as the only textual user would be a human and they > >>don't poke that entry that often. > >> > >>So what default way exists for binary files or how would that be solved? > > > >The same interface should work just fine for binary files, have you > >tried it? > I'll just take the plunge and make myself look stupid ;) > > I tried to change things around, used DEVICE_ATTR(eeprom, S_IRUGO, > sid_read, NULL); So far so good I'd hope. Ick, no. > Of course now I'll have to change the function's parameters from > > static ssize_t sid_read(struct file *fd, struct kobject *kobj, > struct bin_attribute *attr, char *buf, > loff_t pos, size_t size) > > to > > static ssize_t sid_read(struct device *dev, > struct device_attribute *attr, char *buf) Which is what do you do not want, as you find out: > But now, I'm missing things like 'pos' and 'size', both which > determine the requested bytes. True, in this specific driver we are > talking about 'only' 16 bytes, but what if it weren't but a few MiB > and in sysfs we want to read some random byte, will we have to put > the entire blok into the buffer? > > So sorry for not understanding, but ... I don't understand :) Stick with a binary attribute, and attach that to the proper class structure and all should be fine. Ah crap, you're using a platform device. {sigh} Why? Why not use a "real" device which has a "real" class, and then use the interfaces there? greg k-h
On 24-06-13 23:46, Greg KH wrote: > On Mon, Jun 24, 2013 at 11:21:16PM +0200, Oliver Schinagl wrote: >> On 06/24/13 20:15, Greg KH wrote: >>> On Mon, Jun 24, 2013 at 07:11:35PM +0200, Oliver Schinagl wrote: >>>> Hey Greg, >>>> On 06/24/13 18:04, Greg KH wrote: >>>>> On Mon, Jun 24, 2013 at 11:29:42AM +0200, Maxime Ripard wrote: >>>>>> Hi Greg, >>>>>> >>>>>> On Mon, Jun 17, 2013 at 03:58:47PM -0700, Greg KH wrote: >>>>>>> On Mon, Jun 17, 2013 at 10:59:37PM +0200, Oliver Schinagl wrote: >>>>>> >>>>>> [..] >>>>>> >>>>>>>> +static int __init sunxi_sid_probe(struct platform_device *pdev) >>>>>>>> +{ >>>>>>>> + u8 entropy[SID_SIZE]; >>>>>>>> + unsigned int i; >>>>>>>> + struct resource *res; >>>>>>>> + void __iomem *sid_reg_base; >>>>>>>> + int ret; >>>>>>>> + >>>>>>>> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); >>>>>>>> + sid_reg_base = devm_ioremap_resource(&pdev->dev, res); >>>>>>>> + 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) >>>>>>>> + return ret; >>>>>>> >>>>>>> You just raced with userspace, having the file show up after the device >>>>>>> was announced to users that it was there. Please use the proper device >>>>>>> file api to add default attributes to prevent this from happening. >>>>>> >>>>>> Sorry if the question looks dumb, but what kind of race can we generate >>>>>> here? >>>>> >>>>> Userspace gets told about the device from the driver core, udev runs and >>>>> reads all of the attributes, then your probe function comes along and >>>>> adds a new attribute. Userspace will then not know about it at all. >>>>> >>>>>> The device_create_bin_file is the last call that we make (if we except >>>>>> the entropy stuff, but it doesn't really matter here), so after we >>>>>> created the file, we have everything properly initialised so that our >>>>>> functions can be called, right? >>>>>> >>>>>> And another dumb question for you, what is the "proper device file API" >>>>>> you are referring to ? :) >>>>> >>>>> Please read Documentation/driver_model/device.txt and see the section on >>>>> Attributes for what to do. If you have specific questions after reading >>>>> that, please let me know. >>>> Since Maxime kinda asked for me, I hope you don't mind me following up. >>>> >>>> That doc doesn't mention the binary interface at all. Initially I >>>> had both devices up, the 'read' device as a textual representation >>>> and added the binary one later. Maxime and I decided the binary one >>>> made more sense, as the only textual user would be a human and they >>>> don't poke that entry that often. >>>> >>>> So what default way exists for binary files or how would that be solved? >>> >>> The same interface should work just fine for binary files, have you >>> tried it? >> I'll just take the plunge and make myself look stupid ;) >> >> I tried to change things around, used DEVICE_ATTR(eeprom, S_IRUGO, >> sid_read, NULL); So far so good I'd hope. > > Ick, no. > >> Of course now I'll have to change the function's parameters from >> >> static ssize_t sid_read(struct file *fd, struct kobject *kobj, >> struct bin_attribute *attr, char *buf, >> loff_t pos, size_t size) >> >> to >> >> static ssize_t sid_read(struct device *dev, >> struct device_attribute *attr, char *buf) > > Which is what do you do not want, as you find out: > >> But now, I'm missing things like 'pos' and 'size', both which >> determine the requested bytes. True, in this specific driver we are >> talking about 'only' 16 bytes, but what if it weren't but a few MiB >> and in sysfs we want to read some random byte, will we have to put >> the entire blok into the buffer? >> >> So sorry for not understanding, but ... I don't understand :) > > Stick with a binary attribute, and attach that to the proper class > structure and all should be fine. > > Ah crap, you're using a platform device. > > {sigh} > > Why? Why not use a "real" device which has a "real" class, and then use > the interfaces there? Because, as I was told, this really is a platform device. If you have some example code I can look at and learn from that would be awesome. I'm still learning after all, and apparently I'm doing it wrong now :) > > greg k-h >
On Mon, Jun 24, 2013 at 02:46:15PM -0700, Greg KH wrote: > Stick with a binary attribute, and attach that to the proper class > structure and all should be fine. > > Ah crap, you're using a platform device. > > {sigh} > > Why? Why not use a "real" device which has a "real" class, and then use > the interfaces there? And why aren't platform devices "real" devices? If platform devices are second class devices then that's pretty crap because virtually all devices on ARM are platform devices, not something like "first class" PCI devices. We could make them PCI devices if you want us to totally fsck with the PCI code to bend it in ways it was never meant to, but I suspect that'll upset the PCI guys. No, platform devices must be first class devices just like any other.
On Mon, Jun 24, 2013 at 6:04 PM, Greg KH <gregkh@linuxfoundation.org> wrote: > On Mon, Jun 24, 2013 at 11:29:42AM +0200, Maxime Ripard wrote: >> On Mon, Jun 17, 2013 at 03:58:47PM -0700, Greg KH wrote: >> > On Mon, Jun 17, 2013 at 10:59:37PM +0200, Oliver Schinagl wrote: >> > > +static int __init sunxi_sid_probe(struct platform_device *pdev) >> > > +{ >> > > + u8 entropy[SID_SIZE]; >> > > + unsigned int i; >> > > + struct resource *res; >> > > + void __iomem *sid_reg_base; >> > > + int ret; >> > > + >> > > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); >> > > + sid_reg_base = devm_ioremap_resource(&pdev->dev, res); >> > > + 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) >> > > + return ret; >> > >> > You just raced with userspace, having the file show up after the device >> > was announced to users that it was there. Please use the proper device >> > file api to add default attributes to prevent this from happening. >> >> Sorry if the question looks dumb, but what kind of race can we generate >> here? > > Userspace gets told about the device from the driver core, udev runs and > reads all of the attributes, then your probe function comes along and > adds a new attribute. Userspace will then not know about it at all. > >> The device_create_bin_file is the last call that we make (if we except >> the entropy stuff, but it doesn't really matter here), so after we >> created the file, we have everything properly initialised so that our >> functions can be called, right? >> >> And another dumb question for you, what is the "proper device file API" >> you are referring to ? :) > > Please read Documentation/driver_model/device.txt and see the section on > Attributes for what to do. If you have specific questions after reading > that, please let me know. Woops, then we have plenty of existing drivers to fix, e.g. all/most RTC drivers exposing an NVRAM file through sysfs: $ git grep -w sysfs_create_bin_file drivers/rtc/ drivers/rtc/rtc-cmos.c: retval = sysfs_create_bin_file(&dev->kobj, &nvram); drivers/rtc/rtc-ds1305.c: status = sysfs_create_bin_file(&spi->dev.kobj, &nvram); drivers/rtc/rtc-ds1307.c: err = sysfs_create_bin_file(&client->dev.kobj, ds1307->nvram); drivers/rtc/rtc-ds1511.c: ret = sysfs_create_bin_file(&pdev->dev.kobj, &ds1511_nvram_attr); drivers/rtc/rtc-ds1553.c: ret = sysfs_create_bin_file(&pdev->dev.kobj, &ds1553_nvram_attr); drivers/rtc/rtc-ds1742.c: ret = sysfs_create_bin_file(&pdev->dev.kobj, &pdata->nvram_attr); drivers/rtc/rtc-m48t59.c: ret = sysfs_create_bin_file(&pdev->dev.kobj, &m48t59_nvram_attr); drivers/rtc/rtc-rp5c01.c: error = sysfs_create_bin_file(&dev->dev.kobj, &priv->nvram_attr); drivers/rtc/rtc-stk17ta8.c: ret = sysfs_create_bin_file(&pdev->dev.kobj, &stk17ta8_nvram_attr); drivers/rtc/rtc-tx4939.c: ret = sysfs_create_bin_file(&pdev->dev.kobj, &tx4939_rtc_nvram_attr); Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
On Wed, Jun 26, 2013 at 11:22:30AM +0200, Geert Uytterhoeven wrote: > On Mon, Jun 24, 2013 at 6:04 PM, Greg KH <gregkh@linuxfoundation.org> wrote: > > On Mon, Jun 24, 2013 at 11:29:42AM +0200, Maxime Ripard wrote: > >> On Mon, Jun 17, 2013 at 03:58:47PM -0700, Greg KH wrote: > >> > On Mon, Jun 17, 2013 at 10:59:37PM +0200, Oliver Schinagl wrote: > >> > > +static int __init sunxi_sid_probe(struct platform_device *pdev) > >> > > +{ > >> > > + u8 entropy[SID_SIZE]; > >> > > + unsigned int i; > >> > > + struct resource *res; > >> > > + void __iomem *sid_reg_base; > >> > > + int ret; > >> > > + > >> > > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > >> > > + sid_reg_base = devm_ioremap_resource(&pdev->dev, res); > >> > > + 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) > >> > > + return ret; > >> > > >> > You just raced with userspace, having the file show up after the device > >> > was announced to users that it was there. Please use the proper device > >> > file api to add default attributes to prevent this from happening. > >> > >> Sorry if the question looks dumb, but what kind of race can we generate > >> here? > > > > Userspace gets told about the device from the driver core, udev runs and > > reads all of the attributes, then your probe function comes along and > > adds a new attribute. Userspace will then not know about it at all. > > > >> The device_create_bin_file is the last call that we make (if we except > >> the entropy stuff, but it doesn't really matter here), so after we > >> created the file, we have everything properly initialised so that our > >> functions can be called, right? > >> > >> And another dumb question for you, what is the "proper device file API" > >> you are referring to ? :) > > > > Please read Documentation/driver_model/device.txt and see the section on > > Attributes for what to do. If you have specific questions after reading > > that, please let me know. > > Woops, then we have plenty of existing drivers to fix, e.g. all/most RTC drivers > exposing an NVRAM file through sysfs: > > $ git grep -w sysfs_create_bin_file drivers/rtc/ > drivers/rtc/rtc-cmos.c: retval = sysfs_create_bin_file(&dev->kobj, &nvram); > drivers/rtc/rtc-ds1305.c: status = > sysfs_create_bin_file(&spi->dev.kobj, &nvram); > drivers/rtc/rtc-ds1307.c: err = > sysfs_create_bin_file(&client->dev.kobj, ds1307->nvram); > drivers/rtc/rtc-ds1511.c: ret = sysfs_create_bin_file(&pdev->dev.kobj, > &ds1511_nvram_attr); > drivers/rtc/rtc-ds1553.c: ret = sysfs_create_bin_file(&pdev->dev.kobj, > &ds1553_nvram_attr); > drivers/rtc/rtc-ds1742.c: ret = sysfs_create_bin_file(&pdev->dev.kobj, > &pdata->nvram_attr); > drivers/rtc/rtc-m48t59.c: ret = sysfs_create_bin_file(&pdev->dev.kobj, > &m48t59_nvram_attr); > drivers/rtc/rtc-rp5c01.c: error = > sysfs_create_bin_file(&dev->dev.kobj, &priv->nvram_attr); > drivers/rtc/rtc-stk17ta8.c: ret = > sysfs_create_bin_file(&pdev->dev.kobj, &stk17ta8_nvram_attr); > drivers/rtc/rtc-tx4939.c: ret = sysfs_create_bin_file(&pdev->dev.kobj, > &tx4939_rtc_nvram_attr); Yes, they should all be fixed, along with any platform device that creates a sysfs file in the probe function. thanks, greg k-h
On Wed, Jun 26, 2013 at 10:10:33AM +0100, Russell King - ARM Linux wrote: > On Mon, Jun 24, 2013 at 02:46:15PM -0700, Greg KH wrote: > > Stick with a binary attribute, and attach that to the proper class > > structure and all should be fine. > > > > Ah crap, you're using a platform device. > > > > {sigh} > > > > Why? Why not use a "real" device which has a "real" class, and then use > > the interfaces there? > > And why aren't platform devices "real" devices? If platform devices are > second class devices then that's pretty crap because virtually all > devices on ARM are platform devices, not something like "first class" > PCI devices. Ok, they are "real" devices, I'm just tired of seeing people throw everything and the kitchen sink into them, don't you agree? > We could make them PCI devices if you want us to totally fsck with the > PCI code to bend it in ways it was never meant to, but I suspect that'll > upset the PCI guys. > > No, platform devices must be first class devices just like any other. I was wrong, they can, and do, support default attribute groups, it's just that it seems no one uses them (or if they did, my greping can't find them...) So they are "first class" devices, my mistake. thanks, greg k-h
On Wed, Jun 26, 2013 at 10:32:09AM +0200, Oliver Schinagl wrote: > On 24-06-13 23:46, Greg KH wrote: > >On Mon, Jun 24, 2013 at 11:21:16PM +0200, Oliver Schinagl wrote: > >>On 06/24/13 20:15, Greg KH wrote: > >>>On Mon, Jun 24, 2013 at 07:11:35PM +0200, Oliver Schinagl wrote: > >>>>Hey Greg, > >>>>On 06/24/13 18:04, Greg KH wrote: > >>>>>On Mon, Jun 24, 2013 at 11:29:42AM +0200, Maxime Ripard wrote: > >>>>>>Hi Greg, > >>>>>> > >>>>>>On Mon, Jun 17, 2013 at 03:58:47PM -0700, Greg KH wrote: > >>>>>>>On Mon, Jun 17, 2013 at 10:59:37PM +0200, Oliver Schinagl wrote: > >>>>>> > >>>>>>[..] > >>>>>> > >>>>>>>>+static int __init sunxi_sid_probe(struct platform_device *pdev) > >>>>>>>>+{ > >>>>>>>>+ u8 entropy[SID_SIZE]; > >>>>>>>>+ unsigned int i; > >>>>>>>>+ struct resource *res; > >>>>>>>>+ void __iomem *sid_reg_base; > >>>>>>>>+ int ret; > >>>>>>>>+ > >>>>>>>>+ res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > >>>>>>>>+ sid_reg_base = devm_ioremap_resource(&pdev->dev, res); > >>>>>>>>+ 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) > >>>>>>>>+ return ret; > >>>>>>> > >>>>>>>You just raced with userspace, having the file show up after the device > >>>>>>>was announced to users that it was there. Please use the proper device > >>>>>>>file api to add default attributes to prevent this from happening. > >>>>>> > >>>>>>Sorry if the question looks dumb, but what kind of race can we generate > >>>>>>here? > >>>>> > >>>>>Userspace gets told about the device from the driver core, udev runs and > >>>>>reads all of the attributes, then your probe function comes along and > >>>>>adds a new attribute. Userspace will then not know about it at all. > >>>>> > >>>>>>The device_create_bin_file is the last call that we make (if we except > >>>>>>the entropy stuff, but it doesn't really matter here), so after we > >>>>>>created the file, we have everything properly initialised so that our > >>>>>>functions can be called, right? > >>>>>> > >>>>>>And another dumb question for you, what is the "proper device file API" > >>>>>>you are referring to ? :) > >>>>> > >>>>>Please read Documentation/driver_model/device.txt and see the section on > >>>>>Attributes for what to do. If you have specific questions after reading > >>>>>that, please let me know. > >>>>Since Maxime kinda asked for me, I hope you don't mind me following up. > >>>> > >>>>That doc doesn't mention the binary interface at all. Initially I > >>>>had both devices up, the 'read' device as a textual representation > >>>>and added the binary one later. Maxime and I decided the binary one > >>>>made more sense, as the only textual user would be a human and they > >>>>don't poke that entry that often. > >>>> > >>>>So what default way exists for binary files or how would that be solved? > >>> > >>>The same interface should work just fine for binary files, have you > >>>tried it? > >>I'll just take the plunge and make myself look stupid ;) > >> > >>I tried to change things around, used DEVICE_ATTR(eeprom, S_IRUGO, > >>sid_read, NULL); So far so good I'd hope. > > > >Ick, no. > > > >>Of course now I'll have to change the function's parameters from > >> > >>static ssize_t sid_read(struct file *fd, struct kobject *kobj, > >> struct bin_attribute *attr, char *buf, > >> loff_t pos, size_t size) > >> > >>to > >> > >>static ssize_t sid_read(struct device *dev, > >> struct device_attribute *attr, char *buf) > > > >Which is what do you do not want, as you find out: > > > >>But now, I'm missing things like 'pos' and 'size', both which > >>determine the requested bytes. True, in this specific driver we are > >>talking about 'only' 16 bytes, but what if it weren't but a few MiB > >>and in sysfs we want to read some random byte, will we have to put > >>the entire blok into the buffer? > >> > >>So sorry for not understanding, but ... I don't understand :) > > > >Stick with a binary attribute, and attach that to the proper class > >structure and all should be fine. > > > >Ah crap, you're using a platform device. > > > >{sigh} > > > >Why? Why not use a "real" device which has a "real" class, and then use > >the interfaces there? > Because, as I was told, this really is a platform device. If you > have some example code I can look at and learn from that would be > awesome. I'm still learning after all, and apparently I'm doing it > wrong now :) I was wrong, you can do this with a platform device just fine. Set the "groups" field in your platform device->device structure, and all will work properly, right? thanks, greg k-h
Hey Greg, Thanks for the blog post :) it was very helpful and at least something good came from the less-nice bit of the discussion, but: On 26-06-13 19:51, Greg KH wrote: > On Wed, Jun 26, 2013 at 10:32:09AM +0200, Oliver Schinagl wrote: >> On 24-06-13 23:46, Greg KH wrote: >>> On Mon, Jun 24, 2013 at 11:21:16PM +0200, Oliver Schinagl wrote: >>>> On 06/24/13 20:15, Greg KH wrote: >>>>> On Mon, Jun 24, 2013 at 07:11:35PM +0200, Oliver Schinagl wrote: >>>>>> Hey Greg, >>>>>> On 06/24/13 18:04, Greg KH wrote: >>>>>>> On Mon, Jun 24, 2013 at 11:29:42AM +0200, Maxime Ripard wrote: >>>>>>>> Hi Greg, >>>>>>>> >>>>>>>> On Mon, Jun 17, 2013 at 03:58:47PM -0700, Greg KH wrote: >>>>>>>>> On Mon, Jun 17, 2013 at 10:59:37PM +0200, Oliver Schinagl wrote: >>>>>>>> >>>>>>>> [..] >>>>>>>> >>>>>>>>>> +static int __init sunxi_sid_probe(struct platform_device *pdev) >>>>>>>>>> +{ >>>>>>>>>> + u8 entropy[SID_SIZE]; >>>>>>>>>> + unsigned int i; >>>>>>>>>> + struct resource *res; >>>>>>>>>> + void __iomem *sid_reg_base; >>>>>>>>>> + int ret; >>>>>>>>>> + >>>>>>>>>> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); >>>>>>>>>> + sid_reg_base = devm_ioremap_resource(&pdev->dev, res); >>>>>>>>>> + 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) >>>>>>>>>> + return ret; >>>>>>>>> >>>>>>>>> You just raced with userspace, having the file show up after the device >>>>>>>>> was announced to users that it was there. Please use the proper device >>>>>>>>> file api to add default attributes to prevent this from happening. >>>>>>>> >>>>>>>> Sorry if the question looks dumb, but what kind of race can we generate >>>>>>>> here? >>>>>>> >>>>>>> Userspace gets told about the device from the driver core, udev runs and >>>>>>> reads all of the attributes, then your probe function comes along and >>>>>>> adds a new attribute. Userspace will then not know about it at all. >>>>>>> >>>>>>>> The device_create_bin_file is the last call that we make (if we except >>>>>>>> the entropy stuff, but it doesn't really matter here), so after we >>>>>>>> created the file, we have everything properly initialised so that our >>>>>>>> functions can be called, right? >>>>>>>> >>>>>>>> And another dumb question for you, what is the "proper device file API" >>>>>>>> you are referring to ? :) >>>>>>> >>>>>>> Please read Documentation/driver_model/device.txt and see the section on >>>>>>> Attributes for what to do. If you have specific questions after reading >>>>>>> that, please let me know. >>>>>> Since Maxime kinda asked for me, I hope you don't mind me following up. >>>>>> >>>>>> That doc doesn't mention the binary interface at all. Initially I >>>>>> had both devices up, the 'read' device as a textual representation >>>>>> and added the binary one later. Maxime and I decided the binary one >>>>>> made more sense, as the only textual user would be a human and they >>>>>> don't poke that entry that often. >>>>>> >>>>>> So what default way exists for binary files or how would that be solved? >>>>> >>>>> The same interface should work just fine for binary files, have you >>>>> tried it? >>>> I'll just take the plunge and make myself look stupid ;) >>>> >>>> I tried to change things around, used DEVICE_ATTR(eeprom, S_IRUGO, >>>> sid_read, NULL); So far so good I'd hope. >>> >>> Ick, no. >>> >>>> Of course now I'll have to change the function's parameters from >>>> >>>> static ssize_t sid_read(struct file *fd, struct kobject *kobj, >>>> struct bin_attribute *attr, char *buf, >>>> loff_t pos, size_t size) >>>> >>>> to >>>> >>>> static ssize_t sid_read(struct device *dev, >>>> struct device_attribute *attr, char *buf) >>> >>> Which is what do you do not want, as you find out: >>> >>>> But now, I'm missing things like 'pos' and 'size', both which >>>> determine the requested bytes. True, in this specific driver we are >>>> talking about 'only' 16 bytes, but what if it weren't but a few MiB >>>> and in sysfs we want to read some random byte, will we have to put >>>> the entire blok into the buffer? >>>> >>>> So sorry for not understanding, but ... I don't understand :) >>> >>> Stick with a binary attribute, and attach that to the proper class >>> structure and all should be fine. >>> >>> Ah crap, you're using a platform device. >>> >>> {sigh} >>> >>> Why? Why not use a "real" device which has a "real" class, and then use >>> the interfaces there? >> Because, as I was told, this really is a platform device. If you >> have some example code I can look at and learn from that would be >> awesome. I'm still learning after all, and apparently I'm doing it >> wrong now :) > > I was wrong, you can do this with a platform device just fine. Set the > "groups" field in your platform device->device structure, and all will > work properly, right? Not for me :( Firstly, I have a platform_driver structure, but it has a .driver field and i can set the .groups field there just fine, as your blog post said I believe, so that got me on the right track. 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, .groups = sunxi_sid_attr_groups, }, }; module_platform_driver(sunxi_sid_driver); After jumping through a few hoops, creating an array of attribute_groups, filling that with an array of attribute I finally can assign an attribute to .attr, but I have a bin_attribute. static struct attribute *sunxi_sid_attrs[] = { &sid_bin_attr.attr, NULL, }; static const struct attribute_group sunxi_sid_attr_group = { .attrs = sunxi_sid_attrs, }; static const struct attribute_group *sunxi_sid_attr_groups[] = { &sunxi_sid_attr_group, NULL, }; A feeble attempt to use the .attr from the bin_attribute makes it all crash naturally. Being reminded of LDD3, chapter 14, section 2, re-reading sub-section 3) Binary Attributes We clearly read: "Binary attributes must be created explicitly; they cannot be set up as default attributes. To create a binary attribute, call: int sysfs_create_bin_file(); Which brings us right back to where we started. So I clearly am missing something ;) The other 'broken' drivers that where linked earlier, also use the BINARY attributes. So Greg, if you could be so kind and give me an example how to implement this properly, I am at loss and don't know :( Oliver > > thanks, > > greg k-h >
On Fri, Jul 05, 2013 at 09:24:47AM +0200, Oliver Schinagl wrote: > The other 'broken' drivers that where linked earlier, also use the > BINARY attributes. > > So Greg, if you could be so kind and give me an example how to > implement this properly, I am at loss and don't know :( Ah crap, devices don't have a binary attribute group, like struct class does. I'll go add that on Monday and send you the patch to see if that helps you out. I'll also go through and fix up all of the binary attribute drivers to keep them from doing that... Sorry, I missed that earlier, but thanks for trying and pointing out my mistake. greg k-h
On 07/06/13 21:36, Greg KH wrote: > On Fri, Jul 05, 2013 at 09:24:47AM +0200, Oliver Schinagl wrote: >> The other 'broken' drivers that where linked earlier, also use the >> BINARY attributes. >> >> So Greg, if you could be so kind and give me an example how to >> implement this properly, I am at loss and don't know :( > > Ah crap, devices don't have a binary attribute group, like struct class > does. I'll go add that on Monday and send you the patch to see if that > helps you out. I'll also go through and fix up all of the binary > attribute drivers to keep them from doing that... > > Sorry, I missed that earlier, but thanks for trying and pointing out my > mistake. > > greg k-h > Greg, I know you are a busy man and I hate take away some of your time, but could you be so kind and point me into the right direction and show me what I should do? With your latest patches for binary attributes and your blog post, I thought that you want to create your binary attributes before the probe function, to avoid the userspace race. To do that, we have two options, create them in init (ugly?) or fill the .group member if available so it gets automatically created from the register function. Well in my case, I'm using the module_platform_driver() macro which expects the struct platform_driver. Platform_driver has a device_driver member .driver where the .groups is located. Great, using that works and we should have the sysfs entry race-free. However I don't know hot to exchange data between that and the rest of my driver. Before I used to_platform_device(kobj_to_dev(kobj)) as passed via the .read function to obtain a platform_device where i could use platform_get_drvdata on. All was good, but that doesn't fly now and my knowledge is a bit short as to why. The second method is finding some other shared structure given that we get a platform_device in the probe function, yet I couldn't find anything and this platform_device isn't the same as the one from the .read. Of course using a global var bypasses this issue, but I'm sure it won't pass review ;) So using these new patches for binary attributes, how can I pass data between my driver and the sysfs files using a platform_driver? Or are other 'hacks' needed and using the .groups attribute from platform_driver->device_driver->groups is really the wrong approach. I did ask around and still haven't figured it out so far, so I do apologize if you feel I'm wasting your precious time. Oliver
On Mon, Jul 15, 2013 at 11:16:19PM +0200, Oliver Schinagl wrote: > > With your latest patches for binary attributes and your blog post, I > thought that you want to create your binary attributes before the probe > function, to avoid the userspace race. To do that, we have two options, > create them in init (ugly?) or fill the .group member if available so it > gets automatically created from the register function. Yes, the .group thing should be what is needed here. > Well in my case, I'm using the module_platform_driver() macro which > expects the struct platform_driver. Platform_driver has a device_driver > member .driver where the .groups is located. Great, using that works and > we should have the sysfs entry race-free. However I don't know hot to > exchange data between that and the rest of my driver. > > Before I used to_platform_device(kobj_to_dev(kobj)) as passed via the > .read function to obtain a platform_device where i could use > platform_get_drvdata on. All was good, but that doesn't fly now and my > knowledge is a bit short as to why. I don't understand, why not use the platform device that was passed to the binary attribute write function? > The second method is finding some other shared structure given that we > get a platform_device in the probe function, yet I couldn't find > anything and this platform_device isn't the same as the one from the .read. It should be, why isn't it? > Of course using a global var bypasses this issue, but I'm sure it won't > pass review ;) The platform device structure should have what you need, right? > So using these new patches for binary attributes, how can I pass data > between my driver and the sysfs files using a platform_driver? Or are > other 'hacks' needed and using the .groups attribute from > platform_driver->device_driver->groups is really the wrong approach. > > I did ask around and still haven't figured it out so far, so I do > apologize if you feel I'm wasting your precious time. How is the platform device not the same thing that was passed to your probe function? > > Oliver > /* > * 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 byte > * sized chunks. > */ > > #include <linux/compiler.h> > #include <linux/device.h> > #include <linux/err.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/platform_device.h> > #include <linux/random.h> > #include <linux/sysfs.h> > #include <linux/types.h> > > #define DRV_NAME "sunxi-sid" > > /* There are 4 32-bit keys */ > #define SID_KEYS 4 > /* Each key is 4 bytes long */ > #define SID_SIZE (SID_KEYS * 4) > > /* We read the entire key, due to a 32 bit read alignment requirement. Since we > * want to return the requested byte, this resuls in somewhat slower code and > * uses 4 times more reads as needed but keeps code simpler. Since the SID is > * only very rarly probed, this is not really an issue. > */ > static u8 sunxi_sid_read_byte(const void __iomem *sid_reg_base, > const unsigned int offset) > { > u32 sid_key; > > if (offset >= SID_SIZE) > return 0; > > sid_key = ioread32be(sid_reg_base + round_down(offset, 4)); > sid_key >>= (offset % 4) * 8; > > return sid_key; /* Only return the last byte */ > } > > static ssize_t eeprom_read(struct file *fd, struct kobject *kobj, > struct bin_attribute *attr, char *buf, > loff_t pos, size_t size) > { > struct platform_device *pdev; > void __iomem *sid_reg_base; > int i; > > pdev = to_platform_device(kobj_to_dev(kobj)); > sid_reg_base = (void __iomem *)platform_get_drvdata(pdev); Great, isn't that what you need? > printk("0x%x, 0x%x 0x%x 0x%x\n", kobj, kobj_to_dev(kobj), pdev, sid_reg_base); > > 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 i; > } What are you missing in this function that you have in your probe function? This driver looks fine, what is not working properly? totally confused, greg k-h
On 07/16/13 08:41, Greg KH wrote: > On Mon, Jul 15, 2013 at 11:16:19PM +0200, Oliver Schinagl wrote: >> With your latest patches for binary attributes and your blog post, I >> thought that you want to create your binary attributes before the probe >> function, to avoid the userspace race. To do that, we have two options, >> create them in init (ugly?) or fill the .group member if available so it >> gets automatically created from the register function. > Yes, the .group thing should be what is needed here. That's what I thought (and used). > >> Well in my case, I'm using the module_platform_driver() macro which >> expects the struct platform_driver. Platform_driver has a device_driver >> member .driver where the .groups is located. Great, using that works and >> we should have the sysfs entry race-free. However I don't know hot to >> exchange data between that and the rest of my driver. >> >> Before I used to_platform_device(kobj_to_dev(kobj)) as passed via the >> .read function to obtain a platform_device where i could use >> platform_get_drvdata on. All was good, but that doesn't fly now and my >> knowledge is a bit short as to why. > I don't understand, why not use the platform device that was passed to > the binary attribute write function? Because the pointers don't match and I get a null pointer from platform_get_data > >> The second method is finding some other shared structure given that we >> get a platform_device in the probe function, yet I couldn't find >> anything and this platform_device isn't the same as the one from the .read. > It should be, why isn't it? I think that's a little above my grasp :p > >> Of course using a global var bypasses this issue, but I'm sure it won't >> pass review ;) > The platform device structure should have what you need, right? Should, but doesn't :( > >> So using these new patches for binary attributes, how can I pass data >> between my driver and the sysfs files using a platform_driver? Or are >> other 'hacks' needed and using the .groups attribute from >> platform_driver->device_driver->groups is really the wrong approach. >> >> I did ask around and still haven't figured it out so far, so I do >> apologize if you feel I'm wasting your precious time. > How is the platform device not the same thing that was passed to your > probe function? I don't know :( But i'll add the relevant sections with printk results below, which I should have done before, then again those printk's were not supposed to be in that e-mail to begin with ;) So if I'm not seeing something stupidly obvious, feel free to shout at me :) static ssize_t sid_read(struct file *fd, struct kobject *kobj, struct bin_attribute *attr, char *buf, loff_t pos, size_t size) { struct platform_device *pdev; void __iomem *sid_reg_base; int i; pdev = to_platform_device(kobj_to_dev(kobj)); sid_reg_base = (void __iomem *)platform_get_drvdata(pdev); printk("0x%p, 0x%p, 0x%p, 0x%p\n", kobj, kobj_to_dev(kobj), pdev, sid_reg_base); 0xef1e7c80, 0xef1e7c78, 0xef1e7c68, 0x (null) 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 i; } static struct bin_attribute sid_bin_attr = { .attr = { .name = "eeprom", .mode = S_IRUGO, }, .size = SID_SIZE, .read = sid_read, }; static struct bin_attribute *sunxi_sid_attrs[] = { &sid_bin_attr, NULL, }; static const struct attribute_group sunxi_sid_group = { .bin_attrs = sunxi_sid_attrs, }; static const struct attribute_group *sunxi_sid_groups[] = { &sunxi_sid_group, NULL, }; static int __init sunxi_sid_probe(struct platform_device *pdev) { struct resource *res; void __iomem *sid_reg_base; u8 entropy[SID_SIZE]; unsigned int i; res = platform_get_resource(pdev, IORESOURCE_MEM, 0); sid_reg_base = devm_ioremap_resource(&pdev->dev, res); if (IS_ERR(sid_reg_base)) return PTR_ERR(sid_reg_base); platform_set_drvdata(pdev, sid_reg_base); for (i = 0; i < SID_SIZE; i++) entropy[i] = sunxi_sid_read_byte(sid_reg_base, i); add_device_randomness(entropy, SID_SIZE); dev_dbg(&pdev->dev, "%s loaded\n", DRV_NAME); printk("0x%p, 0x%p\n", pdev, sid_reg_base); 0xef02b000, 0xf1c23800 return 0; }
On Tue, Jul 16, 2013 at 11:02:22PM +0200, Oliver Schinagl wrote: > On 07/16/13 08:41, Greg KH wrote: > > On Mon, Jul 15, 2013 at 11:16:19PM +0200, Oliver Schinagl wrote: > >> With your latest patches for binary attributes and your blog post, I > >> thought that you want to create your binary attributes before the probe > >> function, to avoid the userspace race. To do that, we have two options, > >> create them in init (ugly?) or fill the .group member if available so it > >> gets automatically created from the register function. > > Yes, the .group thing should be what is needed here. > That's what I thought (and used). > > > >> Well in my case, I'm using the module_platform_driver() macro which > >> expects the struct platform_driver. Platform_driver has a device_driver > >> member .driver where the .groups is located. Great, using that works and > >> we should have the sysfs entry race-free. However I don't know hot to > >> exchange data between that and the rest of my driver. > >> > >> Before I used to_platform_device(kobj_to_dev(kobj)) as passed via the > >> .read function to obtain a platform_device where i could use > >> platform_get_drvdata on. All was good, but that doesn't fly now and my > >> knowledge is a bit short as to why. > > I don't understand, why not use the platform device that was passed to > > the binary attribute write function? > Because the pointers don't match and I get a null pointer from > platform_get_data That's not good, that shouldn't happen. > >> The second method is finding some other shared structure given that we > >> get a platform_device in the probe function, yet I couldn't find > >> anything and this platform_device isn't the same as the one from the .read. > > It should be, why isn't it? > I think that's a little above my grasp :p > > > >> Of course using a global var bypasses this issue, but I'm sure it won't > >> pass review ;) > > The platform device structure should have what you need, right? > Should, but doesn't :( > > > >> So using these new patches for binary attributes, how can I pass data > >> between my driver and the sysfs files using a platform_driver? Or are > >> other 'hacks' needed and using the .groups attribute from > >> platform_driver->device_driver->groups is really the wrong approach. > >> > >> I did ask around and still haven't figured it out so far, so I do > >> apologize if you feel I'm wasting your precious time. > > How is the platform device not the same thing that was passed to your > > probe function? > I don't know :( But i'll add the relevant sections with printk results > below, which I should have done before, then again those printk's were > not supposed to be in that e-mail to begin with ;) > > So if I'm not seeing something stupidly obvious, feel free to shout at me :) Your code looks good, and correct, to me, I don't see anything obviously wrong. What creates your platform device in the first place? > > static ssize_t sid_read(struct file *fd, struct kobject *kobj, > struct bin_attribute *attr, char *buf, > loff_t pos, size_t size) > { > struct platform_device *pdev; > void __iomem *sid_reg_base; > int i; > > pdev = to_platform_device(kobj_to_dev(kobj)); > sid_reg_base = (void __iomem *)platform_get_drvdata(pdev); > printk("0x%p, 0x%p, 0x%p, 0x%p\n", kobj, kobj_to_dev(kobj), pdev, > sid_reg_base); > > 0xef1e7c80, 0xef1e7c78, 0xef1e7c68, 0x (null) > > 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 i; > } > > > static struct bin_attribute sid_bin_attr = { > .attr = { .name = "eeprom", .mode = S_IRUGO, }, > .size = SID_SIZE, > .read = sid_read, > }; > > static struct bin_attribute *sunxi_sid_attrs[] = { > &sid_bin_attr, > NULL, > }; > > static const struct attribute_group sunxi_sid_group = { > .bin_attrs = sunxi_sid_attrs, > }; If you create a "normal" attribute here as well, does that work properly? > > static const struct attribute_group *sunxi_sid_groups[] = { > &sunxi_sid_group, > NULL, > }; > > static int __init sunxi_sid_probe(struct platform_device *pdev) > { > struct resource *res; > void __iomem *sid_reg_base; > u8 entropy[SID_SIZE]; > unsigned int i; > > res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > sid_reg_base = devm_ioremap_resource(&pdev->dev, res); > if (IS_ERR(sid_reg_base)) > return PTR_ERR(sid_reg_base); > platform_set_drvdata(pdev, sid_reg_base); > > for (i = 0; i < SID_SIZE; i++) > entropy[i] = sunxi_sid_read_byte(sid_reg_base, i); > add_device_randomness(entropy, SID_SIZE); > dev_dbg(&pdev->dev, "%s loaded\n", DRV_NAME); > printk("0x%p, 0x%p\n", pdev, sid_reg_base); > > 0xef02b000, 0xf1c23800 The memory locations are really different here, that's strange, I don't know what is going on, sorry. Try a text attribute to ensure that works properly. greg k-h
On Mon, Jul 15, 2013 at 11:41:07PM -0700, Greg KH wrote: > On Mon, Jul 15, 2013 at 11:16:19PM +0200, Oliver Schinagl wrote: > > So using these new patches for binary attributes, how can I pass data > > between my driver and the sysfs files using a platform_driver? Or are > > other 'hacks' needed and using the .groups attribute from > > platform_driver->device_driver->groups is really the wrong approach. > > > > I did ask around and still haven't figured it out so far, so I do > > apologize if you feel I'm wasting your precious time. > > How is the platform device not the same thing that was passed to your > probe function? One thing I don't get here is why it should be set in the platform_driver structure. From my understanding of the device model, and since what Oliver is trying to do is exposing a few bytes of memory to sysfs, shouldn't the sysfs file be attached to the device instead? I mean, here, the sysfs file will be created under something like .../drivers/sunxi-sid/eeprom. What happens when you have several instances of that driver loaded? I'd expect it to have several sysfs files created, one for each instance. So to me, it should be in the device structure, not the driver one. Couldn't that be also the reason of Oliver's NULL pointer? If the kobj is attached to the platform_driver and not to the platform_device, it should definitely get nasty when we try to cast it and retrieve data from it (and that would match the different pointers stuff as well.) Maxime
On Wed, Jul 17, 2013 at 01:46:50PM +0200, Maxime Ripard wrote: > On Mon, Jul 15, 2013 at 11:41:07PM -0700, Greg KH wrote: > > On Mon, Jul 15, 2013 at 11:16:19PM +0200, Oliver Schinagl wrote: > > > So using these new patches for binary attributes, how can I pass data > > > between my driver and the sysfs files using a platform_driver? Or are > > > other 'hacks' needed and using the .groups attribute from > > > platform_driver->device_driver->groups is really the wrong approach. > > > > > > I did ask around and still haven't figured it out so far, so I do > > > apologize if you feel I'm wasting your precious time. > > > > How is the platform device not the same thing that was passed to your > > probe function? > > One thing I don't get here is why it should be set in the > platform_driver structure. From my understanding of the device model, > and since what Oliver is trying to do is exposing a few bytes of memory > to sysfs, shouldn't the sysfs file be attached to the device instead? It will be created by the driver core for any device attached to the driver automatically. > I mean, here, the sysfs file will be created under something like > .../drivers/sunxi-sid/eeprom. What happens when you have several > instances of that driver loaded? I'd expect it to have several sysfs > files created, one for each instance. So to me, it should be in the > device structure, not the driver one. You can't have multiple drivers with the same name loaded (or the same module loaded multiple times.) You can have multiple devices for a single driver, which is what we do all the time. > Couldn't that be also the reason of Oliver's NULL pointer? If the kobj > is attached to the platform_driver and not to the platform_device, it > should definitely get nasty when we try to cast it and retrieve data > from it (and that would match the different pointers stuff as well.) No, he's getting a kobject that looks quite different at probe that is different from when the file callback happens, something is odd here... greg k-h
On Wed, Jul 17, 2013 at 09:17:58AM -0700, Greg KH wrote: > On Wed, Jul 17, 2013 at 01:46:50PM +0200, Maxime Ripard wrote: > > On Mon, Jul 15, 2013 at 11:41:07PM -0700, Greg KH wrote: > > > On Mon, Jul 15, 2013 at 11:16:19PM +0200, Oliver Schinagl wrote: > > > > So using these new patches for binary attributes, how can I pass data > > > > between my driver and the sysfs files using a platform_driver? Or are > > > > other 'hacks' needed and using the .groups attribute from > > > > platform_driver->device_driver->groups is really the wrong approach. > > > > > > > > I did ask around and still haven't figured it out so far, so I do > > > > apologize if you feel I'm wasting your precious time. > > > > > > How is the platform device not the same thing that was passed to your > > > probe function? > > > > One thing I don't get here is why it should be set in the > > platform_driver structure. From my understanding of the device model, > > and since what Oliver is trying to do is exposing a few bytes of memory > > to sysfs, shouldn't the sysfs file be attached to the device instead? > > It will be created by the driver core for any device attached to the > driver automatically. > > > I mean, here, the sysfs file will be created under something like > > .../drivers/sunxi-sid/eeprom. What happens when you have several > > instances of that driver loaded? I'd expect it to have several sysfs > > files created, one for each instance. So to me, it should be in the > > device structure, not the driver one. > > You can't have multiple drivers with the same name loaded (or the same > module loaded multiple times.) You can have multiple devices for a > single driver, which is what we do all the time. Yes, I know that, and it's actually my point. With the current oliver's code he pasted earlier in this thread: # find /sys/ -name eeprom /sys/bus/platform/drivers/sunxi-sid/eeprom While I'd expect the eeprom file to be located in /sys/bus/platform/devices/X.eeprom/eeprom like it used to be in the v4, since it's an instance-specific content. Maxime
On Fri, Jul 19, 2013 at 11:42:11AM +0200, Maxime Ripard wrote: > On Wed, Jul 17, 2013 at 09:17:58AM -0700, Greg KH wrote: > > On Wed, Jul 17, 2013 at 01:46:50PM +0200, Maxime Ripard wrote: > > > On Mon, Jul 15, 2013 at 11:41:07PM -0700, Greg KH wrote: > > > > On Mon, Jul 15, 2013 at 11:16:19PM +0200, Oliver Schinagl wrote: > > > > > So using these new patches for binary attributes, how can I pass data > > > > > between my driver and the sysfs files using a platform_driver? Or are > > > > > other 'hacks' needed and using the .groups attribute from > > > > > platform_driver->device_driver->groups is really the wrong approach. > > > > > > > > > > I did ask around and still haven't figured it out so far, so I do > > > > > apologize if you feel I'm wasting your precious time. > > > > > > > > How is the platform device not the same thing that was passed to your > > > > probe function? > > > > > > One thing I don't get here is why it should be set in the > > > platform_driver structure. From my understanding of the device model, > > > and since what Oliver is trying to do is exposing a few bytes of memory > > > to sysfs, shouldn't the sysfs file be attached to the device instead? > > > > It will be created by the driver core for any device attached to the > > driver automatically. > > > > > I mean, here, the sysfs file will be created under something like > > > .../drivers/sunxi-sid/eeprom. What happens when you have several > > > instances of that driver loaded? I'd expect it to have several sysfs > > > files created, one for each instance. So to me, it should be in the > > > device structure, not the driver one. > > > > You can't have multiple drivers with the same name loaded (or the same > > module loaded multiple times.) You can have multiple devices for a > > single driver, which is what we do all the time. > > Yes, I know that, and it's actually my point. > With the current oliver's code he pasted earlier in this thread: > > # find /sys/ -name eeprom > /sys/bus/platform/drivers/sunxi-sid/eeprom > > While I'd expect the eeprom file to be located in > /sys/bus/platform/devices/X.eeprom/eeprom like it used to be in the v4, > since it's an instance-specific content. Oh crap. You are totally right. That's why we added the new device create call, to allow this to work properly. Right now you are getting the kobject of the driver, not the device, in the callback, which is not what you want (sure, if you only have once instance, you can work around it, but don't it's the driver core's fault for not giving you the correct api...) Let me go look at how I can make this work "easier", give me a few days. greg k-h
Hey Greg! On 20-07-13 01:49, Greg KH wrote: > On Fri, Jul 19, 2013 at 11:42:11AM +0200, Maxime Ripard wrote: >> On Wed, Jul 17, 2013 at 09:17:58AM -0700, Greg KH wrote: >>> On Wed, Jul 17, 2013 at 01:46:50PM +0200, Maxime Ripard wrote: >>>> On Mon, Jul 15, 2013 at 11:41:07PM -0700, Greg KH wrote: >>>>> On Mon, Jul 15, 2013 at 11:16:19PM +0200, Oliver Schinagl wrote: >>>>>> So using these new patches for binary attributes, how can I pass data >>>>>> between my driver and the sysfs files using a platform_driver? Or are >>>>>> other 'hacks' needed and using the .groups attribute from >>>>>> platform_driver->device_driver->groups is really the wrong approach. >>>>>> >>>>>> I did ask around and still haven't figured it out so far, so I do >>>>>> apologize if you feel I'm wasting your precious time. >>>>> How is the platform device not the same thing that was passed to your >>>>> probe function? >>>> One thing I don't get here is why it should be set in the >>>> platform_driver structure. From my understanding of the device model, >>>> and since what Oliver is trying to do is exposing a few bytes of memory >>>> to sysfs, shouldn't the sysfs file be attached to the device instead? >>> It will be created by the driver core for any device attached to the >>> driver automatically. >>> >>>> I mean, here, the sysfs file will be created under something like >>>> .../drivers/sunxi-sid/eeprom. What happens when you have several >>>> instances of that driver loaded? I'd expect it to have several sysfs >>>> files created, one for each instance. So to me, it should be in the >>>> device structure, not the driver one. >>> You can't have multiple drivers with the same name loaded (or the same >>> module loaded multiple times.) You can have multiple devices for a >>> single driver, which is what we do all the time. >> Yes, I know that, and it's actually my point. >> With the current oliver's code he pasted earlier in this thread: >> >> # find /sys/ -name eeprom >> /sys/bus/platform/drivers/sunxi-sid/eeprom >> >> While I'd expect the eeprom file to be located in >> /sys/bus/platform/devices/X.eeprom/eeprom like it used to be in the v4, >> since it's an instance-specific content. > Oh crap. You are totally right. That's why we added the new device > create call, to allow this to work properly. > > Right now you are getting the kobject of the driver, not the device, in > the callback, which is not what you want (sure, if you only have once > instance, you can work around it, but don't it's the driver core's fault > for not giving you the correct api...) > > Let me go look at how I can make this work "easier", give me a few days. Not wanting to be rude, but it has been a little more then a few days, any progress? Just want to know what I have to modify my driver to so it can go into the next merge window :) oliver > > greg k-h
On Tue, Jul 30, 2013 at 03:22:55PM +0200, Oliver Schinagl wrote: > >Let me go look at how I can make this work "easier", give me a few days. > Not wanting to be rude, but it has been a little more then a few > days, any progress? Just want to know what I have to modify my > driver to so it can go into the next merge window :) What? Oh crap. I saw this old email in my todo box last week and for some stupid reason I thought I had already taken care of this, otherwise why would I have left it around for so long?... Ugh, very sorry about that... Hm, this is a mess. I hate platform devices... Anyway, as you want to get this into 3.12, and I'm not going to be able to get the core infrastructure into the platform device by then, just go ahead and do a sysfs_create_group() call in your device probe callback for now. That will register the needed files for the device (not the driver, DOH that was stupid of me...) and all should be ok. Yes, you will still race with userspace, but as right now, there's no way that _any_ platform driver can do this "correctly", you will be in good company. I'll clean up all platform drivers in a sweep of the tree after 3.12 or so when I get the needed infrastructure in place for the platform_driver code. Again, very sorry for all of this, you have helped me out a lot in figuring out that this is a mess, and should be fixed up better, but in the end, you are pretty much back at the beginning of what you originally wanted to do, right? I owe you a beer, at the least, my apologies... greg k-h
On 30-07-13 16:20, Greg KH wrote: > On Tue, Jul 30, 2013 at 03:22:55PM +0200, Oliver Schinagl wrote: >>> Let me go look at how I can make this work "easier", give me a few days. >> Not wanting to be rude, but it has been a little more then a few >> days, any progress? Just want to know what I have to modify my >> driver to so it can go into the next merge window :) > > What? Oh crap. > > I saw this old email in my todo box last week and for some stupid reason > I thought I had already taken care of this, otherwise why would I have > left it around for so long?... > > Ugh, very sorry about that... > > Hm, this is a mess. I hate platform devices... > > Anyway, as you want to get this into 3.12, and I'm not going to be able > to get the core infrastructure into the platform device by then, just go > ahead and do a sysfs_create_group() call in your device probe callback > for now. That will register the needed files for the device (not the > driver, DOH that was stupid of me...) and all should be ok. > > Yes, you will still race with userspace, but as right now, there's no > way that _any_ platform driver can do this "correctly", you will be in > good company. I'll clean up all platform drivers in a sweep of the tree > after 3.12 or so when I get the needed infrastructure in place for the > platform_driver code. > > Again, very sorry for all of this, you have helped me out a lot in > figuring out that this is a mess, and should be fixed up better, but in > the end, you are pretty much back at the beginning of what you > originally wanted to do, right? Alright, I'll modify it to use sysfs_create_group() and try to leave it as much as it is no to ease the transition. > > I owe you a beer, at the least, my apologies... Pff, free booze is far better ;) I kid I kid, though Maxime helped a lot there. Ok expect a v6 I think for review and hopefully merge tomorrow. oliver > > greg k-h >
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..6a16c19 --- /dev/null +++ b/drivers/misc/eeprom/sunxi_sid.c @@ -0,0 +1,147 @@ +/* + * 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 byte + * sized chunks. + */ + +#include <linux/compiler.h> +#include <linux/device.h> +#include <linux/err.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/platform_device.h> +#include <linux/random.h> +#include <linux/stat.h> +#include <linux/sysfs.h> +#include <linux/types.h> + +#define DRV_NAME "sunxi-sid" + +/* There are 4 32-bit keys */ +#define SID_KEYS 4 +/* Each key is 4 bytes long */ +#define SID_SIZE (SID_KEYS * 4) + +/* We read the entire key, due to a 32 bit read alignment requirement. Since we + * want to return the requested byte, this resuls in somewhat slower code and + * uses 4 times more reads as needed but keeps code simpler. Since the SID is + * only very rarly probed, this is not really an issue. + */ +static u8 sunxi_sid_read_byte(const void __iomem *sid_reg_base, + const unsigned int offset) +{ + u32 sid_key; + + if (offset >= SID_SIZE) + return 0; + + sid_key = ioread32be(sid_reg_base + round_down(offset, 4)); + sid_key >>= (offset % 4) * 8; + + return sid_key; /* Only return 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 = to_platform_device(kobj_to_dev(kobj)); + sid_reg_base = (void __iomem *)platform_get_drvdata(pdev); + + 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 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_dbg(&pdev->dev, "%s driver unloaded\n", DRV_NAME); + + return 0; +} + +static int __init sunxi_sid_probe(struct platform_device *pdev) +{ + u8 entropy[SID_SIZE]; + unsigned int i; + struct resource *res; + void __iomem *sid_reg_base; + int ret; + + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); + sid_reg_base = devm_ioremap_resource(&pdev->dev, res); + 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) + return ret; + + for (i = 0; i < SID_SIZE; i++) + entropy[i] = sunxi_sid_read_byte(sid_reg_base, i); + add_device_randomness(entropy, SID_SIZE); + dev_dbg(&pdev->dev, "%s loaded\n", DRV_NAME); + + 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); + +MODULE_AUTHOR("Oliver Schinagl <oliver@schinagl.nl>"); +MODULE_DESCRIPTION("Allwinner sunxi security id driver"); +MODULE_LICENSE("GPL");