[PATCH 0/2] v4 Driver for Allwinner sunxi Security ID

[PATCH 0/2] v4 Driver for Allwinner sunxi Security ID

[Oliver Schinagl]

Note, this is a resent because i messed up the first send.

Changes from v3:
  * Cleanup comments
  * Remove last byte masking and useless casting, the C standard guarntees
    we are ok
  * Removed some complexity from sid_read, thanks to Russel
  * Replace dev_info with dev_dbg reducing the verbosity
  * Removed driver version
  * Reorderd variable declrations based on usage, return value always last
  * Removed all goto in exchange for return, due to popular request
  * Reduced line count by removing extra lines

Changes from v2:
  * Removed the global pointer, we can change that when the need for external
    access arises
  * Fixed header inclusions
  * Corrected if guards. There where some crude mistakes there
  * Changed offset to an unsigned int so we don't have to worry about negatives
  * Cleaned up variable declarations
  * Changed ret value, ENXIO (No device/io) as that better matches a missing dt
  * Made the loading informercial print version so it is somewhat usefull

Changes from v1:
  * Renamed the sys-fs exported key to eeprom, since it really a read-only eeprom
  * Removed mention of sun[67]i since we haven't tested those
  * Fixed up mistakes in comments
  * Removed PAGE_SIZE references, since this is a binary only driver
  * Removed lookup table and calculate offsets better
  * Use proper endianess
  * Add the SID to seed the kernel entropy pool
  * Rewrite probe to use platform_get_resource/devm_ioremap_resource instead

The Allwinner A-series of SoC's have efuses exposed via registers to read the
factory programmed e-fuses. These should in theory be programmable but this is
still to be confirmed. It does appear that these fuses are unique enough to be
used as serial numbers, RSA keys, generate MAC addresses from etc. If it turns
out to be user programmable, the use obviously increases. Allwinner did use the
fuses initially to determine the chip-type.

This driver supports all currently known chips based on datasheets and 'dumped'
drivers that we have so far, the dts is only implemented for known chips.

It has been tested on a Cubieboard 1

This is my very first driver so please try to be gentle 

Oliver Schinagl (2):
  Initial support for Allwinner's Security ID fuses
  Add sunxi-sid to dts for sun4i and sun5i

 arch/arm/boot/dts/sun4i-a10.dtsi |   5 ++
 arch/arm/boot/dts/sun5i-a13.dtsi |   5 ++
 drivers/misc/eeprom/Kconfig      |  17 +++++
 drivers/misc/eeprom/Makefile     |   1 +
 drivers/misc/eeprom/sunxi_sid.c  | 147 +++++++++++++++++++++++++++++++++++++++
 5 files changed, 175 insertions(+)
 create mode 100644 drivers/misc/eeprom/sunxi_sid.c

-- 
1.8.1.5

[PATCH 1/2] Initial support for Allwinner's Security ID fuses

[Oliver Schinagl]

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;
+
+	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");
-- 
1.8.1.5

Re: [PATCH 1/2] Initial support for Allwinner's Security ID fuses

[Tomasz Figa]

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");

Re: [PATCH 1/2] Initial support for Allwinner's Security ID fuses

[Greg KH]

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

Re: [linux-sunxi] Re: [PATCH 1/2] Initial support for Allwinner's Security ID fuses

[Henrik Nordström]

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

Re: [PATCH 1/2] Initial support for Allwinner's Security ID fuses

[Andy Shevchenko]

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

Re: [PATCH 1/2] Initial support for Allwinner's Security ID fuses

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 1501 bytes --]

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

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 1/2] Initial support for Allwinner's Security ID fuses

[Greg KH]

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

Re: [PATCH 1/2] Initial support for Allwinner's Security ID fuses

[Oliver Schinagl]

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
>

Re: [PATCH 1/2] Initial support for Allwinner's Security ID fuses

[Greg KH]

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

Re: [PATCH 1/2] Initial support for Allwinner's Security ID fuses

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 1664 bytes --]

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

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 1/2] Initial support for Allwinner's Security ID fuses

[Oliver Schinagl]

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
>

Re: [PATCH 1/2] Initial support for Allwinner's Security ID fuses

[Greg KH]

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

Re: [PATCH 1/2] Initial support for Allwinner's Security ID fuses

[Oliver Schinagl]

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
>

Re: [PATCH 1/2] Initial support for Allwinner's Security ID fuses

[Russell King - ARM Linux]

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.

Re: [PATCH 1/2] Initial support for Allwinner's Security ID fuses

[Geert Uytterhoeven]

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

Re: [PATCH 1/2] Initial support for Allwinner's Security ID fuses

[Greg KH]

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

Re: [PATCH 1/2] Initial support for Allwinner's Security ID fuses

[Greg KH]

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

Re: [PATCH 1/2] Initial support for Allwinner's Security ID fuses

[Greg KH]

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

Re: [PATCH 1/2] Initial support for Allwinner's Security ID fuses

[Oliver Schinagl]

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
>

Re: [PATCH 1/2] Initial support for Allwinner's Security ID fuses

[Greg KH]

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

Re: [PATCH 1/2] Initial support for Allwinner's Security ID fuses

[Greg KH]

On Sat, Jul 06, 2013 at 12:36:46PM -0700, 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.

Can you try this patch out, with your driver, and see if it works for
you?

thanks,

greg k-h

------------------

Subject: driver core: add binary attributes to struct device

From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

This lets a device provide a set of default binary attributes, like
normal attributes, that are initialized and torn down by the driver core
at the proper times, so that there are no races with userspace.

Reported-by: : Oliver Schinagl <oliver+list@schinagl.nl>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>


diff --git a/drivers/base/core.c b/drivers/base/core.c
index 2499cef..4040191 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -433,7 +433,7 @@ static void device_remove_attributes(struct device *dev,
 }
 
 static int device_add_bin_attributes(struct device *dev,
-				     struct bin_attribute *attrs)
+				     const struct bin_attribute *attrs)
 {
 	int error = 0;
 	int i;
@@ -452,7 +452,7 @@ static int device_add_bin_attributes(struct device *dev,
 }
 
 static void device_remove_bin_attributes(struct device *dev,
-					 struct bin_attribute *attrs)
+					 const struct bin_attribute *attrs)
 {
 	int i;
 
@@ -516,8 +516,14 @@ static int device_add_attrs(struct device *dev)
 	if (error)
 		goto err_remove_type_groups;
 
+	error = device_add_bin_attributes(dev, dev->bin_attrs);
+	if (error)
+		goto err_remove_groups;
 	return 0;
 
+ err_remove_groups:
+	device_remove_groups(dev, dev->groups);
+
  err_remove_type_groups:
 	if (type)
 		device_remove_groups(dev, type->groups);
@@ -537,6 +543,7 @@ static void device_remove_attrs(struct device *dev)
 	const struct device_type *type = dev->type;
 
 	device_remove_groups(dev, dev->groups);
+	device_remove_bin_attributes(dev, dev->bin_attrs);
 
 	if (type)
 		device_remove_groups(dev, type->groups);
diff --git a/include/linux/device.h b/include/linux/device.h
index c0a1261..6620ad8 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -645,6 +645,7 @@ struct acpi_dev_node {
  * @knode_class: The node used to add the device to the class list.
  * @class:	The class of the device.
  * @groups:	Optional attribute groups.
+ * @bin_attrs:	Optional binary attributes for this device.
  * @release:	Callback to free the device after all references have
  * 		gone away. This should be set by the allocator of the
  * 		device (i.e. the bus driver that discovered the device).
@@ -717,6 +718,7 @@ struct device {
 	struct klist_node	knode_class;
 	struct class		*class;
 	const struct attribute_group **groups;	/* optional groups */
+	const struct bin_attribute *bin_attrs;
 
 	void	(*release)(struct device *dev);
 	struct iommu_group	*iommu_group;

Re: [linux-sunxi] Re: [PATCH 1/2] Initial support for Allwinner's Security ID fuses

[Oliver Schinagl]

[-- Attachment #1: Type: text/plain, Size: 2497 bytes --]

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

[-- Attachment #2: sunxi_sid.c --]
[-- Type: text/x-csrc, Size: 4119 bytes --]

/*
 * 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);
	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;
}

static const struct of_device_id sunxi_sid_of_match[] = {
	{ .compatible = "allwinner,sun4i-sid", },
	{/* sentinel */},
};
MODULE_DEVICE_TABLE(of, sunxi_sid_of_match);

static int sunxi_sid_remove(struct platform_device *pdev)
{
	dev_dbg(&pdev->dev, "%s driver unloaded\n", DRV_NAME);

	return 0;
}

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);
	printk("0x%x, 0%x, 0x%x\n", sid_reg_base, pdev, &pdev->dev.kobj);

	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;
}

struct bin_attribute sunxi_sid_bin_attr = __BIN_ATTR_RO(eeprom, SID_SIZE);

struct bin_attribute *sunxi_sid_bin_attrs[] = {
	&sunxi_sid_bin_attr,
	NULL,
};

static const struct attribute_group sunxi_sid_bin_group = {
	.bin_attrs = sunxi_sid_bin_attrs,
};

static const struct attribute_group *sunxi_sid_bin_groups[] = {
	&sunxi_sid_bin_group,
	NULL,
};

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_bin_groups,
	},
};
module_platform_driver(sunxi_sid_driver);

MODULE_AUTHOR("Oliver Schinagl <oliver@schinagl.nl>");
MODULE_DESCRIPTION("Allwinner sunxi security id driver");
MODULE_LICENSE("GPL");

Re: [linux-sunxi] Re: [PATCH 1/2] Initial support for Allwinner's Security ID fuses

[Greg KH]

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

Re: [linux-sunxi] Re: [PATCH 1/2] Initial support for Allwinner's Security ID fuses

[Oliver Schinagl]

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;
}

Re: [linux-sunxi] Re: [PATCH 1/2] Initial support for Allwinner's Security ID fuses

[Greg KH]

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

Re: [linux-sunxi] Re: [PATCH 1/2] Initial support for Allwinner's Security ID fuses

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 1646 bytes --]

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

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [linux-sunxi] Re: [PATCH 1/2] Initial support for Allwinner's Security ID fuses

[Greg KH]

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

Re: [linux-sunxi] Re: [PATCH 1/2] Initial support for Allwinner's Security ID fuses

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 2233 bytes --]

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

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [linux-sunxi] Re: [PATCH 1/2] Initial support for Allwinner's Security ID fuses

[Greg KH]

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

Re: [linux-sunxi] Re: [PATCH 1/2] Initial support for Allwinner's Security ID fuses

[Oliver Schinagl]

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

Re: [linux-sunxi] Re: [PATCH 1/2] Initial support for Allwinner's Security ID fuses

[Greg KH]

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

Re: [linux-sunxi] Re: [PATCH 1/2] Initial support for Allwinner's Security ID fuses

[Oliver Schinagl]

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
>