Re: [PATCH 2/3] ARM: sunxi: Add an ahci-platform compatible AHCI driver for the Allwinner SUNXi series of SoCs
On 15-12-13 20:04, Tejun Heo wrote: Hello, Hans. On Sun, Dec 15, 2013 at 08:00:20PM +0100, Hans de Goede wrote: I think it would be a good idea to merge ahci upstream using the ahci_imx.c method for now. You already indicated that you were not against doing that for now. Well, the thing is nothing actually happened since ahci_imx got merged, so I'm wondering maybe there isn't enough pressure. I a slowly progressing on the improved platform driver, but still waiting on an answer about my clock question that I asked a few days ago. Oliver is working on getting a cleaner solution for this, but doing this properly takes tinme, and we would like to move forward with getting sunxi ahci support upstream in the mean time. So if it is ok with you we would like to move forward with the version initially posted. Therefor I would like to request you to review it (glossing over the platform device instantiating a platform device approach for now), and provide us with feedback so that we can do a v2 and start moving this towards the mainline kernel. We still have some time left before the next merge window and if it's really necessary, I can submit drivers post -rc1 too, so I'd still *much* prefer doing it properly rather than creating more drivers which would need to be cleaned up later. Since I do this in my spare time and my technical skill isn't as advanced as some, I don't think I have something reviewed and merge-able before that time frame. I do enjoy the challenge and do like working on the new ahci_platform framework. and do wish to continue to work on it however. Oliver Thanks. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/3] ARM: sunxi: Add an ahci-platform compatible AHCI driver for the Allwinner SUNXi series of SoCs
On 15-12-13 20:04, Tejun Heo wrote: Hello, Hans. On Sun, Dec 15, 2013 at 08:00:20PM +0100, Hans de Goede wrote: I think it would be a good idea to merge ahci upstream using the ahci_imx.c method for now. You already indicated that you were not against doing that for now. Well, the thing is nothing actually happened since ahci_imx got merged, so I'm wondering maybe there isn't enough pressure. I a slowly progressing on the improved platform driver, but still waiting on an answer about my clock question that I asked a few days ago. Oliver is working on getting a cleaner solution for this, but doing this properly takes tinme, and we would like to move forward with getting sunxi ahci support upstream in the mean time. So if it is ok with you we would like to move forward with the version initially posted. Therefor I would like to request you to review it (glossing over the platform device instantiating a platform device approach for now), and provide us with feedback so that we can do a v2 and start moving this towards the mainline kernel. We still have some time left before the next merge window and if it's really necessary, I can submit drivers post -rc1 too, so I'd still *much* prefer doing it properly rather than creating more drivers which would need to be cleaned up later. Since I do this in my spare time and my technical skill isn't as advanced as some, I don't think I have something reviewed and merge-able before that time frame. I do enjoy the challenge and do like working on the new ahci_platform framework. and do wish to continue to work on it however. Oliver Thanks. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/3] ARM: sunxi: Add an ahci-platform compatible AHCI driver for the Allwinner SUNXi series of SoCs
On 06-12-13 10:01, Thomas Petazzoni wrote: Dear Tejun Heo, On Wed, 4 Dec 2013 08:23:12 -0500, Tejun Heo wrote: But again, point me (for dummies ;) in the right direction and I'll work on it with some help. Richard and Shawn recently worked on ahci_imx. Can you guys please talk with each other and figure out what can be done to share as much as possible among these new platform-specific drivers? I'd really like to see the common things factored out as much as possible with only the actual hardware differences described for each device. Also, please Cc me on such discussions. I have a pending AHCI platform driver for another ARM SoC family. It is very similar to ahci_platform, but needs to do a few more things that are SoC specific (map an additional register area, and do some SoC-specific stuff with them). For the moment, we're left with two approaches: * Do what Oliver did, where the ahci_ driver will do its own SoC-specific stuff, and then will register an additional platform_device to trigger the ->probe() of the generic ahci_platform driver. I must say I don't really like this solution, since it involves having two platform_device registered for the same piece of hardware (one platform_device to trigger the ->probe of ahci_, and another one to trigger the ->probe of ahci_platform). * Duplicate in ahci_ the (relatively small) amount of code that is present in ahci_platform. From my point of view, ahci_platform should be turned into a small "library", that provides an API for ahci_ drivers to 1/ do their own custom stuff and 2/ do the common ahci_platform stuff. This way we avoid the registration of two platform_device for the same piece of hardware, and we avoid the duplication of code. Want me to propose a RFC for this idea? I've started to do what sdhci does with their pltfrm driver, assuming that's the right approach. Since i'm only dabbling and not always 100% sure what should or shouldn't be done, it may take a little while, but looks promising from my end ;) So is the sdhci-pltfrm approach the correct one? We still have ahci_* drivers, but ahci_platform.c won't be a driver in the sense that it is now anymore. Oliver Best regards, Thomas -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/3] ARM: sunxi: Add an ahci-platform compatible AHCI driver for the Allwinner SUNXi series of SoCs
On 06-12-13 10:01, Thomas Petazzoni wrote: Dear Tejun Heo, On Wed, 4 Dec 2013 08:23:12 -0500, Tejun Heo wrote: But again, point me (for dummies ;) in the right direction and I'll work on it with some help. Richard and Shawn recently worked on ahci_imx. Can you guys please talk with each other and figure out what can be done to share as much as possible among these new platform-specific drivers? I'd really like to see the common things factored out as much as possible with only the actual hardware differences described for each device. Also, please Cc me on such discussions. I have a pending AHCI platform driver for another ARM SoC family. It is very similar to ahci_platform, but needs to do a few more things that are SoC specific (map an additional register area, and do some SoC-specific stuff with them). For the moment, we're left with two approaches: * Do what Oliver did, where the ahci_foo driver will do its own SoC-specific stuff, and then will register an additional platform_device to trigger the -probe() of the generic ahci_platform driver. I must say I don't really like this solution, since it involves having two platform_device registered for the same piece of hardware (one platform_device to trigger the -probe of ahci_foo, and another one to trigger the -probe of ahci_platform). * Duplicate in ahci_foo the (relatively small) amount of code that is present in ahci_platform. From my point of view, ahci_platform should be turned into a small library, that provides an API for ahci_foo drivers to 1/ do their own custom stuff and 2/ do the common ahci_platform stuff. This way we avoid the registration of two platform_device for the same piece of hardware, and we avoid the duplication of code. Want me to propose a RFC for this idea? I've started to do what sdhci does with their pltfrm driver, assuming that's the right approach. Since i'm only dabbling and not always 100% sure what should or shouldn't be done, it may take a little while, but looks promising from my end ;) So is the sdhci-pltfrm approach the correct one? We still have ahci_* drivers, but ahci_platform.c won't be a driver in the sense that it is now anymore. Oliver Best regards, Thomas -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/3] RFC: AHCI: libahci is missing DMA
On 04-12-13 13:47, Sergei Shtylyov wrote:
Hello.
On 04-12-2013 16:10, oli...@schinagl.nl wrote:
From: Oliver Schinagl
The Allwinner sunxi platforms have patched in the following to enable
DMA. This patch enables DMA controllers for the SUNXI Architecture.
Signed-off-by: Olliver Schinagl
---
drivers/ata/ahci.h| 6 ++
drivers/ata/libahci.c | 8
2 files changed, 14 insertions(+)
diff --git a/drivers/ata/ahci.h b/drivers/ata/ahci.h
index 2289efdf..2bf2423 100644
--- a/drivers/ata/ahci.h
+++ b/drivers/ata/ahci.h
[...]
@@ -209,6 +210,11 @@ enum {
PORT_DEVSLP_DSP= (1 << 1), /* DevSlp present */
PORT_DEVSLP_ADSE= (1 << 0), /* Aggressive DevSlp
enable */
+/* PORT_DMA bits */
+PORT_DMA_SETUP_OFFSET= 8, /* dma setup offset */
+PORT_DMA_SETUP_MASK= (0xff << PORT_DMA_SETUP_OFFSET),/* dma
mask */
+PORT_DMA_SETUP_INIT= (0x44 << 0),
Why not shift it right by PORT_DMA_SETUP_OFFSET if you do it in the
next file anyway?
Verbosity and clarity I suppose, try to reduce the number of magic
values. I don't know what the preferred way is as we know so little
about the IP.
[...]
diff --git a/drivers/ata/libahci.c b/drivers/ata/libahci.c
index c482f8c..d697a74 100644
--- a/drivers/ata/libahci.c
+++ b/drivers/ata/libahci.c
@@ -570,6 +570,14 @@ void ahci_start_engine(struct ata_port *ap)
void __iomem *port_mmio = ahci_port_base(ap);
u32 tmp;
+#ifdef CONFIG_ARCH_SUNXI
+/* Setup DMA before DMA start */
+tmp = readl(port_mmio + PORT_DMA);
+tmp &= ~PORT_DMA_SETUP_MASK;
+tmp |= PORT_DMA_SETUP_INIT << PORT_DMA_SETUP_OFFSET;
+writel(tmp, port_mmio + PORT_DMA);
+#endif
+
I don't think #ifdef is appropriate here.
It's not, but I was hoping Smarter devs then me would point me the right
way ;)
Oliver
WBR, Sergei
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/3] ARM: sunxi: Add an ahci-platform compatible AHCI driver for the Allwinner SUNXi series of SoCs
On 04-12-13 13:37, Tejun Heo wrote: On Wed, Dec 04, 2013 at 01:10:54PM +0100, oli...@schinagl.nl wrote: From: Oliver Schinagl This patch adds support for the sunxi series of SoC's by allwinner. It plugs into the ahci-platform framework. Note: Currently it uses a somewhat hackish approach that probably needs a lot more work, but does the same as the IMX SoC's. Signed-off-by: Olliver Schinagl --- .../devicetree/bindings/ata/ahci-sunxi.txt | 24 ++ drivers/ata/Kconfig| 9 + drivers/ata/Makefile | 1 + drivers/ata/ahci_platform.c| 12 + drivers/ata/ahci_sunxi.c | 305 + I'm not really liking the way things are going. Do we really need separate drivers for each platform ahci implementation. Are they really that different? Would it be impossible to make ahci_platform generic enough so that we don't eventually end up with a gazillion ahci_XXX drivers? I took the imx driver as example, as I wasn't sure on where to start. But I don't think it's possible yet without improving ahci_platform as I suggested in the cover letter. So if ahci_platform needs to be improved, I guess a separate patch series would be more appropriate? So would it be acceptable to have this as the 2nd (and last?) ahci_platform driver and go from there? Or do you want to block new ahci_XXX drivers until ahci_platform has been improved? Oliver -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/3] ARM: sunxi: Add an ahci-platform compatible AHCI driver for the Allwinner SUNXi series of SoCs
On 04-12-13 13:26, Mark Rutland wrote: On Wed, Dec 04, 2013 at 12:10:54PM +, oli...@schinagl.nl wrote: From: Oliver Schinagl This patch adds support for the sunxi series of SoC's by allwinner. It plugs into the ahci-platform framework. Note: Currently it uses a somewhat hackish approach that probably needs a lot more work, but does the same as the IMX SoC's. Signed-off-by: Olliver Schinagl --- .../devicetree/bindings/ata/ahci-sunxi.txt | 24 ++ drivers/ata/Kconfig| 9 + drivers/ata/Makefile | 1 + drivers/ata/ahci_platform.c| 12 + drivers/ata/ahci_sunxi.c | 305 + 5 files changed, 351 insertions(+) create mode 100644 Documentation/devicetree/bindings/ata/ahci-sunxi.txt create mode 100644 drivers/ata/ahci_sunxi.c diff --git a/Documentation/devicetree/bindings/ata/ahci-sunxi.txt b/Documentation/devicetree/bindings/ata/ahci-sunxi.txt new file mode 100644 index 000..0792fa5 --- /dev/null +++ b/Documentation/devicetree/bindings/ata/ahci-sunxi.txt @@ -0,0 +1,24 @@ +Allwinner SUNXI AHCI SATA Controller + +SATA nodes are defined to describe on-chip Serial ATA controllers. +Each SATA controller should have its own node. + +Required properties: +- compatible : compatible list, contains "allwinner,sun4i-a10-ahci" - compatible: Should contain "allwinner,sun4i-a10-ahci" +- reg : - reg: The offset and length of the MMIO registers. +- interrupts : - interrupts: An interrupt-specifier for the ACHI interrupt +- clocks : clocks for ACHI +- clock-names : clock names for AHCI Please _define_ the set of clock-names you expect. This binding is meaningless without it. If you require clock-names, define the clocks property in terms of it. I copied ahci_platform.txt and filled in the missing bits, I will improve all the above. Appologies! Thanks, Mark. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/3] RFC: AHCI: libahci is missing DMA
Hey Tejun Heo,
On 04-12-13 13:32, Tejun Heo wrote:
On Wed, Dec 04, 2013 at 01:10:53PM +0100, oli...@schinagl.nl wrote:
From: Oliver Schinagl
The Allwinner sunxi platforms have patched in the following to enable
DMA. This patch enables DMA controllers for the SUNXI Architecture.
Signed-off-by: Olliver Schinagl
---
drivers/ata/ahci.h| 6 ++
drivers/ata/libahci.c | 8
2 files changed, 14 insertions(+)
diff --git a/drivers/ata/ahci.h b/drivers/ata/ahci.h
index 2289efdf..2bf2423 100644
--- a/drivers/ata/ahci.h
+++ b/drivers/ata/ahci.h
@@ -138,6 +138,7 @@ enum {
PORT_SCR_NTF= 0x3c, /* SATA phy register: SNotification */
PORT_FBS= 0x40, /* FIS-based Switching */
PORT_DEVSLP = 0x44, /* device sleep */
+ PORT_DMA= 0x70, /* direct memory access */
/* PORT_IRQ_{STAT,MASK} bits */
PORT_IRQ_COLD_PRES = (1 << 31), /* cold presence detect */
@@ -209,6 +210,11 @@ enum {
PORT_DEVSLP_DSP = (1 << 1), /* DevSlp present */
PORT_DEVSLP_ADSE= (1 << 0), /* Aggressive DevSlp enable */
+ /* PORT_DMA bits */
+ PORT_DMA_SETUP_OFFSET = 8, /* dma setup offset */
+ PORT_DMA_SETUP_MASK = (0xff << PORT_DMA_SETUP_OFFSET),/* dma mask */
+ PORT_DMA_SETUP_INIT = (0x44 << 0),
Ummm... this doesn't belong to ahci proper, right?
I have no idea why Allwinner added that and what it really does. We have
no documentation, only code drops.
I had high hopes someone around here knows what it could mean and where
it does belong.
+
/* hpriv->flags bits */
#define AHCI_HFLAGS(flags) .private_data = (void *)(flags)
diff --git a/drivers/ata/libahci.c b/drivers/ata/libahci.c
index c482f8c..d697a74 100644
--- a/drivers/ata/libahci.c
+++ b/drivers/ata/libahci.c
@@ -570,6 +570,14 @@ void ahci_start_engine(struct ata_port *ap)
void __iomem *port_mmio = ahci_port_base(ap);
u32 tmp;
+#ifdef CONFIG_ARCH_SUNXI
+ /* Setup DMA before DMA start */
+ tmp = readl(port_mmio + PORT_DMA);
+ tmp &= ~PORT_DMA_SETUP_MASK;
+ tmp |= PORT_DMA_SETUP_INIT << PORT_DMA_SETUP_OFFSET;
+ writel(tmp, port_mmio + PORT_DMA);
+#endif
If this is something platform device specific, wouldn't overriding
->port_start() which wraps around ahci_port_start() make more sense?
Again, I don't know, this is where Allwinner had put it. We don't even
know who's IP they use. I'm happy to start experimenting moving this
around a bit and will take your clue to figure out what you mean and if
it could work.
Oliver
Thanks.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/3] RFC: AHCI: libahci is missing DMA
Hey Tejun Heo,
On 04-12-13 13:32, Tejun Heo wrote:
On Wed, Dec 04, 2013 at 01:10:53PM +0100, oli...@schinagl.nl wrote:
From: Oliver Schinagl oli...@schinagl.nl
The Allwinner sunxi platforms have patched in the following to enable
DMA. This patch enables DMA controllers for the SUNXI Architecture.
Signed-off-by: Olliver Schinagl oli...@schinagl.nl
---
drivers/ata/ahci.h| 6 ++
drivers/ata/libahci.c | 8
2 files changed, 14 insertions(+)
diff --git a/drivers/ata/ahci.h b/drivers/ata/ahci.h
index 2289efdf..2bf2423 100644
--- a/drivers/ata/ahci.h
+++ b/drivers/ata/ahci.h
@@ -138,6 +138,7 @@ enum {
PORT_SCR_NTF= 0x3c, /* SATA phy register: SNotification */
PORT_FBS= 0x40, /* FIS-based Switching */
PORT_DEVSLP = 0x44, /* device sleep */
+ PORT_DMA= 0x70, /* direct memory access */
/* PORT_IRQ_{STAT,MASK} bits */
PORT_IRQ_COLD_PRES = (1 31), /* cold presence detect */
@@ -209,6 +210,11 @@ enum {
PORT_DEVSLP_DSP = (1 1), /* DevSlp present */
PORT_DEVSLP_ADSE= (1 0), /* Aggressive DevSlp enable */
+ /* PORT_DMA bits */
+ PORT_DMA_SETUP_OFFSET = 8, /* dma setup offset */
+ PORT_DMA_SETUP_MASK = (0xff PORT_DMA_SETUP_OFFSET),/* dma mask */
+ PORT_DMA_SETUP_INIT = (0x44 0),
Ummm... this doesn't belong to ahci proper, right?
I have no idea why Allwinner added that and what it really does. We have
no documentation, only code drops.
I had high hopes someone around here knows what it could mean and where
it does belong.
+
/* hpriv-flags bits */
#define AHCI_HFLAGS(flags) .private_data = (void *)(flags)
diff --git a/drivers/ata/libahci.c b/drivers/ata/libahci.c
index c482f8c..d697a74 100644
--- a/drivers/ata/libahci.c
+++ b/drivers/ata/libahci.c
@@ -570,6 +570,14 @@ void ahci_start_engine(struct ata_port *ap)
void __iomem *port_mmio = ahci_port_base(ap);
u32 tmp;
+#ifdef CONFIG_ARCH_SUNXI
+ /* Setup DMA before DMA start */
+ tmp = readl(port_mmio + PORT_DMA);
+ tmp = ~PORT_DMA_SETUP_MASK;
+ tmp |= PORT_DMA_SETUP_INIT PORT_DMA_SETUP_OFFSET;
+ writel(tmp, port_mmio + PORT_DMA);
+#endif
If this is something platform device specific, wouldn't overriding
-port_start() which wraps around ahci_port_start() make more sense?
Again, I don't know, this is where Allwinner had put it. We don't even
know who's IP they use. I'm happy to start experimenting moving this
around a bit and will take your clue to figure out what you mean and if
it could work.
Oliver
Thanks.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/3] ARM: sunxi: Add an ahci-platform compatible AHCI driver for the Allwinner SUNXi series of SoCs
On 04-12-13 13:26, Mark Rutland wrote: On Wed, Dec 04, 2013 at 12:10:54PM +, oli...@schinagl.nl wrote: From: Oliver Schinagl oli...@schinagl.nl This patch adds support for the sunxi series of SoC's by allwinner. It plugs into the ahci-platform framework. Note: Currently it uses a somewhat hackish approach that probably needs a lot more work, but does the same as the IMX SoC's. Signed-off-by: Olliver Schinagl oli...@schinagl.nl --- .../devicetree/bindings/ata/ahci-sunxi.txt | 24 ++ drivers/ata/Kconfig| 9 + drivers/ata/Makefile | 1 + drivers/ata/ahci_platform.c| 12 + drivers/ata/ahci_sunxi.c | 305 + 5 files changed, 351 insertions(+) create mode 100644 Documentation/devicetree/bindings/ata/ahci-sunxi.txt create mode 100644 drivers/ata/ahci_sunxi.c diff --git a/Documentation/devicetree/bindings/ata/ahci-sunxi.txt b/Documentation/devicetree/bindings/ata/ahci-sunxi.txt new file mode 100644 index 000..0792fa5 --- /dev/null +++ b/Documentation/devicetree/bindings/ata/ahci-sunxi.txt @@ -0,0 +1,24 @@ +Allwinner SUNXI AHCI SATA Controller + +SATA nodes are defined to describe on-chip Serial ATA controllers. +Each SATA controller should have its own node. + +Required properties: +- compatible : compatible list, contains allwinner,sun4i-a10-ahci - compatible: Should contain allwinner,sun4i-a10-ahci +- reg : registers mapping - reg: The offset and length of the MMIO registers. +- interrupts : interrupt mapping for AHCI IRQ - interrupts: An interrupt-specifier for the ACHI interrupt +- clocks : clocks for ACHI +- clock-names : clock names for AHCI Please _define_ the set of clock-names you expect. This binding is meaningless without it. If you require clock-names, define the clocks property in terms of it. I copied ahci_platform.txt and filled in the missing bits, I will improve all the above. Appologies! Thanks, Mark. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/3] ARM: sunxi: Add an ahci-platform compatible AHCI driver for the Allwinner SUNXi series of SoCs
On 04-12-13 13:37, Tejun Heo wrote: On Wed, Dec 04, 2013 at 01:10:54PM +0100, oli...@schinagl.nl wrote: From: Oliver Schinagl oli...@schinagl.nl This patch adds support for the sunxi series of SoC's by allwinner. It plugs into the ahci-platform framework. Note: Currently it uses a somewhat hackish approach that probably needs a lot more work, but does the same as the IMX SoC's. Signed-off-by: Olliver Schinagl oli...@schinagl.nl --- .../devicetree/bindings/ata/ahci-sunxi.txt | 24 ++ drivers/ata/Kconfig| 9 + drivers/ata/Makefile | 1 + drivers/ata/ahci_platform.c| 12 + drivers/ata/ahci_sunxi.c | 305 + I'm not really liking the way things are going. Do we really need separate drivers for each platform ahci implementation. Are they really that different? Would it be impossible to make ahci_platform generic enough so that we don't eventually end up with a gazillion ahci_XXX drivers? I took the imx driver as example, as I wasn't sure on where to start. But I don't think it's possible yet without improving ahci_platform as I suggested in the cover letter. So if ahci_platform needs to be improved, I guess a separate patch series would be more appropriate? So would it be acceptable to have this as the 2nd (and last?) ahci_platform driver and go from there? Or do you want to block new ahci_XXX drivers until ahci_platform has been improved? Oliver -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/3] RFC: AHCI: libahci is missing DMA
On 04-12-13 13:47, Sergei Shtylyov wrote:
Hello.
On 04-12-2013 16:10, oli...@schinagl.nl wrote:
From: Oliver Schinagl oli...@schinagl.nl
The Allwinner sunxi platforms have patched in the following to enable
DMA. This patch enables DMA controllers for the SUNXI Architecture.
Signed-off-by: Olliver Schinagl oli...@schinagl.nl
---
drivers/ata/ahci.h| 6 ++
drivers/ata/libahci.c | 8
2 files changed, 14 insertions(+)
diff --git a/drivers/ata/ahci.h b/drivers/ata/ahci.h
index 2289efdf..2bf2423 100644
--- a/drivers/ata/ahci.h
+++ b/drivers/ata/ahci.h
[...]
@@ -209,6 +210,11 @@ enum {
PORT_DEVSLP_DSP= (1 1), /* DevSlp present */
PORT_DEVSLP_ADSE= (1 0), /* Aggressive DevSlp
enable */
+/* PORT_DMA bits */
+PORT_DMA_SETUP_OFFSET= 8, /* dma setup offset */
+PORT_DMA_SETUP_MASK= (0xff PORT_DMA_SETUP_OFFSET),/* dma
mask */
+PORT_DMA_SETUP_INIT= (0x44 0),
Why not shift it right by PORT_DMA_SETUP_OFFSET if you do it in the
next file anyway?
Verbosity and clarity I suppose, try to reduce the number of magic
values. I don't know what the preferred way is as we know so little
about the IP.
[...]
diff --git a/drivers/ata/libahci.c b/drivers/ata/libahci.c
index c482f8c..d697a74 100644
--- a/drivers/ata/libahci.c
+++ b/drivers/ata/libahci.c
@@ -570,6 +570,14 @@ void ahci_start_engine(struct ata_port *ap)
void __iomem *port_mmio = ahci_port_base(ap);
u32 tmp;
+#ifdef CONFIG_ARCH_SUNXI
+/* Setup DMA before DMA start */
+tmp = readl(port_mmio + PORT_DMA);
+tmp = ~PORT_DMA_SETUP_MASK;
+tmp |= PORT_DMA_SETUP_INIT PORT_DMA_SETUP_OFFSET;
+writel(tmp, port_mmio + PORT_DMA);
+#endif
+
I don't think #ifdef is appropriate here.
It's not, but I was hoping Smarter devs then me would point me the right
way ;)
Oliver
WBR, Sergei
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] ARM: sunxi: doc: Add sun7i (A20) interrupt table
From: Oliver Schinagl Having the interrupt tables documented in the kernel documentation is useful as a quick reference. Oliver Schinagl (1): ARM: sunxi: doc: Add sun7i (A20) interrupt table .../interrupt-controller/sunxi/sun7i-a20.txt | 102 + 1 file changed, 102 insertions(+) create mode 100644 Documentation/devicetree/bindings/interrupt-controller/sunxi/sun7i-a20.txt -- 1.8.1.5 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH] [PATCH] ARM: sunxi: doc: Add sun7i (A20) interrupt table
From: Oliver Schinagl This patch adds some documentation about the Allwinner sun7i (A20) using the GIC. Signed-off-by: Oliver Schinagl --- .../interrupt-controller/sunxi/sun7i-a20.txt | 102 + 1 file changed, 102 insertions(+) create mode 100644 Documentation/devicetree/bindings/interrupt-controller/sunxi/sun7i-a20.txt diff --git a/Documentation/devicetree/bindings/interrupt-controller/sunxi/sun7i-a20.txt b/Documentation/devicetree/bindings/interrupt-controller/sunxi/sun7i-a20.txt new file mode 100644 index 000..e5e59df --- /dev/null +++ b/Documentation/devicetree/bindings/interrupt-controller/sunxi/sun7i-a20.txt @@ -0,0 +1,102 @@ +Allwinner A20 (sun7i) interrupt sources +--- +The sun7i interrupt controller is actually a standard arm GIC one. For more +information see Documentation/devicetree/bindings/arm/gic.txt + +Interrupts are offset by 32, skipping the SGI and PPI's. The IRQ numbers here +is what is expected in the drivers device tree table. + +The interrupt sources available for the Allwinner A20 SoC are the +following ones: + +0: ENMI +1: UART0 +2: UART1 +3: UART2 +4: UART3 +5: IR0 +6: IR1 +7: I2C0 +8: I2C1 +9: I2C2 +10: SPI0 +11: SPI1 +12: SPI2 +13: SPDIF +14: AC97 +15: TS +16: I2S0 +17: UART4 +18: UART5 +19: UART6 +20: UART7 +21: KEYPAD +22: TIMER0 +23: TIMER1 +24: TIMER2 +25: TIMER3 +26: CAN +27: DMA +28: PIO +29: TOUCH_PANEL +30: AUDIO_CODEC +31: LRADC +32: MMC0 +33: MMC1 +34: MMC2 +35: MMC3 +36: MEMSTICK +37: NAND +38: USB0 +39: USB1 +40: USB2 +41: SCR +42: CSI0 +43: CSI1 +44: LCDCTRL0 +45: LCDCTRL1 +46: MP +47: DEFEBE0 +48: DEFEBE1 +49: PMU +50: SPI3 +51: TZASC +52: PATA +53: VE +54: SS +55: EMAC +56: SATA +57: GPS +58: HDMI +59: TVE +60: ACE +61: TVD +62: PS2_0 +63: PS2_1 +64: USB3 +65: USB4 +66: PLE_PFM +67: TIMER4 +68: TIMER5 +69: GPU_GP +70: GPU_GPMMU +71: GPU_PP0 +72: GPU_PPMMU0 +73: GPU_PMU +74: GPU_PP1 +75: GPU_PPMMU1 +76: GPU_RSV0 +77: GPU_RSV1 +78: GPU_RSV2 +79: GPU_RSV3 +80: GPU_RSV4 +81: HS_TIMER0 +82: HS_TIMER1 +83: HS_TIMER2 +84: HS_TIMER3 +85: GMAC +86: HDMI1 +87: I2S1 +88: I2C3 +89: I2C4 +90: I2S2 -- 1.8.1.5 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH] ARM: sunxi: doc: Add sun7i (A20) interrupt table
From: Oliver Schinagl oli...@schinagl.nl Having the interrupt tables documented in the kernel documentation is useful as a quick reference. Oliver Schinagl (1): ARM: sunxi: doc: Add sun7i (A20) interrupt table .../interrupt-controller/sunxi/sun7i-a20.txt | 102 + 1 file changed, 102 insertions(+) create mode 100644 Documentation/devicetree/bindings/interrupt-controller/sunxi/sun7i-a20.txt -- 1.8.1.5 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH] [PATCH] ARM: sunxi: doc: Add sun7i (A20) interrupt table
From: Oliver Schinagl oli...@schinagl.nl This patch adds some documentation about the Allwinner sun7i (A20) using the GIC. Signed-off-by: Oliver Schinagl oli...@schinagl.nl --- .../interrupt-controller/sunxi/sun7i-a20.txt | 102 + 1 file changed, 102 insertions(+) create mode 100644 Documentation/devicetree/bindings/interrupt-controller/sunxi/sun7i-a20.txt diff --git a/Documentation/devicetree/bindings/interrupt-controller/sunxi/sun7i-a20.txt b/Documentation/devicetree/bindings/interrupt-controller/sunxi/sun7i-a20.txt new file mode 100644 index 000..e5e59df --- /dev/null +++ b/Documentation/devicetree/bindings/interrupt-controller/sunxi/sun7i-a20.txt @@ -0,0 +1,102 @@ +Allwinner A20 (sun7i) interrupt sources +--- +The sun7i interrupt controller is actually a standard arm GIC one. For more +information see Documentation/devicetree/bindings/arm/gic.txt + +Interrupts are offset by 32, skipping the SGI and PPI's. The IRQ numbers here +is what is expected in the drivers device tree table. + +The interrupt sources available for the Allwinner A20 SoC are the +following ones: + +0: ENMI +1: UART0 +2: UART1 +3: UART2 +4: UART3 +5: IR0 +6: IR1 +7: I2C0 +8: I2C1 +9: I2C2 +10: SPI0 +11: SPI1 +12: SPI2 +13: SPDIF +14: AC97 +15: TS +16: I2S0 +17: UART4 +18: UART5 +19: UART6 +20: UART7 +21: KEYPAD +22: TIMER0 +23: TIMER1 +24: TIMER2 +25: TIMER3 +26: CAN +27: DMA +28: PIO +29: TOUCH_PANEL +30: AUDIO_CODEC +31: LRADC +32: MMC0 +33: MMC1 +34: MMC2 +35: MMC3 +36: MEMSTICK +37: NAND +38: USB0 +39: USB1 +40: USB2 +41: SCR +42: CSI0 +43: CSI1 +44: LCDCTRL0 +45: LCDCTRL1 +46: MP +47: DEFEBE0 +48: DEFEBE1 +49: PMU +50: SPI3 +51: TZASC +52: PATA +53: VE +54: SS +55: EMAC +56: SATA +57: GPS +58: HDMI +59: TVE +60: ACE +61: TVD +62: PS2_0 +63: PS2_1 +64: USB3 +65: USB4 +66: PLE_PFM +67: TIMER4 +68: TIMER5 +69: GPU_GP +70: GPU_GPMMU +71: GPU_PP0 +72: GPU_PPMMU0 +73: GPU_PMU +74: GPU_PP1 +75: GPU_PPMMU1 +76: GPU_RSV0 +77: GPU_RSV1 +78: GPU_RSV2 +79: GPU_RSV3 +80: GPU_RSV4 +81: HS_TIMER0 +82: HS_TIMER1 +83: HS_TIMER2 +84: HS_TIMER3 +85: GMAC +86: HDMI1 +87: I2S1 +88: I2C3 +89: I2C4 +90: I2S2 -- 1.8.1.5 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [linux-sunxi] Re: [PATCH 1/2] Initial support for Allwinner's Security ID fuses
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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [linux-sunxi] Re: [PATCH 1/2] Initial support for Allwinner's Security ID fuses
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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [linux-sunxi] Re: [PATCH 1/2] Initial support for Allwinner's Security ID fuses
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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [linux-sunxi] Re: [PATCH 1/2] Initial support for Allwinner's Security ID fuses
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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [linux-sunxi] Re: [PATCH 1/2] Initial support for Allwinner's Security ID fuses
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[] = {
_bin_attr,
NULL,
};
static const struct attribute_group sunxi_sid_group = {
.bin_attrs = sunxi_sid_attrs,
};
static const struct attribute_group *sunxi_sid_groups[] = {
_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(>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(>dev, "%s loaded\n", DRV_NAME);
printk("0x%p, 0x%p\n", pdev, sid_reg_base);
0xef02b000, 0xf1c23800
return 0;
}
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [linux-sunxi] Re: [PATCH 1/2] Initial support for Allwinner's Security ID fuses
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;
}
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [linux-sunxi] Re: [PATCH 1/2] Initial support for Allwinner's Security ID fuses
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
/*
* Copyright (c) 2013 Oliver Schinagl
* http://www.linux-sunxi.org
*
* Oliver Schinagl
*
* 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
#include
#include
#include
#include
#include
#include
#include
#include
#include
#include
#include
#include
#include
#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(>dev, "%s driver unloaded\n", DRV_NAME);
Re: [linux-sunxi] Re: [PATCH 1/2] Initial support for Allwinner's Security ID fuses
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
/*
* Copyright (c) 2013 Oliver Schinagl
* http://www.linux-sunxi.org
*
* Oliver Schinagl oli...@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
Re: [PATCH v2 2/7] sysfs.h: add ATTRIBUTE_GROUPS() macro
Greg,
Haven't heard anything about this v2 patch, I know you are very busy but
with the merge window closing today/yesterday just wondering about the
status of it all.
Oliver
On 11-07-13 22:55, Oliver Schinagl wrote:
On 07/11/13 22:26, Greg Kroah-Hartman wrote:
On Thu, Jul 11, 2013 at 10:09:11PM +0200, Oliver Schinagl wrote:
On 07/11/13 19:06, Greg Kroah-Hartman wrote:
On Thu, Jul 11, 2013 at 01:58:29PM +0200, Oliver Schinagl wrote:
On 11-07-13 02:36, Greg Kroah-Hartman wrote:
To make it easier for driver subsystems to work with attribute
groups,
create the ATTRIBUTE_GROUPS macro to remove some of the repetitive
typing for the most common use for attribute groups.
But binary groups are discriminated against :(
Yes, as they are "rarer" by far, as they should be. binary sysfs
files
should almost never be used, as they are only "pass-through" files to
the hardware, so I want to see you do more work in order to use
them, as
they should not be created lightly.
I guess I can see a valid reason here, but they are only helper
macro's making life easier for people who do need to use these and
are on par with the non-binary versions. And we already have quite
some binary attributes, probably far less then normal ones :)
I only count about 100 valid binary files in the tree at the moment,
that's not really all that many to handle.
100 is quite a few :) But point taken.
Anyway, wouldn't all users be reviewed anyway? But I guess it's a
small safety net to make it not TOO easy.
exactly :)
I aggree and this is a v2 that strips all the additional bits.
A few comments left below.
The attached patch should help here.
Can you give me an example of using these macros? I seem to be
lost in
them somehow, or maybe my morning coffee just hasn't kicked in...
Yeah, I kinda added the whole shebang there :) I was trying being
helpful :(
I suppose one more additional helper wouldn't be bad to have:
#define ATTRIBUTE_(BIN_)GROUPS_R[O/W](_name(, _size)) \
ATTRIBUTE_(BIN_)ATTR_R[O/W](_name, _size); \
ATTRIBUTE_(BIN_)GROUPS(_name)
Would that ever be needed?
Of ourse, by the lazy :)
I think now you create an attribute in a group as this (with this
patch):
ATTRIBUTE_ATTR_RO(name, SIZE);
but "raw" attributes are rare, you really want a "device", "class", or
"bus" attribute, right?
I suppose so, But I got stuck in the rare case some how initially. I
registered my driver with module_platform_driver(); and in that struct
i had the "device_driver" which had a group attribute so I used that.
I already learned from maxime that that is the wrong way :) and
hopefully I'll figure out what the right way will be soon ;)
ATTRIBUTE_GROUPS(name);
.group = name;
After that last addition, you'd simply do:
ATTRIBUTE_GROUPS_RO(name);
.group = name;
saves a whole line :)
Not worth it :)
>From 003ab7a74ff689daa6934e7bc50c498b2d35a1cc Mon Sep 17 00:00:00
2001
From: Oliver Schinagl
Date: Thu, 11 Jul 2013 13:48:18 +0200
Subject: [PATCH] sysfs: add more helper macro's for
(bin_)attribute(_groups)
With the recent changes to sysfs there's various helper macro's.
However there's no RW, RO BIN_ helper macro's. This patch adds them.
Additionally there are no BIN_ group helpers so there's that aswell
Moreso, if both bin and normal attribute groups are used, there's a
simple helper for that, though the naming code be better. _TXT_
for the
show/store ones and neither TXT or BIN for both, but that would
change
things to extensivly.
Finally there's also helpers for ATTRIBUTE_ATTRS.
After this patch, create default attributes can be as easy as:
ATTRIBUTE_(BIN_)ATTR_RO(name, SIZE);
ATTRIBUTE_BIN_GROUPS(name);
Signed-off-by: Oliver Schinagl
---
include/linux/sysfs.h | 96
---
1 file changed, 84 insertions(+), 12 deletions(-)
diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h
index 2c3b6a3..0ebed11 100644
--- a/include/linux/sysfs.h
+++ b/include/linux/sysfs.h
@@ -17,6 +17,7 @@
#include
#include
#include
+#include
#include
struct kobject;
@@ -94,15 +95,32 @@ struct attribute_group {
#define __ATTR_IGNORE_LOCKDEP__ATTR
#endif
-#define ATTRIBUTE_GROUPS(name)\
-static const struct attribute_group name##_group = {\
-.attrs = name##_attrs,\
+#define __ATTRIBUTE_GROUPS(_name)\
+static const struct attribute_group *_name##_groups[] = {\
+&_name##_group,\
+NULL,\
+}
+
+#define ATTRIBUTE_GROUPS(_name)\
+static const struct attribute_group _name##_group = {\
+.attrs = _name##_attrs,\
};\
-static const struct attribute_group *name##_groups[] = {\
-##_group,\
+__ATTRIBUTE_GROUPS(_name)
+
+#
Re: [PATCH v2 2/7] sysfs.h: add ATTRIBUTE_GROUPS() macro
Greg,
Haven't heard anything about this v2 patch, I know you are very busy but
with the merge window closing today/yesterday just wondering about the
status of it all.
Oliver
On 11-07-13 22:55, Oliver Schinagl wrote:
On 07/11/13 22:26, Greg Kroah-Hartman wrote:
On Thu, Jul 11, 2013 at 10:09:11PM +0200, Oliver Schinagl wrote:
On 07/11/13 19:06, Greg Kroah-Hartman wrote:
On Thu, Jul 11, 2013 at 01:58:29PM +0200, Oliver Schinagl wrote:
On 11-07-13 02:36, Greg Kroah-Hartman wrote:
To make it easier for driver subsystems to work with attribute
groups,
create the ATTRIBUTE_GROUPS macro to remove some of the repetitive
typing for the most common use for attribute groups.
But binary groups are discriminated against :(
Yes, as they are rarer by far, as they should be. binary sysfs
files
should almost never be used, as they are only pass-through files to
the hardware, so I want to see you do more work in order to use
them, as
they should not be created lightly.
I guess I can see a valid reason here, but they are only helper
macro's making life easier for people who do need to use these and
are on par with the non-binary versions. And we already have quite
some binary attributes, probably far less then normal ones :)
I only count about 100 valid binary files in the tree at the moment,
that's not really all that many to handle.
100 is quite a few :) But point taken.
Anyway, wouldn't all users be reviewed anyway? But I guess it's a
small safety net to make it not TOO easy.
exactly :)
I aggree and this is a v2 that strips all the additional bits.
A few comments left below.
The attached patch should help here.
Can you give me an example of using these macros? I seem to be
lost in
them somehow, or maybe my morning coffee just hasn't kicked in...
Yeah, I kinda added the whole shebang there :) I was trying being
helpful :(
I suppose one more additional helper wouldn't be bad to have:
#define ATTRIBUTE_(BIN_)GROUPS_R[O/W](_name(, _size)) \
ATTRIBUTE_(BIN_)ATTR_R[O/W](_name, _size); \
ATTRIBUTE_(BIN_)GROUPS(_name)
Would that ever be needed?
Of ourse, by the lazy :)
I think now you create an attribute in a group as this (with this
patch):
ATTRIBUTE_ATTR_RO(name, SIZE);
but raw attributes are rare, you really want a device, class, or
bus attribute, right?
I suppose so, But I got stuck in the rare case some how initially. I
registered my driver with module_platform_driver(); and in that struct
i had the device_driver which had a group attribute so I used that.
I already learned from maxime that that is the wrong way :) and
hopefully I'll figure out what the right way will be soon ;)
ATTRIBUTE_GROUPS(name);
.group = name;
After that last addition, you'd simply do:
ATTRIBUTE_GROUPS_RO(name);
.group = name;
saves a whole line :)
Not worth it :)
From 003ab7a74ff689daa6934e7bc50c498b2d35a1cc Mon Sep 17 00:00:00
2001
From: Oliver Schinagl oli...@schinagl.nl
Date: Thu, 11 Jul 2013 13:48:18 +0200
Subject: [PATCH] sysfs: add more helper macro's for
(bin_)attribute(_groups)
With the recent changes to sysfs there's various helper macro's.
However there's no RW, RO BIN_ helper macro's. This patch adds them.
Additionally there are no BIN_ group helpers so there's that aswell
Moreso, if both bin and normal attribute groups are used, there's a
simple helper for that, though the naming code be better. _TXT_
for the
show/store ones and neither TXT or BIN for both, but that would
change
things to extensivly.
Finally there's also helpers for ATTRIBUTE_ATTRS.
After this patch, create default attributes can be as easy as:
ATTRIBUTE_(BIN_)ATTR_RO(name, SIZE);
ATTRIBUTE_BIN_GROUPS(name);
Signed-off-by: Oliver Schinagl oli...@schinagl.nl
---
include/linux/sysfs.h | 96
---
1 file changed, 84 insertions(+), 12 deletions(-)
diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h
index 2c3b6a3..0ebed11 100644
--- a/include/linux/sysfs.h
+++ b/include/linux/sysfs.h
@@ -17,6 +17,7 @@
#include linux/list.h
#include linux/lockdep.h
#include linux/kobject_ns.h
+#include linux/stat.h
#include linux/atomic.h
struct kobject;
@@ -94,15 +95,32 @@ struct attribute_group {
#define __ATTR_IGNORE_LOCKDEP__ATTR
#endif
-#define ATTRIBUTE_GROUPS(name)\
-static const struct attribute_group name##_group = {\
-.attrs = name##_attrs,\
+#define __ATTRIBUTE_GROUPS(_name)\
+static const struct attribute_group *_name##_groups[] = {\
+_name##_group,\
+NULL,\
+}
+
+#define ATTRIBUTE_GROUPS(_name)\
+static const struct attribute_group _name##_group = {\
+.attrs = _name##_attrs,\
};\
-static const struct attribute_group *name##_groups[] = {\
-name##_group,\
+__ATTRIBUTE_GROUPS
Re: [PATCH v2 2/7] sysfs.h: add ATTRIBUTE_GROUPS() macro
On 07/11/13 22:26, Greg Kroah-Hartman wrote:
On Thu, Jul 11, 2013 at 10:09:11PM +0200, Oliver Schinagl wrote:
On 07/11/13 19:06, Greg Kroah-Hartman wrote:
On Thu, Jul 11, 2013 at 01:58:29PM +0200, Oliver Schinagl wrote:
On 11-07-13 02:36, Greg Kroah-Hartman wrote:
To make it easier for driver subsystems to work with attribute groups,
create the ATTRIBUTE_GROUPS macro to remove some of the repetitive
typing for the most common use for attribute groups.
But binary groups are discriminated against :(
Yes, as they are "rarer" by far, as they should be. binary sysfs files
should almost never be used, as they are only "pass-through" files to
the hardware, so I want to see you do more work in order to use them, as
they should not be created lightly.
I guess I can see a valid reason here, but they are only helper
macro's making life easier for people who do need to use these and
are on par with the non-binary versions. And we already have quite
some binary attributes, probably far less then normal ones :)
I only count about 100 valid binary files in the tree at the moment,
that's not really all that many to handle.
100 is quite a few :) But point taken.
Anyway, wouldn't all users be reviewed anyway? But I guess it's a
small safety net to make it not TOO easy.
exactly :)
I aggree and this is a v2 that strips all the additional bits.
A few comments left below.
The attached patch should help here.
Can you give me an example of using these macros? I seem to be lost in
them somehow, or maybe my morning coffee just hasn't kicked in...
Yeah, I kinda added the whole shebang there :) I was trying being helpful :(
I suppose one more additional helper wouldn't be bad to have:
#define ATTRIBUTE_(BIN_)GROUPS_R[O/W](_name(, _size)) \
ATTRIBUTE_(BIN_)ATTR_R[O/W](_name, _size); \
ATTRIBUTE_(BIN_)GROUPS(_name)
Would that ever be needed?
Of ourse, by the lazy :)
I think now you create an attribute in a group as this (with this patch):
ATTRIBUTE_ATTR_RO(name, SIZE);
but "raw" attributes are rare, you really want a "device", "class", or
"bus" attribute, right?
I suppose so, But I got stuck in the rare case some how initially. I
registered my driver with module_platform_driver(); and in that struct i
had the "device_driver" which had a group attribute so I used that. I
already learned from maxime that that is the wrong way :) and hopefully
I'll figure out what the right way will be soon ;)
ATTRIBUTE_GROUPS(name);
.group = name;
After that last addition, you'd simply do:
ATTRIBUTE_GROUPS_RO(name);
.group = name;
saves a whole line :)
Not worth it :)
>From 003ab7a74ff689daa6934e7bc50c498b2d35a1cc Mon Sep 17 00:00:00 2001
From: Oliver Schinagl
Date: Thu, 11 Jul 2013 13:48:18 +0200
Subject: [PATCH] sysfs: add more helper macro's for (bin_)attribute(_groups)
With the recent changes to sysfs there's various helper macro's.
However there's no RW, RO BIN_ helper macro's. This patch adds them.
Additionally there are no BIN_ group helpers so there's that aswell
Moreso, if both bin and normal attribute groups are used, there's a
simple helper for that, though the naming code be better. _TXT_ for the
show/store ones and neither TXT or BIN for both, but that would change
things to extensivly.
Finally there's also helpers for ATTRIBUTE_ATTRS.
After this patch, create default attributes can be as easy as:
ATTRIBUTE_(BIN_)ATTR_RO(name, SIZE);
ATTRIBUTE_BIN_GROUPS(name);
Signed-off-by: Oliver Schinagl
---
include/linux/sysfs.h | 96 ---
1 file changed, 84 insertions(+), 12 deletions(-)
diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h
index 2c3b6a3..0ebed11 100644
--- a/include/linux/sysfs.h
+++ b/include/linux/sysfs.h
@@ -17,6 +17,7 @@
#include
#include
#include
+#include
#include
struct kobject;
@@ -94,15 +95,32 @@ struct attribute_group {
#define __ATTR_IGNORE_LOCKDEP __ATTR
#endif
-#define ATTRIBUTE_GROUPS(name) \
-static const struct attribute_group name##_group = { \
- .attrs = name##_attrs, \
+#define __ATTRIBUTE_GROUPS(_name) \
+static const struct attribute_group *_name##_groups[] = { \
+ &_name##_group, \
+ NULL, \
+}
+
+#define ATTRIBUTE_GROUPS(_name)\
+static const struct attribute_group _name##_group = { \
+ .attrs = _name##_attrs, \
};\
-static const struct attribute_group *name##_groups[] = { \
- ##_group, \
+__ATTRIBUTE_GROUPS(_name)
+
+#define __ATTRIBUTE_ATTRS(_name)
Re: [PATCH v2 2/7] sysfs.h: add ATTRIBUTE_GROUPS() macro
On 07/11/13 19:06, Greg Kroah-Hartman wrote:
On Thu, Jul 11, 2013 at 01:58:29PM +0200, Oliver Schinagl wrote:
On 11-07-13 02:36, Greg Kroah-Hartman wrote:
To make it easier for driver subsystems to work with attribute groups,
create the ATTRIBUTE_GROUPS macro to remove some of the repetitive
typing for the most common use for attribute groups.
But binary groups are discriminated against :(
Yes, as they are "rarer" by far, as they should be. binary sysfs files
should almost never be used, as they are only "pass-through" files to
the hardware, so I want to see you do more work in order to use them, as
they should not be created lightly.
I guess I can see a valid reason here, but they are only helper macro's
making life easier for people who do need to use these and are on par
with the non-binary versions. And we already have quite some binary
attributes, probably far less then normal ones :)
Anyway, wouldn't all users be reviewed anyway? But I guess it's a small
safety net to make it not TOO easy.
The attached patch should help here.
Can you give me an example of using these macros? I seem to be lost in
them somehow, or maybe my morning coffee just hasn't kicked in...
Yeah, I kinda added the whole shebang there :) I was trying being helpful :(
I suppose one more additional helper wouldn't be bad to have:
#define ATTRIBUTE_(BIN_)GROUPS_R[O/W](_name(, _size)) \
ATTRIBUTE_(BIN_)ATTR_R[O/W](_name, _size); \
ATTRIBUTE_(BIN_)GROUPS(_name)
Would that ever be needed?
Of ourse, by the lazy :)
I think now you create an attribute in a group as this (with this patch):
ATTRIBUTE_ATTR_RO(name, SIZE);
ATTRIBUTE_GROUPS(name);
.group = name;
After that last addition, you'd simply do:
ATTRIBUTE_GROUPS_RO(name);
.group = name;
saves a whole line :)
>From 003ab7a74ff689daa6934e7bc50c498b2d35a1cc Mon Sep 17 00:00:00 2001
From: Oliver Schinagl
Date: Thu, 11 Jul 2013 13:48:18 +0200
Subject: [PATCH] sysfs: add more helper macro's for (bin_)attribute(_groups)
With the recent changes to sysfs there's various helper macro's.
However there's no RW, RO BIN_ helper macro's. This patch adds them.
Additionally there are no BIN_ group helpers so there's that aswell
Moreso, if both bin and normal attribute groups are used, there's a
simple helper for that, though the naming code be better. _TXT_ for the
show/store ones and neither TXT or BIN for both, but that would change
things to extensivly.
Finally there's also helpers for ATTRIBUTE_ATTRS.
After this patch, create default attributes can be as easy as:
ATTRIBUTE_(BIN_)ATTR_RO(name, SIZE);
ATTRIBUTE_BIN_GROUPS(name);
Signed-off-by: Oliver Schinagl
---
include/linux/sysfs.h | 96 ---
1 file changed, 84 insertions(+), 12 deletions(-)
diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h
index 2c3b6a3..0ebed11 100644
--- a/include/linux/sysfs.h
+++ b/include/linux/sysfs.h
@@ -17,6 +17,7 @@
#include
#include
#include
+#include
#include
struct kobject;
@@ -94,15 +95,32 @@ struct attribute_group {
#define __ATTR_IGNORE_LOCKDEP __ATTR
#endif
-#define ATTRIBUTE_GROUPS(name) \
-static const struct attribute_group name##_group = { \
- .attrs = name##_attrs, \
+#define __ATTRIBUTE_GROUPS(_name) \
+static const struct attribute_group *_name##_groups[] = { \
+ &_name##_group, \
+ NULL, \
+}
+
+#define ATTRIBUTE_GROUPS(_name)\
+static const struct attribute_group _name##_group = { \
+ .attrs = _name##_attrs, \
};\
-static const struct attribute_group *name##_groups[] = { \
- ##_group, \
+__ATTRIBUTE_GROUPS(_name)
+
+#define __ATTRIBUTE_ATTRS(_name) \
+struct bin_attribute *_name##_attrs[] = { \
typo here, scrap bin_ copy paste fail!
+ &_name##_attr, \
NULL, \
}
+#define ATTRIBUTE_ATTR_RO(_name, _size)\
+struct attribute _name##_attr = __ATTR_RO(_name, _size); \
+__ATTRIBUTE_ATTRS(_name)
+
+#define ATTRIBUTE_ATTR_RW(_name, _size)\
+struct attribute _name##_attr = __ATTR_RW(_name, _size); \
+__ATTRIBUTE_ATTRS(_name)
What do these two help out with? "attribute attribute read-write" seems
a bit "clunky", don't you think? :)
I aggree, but I tried to stick with the ATTRIBUTE_GROUP naming and
that's the best I could come up with.
Unless I completely misunderstood (whic
Re: [PATCH v2 2/7] sysfs.h: add ATTRIBUTE_GROUPS() macro
On 11-07-13 02:36, Greg Kroah-Hartman wrote:
To make it easier for driver subsystems to work with attribute groups,
create the ATTRIBUTE_GROUPS macro to remove some of the repetitive
typing for the most common use for attribute groups.
Forgot to add this trivial little cleanup patch with my previous mail, I
know you don't like patch dependancies, but this one depends on the
previous one as that included stat.h, if you decide my previous patch
isn't good, i can redo this one with stat.h included (and move atomic.h
to the top to keep includes alphabetically ordered).
Oliver
Signed-off-by: Greg Kroah-Hartman
---
include/linux/sysfs.h | 9 +
1 file changed, 9 insertions(+)
diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h
index 9cd20c8..f62ff01 100644
--- a/include/linux/sysfs.h
+++ b/include/linux/sysfs.h
@@ -94,6 +94,15 @@ struct attribute_group {
#define __ATTR_IGNORE_LOCKDEP __ATTR
#endif
+#define ATTRIBUTE_GROUPS(name) \
+static const struct attribute_group name##_group = { \
+ .attrs = name##_attrs, \
+}; \
+static const struct attribute_group *name##_groups[] = { \
+ ##_group, \
+ NULL, \
+}
+
#define attr_name(_attr) (_attr).attr.name
struct file;
>From a67de9f026363ce821c72a807d98830abece3cf7 Mon Sep 17 00:00:00 2001
From: Oliver Schinagl
Date: Thu, 11 Jul 2013 13:57:42 +0200
Subject: [PATCH] sysfs: use file mode defines from stat.h
With the last patches stat.h was included to the header, and thus those
permission defines should be used.
Signed-off-by: Oliver Schinagl
---
include/linux/sysfs.h | 17 +
1 file changed, 9 insertions(+), 8 deletions(-)
diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h
index 0ebed11..8f820c7 100644
--- a/include/linux/sysfs.h
+++ b/include/linux/sysfs.h
@@ -69,18 +69,19 @@ struct attribute_group {
* for examples..
*/
-#define __ATTR(_name,_mode,_show,_store) { \
- .attr = {.name = __stringify(_name), .mode = _mode }, \
- .show = _show, \
- .store = _store, \
+#define __ATTR(_name,_mode,_show,_store) { \
+ .attr = {.name = __stringify(_name), .mode = _mode }, \
+ .show = _show, \
+ .store = _store, \
}
-#define __ATTR_RO(_name) { \
- .attr = { .name = __stringify(_name), .mode = 0444 }, \
- .show = _name##_show, \
+#define __ATTR_RO(_name) { \
+ .attr = { .name = __stringify(_name), .mode = S_IRUGO }, \
+ .show = _name##_show, \
}
-#define __ATTR_RW(_name) __ATTR(_name, 0644, _name##_show, _name##_store)
+#define __ATTR_RW(_name) __ATTR(_name, (S_IWUSR | S_IRUGO), \
+ _name##_show, _name##_store)
#define __ATTR_NULL { .attr = { .name = NULL } }
--
1.8.1.5
Re: [PATCH v2 2/7] sysfs.h: add ATTRIBUTE_GROUPS() macro
On 11-07-13 02:36, Greg Kroah-Hartman wrote:
To make it easier for driver subsystems to work with attribute groups,
create the ATTRIBUTE_GROUPS macro to remove some of the repetitive
typing for the most common use for attribute groups.
But binary groups are discriminated against :(
The attached patch should help here.
I suppose one more additional helper wouldn't be bad to have:
#define ATTRIBUTE_(BIN_)GROUPS_R[O/W](_name(, _size)) \
ATTRIBUTE_(BIN_)ATTR_R[O/W](_name, _size); \
ATTRIBUTE_(BIN_)GROUPS(_name)
but wasn't sure if that wasn't a little overkill. I gladly add it in an
additional patch.
Signed-off-by: Greg Kroah-Hartman
---
include/linux/sysfs.h | 9 +
1 file changed, 9 insertions(+)
diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h
index 9cd20c8..f62ff01 100644
--- a/include/linux/sysfs.h
+++ b/include/linux/sysfs.h
@@ -94,6 +94,15 @@ struct attribute_group {
#define __ATTR_IGNORE_LOCKDEP __ATTR
#endif
+#define ATTRIBUTE_GROUPS(name) \
+static const struct attribute_group name##_group = { \
+ .attrs = name##_attrs, \
+}; \
+static const struct attribute_group *name##_groups[] = { \
+ ##_group, \
+ NULL, \
+}
+
#define attr_name(_attr) (_attr).attr.name
struct file;
>From 003ab7a74ff689daa6934e7bc50c498b2d35a1cc Mon Sep 17 00:00:00 2001
From: Oliver Schinagl
Date: Thu, 11 Jul 2013 13:48:18 +0200
Subject: [PATCH] sysfs: add more helper macro's for (bin_)attribute(_groups)
With the recent changes to sysfs there's various helper macro's.
However there's no RW, RO BIN_ helper macro's. This patch adds them.
Additionally there are no BIN_ group helpers so there's that aswell
Moreso, if both bin and normal attribute groups are used, there's a
simple helper for that, though the naming code be better. _TXT_ for the
show/store ones and neither TXT or BIN for both, but that would change
things to extensivly.
Finally there's also helpers for ATTRIBUTE_ATTRS.
After this patch, create default attributes can be as easy as:
ATTRIBUTE_(BIN_)ATTR_RO(name, SIZE);
ATTRIBUTE_BIN_GROUPS(name);
Signed-off-by: Oliver Schinagl
---
include/linux/sysfs.h | 96 ---
1 file changed, 84 insertions(+), 12 deletions(-)
diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h
index 2c3b6a3..0ebed11 100644
--- a/include/linux/sysfs.h
+++ b/include/linux/sysfs.h
@@ -17,6 +17,7 @@
#include
#include
#include
+#include
#include
struct kobject;
@@ -94,15 +95,32 @@ struct attribute_group {
#define __ATTR_IGNORE_LOCKDEP __ATTR
#endif
-#define ATTRIBUTE_GROUPS(name) \
-static const struct attribute_group name##_group = { \
- .attrs = name##_attrs, \
+#define __ATTRIBUTE_GROUPS(_name)\
+static const struct attribute_group *_name##_groups[] = { \
+ &_name##_group, \
+ NULL, \
+}
+
+#define ATTRIBUTE_GROUPS(_name) \
+static const struct attribute_group _name##_group = { \
+ .attrs = _name##_attrs, \
};\
-static const struct attribute_group *name##_groups[] = { \
- ##_group, \
+__ATTRIBUTE_GROUPS(_name)
+
+#define __ATTRIBUTE_ATTRS(_name)\
+struct bin_attribute *_name##_attrs[] = { \
+ &_name##_attr, \
NULL, \
}
+#define ATTRIBUTE_ATTR_RO(_name, _size)\
+struct attribute _name##_attr = __ATTR_RO(_name, _size); \
+__ATTRIBUTE_ATTRS(_name)
+
+#define ATTRIBUTE_ATTR_RW(_name, _size)\
+struct attribute _name##_attr = __ATTR_RW(_name, _size); \
+__ATTRIBUTE_ATTRS(_name)
+
#define attr_name(_attr) (_attr).attr.name
struct file;
@@ -132,15 +150,69 @@ struct bin_attribute {
*/
#define sysfs_bin_attr_init(bin_attr) sysfs_attr_init(&(bin_attr)->attr)
-/* macro to create static binary attributes easier */
-#define BIN_ATTR(_name, _mode, _read, _write, _size) \
-struct bin_attribute bin_attr_##_name = { \
- .attr = {.name = __stringify(_name), .mode = _mode }, \
- .read = _read, \
- .write = _write, \
- .size = _size, \
+/* macros to create static binary attributes easier */
+#define __BIN_ATTR(_name, _mode, _read, _write, _size) { \
+ .attr = { .name = __stringify(_name), .mode = _mode }, \
+ .read = _read, \
+ .write = _write, \
+ .size = _size, \
+}
+
+#define __BIN_ATTR_RO(_name, _size) { \
+ .attr = { .name = __stringify(_name), .mode = S_IRUGO }, \
+ .read = _name##_read, \
+ .size = _size, \
+}
+
+#define __BIN_ATTR_RW(_name, _size) __BIN_ATTR(_name, \
+ (S_IWUSR | S_IRUGO), _name##_read, \
+ _name##_write)
+
+#define __BIN_ATTR_NULL __ATTR_NULL
+
+#define BIN_ATTR(_name, _mode, _read, _write, _size) \
+struct bin_attribute bin_attr_##_name = __BIN_ATTR(_name, _mode, _read, \
+ _write, _size)
+
+#define BIN_RO
Re: [PATCH v2 5/7] sysfs: add support for binary attributes in groups
On 11-07-13 02:36, Greg Kroah-Hartman wrote:
groups should be able to support binary attributes, just like it
supports "normal" attributes. This lets us only handle one type of
structure, groups, throughout the driver core and subsystems, making
binary attributes a "full fledged" part of the driver model, and not
something just "tacked on".
However when only using binary attributes it warns and doesn't create
anything. The attached patch fixes that.
Reported-by: Oliver Schinagl
If I may be so bold and ask to change my e-mail address to
that would be kind. I use the +list delimiter to
put all the mailing list mails in a separate mailbox.
Signed-off-by: Greg Kroah-Hartman
---
fs/sysfs/group.c | 66 +++
include/linux/sysfs.h | 4 ++--
2 files changed, 48 insertions(+), 22 deletions(-)
diff --git a/fs/sysfs/group.c b/fs/sysfs/group.c
index aec3d5c..e5719c6 100644
--- a/fs/sysfs/group.c
+++ b/fs/sysfs/group.c
@@ -20,38 +20,64 @@ static void remove_files(struct sysfs_dirent *dir_sd,
struct kobject *kobj,
const struct attribute_group *grp)
{
struct attribute *const* attr;
- int i;
+ struct bin_attribute *const* bin_attr;
- for (i = 0, attr = grp->attrs; *attr; i++, attr++)
- sysfs_hash_and_remove(dir_sd, NULL, (*attr)->name);
+ if (grp->attrs)
+ for (attr = grp->attrs; *attr; attr++)
+ sysfs_hash_and_remove(dir_sd, NULL, (*attr)->name);
+ if (grp->bin_attrs)
+ for (bin_attr = grp->bin_attrs; *bin_attr; bin_attr++)
+ sysfs_remove_bin_file(kobj, *bin_attr);
}
static int create_files(struct sysfs_dirent *dir_sd, struct kobject *kobj,
const struct attribute_group *grp, int update)
{
struct attribute *const* attr;
+ struct bin_attribute *const* bin_attr;
int error = 0, i;
- for (i = 0, attr = grp->attrs; *attr && !error; i++, attr++) {
- umode_t mode = 0;
+ if (grp->attrs) {
+ for (i = 0, attr = grp->attrs; *attr && !error; i++, attr++) {
+ umode_t mode = 0;
+
+ /*
+* In update mode, we're changing the permissions or
+* visibility. Do this by first removing then
+* re-adding (if required) the file.
+*/
+ if (update)
+ sysfs_hash_and_remove(dir_sd, NULL,
+ (*attr)->name);
+ if (grp->is_visible) {
+ mode = grp->is_visible(kobj, *attr, i);
+ if (!mode)
+ continue;
+ }
+ error = sysfs_add_file_mode(dir_sd, *attr,
+ SYSFS_KOBJ_ATTR,
+ (*attr)->mode | mode);
+ if (unlikely(error))
+ break;
+ }
+ if (error) {
+ remove_files(dir_sd, kobj, grp);
+ goto exit;
+ }
+ }
- /* in update mode, we're changing the permissions or
-* visibility. Do this by first removing then
-* re-adding (if required) the file */
- if (update)
- sysfs_hash_and_remove(dir_sd, NULL, (*attr)->name);
- if (grp->is_visible) {
- mode = grp->is_visible(kobj, *attr, i);
- if (!mode)
- continue;
+ if (grp->bin_attrs) {
+ for (bin_attr = grp->bin_attrs; *bin_attr; bin_attr++) {
+ if (update)
+ sysfs_remove_bin_file(kobj, *bin_attr);
+ error = sysfs_create_bin_file(kobj, *bin_attr);
+ if (error)
+ break;
}
- error = sysfs_add_file_mode(dir_sd, *attr, SYSFS_KOBJ_ATTR,
- (*attr)->mode | mode);
- if (unlikely(error))
- break;
+ if (error)
+ remove_files(dir_sd, kobj, grp);
}
- if (error)
- remove_files(dir_sd, kobj, grp);
+exit:
return error;
}
diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h
index d50a96b..2c3b6a3 100644
--- a/include/linux/sysfs.h
+++ b/include/linux/sysfs.h
@@ -21,6 +21,7 @@
struct kobject;
struct module;
+struct bin_attribute;
enum kobj_ns_type;
struct attribute
Re: [PATCH v2 0/7] Driver core and sysfs changes for attribute groups
On 11-07-13 08:28, Greg Kroah-Hartman wrote: On Wed, Jul 10, 2013 at 10:23:57PM -0700, Guenter Roeck wrote: On Wed, Jul 10, 2013 at 05:35:58PM -0700, Greg Kroah-Hartman wrote: Hi all, Here's the second iteration of the patchset to add better attribute group support to the driver core and sysfs. I've tested these (shocker!) and everything works fine with them (I'm sending this from Linus's latest kernel with these 7 on top of it.) I'd like to send these to Linus for 3.11 unless someone objects. Oliver, please use this series instead of the last one, it has fixes that Guenter pointed out that would have crashed your box at boot. It still crashes, but haven't read back all. I do have some extra helper macro's patch incoming once i make sure it boots. (it boots but segfaults) give me a few hours to report back. oliver changes from v2: - actually boots - 7th patch added properly - added BUS_ATTR, CLASS_ATTR, and DRIVER_ATTR RW and RO macros to help with converting code to use attributes properly. Looks good this time. And, yes, it does boot and work, including patch #7. Feel free to add Reviewed-by: Guenter Roeck and if you like Tested-by: Guenter Roeck to the series. Thanks for the testing and review, I'll add this when I commit them sometime next week. greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 0/7] Driver core and sysfs changes for attribute groups
On 11-07-13 08:28, Greg Kroah-Hartman wrote: On Wed, Jul 10, 2013 at 10:23:57PM -0700, Guenter Roeck wrote: On Wed, Jul 10, 2013 at 05:35:58PM -0700, Greg Kroah-Hartman wrote: Hi all, Here's the second iteration of the patchset to add better attribute group support to the driver core and sysfs. I've tested these (shocker!) and everything works fine with them (I'm sending this from Linus's latest kernel with these 7 on top of it.) I'd like to send these to Linus for 3.11 unless someone objects. Oliver, please use this series instead of the last one, it has fixes that Guenter pointed out that would have crashed your box at boot. It still crashes, but haven't read back all. I do have some extra helper macro's patch incoming once i make sure it boots. (it boots but segfaults) give me a few hours to report back. oliver changes from v2: - actually boots - 7th patch added properly - added BUS_ATTR, CLASS_ATTR, and DRIVER_ATTR RW and RO macros to help with converting code to use attributes properly. Looks good this time. And, yes, it does boot and work, including patch #7. Feel free to add Reviewed-by: Guenter Roeck li...@roeck-us.net and if you like Tested-by: Guenter Roeck li...@roeck-us.net to the series. Thanks for the testing and review, I'll add this when I commit them sometime next week. greg k-h -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 5/7] sysfs: add support for binary attributes in groups
On 11-07-13 02:36, Greg Kroah-Hartman wrote:
groups should be able to support binary attributes, just like it
supports normal attributes. This lets us only handle one type of
structure, groups, throughout the driver core and subsystems, making
binary attributes a full fledged part of the driver model, and not
something just tacked on.
However when only using binary attributes it warns and doesn't create
anything. The attached patch fixes that.
Reported-by: Oliver Schinagl oliver+l...@schinagl.nl
If I may be so bold and ask to change my e-mail address to
oli...@schinagl.nl that would be kind. I use the +list delimiter to
put all the mailing list mails in a separate mailbox.
Signed-off-by: Greg Kroah-Hartman gre...@linuxfoundation.org
---
fs/sysfs/group.c | 66 +++
include/linux/sysfs.h | 4 ++--
2 files changed, 48 insertions(+), 22 deletions(-)
diff --git a/fs/sysfs/group.c b/fs/sysfs/group.c
index aec3d5c..e5719c6 100644
--- a/fs/sysfs/group.c
+++ b/fs/sysfs/group.c
@@ -20,38 +20,64 @@ static void remove_files(struct sysfs_dirent *dir_sd,
struct kobject *kobj,
const struct attribute_group *grp)
{
struct attribute *const* attr;
- int i;
+ struct bin_attribute *const* bin_attr;
- for (i = 0, attr = grp-attrs; *attr; i++, attr++)
- sysfs_hash_and_remove(dir_sd, NULL, (*attr)-name);
+ if (grp-attrs)
+ for (attr = grp-attrs; *attr; attr++)
+ sysfs_hash_and_remove(dir_sd, NULL, (*attr)-name);
+ if (grp-bin_attrs)
+ for (bin_attr = grp-bin_attrs; *bin_attr; bin_attr++)
+ sysfs_remove_bin_file(kobj, *bin_attr);
}
static int create_files(struct sysfs_dirent *dir_sd, struct kobject *kobj,
const struct attribute_group *grp, int update)
{
struct attribute *const* attr;
+ struct bin_attribute *const* bin_attr;
int error = 0, i;
- for (i = 0, attr = grp-attrs; *attr !error; i++, attr++) {
- umode_t mode = 0;
+ if (grp-attrs) {
+ for (i = 0, attr = grp-attrs; *attr !error; i++, attr++) {
+ umode_t mode = 0;
+
+ /*
+* In update mode, we're changing the permissions or
+* visibility. Do this by first removing then
+* re-adding (if required) the file.
+*/
+ if (update)
+ sysfs_hash_and_remove(dir_sd, NULL,
+ (*attr)-name);
+ if (grp-is_visible) {
+ mode = grp-is_visible(kobj, *attr, i);
+ if (!mode)
+ continue;
+ }
+ error = sysfs_add_file_mode(dir_sd, *attr,
+ SYSFS_KOBJ_ATTR,
+ (*attr)-mode | mode);
+ if (unlikely(error))
+ break;
+ }
+ if (error) {
+ remove_files(dir_sd, kobj, grp);
+ goto exit;
+ }
+ }
- /* in update mode, we're changing the permissions or
-* visibility. Do this by first removing then
-* re-adding (if required) the file */
- if (update)
- sysfs_hash_and_remove(dir_sd, NULL, (*attr)-name);
- if (grp-is_visible) {
- mode = grp-is_visible(kobj, *attr, i);
- if (!mode)
- continue;
+ if (grp-bin_attrs) {
+ for (bin_attr = grp-bin_attrs; *bin_attr; bin_attr++) {
+ if (update)
+ sysfs_remove_bin_file(kobj, *bin_attr);
+ error = sysfs_create_bin_file(kobj, *bin_attr);
+ if (error)
+ break;
}
- error = sysfs_add_file_mode(dir_sd, *attr, SYSFS_KOBJ_ATTR,
- (*attr)-mode | mode);
- if (unlikely(error))
- break;
+ if (error)
+ remove_files(dir_sd, kobj, grp);
}
- if (error)
- remove_files(dir_sd, kobj, grp);
+exit:
return error;
}
diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h
index d50a96b..2c3b6a3 100644
--- a/include/linux/sysfs.h
+++ b/include/linux/sysfs.h
@@ -21,6 +21,7 @@
struct kobject;
struct module;
+struct bin_attribute;
enum kobj_ns_type;
struct attribute {
@@ -59,10 +60,9 @@ struct attribute_group {
umode_t
Re: [PATCH v2 2/7] sysfs.h: add ATTRIBUTE_GROUPS() macro
On 11-07-13 02:36, Greg Kroah-Hartman wrote:
To make it easier for driver subsystems to work with attribute groups,
create the ATTRIBUTE_GROUPS macro to remove some of the repetitive
typing for the most common use for attribute groups.
But binary groups are discriminated against :(
The attached patch should help here.
I suppose one more additional helper wouldn't be bad to have:
#define ATTRIBUTE_(BIN_)GROUPS_R[O/W](_name(, _size)) \
ATTRIBUTE_(BIN_)ATTR_R[O/W](_name, _size); \
ATTRIBUTE_(BIN_)GROUPS(_name)
but wasn't sure if that wasn't a little overkill. I gladly add it in an
additional patch.
Signed-off-by: Greg Kroah-Hartman gre...@linuxfoundation.org
---
include/linux/sysfs.h | 9 +
1 file changed, 9 insertions(+)
diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h
index 9cd20c8..f62ff01 100644
--- a/include/linux/sysfs.h
+++ b/include/linux/sysfs.h
@@ -94,6 +94,15 @@ struct attribute_group {
#define __ATTR_IGNORE_LOCKDEP __ATTR
#endif
+#define ATTRIBUTE_GROUPS(name) \
+static const struct attribute_group name##_group = { \
+ .attrs = name##_attrs, \
+}; \
+static const struct attribute_group *name##_groups[] = { \
+ name##_group, \
+ NULL, \
+}
+
#define attr_name(_attr) (_attr).attr.name
struct file;
From 003ab7a74ff689daa6934e7bc50c498b2d35a1cc Mon Sep 17 00:00:00 2001
From: Oliver Schinagl oli...@schinagl.nl
Date: Thu, 11 Jul 2013 13:48:18 +0200
Subject: [PATCH] sysfs: add more helper macro's for (bin_)attribute(_groups)
With the recent changes to sysfs there's various helper macro's.
However there's no RW, RO BIN_ helper macro's. This patch adds them.
Additionally there are no BIN_ group helpers so there's that aswell
Moreso, if both bin and normal attribute groups are used, there's a
simple helper for that, though the naming code be better. _TXT_ for the
show/store ones and neither TXT or BIN for both, but that would change
things to extensivly.
Finally there's also helpers for ATTRIBUTE_ATTRS.
After this patch, create default attributes can be as easy as:
ATTRIBUTE_(BIN_)ATTR_RO(name, SIZE);
ATTRIBUTE_BIN_GROUPS(name);
Signed-off-by: Oliver Schinagl oli...@schinagl.nl
---
include/linux/sysfs.h | 96 ---
1 file changed, 84 insertions(+), 12 deletions(-)
diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h
index 2c3b6a3..0ebed11 100644
--- a/include/linux/sysfs.h
+++ b/include/linux/sysfs.h
@@ -17,6 +17,7 @@
#include linux/list.h
#include linux/lockdep.h
#include linux/kobject_ns.h
+#include linux/stat.h
#include linux/atomic.h
struct kobject;
@@ -94,15 +95,32 @@ struct attribute_group {
#define __ATTR_IGNORE_LOCKDEP __ATTR
#endif
-#define ATTRIBUTE_GROUPS(name) \
-static const struct attribute_group name##_group = { \
- .attrs = name##_attrs, \
+#define __ATTRIBUTE_GROUPS(_name)\
+static const struct attribute_group *_name##_groups[] = { \
+ _name##_group, \
+ NULL, \
+}
+
+#define ATTRIBUTE_GROUPS(_name) \
+static const struct attribute_group _name##_group = { \
+ .attrs = _name##_attrs, \
};\
-static const struct attribute_group *name##_groups[] = { \
- name##_group, \
+__ATTRIBUTE_GROUPS(_name)
+
+#define __ATTRIBUTE_ATTRS(_name)\
+struct bin_attribute *_name##_attrs[] = { \
+ _name##_attr, \
NULL, \
}
+#define ATTRIBUTE_ATTR_RO(_name, _size)\
+struct attribute _name##_attr = __ATTR_RO(_name, _size); \
+__ATTRIBUTE_ATTRS(_name)
+
+#define ATTRIBUTE_ATTR_RW(_name, _size)\
+struct attribute _name##_attr = __ATTR_RW(_name, _size); \
+__ATTRIBUTE_ATTRS(_name)
+
#define attr_name(_attr) (_attr).attr.name
struct file;
@@ -132,15 +150,69 @@ struct bin_attribute {
*/
#define sysfs_bin_attr_init(bin_attr) sysfs_attr_init((bin_attr)-attr)
-/* macro to create static binary attributes easier */
-#define BIN_ATTR(_name, _mode, _read, _write, _size) \
-struct bin_attribute bin_attr_##_name = { \
- .attr = {.name = __stringify(_name), .mode = _mode }, \
- .read = _read, \
- .write = _write, \
- .size = _size, \
+/* macros to create static binary attributes easier */
+#define __BIN_ATTR(_name, _mode, _read, _write, _size) { \
+ .attr = { .name = __stringify(_name), .mode = _mode }, \
+ .read = _read, \
+ .write = _write, \
+ .size = _size, \
+}
+
+#define __BIN_ATTR_RO(_name, _size) { \
+ .attr = { .name = __stringify(_name), .mode = S_IRUGO }, \
+ .read = _name##_read, \
+ .size = _size, \
+}
+
+#define __BIN_ATTR_RW(_name, _size) __BIN_ATTR(_name, \
+ (S_IWUSR | S_IRUGO), _name##_read, \
+ _name##_write)
+
+#define __BIN_ATTR_NULL __ATTR_NULL
+
+#define BIN_ATTR(_name, _mode, _read, _write, _size
Re: [PATCH v2 2/7] sysfs.h: add ATTRIBUTE_GROUPS() macro
On 11-07-13 02:36, Greg Kroah-Hartman wrote:
To make it easier for driver subsystems to work with attribute groups,
create the ATTRIBUTE_GROUPS macro to remove some of the repetitive
typing for the most common use for attribute groups.
Forgot to add this trivial little cleanup patch with my previous mail, I
know you don't like patch dependancies, but this one depends on the
previous one as that included stat.h, if you decide my previous patch
isn't good, i can redo this one with stat.h included (and move atomic.h
to the top to keep includes alphabetically ordered).
Oliver
Signed-off-by: Greg Kroah-Hartman gre...@linuxfoundation.org
---
include/linux/sysfs.h | 9 +
1 file changed, 9 insertions(+)
diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h
index 9cd20c8..f62ff01 100644
--- a/include/linux/sysfs.h
+++ b/include/linux/sysfs.h
@@ -94,6 +94,15 @@ struct attribute_group {
#define __ATTR_IGNORE_LOCKDEP __ATTR
#endif
+#define ATTRIBUTE_GROUPS(name) \
+static const struct attribute_group name##_group = { \
+ .attrs = name##_attrs, \
+}; \
+static const struct attribute_group *name##_groups[] = { \
+ name##_group, \
+ NULL, \
+}
+
#define attr_name(_attr) (_attr).attr.name
struct file;
From a67de9f026363ce821c72a807d98830abece3cf7 Mon Sep 17 00:00:00 2001
From: Oliver Schinagl oli...@schinagl.nl
Date: Thu, 11 Jul 2013 13:57:42 +0200
Subject: [PATCH] sysfs: use file mode defines from stat.h
With the last patches stat.h was included to the header, and thus those
permission defines should be used.
Signed-off-by: Oliver Schinagl oli...@schinagl.nl
---
include/linux/sysfs.h | 17 +
1 file changed, 9 insertions(+), 8 deletions(-)
diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h
index 0ebed11..8f820c7 100644
--- a/include/linux/sysfs.h
+++ b/include/linux/sysfs.h
@@ -69,18 +69,19 @@ struct attribute_group {
* for examples..
*/
-#define __ATTR(_name,_mode,_show,_store) { \
- .attr = {.name = __stringify(_name), .mode = _mode }, \
- .show = _show, \
- .store = _store, \
+#define __ATTR(_name,_mode,_show,_store) { \
+ .attr = {.name = __stringify(_name), .mode = _mode }, \
+ .show = _show, \
+ .store = _store, \
}
-#define __ATTR_RO(_name) { \
- .attr = { .name = __stringify(_name), .mode = 0444 }, \
- .show = _name##_show, \
+#define __ATTR_RO(_name) { \
+ .attr = { .name = __stringify(_name), .mode = S_IRUGO }, \
+ .show = _name##_show, \
}
-#define __ATTR_RW(_name) __ATTR(_name, 0644, _name##_show, _name##_store)
+#define __ATTR_RW(_name) __ATTR(_name, (S_IWUSR | S_IRUGO), \
+ _name##_show, _name##_store)
#define __ATTR_NULL { .attr = { .name = NULL } }
--
1.8.1.5
Re: [PATCH v2 2/7] sysfs.h: add ATTRIBUTE_GROUPS() macro
On 07/11/13 19:06, Greg Kroah-Hartman wrote:
On Thu, Jul 11, 2013 at 01:58:29PM +0200, Oliver Schinagl wrote:
On 11-07-13 02:36, Greg Kroah-Hartman wrote:
To make it easier for driver subsystems to work with attribute groups,
create the ATTRIBUTE_GROUPS macro to remove some of the repetitive
typing for the most common use for attribute groups.
But binary groups are discriminated against :(
Yes, as they are rarer by far, as they should be. binary sysfs files
should almost never be used, as they are only pass-through files to
the hardware, so I want to see you do more work in order to use them, as
they should not be created lightly.
I guess I can see a valid reason here, but they are only helper macro's
making life easier for people who do need to use these and are on par
with the non-binary versions. And we already have quite some binary
attributes, probably far less then normal ones :)
Anyway, wouldn't all users be reviewed anyway? But I guess it's a small
safety net to make it not TOO easy.
The attached patch should help here.
Can you give me an example of using these macros? I seem to be lost in
them somehow, or maybe my morning coffee just hasn't kicked in...
Yeah, I kinda added the whole shebang there :) I was trying being helpful :(
I suppose one more additional helper wouldn't be bad to have:
#define ATTRIBUTE_(BIN_)GROUPS_R[O/W](_name(, _size)) \
ATTRIBUTE_(BIN_)ATTR_R[O/W](_name, _size); \
ATTRIBUTE_(BIN_)GROUPS(_name)
Would that ever be needed?
Of ourse, by the lazy :)
I think now you create an attribute in a group as this (with this patch):
ATTRIBUTE_ATTR_RO(name, SIZE);
ATTRIBUTE_GROUPS(name);
.group = name;
After that last addition, you'd simply do:
ATTRIBUTE_GROUPS_RO(name);
.group = name;
saves a whole line :)
From 003ab7a74ff689daa6934e7bc50c498b2d35a1cc Mon Sep 17 00:00:00 2001
From: Oliver Schinagl oli...@schinagl.nl
Date: Thu, 11 Jul 2013 13:48:18 +0200
Subject: [PATCH] sysfs: add more helper macro's for (bin_)attribute(_groups)
With the recent changes to sysfs there's various helper macro's.
However there's no RW, RO BIN_ helper macro's. This patch adds them.
Additionally there are no BIN_ group helpers so there's that aswell
Moreso, if both bin and normal attribute groups are used, there's a
simple helper for that, though the naming code be better. _TXT_ for the
show/store ones and neither TXT or BIN for both, but that would change
things to extensivly.
Finally there's also helpers for ATTRIBUTE_ATTRS.
After this patch, create default attributes can be as easy as:
ATTRIBUTE_(BIN_)ATTR_RO(name, SIZE);
ATTRIBUTE_BIN_GROUPS(name);
Signed-off-by: Oliver Schinagl oli...@schinagl.nl
---
include/linux/sysfs.h | 96 ---
1 file changed, 84 insertions(+), 12 deletions(-)
diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h
index 2c3b6a3..0ebed11 100644
--- a/include/linux/sysfs.h
+++ b/include/linux/sysfs.h
@@ -17,6 +17,7 @@
#include linux/list.h
#include linux/lockdep.h
#include linux/kobject_ns.h
+#include linux/stat.h
#include linux/atomic.h
struct kobject;
@@ -94,15 +95,32 @@ struct attribute_group {
#define __ATTR_IGNORE_LOCKDEP __ATTR
#endif
-#define ATTRIBUTE_GROUPS(name) \
-static const struct attribute_group name##_group = { \
- .attrs = name##_attrs, \
+#define __ATTRIBUTE_GROUPS(_name) \
+static const struct attribute_group *_name##_groups[] = { \
+ _name##_group, \
+ NULL, \
+}
+
+#define ATTRIBUTE_GROUPS(_name)\
+static const struct attribute_group _name##_group = { \
+ .attrs = _name##_attrs, \
};\
-static const struct attribute_group *name##_groups[] = { \
- name##_group, \
+__ATTRIBUTE_GROUPS(_name)
+
+#define __ATTRIBUTE_ATTRS(_name) \
+struct bin_attribute *_name##_attrs[] = { \
typo here, scrap bin_ copy paste fail!
+ _name##_attr, \
NULL, \
}
+#define ATTRIBUTE_ATTR_RO(_name, _size)\
+struct attribute _name##_attr = __ATTR_RO(_name, _size); \
+__ATTRIBUTE_ATTRS(_name)
+
+#define ATTRIBUTE_ATTR_RW(_name, _size)\
+struct attribute _name##_attr = __ATTR_RW(_name, _size); \
+__ATTRIBUTE_ATTRS(_name)
What do these two help out with? attribute attribute read-write seems
a bit clunky, don't you think? :)
I aggree, but I tried to stick with the ATTRIBUTE_GROUP naming and
that's the best I could come up
Re: [PATCH v2 2/7] sysfs.h: add ATTRIBUTE_GROUPS() macro
On 07/11/13 22:26, Greg Kroah-Hartman wrote:
On Thu, Jul 11, 2013 at 10:09:11PM +0200, Oliver Schinagl wrote:
On 07/11/13 19:06, Greg Kroah-Hartman wrote:
On Thu, Jul 11, 2013 at 01:58:29PM +0200, Oliver Schinagl wrote:
On 11-07-13 02:36, Greg Kroah-Hartman wrote:
To make it easier for driver subsystems to work with attribute groups,
create the ATTRIBUTE_GROUPS macro to remove some of the repetitive
typing for the most common use for attribute groups.
But binary groups are discriminated against :(
Yes, as they are rarer by far, as they should be. binary sysfs files
should almost never be used, as they are only pass-through files to
the hardware, so I want to see you do more work in order to use them, as
they should not be created lightly.
I guess I can see a valid reason here, but they are only helper
macro's making life easier for people who do need to use these and
are on par with the non-binary versions. And we already have quite
some binary attributes, probably far less then normal ones :)
I only count about 100 valid binary files in the tree at the moment,
that's not really all that many to handle.
100 is quite a few :) But point taken.
Anyway, wouldn't all users be reviewed anyway? But I guess it's a
small safety net to make it not TOO easy.
exactly :)
I aggree and this is a v2 that strips all the additional bits.
A few comments left below.
The attached patch should help here.
Can you give me an example of using these macros? I seem to be lost in
them somehow, or maybe my morning coffee just hasn't kicked in...
Yeah, I kinda added the whole shebang there :) I was trying being helpful :(
I suppose one more additional helper wouldn't be bad to have:
#define ATTRIBUTE_(BIN_)GROUPS_R[O/W](_name(, _size)) \
ATTRIBUTE_(BIN_)ATTR_R[O/W](_name, _size); \
ATTRIBUTE_(BIN_)GROUPS(_name)
Would that ever be needed?
Of ourse, by the lazy :)
I think now you create an attribute in a group as this (with this patch):
ATTRIBUTE_ATTR_RO(name, SIZE);
but raw attributes are rare, you really want a device, class, or
bus attribute, right?
I suppose so, But I got stuck in the rare case some how initially. I
registered my driver with module_platform_driver(); and in that struct i
had the device_driver which had a group attribute so I used that. I
already learned from maxime that that is the wrong way :) and hopefully
I'll figure out what the right way will be soon ;)
ATTRIBUTE_GROUPS(name);
.group = name;
After that last addition, you'd simply do:
ATTRIBUTE_GROUPS_RO(name);
.group = name;
saves a whole line :)
Not worth it :)
From 003ab7a74ff689daa6934e7bc50c498b2d35a1cc Mon Sep 17 00:00:00 2001
From: Oliver Schinagl oli...@schinagl.nl
Date: Thu, 11 Jul 2013 13:48:18 +0200
Subject: [PATCH] sysfs: add more helper macro's for (bin_)attribute(_groups)
With the recent changes to sysfs there's various helper macro's.
However there's no RW, RO BIN_ helper macro's. This patch adds them.
Additionally there are no BIN_ group helpers so there's that aswell
Moreso, if both bin and normal attribute groups are used, there's a
simple helper for that, though the naming code be better. _TXT_ for the
show/store ones and neither TXT or BIN for both, but that would change
things to extensivly.
Finally there's also helpers for ATTRIBUTE_ATTRS.
After this patch, create default attributes can be as easy as:
ATTRIBUTE_(BIN_)ATTR_RO(name, SIZE);
ATTRIBUTE_BIN_GROUPS(name);
Signed-off-by: Oliver Schinagl oli...@schinagl.nl
---
include/linux/sysfs.h | 96 ---
1 file changed, 84 insertions(+), 12 deletions(-)
diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h
index 2c3b6a3..0ebed11 100644
--- a/include/linux/sysfs.h
+++ b/include/linux/sysfs.h
@@ -17,6 +17,7 @@
#include linux/list.h
#include linux/lockdep.h
#include linux/kobject_ns.h
+#include linux/stat.h
#include linux/atomic.h
struct kobject;
@@ -94,15 +95,32 @@ struct attribute_group {
#define __ATTR_IGNORE_LOCKDEP __ATTR
#endif
-#define ATTRIBUTE_GROUPS(name) \
-static const struct attribute_group name##_group = { \
- .attrs = name##_attrs, \
+#define __ATTRIBUTE_GROUPS(_name) \
+static const struct attribute_group *_name##_groups[] = { \
+ _name##_group, \
+ NULL, \
+}
+
+#define ATTRIBUTE_GROUPS(_name)\
+static const struct attribute_group _name##_group = { \
+ .attrs = _name##_attrs, \
};\
-static const struct attribute_group *name##_groups[] = { \
- name##_group, \
+__ATTRIBUTE_GROUPS(_name)
+
+#define __ATTRIBUTE_ATTRS(_name
Re: Driver core and sysfs changes for attribute groups
On 07/10/13 22:05, Greg Kroah-Hartman wrote: Hi all, Hey Greg, Guenter and Oliver have been pointing out a few limitations of the driver core's ability to create files properly (i.e. in a way that doesn't race with userspace.) The driver core allows this, but it doesn't export that ability to drivers very easily, and for binary files, not at all. Exactly, even the patch you supplied didn't help, as it created the binary attributes for devices, not device_drivers or platform_drivers. So here's a set of 6 patches that I'll be queueing up to go to Linus in time for 3.11 so that people can start using them in their driver subsystems. It adds some new macros to make using attributes and attribute groups easier, adds binary file capabilities to attribute groups, and finally, lets subsystems (like platform drivers) set a attribute group for when their device is created. Oh, that does some like iceing on the cake. If anyone has any problems with these patches, please let me know. I will try this patch set immediately and report back. Guenter, I've tweaked your original patch a bit, changing the name of the function and putting the kernel doc comments in the correct place so the build doesn't complain about it. I also have a set of follow-on patches, about 50+ big so far, that goes through the kernel and converts different drivers and subsystems to properly use attribute groups, instead of open-coding binary files and attributes. Those patches will be sent out later, and will be for 3.12 as they aren't needed at the moment, this infrastructure changes are needed first. thanks, Thank you Greg, this is far more robust that what I cooked up (adding bin_attrs to struct device_driver. I was just going to send it to the list as well when I saw this message :) greg k-h Oliver -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Driver core and sysfs changes for attribute groups
On 07/10/13 22:05, Greg Kroah-Hartman wrote: Hi all, Hey Greg, Guenter and Oliver have been pointing out a few limitations of the driver core's ability to create files properly (i.e. in a way that doesn't race with userspace.) The driver core allows this, but it doesn't export that ability to drivers very easily, and for binary files, not at all. Exactly, even the patch you supplied didn't help, as it created the binary attributes for devices, not device_drivers or platform_drivers. So here's a set of 6 patches that I'll be queueing up to go to Linus in time for 3.11 so that people can start using them in their driver subsystems. It adds some new macros to make using attributes and attribute groups easier, adds binary file capabilities to attribute groups, and finally, lets subsystems (like platform drivers) set a attribute group for when their device is created. Oh, that does some like iceing on the cake. If anyone has any problems with these patches, please let me know. I will try this patch set immediately and report back. Guenter, I've tweaked your original patch a bit, changing the name of the function and putting the kernel doc comments in the correct place so the build doesn't complain about it. I also have a set of follow-on patches, about 50+ big so far, that goes through the kernel and converts different drivers and subsystems to properly use attribute groups, instead of open-coding binary files and attributes. Those patches will be sent out later, and will be for 3.12 as they aren't needed at the moment, this infrastructure changes are needed first. thanks, Thank you Greg, this is far more robust that what I cooked up (adding bin_attrs to struct device_driver. I was just going to send it to the list as well when I saw this message :) greg k-h Oliver -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [linux-sunxi] Re: [PATCHv4 08/10] clocksource: sun4i: Remove TIMER_SCAL variable
On 06-07-13 10:10, Maxime Ripard wrote: I'll send a v5. Do you have any additionnal comments on those patches to avoid wasting more electrons? Pff, we all know Maxime's electrons are free Maxime -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [linux-sunxi] Re: [PATCHv4 08/10] clocksource: sun4i: Remove TIMER_SCAL variable
On 06-07-13 10:10, Maxime Ripard wrote: I'll send a v5. Do you have any additionnal comments on those patches to avoid wasting more electrons? Pff, we all know Maxime's electrons are free Maxime -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2] Initial support for Allwinner's Security ID fuses
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(>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(>dev, _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_
Re: [PATCH 1/2] Initial support for Allwinner's Security ID fuses
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
Re: [PATCH 1/2] Initial support for Allwinner's Security ID fuses
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(>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(>dev, _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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2] Initial support for Allwinner's Security ID fuses
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
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2] Initial support for Allwinner's Security ID fuses
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(>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(>dev, _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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2] Initial support for Allwinner's Security ID fuses
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(>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(>dev, _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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2] Initial support for Allwinner's Security ID fuses
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
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2] Initial support for Allwinner's Security ID fuses
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
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH 1/2] Initial support for Allwinner's Security ID fuses
From: Oliver Schinagl
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
---
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 000..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
+ *
+ * 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
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#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_rea
[PATCH 0/2] v4 Driver for Allwinner sunxi Security ID
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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH 2/2] Add sunxi-sid to dts for sun4i and sun5i
From: Oliver Schinagl
This patch shall add support for the sunxi-sid driver to the device table for
sun4i and sun5i.
Signed-off-by: Oliver Schinagl
---
arch/arm/boot/dts/sun4i-a10.dtsi | 5 +
arch/arm/boot/dts/sun5i-a13.dtsi | 5 +
2 files changed, 10 insertions(+)
diff --git a/arch/arm/boot/dts/sun4i-a10.dtsi b/arch/arm/boot/dts/sun4i-a10.dtsi
index e7ef619..bc71d64 100644
--- a/arch/arm/boot/dts/sun4i-a10.dtsi
+++ b/arch/arm/boot/dts/sun4i-a10.dtsi
@@ -213,6 +213,11 @@
reg = <0x01c20c90 0x10>;
};
+ sid: eeprom@01c23800 {
+ compatible = "allwinner,sun4i-sid";
+ reg = <0x01c23800 0x10>;
+ };
+
uart0: serial@01c28000 {
compatible = "snps,dw-apb-uart";
reg = <0x01c28000 0x400>;
diff --git a/arch/arm/boot/dts/sun5i-a13.dtsi b/arch/arm/boot/dts/sun5i-a13.dtsi
index 8ba65c1..c80c81b 100644
--- a/arch/arm/boot/dts/sun5i-a13.dtsi
+++ b/arch/arm/boot/dts/sun5i-a13.dtsi
@@ -196,6 +196,11 @@
reg = <0x01c20c90 0x10>;
};
+ sid: eeprom@01c23800 {
+ compatible = "allwinner,sun4i-sid";
+ reg = <0x01c23800 0x10>;
+ };
+
uart1: serial@01c28400 {
compatible = "snps,dw-apb-uart";
reg = <0x01c28400 0x400>;
--
1.8.1.5
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] Add sunxi-sid to dts for sun4i and sun5i
From: Oliver Schinagl
This patch shall add support for the sunxi-sid driver to the device table for
sun4i and sun5i.
Signed-off-by: Oliver Schinagl
---
arch/arm/boot/dts/sun4i-a10.dtsi | 5 +
arch/arm/boot/dts/sun5i-a13.dtsi | 5 +
2 files changed, 10 insertions(+)
diff --git a/arch/arm/boot/dts/sun4i-a10.dtsi b/arch/arm/boot/dts/sun4i-a10.dtsi
index e7ef619..bc71d64 100644
--- a/arch/arm/boot/dts/sun4i-a10.dtsi
+++ b/arch/arm/boot/dts/sun4i-a10.dtsi
@@ -213,6 +213,11 @@
reg = <0x01c20c90 0x10>;
};
+ sid: eeprom@01c23800 {
+ compatible = "allwinner,sun4i-sid";
+ reg = <0x01c23800 0x10>;
+ };
+
uart0: serial@01c28000 {
compatible = "snps,dw-apb-uart";
reg = <0x01c28000 0x400>;
diff --git a/arch/arm/boot/dts/sun5i-a13.dtsi b/arch/arm/boot/dts/sun5i-a13.dtsi
index 8ba65c1..c80c81b 100644
--- a/arch/arm/boot/dts/sun5i-a13.dtsi
+++ b/arch/arm/boot/dts/sun5i-a13.dtsi
@@ -196,6 +196,11 @@
reg = <0x01c20c90 0x10>;
};
+ sid: eeprom@01c23800 {
+ compatible = "allwinner,sun4i-sid";
+ reg = <0x01c23800 0x10>;
+ };
+
uart1: serial@01c28400 {
compatible = "snps,dw-apb-uart";
reg = <0x01c28400 0x400>;
--
1.8.1.5
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] v4 Driver for Allwinner sunxi Security ID
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 (1): 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 + 2 files changed, 10 insertions(+) -- 1.8.1.5 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2] Initial support for Allwinner's Security ID fuses
On 17-06-13 15:23, Tomasz Figa wrote: On Monday 17 of June 2013 15:10:47 Oliver Schinagl wrote: On 17-06-13 14:51, Tomasz Figa wrote: On Monday 17 of June 2013 12:36:47 Oliver Schinagl wrote: On 15-06-13 12:28, Tomasz Figa wrote: Now we pass this array to add_randomness(array, 16). So add_randomness sees 16 ints in an array. So while there will be a lot of extra zero's, there still be 16 elements copied/processed, no? The second argument of add_randomness is number of bytes, not number of elements in array, as far as I can tell. Oh wow, very good catch. I had to dig a little into the source where it said 'nbytes'. I guess the function prototype would have been nicer if it said nbytes instead of size. Anyway, with using an array of u8 this is nicely handled. Best regards, Tomasz oliver -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2] Initial support for Allwinner's Security ID fuses
On 17-06-13 14:51, Tomasz Figa wrote:
On Monday 17 of June 2013 12:36:47 Oliver Schinagl wrote:
On 15-06-13 12:28, Tomasz Figa wrote:
Hi,
Some comments inline.
Thank you
You're welcome. :)
On Saturday 15 of June 2013 01:16:20 Oliver Schinagl wrote:
From: Oliver Schinagl
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.
I will change the comment, 'and 4 byte sized keys per SID' is probably
better
The array is 128 bits split into 32 bit words. Each 32 bit word consists
of 8 bits (1 byte).
So 4 * 4 = 16 bytes (SID_SIZE), is 128 bits.
What about:
/* There are 4 keys. */
#define SID_KEYS 4
/* Each key is 4 byte long (32-bit). */
#define SID_SIZE (SID_KEYS * 4)
I'll ommit the 'long (32-bit)' part but yeah that's probably enough.
+
+ if (offset >= SID_SIZE)
+ goto exit;
return 0; ...
I did say in the changelog I opted for goto over return. But since
everybody keeps preferring returns (I personally like 'one single exit
point much more' I have already changed it all over to many returns, who
am I to argue :)
Well, single exit points makes sense (and is much nicer) when you have
something to do before exiting, take error paths as an example. But jumping
just to return makes no sense, because when reading the code you must scroll
down to the label to check what actually happens.
But functions shouldn't be so large :p But that is the first reasonable
reason I can live with :)
+
+ for (i = 0; i < SID_SIZE; i++)
+ entropy[i] = sunxi_sid_read_byte(sid_reg_base, i);
You seem to read bytes into an array of ints. Your entropy data will
always have most significant 24-bits cleared. Is this behavior correct?
Yes, though I changed it so that entropy is an array of u8's, since
that's what sunxi_sid_read_byte returns.
+ add_device_randomness(entropy, SID_SIZE);
Now I'm pretty sure that above is not the correct behavior. You are adding
here first 16 bytes (=SID_SIZE) of entropy[], while it is an array of 16
ints (=4*SID_SIZE)...
Well technically, doesn't to compiler see that entropy is never larger
then 8 bits and thus uses only 8 bits? uint8_atleast or something. But
yeah, it's better to use the specified size to not waste 24 empty bits.
I mean, the loop fills the array with SID_SIZE ints, each with 3 zero bytes and
1 byte of actual data, so you get:
S0 0x00 0x00 0x00 S1 0x00 0x00 0x00 ... S15 0x00 0x00 0x00
Ok, I get that
but by calling add_device_randomness() with SID_SIZE as size argument, you add
only 16 first bytes of data from the array:
S0 0x00 0x00 0x00 S1 0x00 0x00 0x00 ... S3 0x00 0x00 0x00
That bit I'm not quite sure I understand:
We have an array of ints, { 0x, 0x, 0x }
We read 1 byte and copy it to the array (x16) (say sid = 0xdeadbeef...)
{ 0x00de, 0x00ad, 0x00be, ... }
Now we pass this array to add_randomness(array, 16). So add_randomness
sees 16 ints in an array. So while there will be a lot of extra zero's,
there still be 16 elements copied/processed, no?
Otherwise, how does add_randomness() know it's dealing with bytes or
ints? it just see's the pointer to an int array that is 16 long? Or what
am I overlooking?
I did already change the array to be u8 big so it is only to help me
understand.
(little endianness assumed)
Best regards,
Tomasz
(for rest of comments I think it's enough said in Russell's and Maxime's
replies)
Yes it has :) thanks to all of you
Oliver
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2] Initial support for Allwinner's Security ID fuses
On 17-06-13 13:51, Maxime Ripard wrote:
On Mon, Jun 17, 2013 at 12:36:47PM +0200, Oliver Schinagl wrote:
On 15-06-13 12:28, Tomasz Figa wrote:
+#define DRV_VERSION "1.0"
What is this version thingy?
Is there a versioning scheme defined for this driver? Do you expect it to
be changed every modification of this driver?
I don't see any point of having such thing in a project with a version
control system, where you have all change history.
Well we export something to userspace, while trivial there is the
possibility it changes over time. Say A40 which outputs 256 bits
instead of the current 128 bits. That would validate a bump in
version number. It's purely so the user can be aware of differences
in the driver. So maybe DRV_A[BP]I_VERSION would be better?
Except that in that case, the userspace API won't change. You'll still
call read on the same file in sysfs. The only thing that will change
will be the number of bytes returned by your read function, which is
(or should be) totally fine.
Aye, russel pointed this out as well, and I agree.
+/* We read the entire key, but only return the requested byte. This is
of + * course slower then it could be and uses 4 times more reads as
needed but + * keeps code simpler.
I have no idea how often this is going to be read, but since the whole sid
is really small (16 bytes), maybe it would be better to simply read the ID
in probe to a buffer and then just memcpy from it in read().
The sid will be read once (well all 16 bytes) during probe and after
that only on user request. Right now we don't have a user (yet)
other then the sysfs entry.
In future, this key can be used to read the MAC (low reads) or AES
keys for example (also low reads).
Initially I had such an approach, but Maxime recommended against
having the value cached.
As I wrote to andy, the only 'more efficient' way would be to store
the previously read key and see on the next read if its the same, So
best case, we could save 4 reads. I think it makes the code
unnecessarily complex for something that is read so little.
I asked Oliver that because I felt like it could still be updated by the
user, and sysfs should report that.
And since it's not like it would be used extensively and very often,
it's not a big deal anyway.
Actually it might still be possible to change the sid. We have figured
out how we possible can program it (the 2.5 EFUSE_VDD pin, which olimex
actually mapped out on the board) but requires someone to actually take
the plunge and test it. So in theory, it can be changed still.
+
+ if (offset >= SID_SIZE)
+ goto exit;
return 0; ...
I did say in the changelog I opted for goto over return. But since
everybody keeps preferring returns (I personally like 'one single
exit point much more' I have already changed it all over to many
returns, who am I to argue :)
Yet, you don't have a single exit point neither, you have several of
them. You can say that you have a single return statement in your code,
which is true, yet you have several times a jump to this location, so we
can definitely say that you actually have several exit points. Please
also refer to the chapter 7 of the Documentation/CodingStyle file.
You are right of course :)
+ return (u8)sid_key;
Unnecessary casting.
Unnecessary because of the &= 0xff above, or because you can put a
32bit int in an 8bit int without worries? (we only want the last
byte).
The latter. The & 0xff only filter out non-relevant informations from
your 32-bits integer in that case, that's all, it doesn't do any
casting.
So for clarity not leave the cast? Will it hurt? Will the compiler not
do the cast anyway?
+ for (i = 0; i < size; i++) {
+ if ((pos + i) >= SID_SIZE || (pos < 0))
+ break;
+ buf[i] = sunxi_sid_read_byte(sid_reg_base, pos + i);
+ }
This could be greatly simplified if you just read the whole sid to memory
in probe and memcpy from it here.
But can't because we don't want to cache it.
And we can't simply use memcpy, since we will need to do some endianness
conversions. The data stored in the SID are big-endian, while we're
running most likely in little endian mode.
+ if (!pdev->dev.of_node) {
+ dev_err(>dev, "No devicetree data available\n");
+ ret = -ENXIO;
+ goto exit;
+ }
What is this check for? You don't seem to need anything from dev.of_node
in this driver.
My understanding was, that when using the device tree, we check if
the device tree is atleast available. And we use
platform_get_resource, doesn't that get the data from the device
tree?
If there's no device tree, you won't be probed in the first place. And
resources get filled by the kernel from the device tree automatically at
boot, so you're safe to assume that the resources are there when you get
probed.
Ahh, assumption, the mot
Re: [PATCH 1/2] Initial support for Allwinner's Security ID fuses
On 17-06-13 13:25, Russell King - ARM Linux wrote:
On Mon, Jun 17, 2013 at 12:36:47PM +0200, Oliver Schinagl wrote:
On 15-06-13 12:28, Tomasz Figa wrote:
What is this version thingy?
Is there a versioning scheme defined for this driver? Do you expect it to
be changed every modification of this driver?
I don't see any point of having such thing in a project with a version
control system, where you have all change history.
Well we export something to userspace, while trivial there is the
possibility it changes over time. Say A40 which outputs 256 bits instead
of the current 128 bits. That would validate a bump in version number.
It's purely so the user can be aware of differences in the driver. So
maybe DRV_A[BP]I_VERSION would be better?
What is better to do is to export such things as properties, or
design the API in such a way that the length of the ID is reportable.
However, it's actually quite easy to do if you only care about the
number of bytes - you just arrange for the read() function to return
the number of bytes read. So in the case of 128 bits available, that's
16 bytes, so a read() of the sysfs attribute with a buffer of (say)
256 bytes should report only 16 bytes read.
If it were to become 256 bytes later, then the read() would return
32 bytes read. So there's no need for any new APIs to do this.
That makes sense for the sysfs bit and as the only user, I guess makes
the version information obsolete for now.
Also, this is over-complicated:
+ for (i = 0; i < size; i++) {
+ if ((pos + i) >= SID_SIZE || (pos < 0))
+ break;
+ buf[i] = sunxi_sid_read_byte(sid_reg_base, pos + i);
+ }
Maybe:
if (pos < 0 || pos >= SID_SIZE)
return 0;
if (size > SID_SIZE - pos)
size = SID_SIZE - pos;
for (i = 0; i < size; i++)
buf[i] = sunxi_sid_read_byte(sid_reg_base, pos + i);
return size;
I do like your approach, but takes a second to read ;) How is it less
complicated though? It's more LOC i suppose. I do appreciate that we
only perform the read function when our size is correct, thus making the
for loop only execute the minimally required code. While in this driver
is insignificant and not important, I am a proponent of it.
Consider it changed.
Will wait a bit for Thomaz to optionally reply and then send yet a
nother version ;)
Thanks for your time,
Oliver
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2] Initial support for Allwinner's Security ID fuses
On 15-06-13 12:28, Tomasz Figa wrote: Hi, Some comments inline. Thank you On Saturday 15 of June 2013 01:16:20 Oliver Schinagl wrote: From: Oliver Schinagl 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 --- drivers/misc/eeprom/Kconfig | 17 drivers/misc/eeprom/Makefile| 1 + drivers/misc/eeprom/sunxi_sid.c | 167 3 files changed, 185 insertions(+) create mode 100644 drivers/misc/eeprom/sunxi_sid.c diff --git a/drivers/misc/eeprom/Kconfig b/drivers/misc/eeprom/Kconfig index 04f2e1f..c7bc6ed 100644 --- a/drivers/misc/eeprom/Kconfig +++ b/drivers/misc/eeprom/Kconfig @@ -96,4 +96,21 @@ config EEPROM_DIGSY_MTC_CFG If unsure, say N. +config EEPROM_SUNXI_SID + tristate "Allwinner sunxi security ID support" + depends on ARCH_SUNXI && SYSFS + help + This is a driver for the 'security ID' available on various Allwinner + devices. Currently supported are: + sun4i (A10) + sun5i (A13) + + Due to the potential risks involved with changing e-fuses, + this driver is read-only + + For more information visit http://linux-sunxi.org/SID + + This driver can also be built as a module. If so, the module + will be called sunxi_sid. + endmenu diff --git a/drivers/misc/eeprom/Makefile b/drivers/misc/eeprom/Makefile index fc1e81d..9507aec 100644 --- a/drivers/misc/eeprom/Makefile +++ b/drivers/misc/eeprom/Makefile @@ -4,4 +4,5 @@ obj-$(CONFIG_EEPROM_LEGACY) += eeprom.o obj-$(CONFIG_EEPROM_MAX6875) += max6875.o obj-$(CONFIG_EEPROM_93CX6)+= eeprom_93cx6.o obj-$(CONFIG_EEPROM_93XX46) += eeprom_93xx46.o +obj-$(CONFIG_EEPROM_SUNXI_SID) += sunxi_sid.o obj-$(CONFIG_EEPROM_DIGSY_MTC_CFG) += digsy_mtc_eeprom.o diff --git a/drivers/misc/eeprom/sunxi_sid.c b/drivers/misc/eeprom/sunxi_sid.c new file mode 100644 index 000..f014e1b --- /dev/null +++ b/drivers/misc/eeprom/sunxi_sid.c @@ -0,0 +1,167 @@ +/* + * Copyright (c) 2013 Oliver Schinagl + * http://www.linux-sunxi.org + * + * Oliver Schinagl + * + * 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 +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +#define DRV_NAME "sunxi-sid" +#define DRV_VERSION "1.0" What is this version thingy? Is there a versioning scheme defined for this driver? Do you expect it to be changed every modification of this driver? I don't see any point of having such thing in a project with a version control system, where you have all change history. Well we export something to userspace, while trivial there is the possibility it changes over time. Say A40 which outputs 256 bits instead of the current 128 bits. That would validate a bump in version number. It's purely so the user can be aware of differences in the driver. So maybe DRV_A[BP]I_VERSION would be better? + +/* There are 4 32-bit keys */ +#define SID_KEYS 4 +/* and 4 byte sized keys per 32-bit key */ +#define SID_SIZE (SID_KEYS * 4) From this definition it looks like there are just 4 32-bit keys, I don't see those extra four byte-sized keys accounted by this size. I will change the comment, 'and 4 byte sized keys per SID' is probably better The array is 128 bits split into 32 bit words. Each 32 bit word consists of 8 bits (1 byte). So 4 * 4 = 16 bytes (SID_SIZE), is 128 bits. + + One _empty_ line is enough to separate definitions from code. Okay, I find it personally a little clearer to separate them, b
Re: [PATCH 1/2] Initial support for Allwinner's Security ID fuses
On 15-06-13 12:28, Tomasz Figa wrote: Hi, Some comments inline. Thank you On Saturday 15 of June 2013 01:16:20 Oliver Schinagl wrote: From: Oliver Schinagl oli...@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 oli...@schinagl.nl --- drivers/misc/eeprom/Kconfig | 17 drivers/misc/eeprom/Makefile| 1 + drivers/misc/eeprom/sunxi_sid.c | 167 3 files changed, 185 insertions(+) create mode 100644 drivers/misc/eeprom/sunxi_sid.c diff --git a/drivers/misc/eeprom/Kconfig b/drivers/misc/eeprom/Kconfig index 04f2e1f..c7bc6ed 100644 --- a/drivers/misc/eeprom/Kconfig +++ b/drivers/misc/eeprom/Kconfig @@ -96,4 +96,21 @@ config EEPROM_DIGSY_MTC_CFG If unsure, say N. +config EEPROM_SUNXI_SID + tristate Allwinner sunxi security ID support + depends on ARCH_SUNXI SYSFS + help + This is a driver for the 'security ID' available on various Allwinner + devices. Currently supported are: + sun4i (A10) + sun5i (A13) + + Due to the potential risks involved with changing e-fuses, + this driver is read-only + + For more information visit http://linux-sunxi.org/SID + + This driver can also be built as a module. If so, the module + will be called sunxi_sid. + endmenu diff --git a/drivers/misc/eeprom/Makefile b/drivers/misc/eeprom/Makefile index fc1e81d..9507aec 100644 --- a/drivers/misc/eeprom/Makefile +++ b/drivers/misc/eeprom/Makefile @@ -4,4 +4,5 @@ obj-$(CONFIG_EEPROM_LEGACY) += eeprom.o obj-$(CONFIG_EEPROM_MAX6875) += max6875.o obj-$(CONFIG_EEPROM_93CX6)+= eeprom_93cx6.o obj-$(CONFIG_EEPROM_93XX46) += eeprom_93xx46.o +obj-$(CONFIG_EEPROM_SUNXI_SID) += sunxi_sid.o obj-$(CONFIG_EEPROM_DIGSY_MTC_CFG) += digsy_mtc_eeprom.o diff --git a/drivers/misc/eeprom/sunxi_sid.c b/drivers/misc/eeprom/sunxi_sid.c new file mode 100644 index 000..f014e1b --- /dev/null +++ b/drivers/misc/eeprom/sunxi_sid.c @@ -0,0 +1,167 @@ +/* + * Copyright (c) 2013 Oliver Schinagl + * http://www.linux-sunxi.org + * + * Oliver Schinagl oli...@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/errno.h +#include linux/export.h +#include linux/fs.h +#include linux/init.h +#include linux/io.h +#include linux/kernel.h +#include linux/kobject.h +#include linux/module.h +#include linux/of_address.h +#include linux/platform_device.h +#include linux/random.h +#include linux/stat.h +#include linux/sysfs.h +#include linux/types.h + +#define DRV_NAME sunxi-sid +#define DRV_VERSION 1.0 What is this version thingy? Is there a versioning scheme defined for this driver? Do you expect it to be changed every modification of this driver? I don't see any point of having such thing in a project with a version control system, where you have all change history. Well we export something to userspace, while trivial there is the possibility it changes over time. Say A40 which outputs 256 bits instead of the current 128 bits. That would validate a bump in version number. It's purely so the user can be aware of differences in the driver. So maybe DRV_A[BP]I_VERSION would be better? + +/* There are 4 32-bit keys */ +#define SID_KEYS 4 +/* and 4 byte sized keys per 32-bit key */ +#define SID_SIZE (SID_KEYS * 4) From this definition it looks like there are just 4 32-bit keys, I don't see those extra four byte-sized keys accounted by this size. I will change the comment, 'and 4 byte sized keys per SID' is probably better The array is 128 bits split into 32 bit
Re: [PATCH 1/2] Initial support for Allwinner's Security ID fuses
On 17-06-13 13:25, Russell King - ARM Linux wrote:
On Mon, Jun 17, 2013 at 12:36:47PM +0200, Oliver Schinagl wrote:
On 15-06-13 12:28, Tomasz Figa wrote:
What is this version thingy?
Is there a versioning scheme defined for this driver? Do you expect it to
be changed every modification of this driver?
I don't see any point of having such thing in a project with a version
control system, where you have all change history.
Well we export something to userspace, while trivial there is the
possibility it changes over time. Say A40 which outputs 256 bits instead
of the current 128 bits. That would validate a bump in version number.
It's purely so the user can be aware of differences in the driver. So
maybe DRV_A[BP]I_VERSION would be better?
What is better to do is to export such things as properties, or
design the API in such a way that the length of the ID is reportable.
However, it's actually quite easy to do if you only care about the
number of bytes - you just arrange for the read() function to return
the number of bytes read. So in the case of 128 bits available, that's
16 bytes, so a read() of the sysfs attribute with a buffer of (say)
256 bytes should report only 16 bytes read.
If it were to become 256 bytes later, then the read() would return
32 bytes read. So there's no need for any new APIs to do this.
That makes sense for the sysfs bit and as the only user, I guess makes
the version information obsolete for now.
Also, this is over-complicated:
+ for (i = 0; i size; i++) {
+ if ((pos + i) = SID_SIZE || (pos 0))
+ break;
+ buf[i] = sunxi_sid_read_byte(sid_reg_base, pos + i);
+ }
Maybe:
if (pos 0 || pos = SID_SIZE)
return 0;
if (size SID_SIZE - pos)
size = SID_SIZE - pos;
for (i = 0; i size; i++)
buf[i] = sunxi_sid_read_byte(sid_reg_base, pos + i);
return size;
I do like your approach, but takes a second to read ;) How is it less
complicated though? It's more LOC i suppose. I do appreciate that we
only perform the read function when our size is correct, thus making the
for loop only execute the minimally required code. While in this driver
is insignificant and not important, I am a proponent of it.
Consider it changed.
Will wait a bit for Thomaz to optionally reply and then send yet a
nother version ;)
Thanks for your time,
Oliver
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2] Initial support for Allwinner's Security ID fuses
On 17-06-13 13:51, Maxime Ripard wrote:
On Mon, Jun 17, 2013 at 12:36:47PM +0200, Oliver Schinagl wrote:
On 15-06-13 12:28, Tomasz Figa wrote:
+#define DRV_VERSION 1.0
What is this version thingy?
Is there a versioning scheme defined for this driver? Do you expect it to
be changed every modification of this driver?
I don't see any point of having such thing in a project with a version
control system, where you have all change history.
Well we export something to userspace, while trivial there is the
possibility it changes over time. Say A40 which outputs 256 bits
instead of the current 128 bits. That would validate a bump in
version number. It's purely so the user can be aware of differences
in the driver. So maybe DRV_A[BP]I_VERSION would be better?
Except that in that case, the userspace API won't change. You'll still
call read on the same file in sysfs. The only thing that will change
will be the number of bytes returned by your read function, which is
(or should be) totally fine.
Aye, russel pointed this out as well, and I agree.
+/* We read the entire key, but only return the requested byte. This is
of + * course slower then it could be and uses 4 times more reads as
needed but + * keeps code simpler.
I have no idea how often this is going to be read, but since the whole sid
is really small (16 bytes), maybe it would be better to simply read the ID
in probe to a buffer and then just memcpy from it in read().
The sid will be read once (well all 16 bytes) during probe and after
that only on user request. Right now we don't have a user (yet)
other then the sysfs entry.
In future, this key can be used to read the MAC (low reads) or AES
keys for example (also low reads).
Initially I had such an approach, but Maxime recommended against
having the value cached.
As I wrote to andy, the only 'more efficient' way would be to store
the previously read key and see on the next read if its the same, So
best case, we could save 4 reads. I think it makes the code
unnecessarily complex for something that is read so little.
I asked Oliver that because I felt like it could still be updated by the
user, and sysfs should report that.
And since it's not like it would be used extensively and very often,
it's not a big deal anyway.
Actually it might still be possible to change the sid. We have figured
out how we possible can program it (the 2.5 EFUSE_VDD pin, which olimex
actually mapped out on the board) but requires someone to actually take
the plunge and test it. So in theory, it can be changed still.
+
+ if (offset = SID_SIZE)
+ goto exit;
return 0; ...
I did say in the changelog I opted for goto over return. But since
everybody keeps preferring returns (I personally like 'one single
exit point much more' I have already changed it all over to many
returns, who am I to argue :)
Yet, you don't have a single exit point neither, you have several of
them. You can say that you have a single return statement in your code,
which is true, yet you have several times a jump to this location, so we
can definitely say that you actually have several exit points. Please
also refer to the chapter 7 of the Documentation/CodingStyle file.
You are right of course :)
+ return (u8)sid_key;
Unnecessary casting.
Unnecessary because of the = 0xff above, or because you can put a
32bit int in an 8bit int without worries? (we only want the last
byte).
The latter. The 0xff only filter out non-relevant informations from
your 32-bits integer in that case, that's all, it doesn't do any
casting.
So for clarity not leave the cast? Will it hurt? Will the compiler not
do the cast anyway?
+ for (i = 0; i size; i++) {
+ if ((pos + i) = SID_SIZE || (pos 0))
+ break;
+ buf[i] = sunxi_sid_read_byte(sid_reg_base, pos + i);
+ }
This could be greatly simplified if you just read the whole sid to memory
in probe and memcpy from it here.
But can't because we don't want to cache it.
And we can't simply use memcpy, since we will need to do some endianness
conversions. The data stored in the SID are big-endian, while we're
running most likely in little endian mode.
+ if (!pdev-dev.of_node) {
+ dev_err(pdev-dev, No devicetree data available\n);
+ ret = -ENXIO;
+ goto exit;
+ }
What is this check for? You don't seem to need anything from dev.of_node
in this driver.
My understanding was, that when using the device tree, we check if
the device tree is atleast available. And we use
platform_get_resource, doesn't that get the data from the device
tree?
If there's no device tree, you won't be probed in the first place. And
resources get filled by the kernel from the device tree automatically at
boot, so you're safe to assume that the resources are there when you get
probed.
Ahh, assumption, the mother.
But I put my trust in you ;)
I'll remove it
You need
Re: [PATCH 1/2] Initial support for Allwinner's Security ID fuses
On 17-06-13 14:51, Tomasz Figa wrote:
On Monday 17 of June 2013 12:36:47 Oliver Schinagl wrote:
On 15-06-13 12:28, Tomasz Figa wrote:
Hi,
Some comments inline.
Thank you
You're welcome. :)
On Saturday 15 of June 2013 01:16:20 Oliver Schinagl wrote:
From: Oliver Schinagl oli...@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.
snip
I will change the comment, 'and 4 byte sized keys per SID' is probably
better
The array is 128 bits split into 32 bit words. Each 32 bit word consists
of 8 bits (1 byte).
So 4 * 4 = 16 bytes (SID_SIZE), is 128 bits.
What about:
/* There are 4 keys. */
#define SID_KEYS 4
/* Each key is 4 byte long (32-bit). */
#define SID_SIZE (SID_KEYS * 4)
I'll ommit the 'long (32-bit)' part but yeah that's probably enough.
snip
+
+ if (offset = SID_SIZE)
+ goto exit;
return 0; ...
I did say in the changelog I opted for goto over return. But since
everybody keeps preferring returns (I personally like 'one single exit
point much more' I have already changed it all over to many returns, who
am I to argue :)
Well, single exit points makes sense (and is much nicer) when you have
something to do before exiting, take error paths as an example. But jumping
just to return makes no sense, because when reading the code you must scroll
down to the label to check what actually happens.
But functions shouldn't be so large :p But that is the first reasonable
reason I can live with :)
snip
+
+ for (i = 0; i SID_SIZE; i++)
+ entropy[i] = sunxi_sid_read_byte(sid_reg_base, i);
You seem to read bytes into an array of ints. Your entropy data will
always have most significant 24-bits cleared. Is this behavior correct?
Yes, though I changed it so that entropy is an array of u8's, since
that's what sunxi_sid_read_byte returns.
+ add_device_randomness(entropy, SID_SIZE);
Now I'm pretty sure that above is not the correct behavior. You are adding
here first 16 bytes (=SID_SIZE) of entropy[], while it is an array of 16
ints (=4*SID_SIZE)...
Well technically, doesn't to compiler see that entropy is never larger
then 8 bits and thus uses only 8 bits? uint8_atleast or something. But
yeah, it's better to use the specified size to not waste 24 empty bits.
I mean, the loop fills the array with SID_SIZE ints, each with 3 zero bytes and
1 byte of actual data, so you get:
S0 0x00 0x00 0x00 S1 0x00 0x00 0x00 ... S15 0x00 0x00 0x00
Ok, I get that
but by calling add_device_randomness() with SID_SIZE as size argument, you add
only 16 first bytes of data from the array:
S0 0x00 0x00 0x00 S1 0x00 0x00 0x00 ... S3 0x00 0x00 0x00
That bit I'm not quite sure I understand:
We have an array of ints, { 0x, 0x, 0x }
We read 1 byte and copy it to the array (x16) (say sid = 0xdeadbeef...)
{ 0x00de, 0x00ad, 0x00be, ... }
Now we pass this array to add_randomness(array, 16). So add_randomness
sees 16 ints in an array. So while there will be a lot of extra zero's,
there still be 16 elements copied/processed, no?
Otherwise, how does add_randomness() know it's dealing with bytes or
ints? it just see's the pointer to an int array that is 16 long? Or what
am I overlooking?
I did already change the array to be u8 big so it is only to help me
understand.
(little endianness assumed)
Best regards,
Tomasz
(for rest of comments I think it's enough said in Russell's and Maxime's
replies)
Yes it has :) thanks to all of you
Oliver
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2] Initial support for Allwinner's Security ID fuses
On 17-06-13 15:23, Tomasz Figa wrote: On Monday 17 of June 2013 15:10:47 Oliver Schinagl wrote: On 17-06-13 14:51, Tomasz Figa wrote: On Monday 17 of June 2013 12:36:47 Oliver Schinagl wrote: On 15-06-13 12:28, Tomasz Figa wrote: snip Now we pass this array to add_randomness(array, 16). So add_randomness sees 16 ints in an array. So while there will be a lot of extra zero's, there still be 16 elements copied/processed, no? The second argument of add_randomness is number of bytes, not number of elements in array, as far as I can tell. Oh wow, very good catch. I had to dig a little into the source where it said 'nbytes'. I guess the function prototype would have been nicer if it said nbytes instead of size. Anyway, with using an array of u8 this is nicely handled. Best regards, Tomasz oliver -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH] v4 Driver for Allwinner sunxi Security ID
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 (1): 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 + 2 files changed, 10 insertions(+) -- 1.8.1.5 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH] Add sunxi-sid to dts for sun4i and sun5i
From: Oliver Schinagl oli...@schinagl.nl
This patch shall add support for the sunxi-sid driver to the device table for
sun4i and sun5i.
Signed-off-by: Oliver Schinagl oli...@schinagl.nl
---
arch/arm/boot/dts/sun4i-a10.dtsi | 5 +
arch/arm/boot/dts/sun5i-a13.dtsi | 5 +
2 files changed, 10 insertions(+)
diff --git a/arch/arm/boot/dts/sun4i-a10.dtsi b/arch/arm/boot/dts/sun4i-a10.dtsi
index e7ef619..bc71d64 100644
--- a/arch/arm/boot/dts/sun4i-a10.dtsi
+++ b/arch/arm/boot/dts/sun4i-a10.dtsi
@@ -213,6 +213,11 @@
reg = 0x01c20c90 0x10;
};
+ sid: eeprom@01c23800 {
+ compatible = allwinner,sun4i-sid;
+ reg = 0x01c23800 0x10;
+ };
+
uart0: serial@01c28000 {
compatible = snps,dw-apb-uart;
reg = 0x01c28000 0x400;
diff --git a/arch/arm/boot/dts/sun5i-a13.dtsi b/arch/arm/boot/dts/sun5i-a13.dtsi
index 8ba65c1..c80c81b 100644
--- a/arch/arm/boot/dts/sun5i-a13.dtsi
+++ b/arch/arm/boot/dts/sun5i-a13.dtsi
@@ -196,6 +196,11 @@
reg = 0x01c20c90 0x10;
};
+ sid: eeprom@01c23800 {
+ compatible = allwinner,sun4i-sid;
+ reg = 0x01c23800 0x10;
+ };
+
uart1: serial@01c28400 {
compatible = snps,dw-apb-uart;
reg = 0x01c28400 0x400;
--
1.8.1.5
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH 2/2] Add sunxi-sid to dts for sun4i and sun5i
From: Oliver Schinagl oli...@schinagl.nl
This patch shall add support for the sunxi-sid driver to the device table for
sun4i and sun5i.
Signed-off-by: Oliver Schinagl oli...@schinagl.nl
---
arch/arm/boot/dts/sun4i-a10.dtsi | 5 +
arch/arm/boot/dts/sun5i-a13.dtsi | 5 +
2 files changed, 10 insertions(+)
diff --git a/arch/arm/boot/dts/sun4i-a10.dtsi b/arch/arm/boot/dts/sun4i-a10.dtsi
index e7ef619..bc71d64 100644
--- a/arch/arm/boot/dts/sun4i-a10.dtsi
+++ b/arch/arm/boot/dts/sun4i-a10.dtsi
@@ -213,6 +213,11 @@
reg = 0x01c20c90 0x10;
};
+ sid: eeprom@01c23800 {
+ compatible = allwinner,sun4i-sid;
+ reg = 0x01c23800 0x10;
+ };
+
uart0: serial@01c28000 {
compatible = snps,dw-apb-uart;
reg = 0x01c28000 0x400;
diff --git a/arch/arm/boot/dts/sun5i-a13.dtsi b/arch/arm/boot/dts/sun5i-a13.dtsi
index 8ba65c1..c80c81b 100644
--- a/arch/arm/boot/dts/sun5i-a13.dtsi
+++ b/arch/arm/boot/dts/sun5i-a13.dtsi
@@ -196,6 +196,11 @@
reg = 0x01c20c90 0x10;
};
+ sid: eeprom@01c23800 {
+ compatible = allwinner,sun4i-sid;
+ reg = 0x01c23800 0x10;
+ };
+
uart1: serial@01c28400 {
compatible = snps,dw-apb-uart;
reg = 0x01c28400 0x400;
--
1.8.1.5
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH 1/2] Initial support for Allwinner's Security ID fuses
From: Oliver Schinagl oli...@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 oli...@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 000..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 oli...@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
[PATCH 0/2] v4 Driver for Allwinner sunxi Security ID
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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2] Initial support for Allwinner's Security ID fuses
On 06/15/13 04:14, Andy Shevchenko wrote:
On Sat, Jun 15, 2013 at 2:16 AM, Oliver Schinagl
wrote:
From: Oliver Schinagl
Allwinner has electric fuses (efuse) on their line of chips. This driver
reads those fuses, seeds the kernel entropy and exports them as a sysfs node.
These fuses are most likly to be programmed at the factory, encoding
things like Chip ID, some sort of serial number etc and appear to be
reasonable unique.
While in theory, these should be writeable by the user, it will probably
be inconvinient to do so. Allwinner recommends that a certain input pin,
labeled 'efuse_vddq', be connected to GND. To write these fuses, 2.5 V
needs to be applied to this pin.
Even so, they can still be used to generate a board-unique mac from, board
unique RSA key and seed the kernel RNG.
Currently supported are the following known chips:
Allwinner sun4i (A10)
Allwinner sun5i (A10s, A13)
Few comments below.
+++ b/drivers/misc/eeprom/sunxi_sid.c
+#include
Are you sure this has to be explicitly mentioned?
Well we use __iomem from it, so I think yes
+#define SID_SIZE (SID_KEYS * 4)
+
+
Extra line.
to seperate defines/includes from the code?
+/* We read the entire key, but only return the requested byte. This is of
+ * course slower then it could be and uses 4 times more reads as needed but
+ * keeps code simpler.
May be better to rewrite this logic and save CPU and I/O resources?
Well we can only read 32 bits at a time and we only want to return 8
bits. The only think I can think of, is read 32 bits and store those
statically to the function and store with an extra int the location.
Then check against the location and if it's the same use the int. Not
sure all that is worth it.
+ */
+static u8 sunxi_sid_read_byte(const void __iomem *sid_reg_base,
+ const unsigned int offset)
+{
+ u32 sid_key = 0;
+
+ if (offset >= SID_SIZE)
+ goto exit;
Just return here.
Well as said before, return vs goto; i choose goto ;)
+ sid_key = ioread32be(sid_reg_base + round_down(offset, 4));
+ sid_key >>= (offset % 4) * 8;
+ sid_key &= 0xff;
Redundant 0xff.
Yes, but does clarify the intention, which helps in readability?
+ /* fall through */
+
+exit:
+ return (u8)sid_key;
No need to have explicit casting here.
Also here, it makes the intention clear, that we are only interested in
the last 8 bits. The compiler should optimize this and the above bit
away so it's only for readability.
+ pdev = (struct platform_device *)to_platform_device(kobj_to_dev(kobj));
Ditto.
Yes, I just looked more closy to container_of, and the 'const
typeof(((type *)0)->member) takes care of the typecast, does it not?
+ sid_reg_base = (void __iomem *)platform_get_drvdata(pdev);
Ditto.
Not sure on this one, since technically, it returns only void * (always)
and we had a void '__iomem *' pointer. for clarity and completness I
would keep it?
+static int sunxi_sid_remove(struct platform_device *pdev)
+{
+ device_remove_bin_file(>dev, _bin_attr);
+ dev_info(>dev, "sunxi SID driver unloaded\n");
Often this is useless message. In what case this is crucial?
well it's dev_info, so it is only information to the user.
+static int __init sunxi_sid_probe(struct platform_device *pdev)
+{
+ int entropy[SID_SIZE], i;
+ struct resource *res;
+ void __iomem *sid_reg_base;
+ int ret;
+
+ if (!pdev->dev.of_node) {
+ dev_err(>dev, "No devicetree data available\n");
+ ret = -ENXIO;
+ goto exit;
You have only return, use it. It's common practice in the .probe() function.
+ if (IS_ERR(sid_reg_base)) {
+ ret = PTR_ERR(sid_reg_base);
+ goto exit;
Ditto.
+ ret = device_create_bin_file(>dev, _bin_attr);
+ if (ret) {
+ dev_err(>dev, "Unable to create sysfs bin entry\n");
+ goto exit;
Ditto.
+ dev_info(>dev, "sunxi SID ver %s loaded\n", DRV_VERSION);
+ ret = 0;
+ /* fall through */
Ditto.
+
+exit:
+ return ret;
Useless lines.
+module_platform_driver(sunxi_sid_driver);
+
+
Extra line.
Seperate the code from the trailing macro's
--
With Best Regards,
Andy Shevchenko
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2] Initial support for Allwinner's Security ID fuses
On 06/15/13 04:14, Andy Shevchenko wrote:
On Sat, Jun 15, 2013 at 2:16 AM, Oliver Schinagl
oliver+l...@schinagl.nl wrote:
From: Oliver Schinagl oli...@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)
Few comments below.
+++ b/drivers/misc/eeprom/sunxi_sid.c
+#include linux/compiler.h
Are you sure this has to be explicitly mentioned?
Well we use __iomem from it, so I think yes
+#define SID_SIZE (SID_KEYS * 4)
+
+
Extra line.
to seperate defines/includes from the code?
+/* We read the entire key, but only return the requested byte. This is of
+ * course slower then it could be and uses 4 times more reads as needed but
+ * keeps code simpler.
May be better to rewrite this logic and save CPU and I/O resources?
Well we can only read 32 bits at a time and we only want to return 8
bits. The only think I can think of, is read 32 bits and store those
statically to the function and store with an extra int the location.
Then check against the location and if it's the same use the int. Not
sure all that is worth it.
+ */
+static u8 sunxi_sid_read_byte(const void __iomem *sid_reg_base,
+ const unsigned int offset)
+{
+ u32 sid_key = 0;
+
+ if (offset = SID_SIZE)
+ goto exit;
Just return here.
Well as said before, return vs goto; i choose goto ;)
+ sid_key = ioread32be(sid_reg_base + round_down(offset, 4));
+ sid_key = (offset % 4) * 8;
+ sid_key = 0xff;
Redundant 0xff.
Yes, but does clarify the intention, which helps in readability?
+ /* fall through */
+
+exit:
+ return (u8)sid_key;
No need to have explicit casting here.
Also here, it makes the intention clear, that we are only interested in
the last 8 bits. The compiler should optimize this and the above bit
away so it's only for readability.
+ pdev = (struct platform_device *)to_platform_device(kobj_to_dev(kobj));
Ditto.
Yes, I just looked more closy to container_of, and the 'const
typeof(((type *)0)-member) takes care of the typecast, does it not?
+ sid_reg_base = (void __iomem *)platform_get_drvdata(pdev);
Ditto.
Not sure on this one, since technically, it returns only void * (always)
and we had a void '__iomem *' pointer. for clarity and completness I
would keep it?
+static int sunxi_sid_remove(struct platform_device *pdev)
+{
+ device_remove_bin_file(pdev-dev, sid_bin_attr);
+ dev_info(pdev-dev, sunxi SID driver unloaded\n);
Often this is useless message. In what case this is crucial?
well it's dev_info, so it is only information to the user.
+static int __init sunxi_sid_probe(struct platform_device *pdev)
+{
+ int entropy[SID_SIZE], i;
+ struct resource *res;
+ void __iomem *sid_reg_base;
+ int ret;
+
+ if (!pdev-dev.of_node) {
+ dev_err(pdev-dev, No devicetree data available\n);
+ ret = -ENXIO;
+ goto exit;
You have only return, use it. It's common practice in the .probe() function.
+ if (IS_ERR(sid_reg_base)) {
+ ret = PTR_ERR(sid_reg_base);
+ goto exit;
Ditto.
+ ret = device_create_bin_file(pdev-dev, sid_bin_attr);
+ if (ret) {
+ dev_err(pdev-dev, Unable to create sysfs bin entry\n);
+ goto exit;
Ditto.
+ dev_info(pdev-dev, sunxi SID ver %s loaded\n, DRV_VERSION);
+ ret = 0;
+ /* fall through */
Ditto.
+
+exit:
+ return ret;
Useless lines.
+module_platform_driver(sunxi_sid_driver);
+
+
Extra line.
Seperate the code from the trailing macro's
--
With Best Regards,
Andy Shevchenko
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH 2/2] Add sunxi-sid to dts for sun4i and sun5i
From: Oliver Schinagl
This patch shall add support for the sunxi-sid driver to the device table for
sun4i and sun5i.
Signed-off-by: Oliver Schinagl
---
arch/arm/boot/dts/sun4i-a10.dtsi | 5 +
arch/arm/boot/dts/sun5i-a13.dtsi | 5 +
2 files changed, 10 insertions(+)
diff --git a/arch/arm/boot/dts/sun4i-a10.dtsi b/arch/arm/boot/dts/sun4i-a10.dtsi
index e7ef619..bc71d64 100644
--- a/arch/arm/boot/dts/sun4i-a10.dtsi
+++ b/arch/arm/boot/dts/sun4i-a10.dtsi
@@ -213,6 +213,11 @@
reg = <0x01c20c90 0x10>;
};
+ sid: eeprom@01c23800 {
+ compatible = "allwinner,sun4i-sid";
+ reg = <0x01c23800 0x10>;
+ };
+
uart0: serial@01c28000 {
compatible = "snps,dw-apb-uart";
reg = <0x01c28000 0x400>;
diff --git a/arch/arm/boot/dts/sun5i-a13.dtsi b/arch/arm/boot/dts/sun5i-a13.dtsi
index 8ba65c1..c80c81b 100644
--- a/arch/arm/boot/dts/sun5i-a13.dtsi
+++ b/arch/arm/boot/dts/sun5i-a13.dtsi
@@ -196,6 +196,11 @@
reg = <0x01c20c90 0x10>;
};
+ sid: eeprom@01c23800 {
+ compatible = "allwinner,sun4i-sid";
+ reg = <0x01c23800 0x10>;
+ };
+
uart1: serial@01c28400 {
compatible = "snps,dw-apb-uart";
reg = <0x01c28400 0x400>;
--
1.8.1.5
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH 0/2] v3 Driver for Allwinner sunxi Security ID
From: Oliver Schinagl I've tried to incoperate all requests/issues but as always could have possibly missed some. I've talked to a few people, maxime mostly about the return vs goto and he said it was up to me, and have choosen to stick with the goto for the error handling. Changes from v2: * Removed the global pointer, we can change that when the need for external access arises * Fixed header inclusions * Corrected if guards. There where some crude mistakes there * Changed offset to an unsigned int so we don't have to worry about negatives * Cleaned up variable declarations * Changed ret value, ENXIO (No device/io) as that better matches a missing dt * Made the loading informercial print version so it is somewhat usefull Changes from v1: * Renamed the sys-fs exported key to eeprom, since it really a read-only eeprom * Removed mention of sun[67]i since we haven't tested those * Fixed up mistakes in comments * Removed PAGE_SIZE references, since this is a binary only driver * Removed lookup table and calculate offsets better * Use proper endianess * Add the SID to seed the kernel entropy pool * Rewrite probe to use platform_get_resource/devm_ioremap_resource instead The Allwinner A-series of SoC's have efuses exposed via registers to read the factory programmed e-fuses. These should in theory be programmable but this is still to be confirmed. It does appear that these fuses are unique enough to be used as serial numbers, RSA keys, generate MAC addresses from etc. If it turns out to be user programmable, the use obviously increases. Allwinner did use the fuses initially to determine the chip-type. This driver supports all currently known chips based on datasheets and 'dumped' drivers that we have so far, the dts is only implemented for known chips. It has been tested on a Cubieboard 1 This is my very first driver so please try to be gentle Oliver Schinagl (2): Initial support for Allwinner's Security ID fuses Add sunxi-sid to dts for sun4i and sun5i arch/arm/boot/dts/sun4i-a10.dtsi | 5 ++ arch/arm/boot/dts/sun5i-a13.dtsi | 5 ++ drivers/misc/eeprom/Kconfig | 17 drivers/misc/eeprom/Makefile | 1 + drivers/misc/eeprom/sunxi_sid.c | 167 +++ 5 files changed, 195 insertions(+) create mode 100644 drivers/misc/eeprom/sunxi_sid.c -- 1.8.1.5 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH 1/2] Initial support for Allwinner's Security ID fuses
From: Oliver Schinagl
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
---
drivers/misc/eeprom/Kconfig | 17
drivers/misc/eeprom/Makefile| 1 +
drivers/misc/eeprom/sunxi_sid.c | 167
3 files changed, 185 insertions(+)
create mode 100644 drivers/misc/eeprom/sunxi_sid.c
diff --git a/drivers/misc/eeprom/Kconfig b/drivers/misc/eeprom/Kconfig
index 04f2e1f..c7bc6ed 100644
--- a/drivers/misc/eeprom/Kconfig
+++ b/drivers/misc/eeprom/Kconfig
@@ -96,4 +96,21 @@ config EEPROM_DIGSY_MTC_CFG
If unsure, say N.
+config EEPROM_SUNXI_SID
+ tristate "Allwinner sunxi security ID support"
+ depends on ARCH_SUNXI && SYSFS
+ help
+ This is a driver for the 'security ID' available on various Allwinner
+ devices. Currently supported are:
+ sun4i (A10)
+ sun5i (A13)
+
+ Due to the potential risks involved with changing e-fuses,
+ this driver is read-only
+
+ For more information visit http://linux-sunxi.org/SID
+
+ This driver can also be built as a module. If so, the module
+ will be called sunxi_sid.
+
endmenu
diff --git a/drivers/misc/eeprom/Makefile b/drivers/misc/eeprom/Makefile
index fc1e81d..9507aec 100644
--- a/drivers/misc/eeprom/Makefile
+++ b/drivers/misc/eeprom/Makefile
@@ -4,4 +4,5 @@ obj-$(CONFIG_EEPROM_LEGACY) += eeprom.o
obj-$(CONFIG_EEPROM_MAX6875) += max6875.o
obj-$(CONFIG_EEPROM_93CX6) += eeprom_93cx6.o
obj-$(CONFIG_EEPROM_93XX46)+= eeprom_93xx46.o
+obj-$(CONFIG_EEPROM_SUNXI_SID) += sunxi_sid.o
obj-$(CONFIG_EEPROM_DIGSY_MTC_CFG) += digsy_mtc_eeprom.o
diff --git a/drivers/misc/eeprom/sunxi_sid.c b/drivers/misc/eeprom/sunxi_sid.c
new file mode 100644
index 000..f014e1b
--- /dev/null
+++ b/drivers/misc/eeprom/sunxi_sid.c
@@ -0,0 +1,167 @@
+/*
+ * Copyright (c) 2013 Oliver Schinagl
+ * http://www.linux-sunxi.org
+ *
+ * Oliver Schinagl
+ *
+ * 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
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#define DRV_NAME "sunxi-sid"
+#define DRV_VERSION "1.0"
+
+/* There are 4 32-bit keys */
+#define SID_KEYS 4
+/* and 4 byte sized keys per 32-bit key */
+#define SID_SIZE (SID_KEYS * 4)
+
+
+/* We read the entire key, but only return the requested byte. This is of
+ * course slower then it could be and uses 4 times more reads as needed but
+ * keeps code simpler.
+ */
+static u8 sunxi_sid_read_byte(const void __iomem *sid_reg_base,
+ const unsigned int offset)
+{
+ u32 sid_key = 0;
+
+ if (offset >= SID_SIZE)
+ goto exit;
+
+ sid_key = ioread32be(sid_reg_base + round_down(offset, 4));
+ sid_key >>= (offset % 4) * 8;
+ sid_key &= 0xff;
+ /* fall through */
+
+exit:
+ return (u8)sid_key;
+}
+
+static ssize_t sid_read(struct file *fd, struct kobject *kobj,
+ struct bin_attribute *attr, char *buf,
+ loff_t pos, size_t size)
+{
+ int i;
+ struct platform_device *pdev;
+ void __iomem *sid_reg_base;
+
+ pdev = (struct platform_device *)to_platform_device(kobj_to_dev(kobj));
+ sid_reg_base = (void __iomem *)platform_get_drvdata(pdev);
+
+ for (i = 0; i < size; i++) {
+ if ((pos + i) >= SID_SIZE || (pos < 0))
+ break;
+ buf[i] = sunxi_sid_read_byte(sid_reg_base, pos + i);
+
[PATCH 0/2] v3 Driver for Allwinner sunxi Security ID
From: Oliver Schinagl oli...@schinagl.nl I've tried to incoperate all requests/issues but as always could have possibly missed some. I've talked to a few people, maxime mostly about the return vs goto and he said it was up to me, and have choosen to stick with the goto for the error handling. Changes from v2: * Removed the global pointer, we can change that when the need for external access arises * Fixed header inclusions * Corrected if guards. There where some crude mistakes there * Changed offset to an unsigned int so we don't have to worry about negatives * Cleaned up variable declarations * Changed ret value, ENXIO (No device/io) as that better matches a missing dt * Made the loading informercial print version so it is somewhat usefull Changes from v1: * Renamed the sys-fs exported key to eeprom, since it really a read-only eeprom * Removed mention of sun[67]i since we haven't tested those * Fixed up mistakes in comments * Removed PAGE_SIZE references, since this is a binary only driver * Removed lookup table and calculate offsets better * Use proper endianess * Add the SID to seed the kernel entropy pool * Rewrite probe to use platform_get_resource/devm_ioremap_resource instead The Allwinner A-series of SoC's have efuses exposed via registers to read the factory programmed e-fuses. These should in theory be programmable but this is still to be confirmed. It does appear that these fuses are unique enough to be used as serial numbers, RSA keys, generate MAC addresses from etc. If it turns out to be user programmable, the use obviously increases. Allwinner did use the fuses initially to determine the chip-type. This driver supports all currently known chips based on datasheets and 'dumped' drivers that we have so far, the dts is only implemented for known chips. It has been tested on a Cubieboard 1 This is my very first driver so please try to be gentle Oliver Schinagl (2): Initial support for Allwinner's Security ID fuses Add sunxi-sid to dts for sun4i and sun5i arch/arm/boot/dts/sun4i-a10.dtsi | 5 ++ arch/arm/boot/dts/sun5i-a13.dtsi | 5 ++ drivers/misc/eeprom/Kconfig | 17 drivers/misc/eeprom/Makefile | 1 + drivers/misc/eeprom/sunxi_sid.c | 167 +++ 5 files changed, 195 insertions(+) create mode 100644 drivers/misc/eeprom/sunxi_sid.c -- 1.8.1.5 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH 1/2] Initial support for Allwinner's Security ID fuses
From: Oliver Schinagl oli...@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 oli...@schinagl.nl
---
drivers/misc/eeprom/Kconfig | 17
drivers/misc/eeprom/Makefile| 1 +
drivers/misc/eeprom/sunxi_sid.c | 167
3 files changed, 185 insertions(+)
create mode 100644 drivers/misc/eeprom/sunxi_sid.c
diff --git a/drivers/misc/eeprom/Kconfig b/drivers/misc/eeprom/Kconfig
index 04f2e1f..c7bc6ed 100644
--- a/drivers/misc/eeprom/Kconfig
+++ b/drivers/misc/eeprom/Kconfig
@@ -96,4 +96,21 @@ config EEPROM_DIGSY_MTC_CFG
If unsure, say N.
+config EEPROM_SUNXI_SID
+ tristate Allwinner sunxi security ID support
+ depends on ARCH_SUNXI SYSFS
+ help
+ This is a driver for the 'security ID' available on various Allwinner
+ devices. Currently supported are:
+ sun4i (A10)
+ sun5i (A13)
+
+ Due to the potential risks involved with changing e-fuses,
+ this driver is read-only
+
+ For more information visit http://linux-sunxi.org/SID
+
+ This driver can also be built as a module. If so, the module
+ will be called sunxi_sid.
+
endmenu
diff --git a/drivers/misc/eeprom/Makefile b/drivers/misc/eeprom/Makefile
index fc1e81d..9507aec 100644
--- a/drivers/misc/eeprom/Makefile
+++ b/drivers/misc/eeprom/Makefile
@@ -4,4 +4,5 @@ obj-$(CONFIG_EEPROM_LEGACY) += eeprom.o
obj-$(CONFIG_EEPROM_MAX6875) += max6875.o
obj-$(CONFIG_EEPROM_93CX6) += eeprom_93cx6.o
obj-$(CONFIG_EEPROM_93XX46)+= eeprom_93xx46.o
+obj-$(CONFIG_EEPROM_SUNXI_SID) += sunxi_sid.o
obj-$(CONFIG_EEPROM_DIGSY_MTC_CFG) += digsy_mtc_eeprom.o
diff --git a/drivers/misc/eeprom/sunxi_sid.c b/drivers/misc/eeprom/sunxi_sid.c
new file mode 100644
index 000..f014e1b
--- /dev/null
+++ b/drivers/misc/eeprom/sunxi_sid.c
@@ -0,0 +1,167 @@
+/*
+ * Copyright (c) 2013 Oliver Schinagl
+ * http://www.linux-sunxi.org
+ *
+ * Oliver Schinagl oli...@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/errno.h
+#include linux/export.h
+#include linux/fs.h
+#include linux/init.h
+#include linux/io.h
+#include linux/kernel.h
+#include linux/kobject.h
+#include linux/module.h
+#include linux/of_address.h
+#include linux/platform_device.h
+#include linux/random.h
+#include linux/stat.h
+#include linux/sysfs.h
+#include linux/types.h
+
+#define DRV_NAME sunxi-sid
+#define DRV_VERSION 1.0
+
+/* There are 4 32-bit keys */
+#define SID_KEYS 4
+/* and 4 byte sized keys per 32-bit key */
+#define SID_SIZE (SID_KEYS * 4)
+
+
+/* We read the entire key, but only return the requested byte. This is of
+ * course slower then it could be and uses 4 times more reads as needed but
+ * keeps code simpler.
+ */
+static u8 sunxi_sid_read_byte(const void __iomem *sid_reg_base,
+ const unsigned int offset)
+{
+ u32 sid_key = 0;
+
+ if (offset = SID_SIZE)
+ goto exit;
+
+ sid_key = ioread32be(sid_reg_base + round_down(offset, 4));
+ sid_key = (offset % 4) * 8;
+ sid_key = 0xff;
+ /* fall through */
+
+exit:
+ return (u8)sid_key;
+}
+
+static ssize_t sid_read(struct file *fd, struct kobject *kobj,
+ struct bin_attribute *attr, char *buf,
+ loff_t pos, size_t size)
+{
+ int i;
+ struct platform_device *pdev;
+ void __iomem *sid_reg_base;
+
+ pdev = (struct platform_device *)to_platform_device(kobj_to_dev(kobj));
+ sid_reg_base = (void __iomem
[PATCH 2/2] Add sunxi-sid to dts for sun4i and sun5i
From: Oliver Schinagl oli...@schinagl.nl
This patch shall add support for the sunxi-sid driver to the device table for
sun4i and sun5i.
Signed-off-by: Oliver Schinagl oli...@schinagl.nl
---
arch/arm/boot/dts/sun4i-a10.dtsi | 5 +
arch/arm/boot/dts/sun5i-a13.dtsi | 5 +
2 files changed, 10 insertions(+)
diff --git a/arch/arm/boot/dts/sun4i-a10.dtsi b/arch/arm/boot/dts/sun4i-a10.dtsi
index e7ef619..bc71d64 100644
--- a/arch/arm/boot/dts/sun4i-a10.dtsi
+++ b/arch/arm/boot/dts/sun4i-a10.dtsi
@@ -213,6 +213,11 @@
reg = 0x01c20c90 0x10;
};
+ sid: eeprom@01c23800 {
+ compatible = allwinner,sun4i-sid;
+ reg = 0x01c23800 0x10;
+ };
+
uart0: serial@01c28000 {
compatible = snps,dw-apb-uart;
reg = 0x01c28000 0x400;
diff --git a/arch/arm/boot/dts/sun5i-a13.dtsi b/arch/arm/boot/dts/sun5i-a13.dtsi
index 8ba65c1..c80c81b 100644
--- a/arch/arm/boot/dts/sun5i-a13.dtsi
+++ b/arch/arm/boot/dts/sun5i-a13.dtsi
@@ -196,6 +196,11 @@
reg = 0x01c20c90 0x10;
};
+ sid: eeprom@01c23800 {
+ compatible = allwinner,sun4i-sid;
+ reg = 0x01c23800 0x10;
+ };
+
uart1: serial@01c28400 {
compatible = snps,dw-apb-uart;
reg = 0x01c28400 0x400;
--
1.8.1.5
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2] Initial support for Allwinner's Security ID fuses; Unanswered comments
Hello all,
While waiting/working in comments I realized/was reminded that there
where a few things I hadn't addressed. I'll try to go over all the
missing bits to be sure to answer all questions before submitting the
hopefully final version. This because I wouldn't want anybody to think
I'm ungrateful, because I am very much so.
> On 05/17/13 23:18, Maxime Ripard wrote:
+
+struct sid_priv {
+ void __iomem *sid_base;
+};
+
+struct sid_priv *p;
What's the point of having a structure here? And why putting a global
value, !static, with a generic name, while you could have an
instance-specific one?
and related (with the struct killed and the mem* renamed)
> On 02-06-13 17:09, Russell King - ARM Linux wrote:
So what happens if you have two of these devices? Maybe you want to check
whether p_sid_reg_base is already set?
I had to think a while about it, as my first answer was 'i don't know'.
But yeah you can't without some smart trick that I don't yet know.
> On 06-06-13 21:16, Andy Shevchenko wrote:
>> +static void __iomem *p_sid_reg_base;
> So, why it's global?
>
The idea behind the global pointer is, to have sunxi_sid_read_byte() be
usable without anything other then a memory offset/address of what byte
you want to read. The idea behind that is that you can use it from
within the kernel directly. I'll admit it's a future-proofing thing,
it's not exported now as there is no user for it yet. I talked to Maxime
about it and we (he) decided it's best to remove it for now, as we can
always add it back later when we find a user for it.
Having two SID's however is almost impossible too, since you get 1 per
SoC, and only 1 SoC per kernel? I added a guard in .probe() which only
loads the driver if the pointer is NULL.
> On 02-06-13 17:09, Russell King - ARM Linux wrote:
+ if (likely((SID_SIZE))) {
+ sid_key = ioread32be(p_sid_reg_base + round_down(offset, 4));
+ sid_key >>= (offset % 4) * 8;
+ ret = sid_key & 0xff;
+ }
What happens if you unbind the device in sysfs and then try and use
this function?
It goes kaboom. Again, i had no idea naturally. I think what you meant
was, if you unbind and p_sid_reg_base points to 'somewhere' and we just
randomly read bytes. I added p_sid_reg_base to the guard above to at
least check if it's not NULL.
> On 02-06-13 17:09, Russell King - ARM Linux wrote:
+static int sunxi_sid_remove(struct platform_device *pdev)
+{
+ device_remove_bin_file(>dev, _bin_attr);
+ dev_info(>dev, "Sunxi SID driver unloaded successfully.\n");
Maybe you want to set p_sid_reg_base to NULL here?
Yeah, that made even more sense to the above thing and added it. When we
unbind or unload the driver, we point to NULL and a) can check for it
above and don't have nasty things dangling around. I think it was save
to assume, that when the driver is unloaded/unbound, the devm_set_data()
private data gets cleaned and the pointer set to NULL? I think that's
what Maxime/Emilio said, by using the devm stuff, those should get
cleaned up properly.
> On 02-06-13 17:09, Russell King - ARM Linux wrote:
+ platform_set_drvdata(pdev, sid_reg_base);
+ p_sid_reg_base = sid_reg_base;
So what happens if you have two of these devices? Maybe you want to check
whether p_sid_reg_base is already set?
Even that hint didn't trigger me as to why this could cause problems.
Userspace guy here, every driver has its own memory space right? So I've
added a guard that we only load this driver once and only if
p_sid_reg_base is NULL.
> On 02-06-13 17:09, Russell King - ARM Linux wrote:
+ dev_info(dev, "Sunxi SID driver loaded successfully.\n");
Do we really need to report that the driver "loaded successfully" ?
Do we need lots of lines in the kernel log telling us simply that
random driver X was built into the kernel or the module was loaded?
Don't we do that already though? I made the string more informative by
adding version information, but yeah I see tons of 'all is ok' rolling
by, isn't that a good thing? Isn't that only printed if we actually have
a SID onboard, so the user knows 'sid is available'. I know I often
scroll through dmesg to see if things got brought up nicely.
On 06-06-13 21:16, Andy Shevchenko wrote:
+ if (likely((SID_SIZE))) {
Extra braces.
Use antipattern here.
While this is a really small function, I guess if that's the normal
course of things; why not.
On 06-06-13 21:16, Andy Shevchenko wrote:
+ ret = sid_key & 0xff;
No need to do & 0xff, since return type is byte.
With ret removed, I assume having return (u8)sid_key; is okay? Does that
typecast only return the last byte? Or do we still want & 0xff in the
return added for clarity?
> On 06-06-13 21:16, Andy Shevchenko wrote
+static int __init sunxi_sid_probe(struct platform_device *pdev)
+{
+ int entropy[SID_SIZE], i, ret;
Usually ret variable is located at the end of
Re: [PATCH 1/2] Initial support for Allwinner's Security ID fuses; Unanswered comments
Hello all,
While waiting/working in comments I realized/was reminded that there
where a few things I hadn't addressed. I'll try to go over all the
missing bits to be sure to answer all questions before submitting the
hopefully final version. This because I wouldn't want anybody to think
I'm ungrateful, because I am very much so.
On 05/17/13 23:18, Maxime Ripard wrote:
+
+struct sid_priv {
+ void __iomem *sid_base;
+};
+
+struct sid_priv *p;
What's the point of having a structure here? And why putting a global
value, !static, with a generic name, while you could have an
instance-specific one?
and related (with the struct killed and the mem* renamed)
On 02-06-13 17:09, Russell King - ARM Linux wrote:
So what happens if you have two of these devices? Maybe you want to check
whether p_sid_reg_base is already set?
I had to think a while about it, as my first answer was 'i don't know'.
But yeah you can't without some smart trick that I don't yet know.
On 06-06-13 21:16, Andy Shevchenko wrote:
+static void __iomem *p_sid_reg_base;
So, why it's global?
The idea behind the global pointer is, to have sunxi_sid_read_byte() be
usable without anything other then a memory offset/address of what byte
you want to read. The idea behind that is that you can use it from
within the kernel directly. I'll admit it's a future-proofing thing,
it's not exported now as there is no user for it yet. I talked to Maxime
about it and we (he) decided it's best to remove it for now, as we can
always add it back later when we find a user for it.
Having two SID's however is almost impossible too, since you get 1 per
SoC, and only 1 SoC per kernel? I added a guard in .probe() which only
loads the driver if the pointer is NULL.
On 02-06-13 17:09, Russell King - ARM Linux wrote:
+ if (likely((SID_SIZE))) {
+ sid_key = ioread32be(p_sid_reg_base + round_down(offset, 4));
+ sid_key = (offset % 4) * 8;
+ ret = sid_key 0xff;
+ }
What happens if you unbind the device in sysfs and then try and use
this function?
It goes kaboom. Again, i had no idea naturally. I think what you meant
was, if you unbind and p_sid_reg_base points to 'somewhere' and we just
randomly read bytes. I added p_sid_reg_base to the guard above to at
least check if it's not NULL.
On 02-06-13 17:09, Russell King - ARM Linux wrote:
+static int sunxi_sid_remove(struct platform_device *pdev)
+{
+ device_remove_bin_file(pdev-dev, sid_bin_attr);
+ dev_info(pdev-dev, Sunxi SID driver unloaded successfully.\n);
Maybe you want to set p_sid_reg_base to NULL here?
Yeah, that made even more sense to the above thing and added it. When we
unbind or unload the driver, we point to NULL and a) can check for it
above and don't have nasty things dangling around. I think it was save
to assume, that when the driver is unloaded/unbound, the devm_set_data()
private data gets cleaned and the pointer set to NULL? I think that's
what Maxime/Emilio said, by using the devm stuff, those should get
cleaned up properly.
On 02-06-13 17:09, Russell King - ARM Linux wrote:
+ platform_set_drvdata(pdev, sid_reg_base);
+ p_sid_reg_base = sid_reg_base;
So what happens if you have two of these devices? Maybe you want to check
whether p_sid_reg_base is already set?
Even that hint didn't trigger me as to why this could cause problems.
Userspace guy here, every driver has its own memory space right? So I've
added a guard that we only load this driver once and only if
p_sid_reg_base is NULL.
On 02-06-13 17:09, Russell King - ARM Linux wrote:
+ dev_info(dev, Sunxi SID driver loaded successfully.\n);
Do we really need to report that the driver loaded successfully ?
Do we need lots of lines in the kernel log telling us simply that
random driver X was built into the kernel or the module was loaded?
Don't we do that already though? I made the string more informative by
adding version information, but yeah I see tons of 'all is ok' rolling
by, isn't that a good thing? Isn't that only printed if we actually have
a SID onboard, so the user knows 'sid is available'. I know I often
scroll through dmesg to see if things got brought up nicely.
On 06-06-13 21:16, Andy Shevchenko wrote:
+ if (likely((SID_SIZE))) {
Extra braces.
Use antipattern here.
While this is a really small function, I guess if that's the normal
course of things; why not.
On 06-06-13 21:16, Andy Shevchenko wrote:
+ ret = sid_key 0xff;
No need to do 0xff, since return type is byte.
With ret removed, I assume having return (u8)sid_key; is okay? Does that
typecast only return the last byte? Or do we still want 0xff in the
return added for clarity?
On 06-06-13 21:16, Andy Shevchenko wrote
+static int __init sunxi_sid_probe(struct platform_device *pdev)
+{
+ int entropy[SID_SIZE], i, ret;
Usually ret variable is located at the end of definition
Re: [PATCH 1/2] Initial support for Allwinner's Security ID fuses
On 06/06/13 21:16, Andy Shevchenko wrote:
Thank you andy for your review, I do have a few questions/comments if
you don't mind.
On Sun, Jun 2, 2013 at 5:58 PM, Oliver Schinagl wrote:
From: Oliver Schinagl
+ if (likely((SID_SIZE))) {
Extra braces.
Use antipattern here.
While I accidentally dropped the pointer here, sorry for the confusion,
what is antipattern? I have asked around and nobody really knew.
Wikipedia mentions it as a software development thing, but you make it
sound like it is some sort of tool?
+ if (unlikely(!pdev->dev.of_node)) {
+ dev_err(dev, "No devicetree data available\n");
+ ret = -EFAULT;
+ goto exit;
Plain return here and in entire function where it applies.
Why? I know there's conflicting preferences here. The general consensus
seems, don't return mid function if you don't absolutely have to. Yet,
you make it sound, just return wherever. I take it that really is just a
preference? I think i see both constructs throughout the kernel. So one
review prefers the one method, the next the other?
+
+ ret = device_create_bin_file(dev, _bin_attr);
+ if (unlikely(ret)) {
Any benifit of (un)likely in probe()?
Does it hurt however in any way though? It's just a compiler
optimization isn't it.
--
With Best Regards,
Andy Shevchenko
Thank you for your time, it is much appreciated :)
Oliver
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2] Initial support for Allwinner's Security ID fuses
On 06/06/13 21:16, Andy Shevchenko wrote:
Thank you andy for your review, I do have a few questions/comments if
you don't mind.
On Sun, Jun 2, 2013 at 5:58 PM, Oliver Schinagl oliver+l...@schinagl.nl wrote:
From: Oliver Schinagl oli...@schinagl.nl
snip
+ if (likely((SID_SIZE))) {
Extra braces.
Use antipattern here.
While I accidentally dropped the pointer here, sorry for the confusion,
what is antipattern? I have asked around and nobody really knew.
Wikipedia mentions it as a software development thing, but you make it
sound like it is some sort of tool?
snip
+ if (unlikely(!pdev-dev.of_node)) {
+ dev_err(dev, No devicetree data available\n);
+ ret = -EFAULT;
+ goto exit;
Plain return here and in entire function where it applies.
Why? I know there's conflicting preferences here. The general consensus
seems, don't return mid function if you don't absolutely have to. Yet,
you make it sound, just return wherever. I take it that really is just a
preference? I think i see both constructs throughout the kernel. So one
review prefers the one method, the next the other?
snip
+
+ ret = device_create_bin_file(dev, sid_bin_attr);
+ if (unlikely(ret)) {
Any benifit of (un)likely in probe()?
Does it hurt however in any way though? It's just a compiler
optimization isn't it.
snip
--
With Best Regards,
Andy Shevchenko
Thank you for your time, it is much appreciated :)
Oliver
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2] Initial support for Allwinner's Security ID fuses
On 06/02/13 17:09, Russell King - ARM Linux wrote:
On Sun, Jun 02, 2013 at 04:58:49PM +0200, Oliver Schinagl wrote:
+#include
We have an include file called linux/io.h. Please use linux/*.h files which
include asm/*.h files in preference to directly using asm/*.h.
In fact, no driver should include an asm/*.h header.
And I didn't have that there, but it kept refusing to find ioread32be.
Of course, now I change it back to linux/io.h (which I had, I swear) and
all works swell.
Concider it changed.
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#define DRV_NAME "sunxi-sid"
+#define DRV_VERSION "1.0"
+
+/* There are 4 32-bit keys */
+#define SID_KEYS 4
+/* and 4 byte sized keys per 32-bit key */
+#define SID_SIZE (SID_KEYS * 4)
+
+static void __iomem *p_sid_reg_base;
+
+/* We read the entire key, but only return the requested byte. This is of
+ * course slower then it could be and uses 4 times more reads as needed but
+ * keeps code a simpler.
+ */
+u8 sunxi_sid_read_byte(const int offset)
+{
+ u32 sid_key;
+ u8 ret;
+
+ ret = 0;
+
+ if (likely((SID_SIZE))) {
+ sid_key = ioread32be(p_sid_reg_base + round_down(offset, 4));
+ sid_key >>= (offset % 4) * 8;
+ ret = sid_key & 0xff;
+ }
What happens if you unbind the device in sysfs and then try and use
this function?
+static int sunxi_sid_remove(struct platform_device *pdev)
+{
+ device_remove_bin_file(>dev, _bin_attr);
+ dev_info(>dev, "Sunxi SID driver unloaded successfully.\n");
Maybe you want to set p_sid_reg_base to NULL here?
+
+ return 0;
+}
+
+static int __init sunxi_sid_probe(struct platform_device *pdev)
+{
+ int entropy[SID_SIZE], i, ret;
+ struct device *dev;
+ struct resource *res;
+ void __iomem *sid_reg_base;
+
+ dev = >dev;
+ if (unlikely(!pdev->dev.of_node)) {
+ dev_err(dev, "No devicetree data available\n");
+ ret = -EFAULT;
+ goto exit;
+ }
+
+ res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ sid_reg_base = devm_ioremap_resource(>dev, res);
+ if (IS_ERR(sid_reg_base)) {
+ dev_err(dev, "Unable to obtain resource\n");
+ ret = PTR_ERR(sid_reg_base);
+ goto exit;
+ }
+ platform_set_drvdata(pdev, sid_reg_base);
+ p_sid_reg_base = sid_reg_base;
So what happens if you have two of these devices? Maybe you want to check
whether p_sid_reg_base is already set?
+
+ ret = device_create_bin_file(dev, _bin_attr);
+ if (unlikely(ret)) {
+ dev_err(dev, "Unable to create sysfs bin entry\n");
+ goto exit;
+ }
+
+ for (i = 0; i < SID_SIZE; i++)
+ entropy[i] = sunxi_sid_read_byte(i);
+ add_device_randomness(entropy, SID_SIZE);
+
+ dev_info(dev, "Sunxi SID driver loaded successfully.\n");
Do we really need to report that the driver "loaded successfully" ?
Do we need lots of lines in the kernel log telling us simply that
random driver X was built into the kernel or the module was loaded?
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH 0/2] v2 Driver for Allwinner sunxi Security ID
From: Oliver Schinagl Changes from v1: * Renamed the sys-fs exported key to eeprom, since it really a read-only eeprom * Removed mention of sun[67]i since we haven't tested those * Fixed up mistakes in comments * Removed PAGE_SIZE references, since this is a binary only driver * Removed lookup table and calculate offsets better * Use proper endianess * Add the SID to seed the kernel entropy pool * Rewrite probe to use platform_get_resource/devm_ioremap_resource instead The Allwinner A-series of SoC's have efuses exposed via registers to read the factory programmed e-fuses. These should in theory be programmable but this is still to be confirmed. It does appear that these fuses are unique enough to be used as serial numbers, RSA keys, generate MAC addresses from etc. If it turns out to be user programmable, the use obviously increases. Allwinner did use the fuses initially to determine the chip-type. This driver supports all currently known chips based on datasheets and 'dumped' drivers that we have so far, the dts is only implemented for known chips. It has been tested on a Cubieboard 1 This is my very first driver so please try to be gentle Oliver Schinagl (2): Initial support for Allwinner's Security ID fuses Add sunxi-sid to dts for sun4i and sun5i arch/arm/boot/dts/sun4i-a10.dtsi | 5 ++ arch/arm/boot/dts/sun5i-a13.dtsi | 5 ++ drivers/misc/eeprom/Kconfig | 17 drivers/misc/eeprom/Makefile | 1 + drivers/misc/eeprom/sunxi_sid.c | 172 +++ 5 files changed, 200 insertions(+) create mode 100644 drivers/misc/eeprom/sunxi_sid.c -- 1.8.1.5 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH 1/2] Initial support for Allwinner's Security ID fuses
From: Oliver Schinagl
Allwinner has electric fuses (efuse) on their line of chips. This driver
reads those fuses, seeds the kernel entropy and exports them as a sysfs node.
These fuses are most likly to be programmed at the factory, encoding
things like Chip ID, some sort of serial number etc and appear to be
reasonable unique.
While in theory, these should be writeable by the user, it will probably
be inconvinient to do so. Allwinner recommends that a certain input pin,
labeled 'efuse_vddq', be connected to GND. From the name however it is
highly likly that this name is the programming voltage, required to
write these fuses.
Even so, they can still be used to generate a board-unique mac from, board
unique RSA key and seed the kernel RNG.
Currently supported are the following known chips:
Allwinner sun4i (A10)
Allwinner sun5i (A13)
Signed-off-by: Oliver Schinagl
---
drivers/misc/eeprom/Kconfig | 17
drivers/misc/eeprom/Makefile| 1 +
drivers/misc/eeprom/sunxi_sid.c | 172
3 files changed, 190 insertions(+)
create mode 100644 drivers/misc/eeprom/sunxi_sid.c
diff --git a/drivers/misc/eeprom/Kconfig b/drivers/misc/eeprom/Kconfig
index 04f2e1f..c7bc6ed 100644
--- a/drivers/misc/eeprom/Kconfig
+++ b/drivers/misc/eeprom/Kconfig
@@ -96,4 +96,21 @@ config EEPROM_DIGSY_MTC_CFG
If unsure, say N.
+config EEPROM_SUNXI_SID
+ tristate "Allwinner sunxi security ID support"
+ depends on ARCH_SUNXI && SYSFS
+ help
+ This is a driver for the 'security ID' available on various Allwinner
+ devices. Currently supported are:
+ sun4i (A10)
+ sun5i (A13)
+
+ Due to the potential risks involved with changing e-fuses,
+ this driver is read-only
+
+ For more information visit http://linux-sunxi.org/SID
+
+ This driver can also be built as a module. If so, the module
+ will be called sunxi_sid.
+
endmenu
diff --git a/drivers/misc/eeprom/Makefile b/drivers/misc/eeprom/Makefile
index fc1e81d..9507aec 100644
--- a/drivers/misc/eeprom/Makefile
+++ b/drivers/misc/eeprom/Makefile
@@ -4,4 +4,5 @@ obj-$(CONFIG_EEPROM_LEGACY) += eeprom.o
obj-$(CONFIG_EEPROM_MAX6875) += max6875.o
obj-$(CONFIG_EEPROM_93CX6) += eeprom_93cx6.o
obj-$(CONFIG_EEPROM_93XX46)+= eeprom_93xx46.o
+obj-$(CONFIG_EEPROM_SUNXI_SID) += sunxi_sid.o
obj-$(CONFIG_EEPROM_DIGSY_MTC_CFG) += digsy_mtc_eeprom.o
diff --git a/drivers/misc/eeprom/sunxi_sid.c b/drivers/misc/eeprom/sunxi_sid.c
new file mode 100644
index 000..4ec86d5
--- /dev/null
+++ b/drivers/misc/eeprom/sunxi_sid.c
@@ -0,0 +1,172 @@
+/*
+ * Copyright (c) 2013 Oliver Schinagl
+ * http://www.linux-sunxi.org
+ *
+ * Oliver Schinagl
+ *
+ * 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
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#define DRV_NAME "sunxi-sid"
+#define DRV_VERSION "1.0"
+
+/* There are 4 32-bit keys */
+#define SID_KEYS 4
+/* and 4 byte sized keys per 32-bit key */
+#define SID_SIZE (SID_KEYS * 4)
+
+static void __iomem *p_sid_reg_base;
+
+/* We read the entire key, but only return the requested byte. This is of
+ * course slower then it could be and uses 4 times more reads as needed but
+ * keeps code a simpler.
+ */
+u8 sunxi_sid_read_byte(const int offset)
+{
+ u32 sid_key;
+ u8 ret;
+
+ ret = 0;
+
+ if (likely((SID_SIZE))) {
+ sid_key = ioread32be(p_sid_reg_base + round_down(offset, 4));
+ sid_key >>= (offset % 4) * 8;
+ ret = sid_key & 0xff;
+ }
+
+ return ret;
+}
+
+static ssize_t sid_read(struct file *fd, struct kobject *kobj,
+ struct bin_attribute *attr, char *buf,
+ loff_t pos, size_t size)
+{
+ ssize_t ret;
+ int i;
+
+ ret = -EPERM;
+
+ if ((likely(size > 0)) && ((size + pos) <= SID_SIZE)) {
+ for (i = 0; i < size; i++)
+ buf[i] = sunxi_sid_read_byte(pos + i);
+ ret = (ssize_t)size;
+ } else {
+ ret = 0;
+ }
+
+ return ret;
+}
+
+static const struct of_device_id sunxi_sid_of_match[]
[PATCH 2/2] Add sunxi-sid to dts for sun4i and sun5i
From: Oliver Schinagl
This should add support for the sunxi-sid driver to the device table for sun4i
and sun5i
Signed-off-by: Oliver Schinagl
---
arch/arm/boot/dts/sun4i-a10.dtsi | 5 +
arch/arm/boot/dts/sun5i-a13.dtsi | 5 +
2 files changed, 10 insertions(+)
diff --git a/arch/arm/boot/dts/sun4i-a10.dtsi b/arch/arm/boot/dts/sun4i-a10.dtsi
index e7ef619..bc71d64 100644
--- a/arch/arm/boot/dts/sun4i-a10.dtsi
+++ b/arch/arm/boot/dts/sun4i-a10.dtsi
@@ -213,6 +213,11 @@
reg = <0x01c20c90 0x10>;
};
+ sid: eeprom@01c23800 {
+ compatible = "allwinner,sun4i-sid";
+ reg = <0x01c23800 0x10>;
+ };
+
uart0: serial@01c28000 {
compatible = "snps,dw-apb-uart";
reg = <0x01c28000 0x400>;
diff --git a/arch/arm/boot/dts/sun5i-a13.dtsi b/arch/arm/boot/dts/sun5i-a13.dtsi
index 8ba65c1..c80c81b 100644
--- a/arch/arm/boot/dts/sun5i-a13.dtsi
+++ b/arch/arm/boot/dts/sun5i-a13.dtsi
@@ -196,6 +196,11 @@
reg = <0x01c20c90 0x10>;
};
+ sid: eeprom@01c23800 {
+ compatible = "allwinner,sun4i-sid";
+ reg = <0x01c23800 0x10>;
+ };
+
uart1: serial@01c28400 {
compatible = "snps,dw-apb-uart";
reg = <0x01c28400 0x400>;
--
1.8.1.5
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH 2/2] Add sunxi-sid to dts for sun4i and sun5i
From: Oliver Schinagl oli...@schinagl.nl
This should add support for the sunxi-sid driver to the device table for sun4i
and sun5i
Signed-off-by: Oliver Schinagl oli...@schinagl.nl
---
arch/arm/boot/dts/sun4i-a10.dtsi | 5 +
arch/arm/boot/dts/sun5i-a13.dtsi | 5 +
2 files changed, 10 insertions(+)
diff --git a/arch/arm/boot/dts/sun4i-a10.dtsi b/arch/arm/boot/dts/sun4i-a10.dtsi
index e7ef619..bc71d64 100644
--- a/arch/arm/boot/dts/sun4i-a10.dtsi
+++ b/arch/arm/boot/dts/sun4i-a10.dtsi
@@ -213,6 +213,11 @@
reg = 0x01c20c90 0x10;
};
+ sid: eeprom@01c23800 {
+ compatible = allwinner,sun4i-sid;
+ reg = 0x01c23800 0x10;
+ };
+
uart0: serial@01c28000 {
compatible = snps,dw-apb-uart;
reg = 0x01c28000 0x400;
diff --git a/arch/arm/boot/dts/sun5i-a13.dtsi b/arch/arm/boot/dts/sun5i-a13.dtsi
index 8ba65c1..c80c81b 100644
--- a/arch/arm/boot/dts/sun5i-a13.dtsi
+++ b/arch/arm/boot/dts/sun5i-a13.dtsi
@@ -196,6 +196,11 @@
reg = 0x01c20c90 0x10;
};
+ sid: eeprom@01c23800 {
+ compatible = allwinner,sun4i-sid;
+ reg = 0x01c23800 0x10;
+ };
+
uart1: serial@01c28400 {
compatible = snps,dw-apb-uart;
reg = 0x01c28400 0x400;
--
1.8.1.5
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH 0/2] v2 Driver for Allwinner sunxi Security ID
From: Oliver Schinagl oli...@schinagl.nl Changes from v1: * Renamed the sys-fs exported key to eeprom, since it really a read-only eeprom * Removed mention of sun[67]i since we haven't tested those * Fixed up mistakes in comments * Removed PAGE_SIZE references, since this is a binary only driver * Removed lookup table and calculate offsets better * Use proper endianess * Add the SID to seed the kernel entropy pool * Rewrite probe to use platform_get_resource/devm_ioremap_resource instead The Allwinner A-series of SoC's have efuses exposed via registers to read the factory programmed e-fuses. These should in theory be programmable but this is still to be confirmed. It does appear that these fuses are unique enough to be used as serial numbers, RSA keys, generate MAC addresses from etc. If it turns out to be user programmable, the use obviously increases. Allwinner did use the fuses initially to determine the chip-type. This driver supports all currently known chips based on datasheets and 'dumped' drivers that we have so far, the dts is only implemented for known chips. It has been tested on a Cubieboard 1 This is my very first driver so please try to be gentle Oliver Schinagl (2): Initial support for Allwinner's Security ID fuses Add sunxi-sid to dts for sun4i and sun5i arch/arm/boot/dts/sun4i-a10.dtsi | 5 ++ arch/arm/boot/dts/sun5i-a13.dtsi | 5 ++ drivers/misc/eeprom/Kconfig | 17 drivers/misc/eeprom/Makefile | 1 + drivers/misc/eeprom/sunxi_sid.c | 172 +++ 5 files changed, 200 insertions(+) create mode 100644 drivers/misc/eeprom/sunxi_sid.c -- 1.8.1.5 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH 1/2] Initial support for Allwinner's Security ID fuses
From: Oliver Schinagl oli...@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. From the name however it is
highly likly that this name is the programming voltage, required to
write these fuses.
Even so, they can still be used to generate a board-unique mac from, board
unique RSA key and seed the kernel RNG.
Currently supported are the following known chips:
Allwinner sun4i (A10)
Allwinner sun5i (A13)
Signed-off-by: Oliver Schinagl oli...@schinagl.nl
---
drivers/misc/eeprom/Kconfig | 17
drivers/misc/eeprom/Makefile| 1 +
drivers/misc/eeprom/sunxi_sid.c | 172
3 files changed, 190 insertions(+)
create mode 100644 drivers/misc/eeprom/sunxi_sid.c
diff --git a/drivers/misc/eeprom/Kconfig b/drivers/misc/eeprom/Kconfig
index 04f2e1f..c7bc6ed 100644
--- a/drivers/misc/eeprom/Kconfig
+++ b/drivers/misc/eeprom/Kconfig
@@ -96,4 +96,21 @@ config EEPROM_DIGSY_MTC_CFG
If unsure, say N.
+config EEPROM_SUNXI_SID
+ tristate Allwinner sunxi security ID support
+ depends on ARCH_SUNXI SYSFS
+ help
+ This is a driver for the 'security ID' available on various Allwinner
+ devices. Currently supported are:
+ sun4i (A10)
+ sun5i (A13)
+
+ Due to the potential risks involved with changing e-fuses,
+ this driver is read-only
+
+ For more information visit http://linux-sunxi.org/SID
+
+ This driver can also be built as a module. If so, the module
+ will be called sunxi_sid.
+
endmenu
diff --git a/drivers/misc/eeprom/Makefile b/drivers/misc/eeprom/Makefile
index fc1e81d..9507aec 100644
--- a/drivers/misc/eeprom/Makefile
+++ b/drivers/misc/eeprom/Makefile
@@ -4,4 +4,5 @@ obj-$(CONFIG_EEPROM_LEGACY) += eeprom.o
obj-$(CONFIG_EEPROM_MAX6875) += max6875.o
obj-$(CONFIG_EEPROM_93CX6) += eeprom_93cx6.o
obj-$(CONFIG_EEPROM_93XX46)+= eeprom_93xx46.o
+obj-$(CONFIG_EEPROM_SUNXI_SID) += sunxi_sid.o
obj-$(CONFIG_EEPROM_DIGSY_MTC_CFG) += digsy_mtc_eeprom.o
diff --git a/drivers/misc/eeprom/sunxi_sid.c b/drivers/misc/eeprom/sunxi_sid.c
new file mode 100644
index 000..4ec86d5
--- /dev/null
+++ b/drivers/misc/eeprom/sunxi_sid.c
@@ -0,0 +1,172 @@
+/*
+ * Copyright (c) 2013 Oliver Schinagl
+ * http://www.linux-sunxi.org
+ *
+ * Oliver Schinagl oli...@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 asm/io.h
+#include linux/compiler.h
+#include linux/device.h
+#include linux/err.h
+#include linux/errno.h
+#include linux/export.h
+#include linux/fs.h
+#include linux/init.h
+#include linux/kernel.h
+#include linux/kobject.h
+#include linux/module.h
+#include linux/of_address.h
+#include linux/platform_device.h
+#include linux/random.h
+#include linux/stat.h
+#include linux/sysfs.h
+#include linux/types.h
+
+#define DRV_NAME sunxi-sid
+#define DRV_VERSION 1.0
+
+/* There are 4 32-bit keys */
+#define SID_KEYS 4
+/* and 4 byte sized keys per 32-bit key */
+#define SID_SIZE (SID_KEYS * 4)
+
+static void __iomem *p_sid_reg_base;
+
+/* We read the entire key, but only return the requested byte. This is of
+ * course slower then it could be and uses 4 times more reads as needed but
+ * keeps code a simpler.
+ */
+u8 sunxi_sid_read_byte(const int offset)
+{
+ u32 sid_key;
+ u8 ret;
+
+ ret = 0;
+
+ if (likely((SID_SIZE))) {
+ sid_key = ioread32be(p_sid_reg_base + round_down(offset, 4));
+ sid_key = (offset % 4) * 8;
+ ret = sid_key 0xff;
+ }
+
+ return ret;
+}
+
+static ssize_t sid_read(struct file *fd, struct kobject *kobj,
+ struct bin_attribute *attr, char *buf,
+ loff_t pos, size_t size)
+{
+ ssize_t ret;
+ int i;
+
+ ret = -EPERM;
+
+ if ((likely(size 0)) ((size + pos) = SID_SIZE)) {
+ for (i = 0; i size; i++)
+ buf[i] = sunxi_sid_read_byte
Re: [PATCH 1/2] Initial support for Allwinner's Security ID fuses
On 06/02/13 17:09, Russell King - ARM Linux wrote:
On Sun, Jun 02, 2013 at 04:58:49PM +0200, Oliver Schinagl wrote:
+#include asm/io.h
We have an include file called linux/io.h. Please use linux/*.h files which
include asm/*.h files in preference to directly using asm/*.h.
In fact, no driver should include an asm/*.h header.
And I didn't have that there, but it kept refusing to find ioread32be.
Of course, now I change it back to linux/io.h (which I had, I swear) and
all works swell.
Concider it changed.
+#include linux/compiler.h
+#include linux/device.h
+#include linux/err.h
+#include linux/errno.h
+#include linux/export.h
+#include linux/fs.h
+#include linux/init.h
+#include linux/kernel.h
+#include linux/kobject.h
+#include linux/module.h
+#include linux/of_address.h
+#include linux/platform_device.h
+#include linux/random.h
+#include linux/stat.h
+#include linux/sysfs.h
+#include linux/types.h
+
+#define DRV_NAME sunxi-sid
+#define DRV_VERSION 1.0
+
+/* There are 4 32-bit keys */
+#define SID_KEYS 4
+/* and 4 byte sized keys per 32-bit key */
+#define SID_SIZE (SID_KEYS * 4)
+
+static void __iomem *p_sid_reg_base;
+
+/* We read the entire key, but only return the requested byte. This is of
+ * course slower then it could be and uses 4 times more reads as needed but
+ * keeps code a simpler.
+ */
+u8 sunxi_sid_read_byte(const int offset)
+{
+ u32 sid_key;
+ u8 ret;
+
+ ret = 0;
+
+ if (likely((SID_SIZE))) {
+ sid_key = ioread32be(p_sid_reg_base + round_down(offset, 4));
+ sid_key = (offset % 4) * 8;
+ ret = sid_key 0xff;
+ }
What happens if you unbind the device in sysfs and then try and use
this function?
+static int sunxi_sid_remove(struct platform_device *pdev)
+{
+ device_remove_bin_file(pdev-dev, sid_bin_attr);
+ dev_info(pdev-dev, Sunxi SID driver unloaded successfully.\n);
Maybe you want to set p_sid_reg_base to NULL here?
+
+ return 0;
+}
+
+static int __init sunxi_sid_probe(struct platform_device *pdev)
+{
+ int entropy[SID_SIZE], i, ret;
+ struct device *dev;
+ struct resource *res;
+ void __iomem *sid_reg_base;
+
+ dev = pdev-dev;
+ if (unlikely(!pdev-dev.of_node)) {
+ dev_err(dev, No devicetree data available\n);
+ ret = -EFAULT;
+ goto exit;
+ }
+
+ res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ sid_reg_base = devm_ioremap_resource(pdev-dev, res);
+ if (IS_ERR(sid_reg_base)) {
+ dev_err(dev, Unable to obtain resource\n);
+ ret = PTR_ERR(sid_reg_base);
+ goto exit;
+ }
+ platform_set_drvdata(pdev, sid_reg_base);
+ p_sid_reg_base = sid_reg_base;
So what happens if you have two of these devices? Maybe you want to check
whether p_sid_reg_base is already set?
+
+ ret = device_create_bin_file(dev, sid_bin_attr);
+ if (unlikely(ret)) {
+ dev_err(dev, Unable to create sysfs bin entry\n);
+ goto exit;
+ }
+
+ for (i = 0; i SID_SIZE; i++)
+ entropy[i] = sunxi_sid_read_byte(i);
+ add_device_randomness(entropy, SID_SIZE);
+
+ dev_info(dev, Sunxi SID driver loaded successfully.\n);
Do we really need to report that the driver loaded successfully ?
Do we need lots of lines in the kernel log telling us simply that
random driver X was built into the kernel or the module was loaded?
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2] Initial support for Allwinner's Security ID fuses
On 05/25/13 14:22, Maxime Ripard wrote:
Hi Oliver,
On Fri, May 24, 2013 at 11:50:38PM +0200, Oliver Schinagl wrote:
On 05/18/13 19:19, Oliver Schinagl wrote:
+
+
+/* We read the entire key, using a look up table. Returned is only the
+ * requested byte. This is of course slower then it could be and uses 4 times
+ * more reads as needed but keeps code a little simpler.
+ */
+u8 sunxi_sid_read_byte(const int key)
+{
+ u32 sid_key;
+ u8 ret;
+
+ ret = 0;
+ if (likely((key <= SUNXI_SID_SIZE))) {
+ sid_key = ioread32(p->sid_base + keys_lut[key >> 2]);
+ switch (key % 4) {
+ case 0:
+ ret = (sid_key >> 24) & 0xff;
+ break;
+ case 1:
+ ret = (sid_key >> 16) & 0xff;
+ break;
+ case 2:
+ ret = (sid_key >> 8) & 0xff;
+ break;
+ case 3:
+ ret = sid_key & 0xff;
+ break;
+ }
+ }
Come on, you can do better. This lookup table is useless.
I didn't want to depend on the fixed layout of memory, but consider it
removed.
But i'm not smart enough :p
We can either use the look up table (which does have benefits as its
potentially more future proof), or do some ((key >> 2) << 2) to
'drop' the LSB's that we want to ignore (unless there's some smarter
way).
Personally, I think the LUT is a little cleaner and more readable,
but I guess if you look at poor efficiency, the lut costs some
memory, the left/right shift cost an additional >> 2 ... what you
prefer.
What about:
val = ioread32be(base + (key / 4));
val >>= (key % 4) * 8;
return val & 0xff;
No lookup table, no weird swich statement, and you get the endianness
conversion only when you need it.
Ok I think I like the Endianess, ioread32be does the right thing then?
I'll read up on that.
As for key / 4; how will that work without the lut?
Lets take byte 14 (out of the available 16). Byte 14 (0x0e) is located
in SID_KEY3, so base + 0x0c. We need to write a whole 32bit word to keep
with alignment, the registers go wakko if you do unaligned reads. So we
need to read the entire 32 bits, then find our byte.
key / 4 for 14 yields 0x03. So we have base + 0x03, which is not what we
want. We want base + 0x0c, which is either ((key >> 2) << 2)) Or, ((key
/ 4) * 4)) which to me, are both equally confusing. So we either use the
look up table, which is a little cleaner and is a bit more future proof
if either a) there's more 'eeprom like' storage which can be combined
herein or b) 'a' next version has non-continuing regions. Granted
neither is something to worry about right now.
Turl already mentioned the calculated shift, instead of the switch. I
agree to also like it better and have already rewritten that bit.
If I made a really stupid thinking mistake or my math is somehow wrong,
feel free to point it out :) I don't mind manning up to my mistakes :)
Maxime
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2] Initial support for Allwinner's Security ID fuses
On 05/25/13 14:22, Maxime Ripard wrote:
Hi Oliver,
On Fri, May 24, 2013 at 11:50:38PM +0200, Oliver Schinagl wrote:
On 05/18/13 19:19, Oliver Schinagl wrote:
snip
+
+
+/* We read the entire key, using a look up table. Returned is only the
+ * requested byte. This is of course slower then it could be and uses 4 times
+ * more reads as needed but keeps code a little simpler.
+ */
+u8 sunxi_sid_read_byte(const int key)
+{
+ u32 sid_key;
+ u8 ret;
+
+ ret = 0;
+ if (likely((key = SUNXI_SID_SIZE))) {
+ sid_key = ioread32(p-sid_base + keys_lut[key 2]);
+ switch (key % 4) {
+ case 0:
+ ret = (sid_key 24) 0xff;
+ break;
+ case 1:
+ ret = (sid_key 16) 0xff;
+ break;
+ case 2:
+ ret = (sid_key 8) 0xff;
+ break;
+ case 3:
+ ret = sid_key 0xff;
+ break;
+ }
+ }
Come on, you can do better. This lookup table is useless.
I didn't want to depend on the fixed layout of memory, but consider it
removed.
But i'm not smart enough :p
We can either use the look up table (which does have benefits as its
potentially more future proof), or do some ((key 2) 2) to
'drop' the LSB's that we want to ignore (unless there's some smarter
way).
Personally, I think the LUT is a little cleaner and more readable,
but I guess if you look at poor efficiency, the lut costs some
memory, the left/right shift cost an additional 2 ... what you
prefer.
What about:
val = ioread32be(base + (key / 4));
val = (key % 4) * 8;
return val 0xff;
No lookup table, no weird swich statement, and you get the endianness
conversion only when you need it.
Ok I think I like the Endianess, ioread32be does the right thing then?
I'll read up on that.
As for key / 4; how will that work without the lut?
Lets take byte 14 (out of the available 16). Byte 14 (0x0e) is located
in SID_KEY3, so base + 0x0c. We need to write a whole 32bit word to keep
with alignment, the registers go wakko if you do unaligned reads. So we
need to read the entire 32 bits, then find our byte.
key / 4 for 14 yields 0x03. So we have base + 0x03, which is not what we
want. We want base + 0x0c, which is either ((key 2) 2)) Or, ((key
/ 4) * 4)) which to me, are both equally confusing. So we either use the
look up table, which is a little cleaner and is a bit more future proof
if either a) there's more 'eeprom like' storage which can be combined
herein or b) 'a' next version has non-continuing regions. Granted
neither is something to worry about right now.
Turl already mentioned the calculated shift, instead of the switch. I
agree to also like it better and have already rewritten that bit.
If I made a really stupid thinking mistake or my math is somehow wrong,
feel free to point it out :) I don't mind manning up to my mistakes :)
Maxime
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2] Initial support for Allwinner's Security ID fuses
On 05/18/13 19:19, Oliver Schinagl wrote:
+
+
+/* We read the entire key, using a look up table. Returned is only the
+ * requested byte. This is of course slower then it could be and uses 4 times
+ * more reads as needed but keeps code a little simpler.
+ */
+u8 sunxi_sid_read_byte(const int key)
+{
+ u32 sid_key;
+ u8 ret;
+
+ ret = 0;
+ if (likely((key <= SUNXI_SID_SIZE))) {
+ sid_key = ioread32(p->sid_base + keys_lut[key >> 2]);
+ switch (key % 4) {
+ case 0:
+ ret = (sid_key >> 24) & 0xff;
+ break;
+ case 1:
+ ret = (sid_key >> 16) & 0xff;
+ break;
+ case 2:
+ ret = (sid_key >> 8) & 0xff;
+ break;
+ case 3:
+ ret = sid_key & 0xff;
+ break;
+ }
+ }
Come on, you can do better. This lookup table is useless.
I didn't want to depend on the fixed layout of memory, but consider it
removed.
But i'm not smart enough :p
We can either use the look up table (which does have benefits as its
potentially more future proof), or do some ((key >> 2) << 2) to 'drop'
the LSB's that we want to ignore (unless there's some smarter way).
Personally, I think the LUT is a little cleaner and more readable, but I
guess if you look at poor efficiency, the lut costs some memory, the
left/right shift cost an additional >> 2 ... what you prefer.
Also, why the first key is the one with the MSBs?
I'd expect that the key 0 is the one holding the LSBs.
Strangely enough, they have swapped the MSB and LSB bytes. I double
checked it with u-boot and yep, swapped. Though in the end, if we write
stuff there and we read stuff from there, order doesn't matter? So what
do we prefer. Have it so that it makes sense and ignore how u-boot reads
it, or correct it and be consistent?
You had me confused and I was looking at this for a little while.
Bit-ordering does not change, Byte endianness is a different story of
course. As it is now, I decided to use Big endianess. So now a 32bit key
looks like:
0x162367c7 and if we read one byte at a time, we get 0x16, 0x23, 0x67
and 0xc7. I made a comment that data is read as Big endian. If it is
important, for eeprom data, to be stored little endian, I'll obviously
change it per request.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2] Initial support for Allwinner's Security ID fuses
On 05/18/13 19:19, Oliver Schinagl wrote:
snip
+
+
+/* We read the entire key, using a look up table. Returned is only the
+ * requested byte. This is of course slower then it could be and uses 4 times
+ * more reads as needed but keeps code a little simpler.
+ */
+u8 sunxi_sid_read_byte(const int key)
+{
+ u32 sid_key;
+ u8 ret;
+
+ ret = 0;
+ if (likely((key = SUNXI_SID_SIZE))) {
+ sid_key = ioread32(p-sid_base + keys_lut[key 2]);
+ switch (key % 4) {
+ case 0:
+ ret = (sid_key 24) 0xff;
+ break;
+ case 1:
+ ret = (sid_key 16) 0xff;
+ break;
+ case 2:
+ ret = (sid_key 8) 0xff;
+ break;
+ case 3:
+ ret = sid_key 0xff;
+ break;
+ }
+ }
Come on, you can do better. This lookup table is useless.
I didn't want to depend on the fixed layout of memory, but consider it
removed.
But i'm not smart enough :p
We can either use the look up table (which does have benefits as its
potentially more future proof), or do some ((key 2) 2) to 'drop'
the LSB's that we want to ignore (unless there's some smarter way).
Personally, I think the LUT is a little cleaner and more readable, but I
guess if you look at poor efficiency, the lut costs some memory, the
left/right shift cost an additional 2 ... what you prefer.
Also, why the first key is the one with the MSBs?
I'd expect that the key 0 is the one holding the LSBs.
Strangely enough, they have swapped the MSB and LSB bytes. I double
checked it with u-boot and yep, swapped. Though in the end, if we write
stuff there and we read stuff from there, order doesn't matter? So what
do we prefer. Have it so that it makes sense and ignore how u-boot reads
it, or correct it and be consistent?
You had me confused and I was looking at this for a little while.
Bit-ordering does not change, Byte endianness is a different story of
course. As it is now, I decided to use Big endianess. So now a 32bit key
looks like:
0x162367c7 and if we read one byte at a time, we get 0x16, 0x23, 0x67
and 0xc7. I made a comment that data is read as Big endian. If it is
important, for eeprom data, to be stored little endian, I'll obviously
change it per request.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2] Initial support for Allwinner's Security ID fuses
On 05/23/13 16:58, Maxime Ripard wrote: On Thu, May 23, 2013 at 10:10:17AM +0200, Oliver Schinagl wrote: Then, i'm not sure if the driver is the best for this to be loaded? Maxime, what do you think? Personally I would feel more in having this in the mach-sunxi/core.c bit, but then again, this is currently a module and wouldn't be useful to have there. Maxime is far more knowledgeable to answer that. Hmmm, I don't understand what you mean here. Could you explain what you have in mind? I've thought about it a little, and don't think core.c is a good spot, since the module will have to be loaded, or available there. And that's really early. So I guess, during probe, controlled by a parameter perhaps, load all 16 bytes into the random pool as Linus suggested? Thanks, Maxime -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC] pwm: add sysfs interface
On 05/23/13 14:12, Lars Poeschel wrote: Hi Oliver! As you are not the first one asking for the status of the pwm sysfs interface I post my answer to the linux-kernel list in hope others will benefit. On Thursday 23 May 2013 at 11:07:39, Oliver Schinagl wrote: Hi lars, I have trouble seeing if your patch has been applied yet to the kernel tree. I do see it in patchwork. https://patchwork.kernel.org/patch/2163151/ But don't see sysfs features in https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/dri vers/pwm/core.c It is not merged and will never be. If you haven't had time to take Greg's comments into account, would there be a possibility for you to correct those and re-submit? I already did resubmit an updated version taking Greg's comments into account. You can find the thread here: http://marc.info/?l=linux-kernel=136499756101273=2 https://patchwork.kernel.org/patch/2387391/ Also this version will not get merged into mainline. I have to do a v2 according to Thierry's comments. I have planed to do it, but I will not find the time during the next say 2 months. Ok, keep me updated. If in the meantime I find time, I'll take the next set of comments into account and re-submit it, with a CC to you. If you hear nothing, don't let that hold you back ;) If not, I'd be happy to pull your patch, fix and re-submit (giving you full credit of course). You're welcome! This is a community project. And there is nothing special to pull or something. Just take the patch from patchwork and work on it. Your sysfs driver feature is quite awesome and makes using PWM's quite cool, so that's definitely a good feature to have in the kernel. Thanks! Thank you for your interest! Regards, Lars -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2] Initial support for Allwinner's Security ID fuses
On 05/23/13 09:56, Linus Walleij wrote: On Fri, May 17, 2013 at 3:35 PM, Oliver Schinagl wrote: (...) While initially these fuses are used to somewhat determin the chipID, these appear to be writeable by the user and thus can be used for other purpouses. For example storing a 128 bit root key, a unique serial number, which could then even be used as a MAC address. (...) Then follows some code to read out the keys from sysfs I guess.. +static int __init sid_probe(struct platform_device *pdev) It's really simple to actually make the kernel use this to seed the entropy pool. #include add_device_randomness(u8 *, num); If you know after probe that you can read out a number of bytes of device-unique data, I think you should add those bytes to the entropy pool like this. While that is a great idea, we can't guarantee device uniqueness. We've already seen some chips that where 'forgotten' to program and default set to all 0. I guess that doesn't have to be a bad thing. Then, i'm not sure if the driver is the best for this to be loaded? Maxime, what do you think? Personally I would feel more in having this in the mach-sunxi/core.c bit, but then again, this is currently a module and wouldn't be useful to have there. Maxime is far more knowledgeable to answer that. It should probably be noted, that the sunxi series have a hardware crypto engine, with hardware random seed generator, one for a later project. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2] Initial support for Allwinner's Security ID fuses
On 05/23/13 09:56, Linus Walleij wrote: On Fri, May 17, 2013 at 3:35 PM, Oliver Schinagl oliver+l...@schinagl.nl wrote: (...) While initially these fuses are used to somewhat determin the chipID, these appear to be writeable by the user and thus can be used for other purpouses. For example storing a 128 bit root key, a unique serial number, which could then even be used as a MAC address. (...) Then follows some code to read out the keys from sysfs I guess.. +static int __init sid_probe(struct platform_device *pdev) It's really simple to actually make the kernel use this to seed the entropy pool. #include linux/random.h add_device_randomness(u8 *, num); If you know after probe that you can read out a number of bytes of device-unique data, I think you should add those bytes to the entropy pool like this. While that is a great idea, we can't guarantee device uniqueness. We've already seen some chips that where 'forgotten' to program and default set to all 0. I guess that doesn't have to be a bad thing. Then, i'm not sure if the driver is the best for this to be loaded? Maxime, what do you think? Personally I would feel more in having this in the mach-sunxi/core.c bit, but then again, this is currently a module and wouldn't be useful to have there. Maxime is far more knowledgeable to answer that. It should probably be noted, that the sunxi series have a hardware crypto engine, with hardware random seed generator, one for a later project. Yours, Linus Walleij -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC] pwm: add sysfs interface
On 05/23/13 14:12, Lars Poeschel wrote: Hi Oliver! As you are not the first one asking for the status of the pwm sysfs interface I post my answer to the linux-kernel list in hope others will benefit. On Thursday 23 May 2013 at 11:07:39, Oliver Schinagl wrote: Hi lars, I have trouble seeing if your patch has been applied yet to the kernel tree. I do see it in patchwork. https://patchwork.kernel.org/patch/2163151/ But don't see sysfs features in https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/dri vers/pwm/core.c It is not merged and will never be. If you haven't had time to take Greg's comments into account, would there be a possibility for you to correct those and re-submit? I already did resubmit an updated version taking Greg's comments into account. You can find the thread here: http://marc.info/?l=linux-kernelm=136499756101273w=2 https://patchwork.kernel.org/patch/2387391/ Also this version will not get merged into mainline. I have to do a v2 according to Thierry's comments. I have planed to do it, but I will not find the time during the next say 2 months. Ok, keep me updated. If in the meantime I find time, I'll take the next set of comments into account and re-submit it, with a CC to you. If you hear nothing, don't let that hold you back ;) If not, I'd be happy to pull your patch, fix and re-submit (giving you full credit of course). You're welcome! This is a community project. And there is nothing special to pull or something. Just take the patch from patchwork and work on it. Your sysfs driver feature is quite awesome and makes using PWM's quite cool, so that's definitely a good feature to have in the kernel. Thanks! Thank you for your interest! Regards, Lars -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2] Initial support for Allwinner's Security ID fuses
On 05/23/13 16:58, Maxime Ripard wrote: On Thu, May 23, 2013 at 10:10:17AM +0200, Oliver Schinagl wrote: Then, i'm not sure if the driver is the best for this to be loaded? Maxime, what do you think? Personally I would feel more in having this in the mach-sunxi/core.c bit, but then again, this is currently a module and wouldn't be useful to have there. Maxime is far more knowledgeable to answer that. Hmmm, I don't understand what you mean here. Could you explain what you have in mind? I've thought about it a little, and don't think core.c is a good spot, since the module will have to be loaded, or available there. And that's really early. So I guess, during probe, controlled by a parameter perhaps, load all 16 bytes into the random pool as Linus suggested? Thanks, Maxime -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
