Skip to contentSkip to conversation

UIO: Implement a UIO interface for the SMX Cryptengine

PATCH Superseded Ben Nizette 3 participants 4 messages Mar 13, 2008 Userspace I/O (UIO)

v3 of 4 Superseded Latest is v4 →

lore
Share & tools
lore
#0 lore raw

PATCH v3 1/1 UIO: Implement a UIO interface for the SMX Cryptengine

This patch implements a UIO interface for the SMX Cryptengine.

The Cryptengine found on the Nias Digital SMX board is best suited for a UIO interface. It is not wired in to the cryptographic API as the engine handles it's own keys, algorithms, everything. All that we know about is that if there's room in the buffer, you can write data to it and when there's data ready, you read it out again.

There isn't necessarily even any direct correlation between data going in and data coming out again, the engine may consume or generate data all on its own.

This driver is for proprietary hardware but we're always told to submit the drivers anyway; here you are. :-)

This is version 2 of this patch and addresses all issues raised by Hans-Jürgen Koch and Paul Mundt in their reviews.

---
Apologies for the confusion around this patch, I didn't mean to give the impression that I'd ignored Paul; I am of course grateful for his review and bug catch. I received the review just a minute after sending out the last version, made the change locally but didn't immediately resend. I'll know better next time.

MAINTAINERS +5 -0
index 2cdb591..480fd44 100644
@@ -3554,6 +3554,11 @@ M:	[email protected]
 L:	[email protected]
 S:	Maintained
 
+SMX UIO Interface
+P:	Ben Nizette
+M:	[email protected]
+S:	Maintained
+
 SOFTMAC LAYER (IEEE 802.11)
 P:	Daniel Drake
 M:	[email protected]
drivers/uio/Kconfig +12 -0
index b778ed7..f57b347 100644
@@ -26,4 +26,16 @@ config UIO_CIF
 	  To compile this driver as a module, choose M here: the module
 	  will be called uio_cif.
 
+config UIO_SMX
+	tristate "SMX cryptengine UIO interface"
+	depends on UIO
+	default n
+	help
+	  Userspace IO interface to the Cryptography engine found on the
+	  Nias Digital SMX boards.  These will be available from Q4 2008
+	  from http://www.niasdigital.com.  The userspace part of this
+	  driver will be released under the GPL at the same time as the
+	  hardware and will be able to be downloaded from the same site.
+
+	  If you compile this as a module, it will be called uio_smx.
 endmenu
drivers/uio/Makefile +1 -0
index 7fecfb4..18c4566 100644
@@ -1,2 +1,3 @@
 obj-$(CONFIG_UIO)	+= uio.o
 obj-$(CONFIG_UIO_CIF)	+= uio_cif.o
+obj-$(CONFIG_UIO_SMX)	+= uio_smx.o
drivers/uio/uio_smx.c new file +118 -0

118 lines not on this page · raw message

#1 lore raw
Quoted 14 lines · Ben Nizette
drivers/uio/uio_smx.c new file +8 -0
new file mode 100644
index 0000000..fda8b8b
@@ -0,0 +1,118 @@
+/*
+ * UIO SMX Cryptengine driver.
+ *
+ * (C) 2008 Nias Digital P/L <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.

drivers/uio/uio_smx.c line 18

Quoted 10 lines
drivers/uio/uio_smx.c new file +10 -0
+ *
+ */
+
+#include <linux/device.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/uio_driver.h>
+
+#include <asm/io.h>
+

linux/io.h is preferable here.

Quoted 16 lines
drivers/uio/uio_smx.c new file +16 -0
+static int __devinit smx_ce_probe(struct platform_device *dev)
+{
+	struct uio_info *info;
+	struct resource *regs;
+
+	info = kzalloc(sizeof(struct uio_info), GFP_KERNEL);
+	if (!info)
+		return -ENOMEM;
+
+	regs = platform_get_resource(dev, IORESOURCE_MEM, 0);
+	if (!regs)
+		goto out_free;
+
+	info->mem[0].addr = regs->start;
+	if (!info->mem[0].addr)
+		goto out_free;

drivers/uio/uio_smx.c line 64

Quoted 10 lines
drivers/uio/uio_smx.c new file +10 -0
+
+	info->mem[0].size = regs->end - regs->start + 1;
+	info->mem[0].internal_addr = ioremap(regs->start, info->mem[0].size);
+	if (!info->mem[0].internal_addr)
+		goto out_free;
+
+	info->mem[0].memtype = UIO_MEM_PHYS;
+
+	info->name = "smx-ce";
+	info->version = "0.03";

You may as well use DRV_NAME and DRV_VERSION, then you can toss that over to MODULE_VERSION() if you're so inclined. It's generally nice to keep modinfo happy.

drivers/uio/uio_smx.c line 68

Quoted 4 lines
drivers/uio/uio_smx.c new file +4 -0
+	info->irq = platform_get_irq(dev, 0);
+	info->irq_flags = IRQF_SHARED;
+	info->handler = smx_handler;
+

platform_get_irq() can fail, which you don't presently handle (though I guess uio_register_device() will error out if you hand off -ENXIO as your IRQ anyways, so it's not technically an issue). That's a pretty minor issue, though, but might be something you wish to fix up if you are planning on using this as an example driver for others.

Quoted 4 lines
drivers/uio/uio_smx.c new file +4 -0
+	platform_set_drvdata(dev, info);
+
+	if (uio_register_device(&dev->dev, info))
+		goto out_unmap;

drivers/uio/uio_smx.c line 82

Quoted 10 lines
drivers/uio/uio_smx.c new file +10 -0
+
+	return 0;
+out_unmap:
+	iounmap(info->mem[0].internal_addr);
+out_free:
+	kfree(info);
+
+	return -ENODEV;
+}
+

Your error paths are also pretty quiet. A dev_err() or so in the few failure cases you do have might not be a terrible idea.

uio_register_device() also has quite a few different error cases, so it's at least worth preserving the error value and returning that back, rather than always returning -ENODEV.

#2 lore raw

Am Thu, 13 Mar 2008 19:06:55 +0900
schrieb Paul Mundt <[email protected]>:

Quoted 7 lines

drivers/uio/uio_smx.c line 64

Quoted 3 lines
drivers/uio/uio_smx.c new file +3 -0
+
+	info->name = "smx-ce";
+	info->version = "0.03";

You may as well use DRV_NAME and DRV_VERSION, then you can toss that over to MODULE_VERSION() if you're so inclined. It's generally nice to keep modinfo happy.

UIO name and version are intentionally independent of module name and version. It's meant to be an information for the userspace part of the driver and should be set to values that are meaningful for it.

Quoted 10 lines

drivers/uio/uio_smx.c line 68

Quoted 5 lines
drivers/uio/uio_smx.c new file +4 -0
+	info->irq = platform_get_irq(dev, 0);
+	info->irq_flags = IRQF_SHARED;
+	info->handler = smx_handler;
+

platform_get_irq() can fail, which you don't presently handle (though I guess uio_register_device() will error out if you hand off -ENXIO as your IRQ anyways, so it's not technically an issue). That's a pretty minor issue, though, but might be something you wish to fix up if you are planning on using this as an example driver for others.

uio_register_device() will silently not register an interrupt if the number is negative. This allows us to register a driver without interrupts. Hmm. Maybe we should explicitly check for UIO_IRQ_NONE there and fail if a different negative irq is passed in. Thanks for pointing this out.

But I agree, Ben should check if platform_get_irq() failed.

Quoted 17 lines
Quoted 5 lines
drivers/uio/uio_smx.c new file +4 -0
+	platform_set_drvdata(dev, info);
+
+	if (uio_register_device(&dev->dev, info))
+		goto out_unmap;

drivers/uio/uio_smx.c line 82

Quoted 10 lines
drivers/uio/uio_smx.c new file +10 -0
+
+	return 0;
+out_unmap:
+	iounmap(info->mem[0].internal_addr);
+out_free:
+	kfree(info);
+
+	return -ENODEV;
+}
+

Your error paths are also pretty quiet. A dev_err() or so in the few failure cases you do have might not be a terrible idea.

I agree.

Quoted 4 lines

uio_register_device() also has quite a few different error cases, so it's at least worth preserving the error value and returning that back, rather than always returning -ENODEV.

Right. Maybe we should make uio_register_device a bit more verbose, too.

Thanks for your detailed review!

Hans

#3 lore raw
In reply to Ben Nizette

Am Thu, 13 Mar 2008 20:35:40 +1100
schrieb Ben Nizette <[email protected]>:

Ben,
sorry, but your code doesn't compile. See below.

Thanks,
Hans

Quoted 98 lines

This patch implements a UIO interface for the SMX Cryptengine.

The Cryptengine found on the Nias Digital SMX board is best suited for a UIO interface. It is not wired in to the cryptographic API as the engine handles it's own keys, algorithms, everything. All that we know about is that if there's room in the buffer, you can write data to it and when there's data ready, you read it out again.

There isn't necessarily even any direct correlation between data going in and data coming out again, the engine may consume or generate data all on its own.

This driver is for proprietary hardware but we're always told to submit the drivers anyway; here you are. :-)

This is version 2 of this patch and addresses all issues raised by Hans-Jürgen Koch and Paul Mundt in their reviews.

---
Apologies for the confusion around this patch, I didn't mean to give the impression that I'd ignored Paul; I am of course grateful for his review and bug catch. I received the review just a minute after sending out the last version, made the change locally but didn't immediately resend. I'll know better next time.

MAINTAINERS +5 -0
index 2cdb591..480fd44 100644
@@ -3554,6 +3554,11 @@ M:	[email protected]
 L:	[email protected]
 S:	Maintained
 
+SMX UIO Interface
+P:	Ben Nizette
+M:	[email protected]
+S:	Maintained
+
 SOFTMAC LAYER (IEEE 802.11)
 P:	Daniel Drake
 M:	[email protected]
drivers/uio/Kconfig +12 -0
index b778ed7..f57b347 100644
@@ -26,4 +26,16 @@ config UIO_CIF
 	  To compile this driver as a module, choose M here: the module
 	  will be called uio_cif.
 
+config UIO_SMX
+	tristate "SMX cryptengine UIO interface"
+	depends on UIO
+	default n
+	help
+	  Userspace IO interface to the Cryptography engine found on the
+	  Nias Digital SMX boards.  These will be available from Q4 2008
+	  from http://www.niasdigital.com.  The userspace part of this
+	  driver will be released under the GPL at the same time as the
+	  hardware and will be able to be downloaded from the same site.
+
+	  If you compile this as a module, it will be called uio_smx.
 endmenu
drivers/uio/Makefile +1 -0
index 7fecfb4..18c4566 100644
@@ -1,2 +1,3 @@
 obj-$(CONFIG_UIO)	+= uio.o
 obj-$(CONFIG_UIO_CIF)	+= uio_cif.o
+obj-$(CONFIG_UIO_SMX)	+= uio_smx.o
drivers/uio/uio_smx.c new file +21 -0
new file mode 100644
index 0000000..fda8b8b
@@ -0,0 +1,118 @@
+/*
+ * UIO SMX Cryptengine driver.
+ *
+ * (C) 2008 Nias Digital P/L <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ */
+
+#include <linux/device.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/uio_driver.h>
+
+#include <asm/io.h>
+
+#define SMX_CSR  0x00000000
+#define SMX_EnD  0x00000001
+#define SMX_RUN  0x00000002

drivers/uio/uio_smx.c line 31

Quoted 10 lines
drivers/uio/uio_smx.c new file +10 -0
+#define SMX_DRDY 0x00000004
+#define SMX_ERR  0x00000008
+
+static irqreturn_t smx_handler(int irq, struct uio_info *dev_info)
+{
+	void __iomem *csr = dev_info->mem[0].internal_addr + SMX_CSR;
+
+	u32 status = ioread32(csr);
+
+	if (!(csr & SMX_DRDY))

This doesn't compile.

Quoted 88 lines
drivers/uio/uio_smx.c new file +87 -0
+		return IRQ_NONE;
+
+	/* Disable interrupt */
+	iowrite32(status & ~SMX_DRDY, csr);
+	return IRQ_HANDLED;
+}
+
+static int __devinit smx_ce_probe(struct platform_device *dev)
+{
+	struct uio_info *info;
+	struct resource *regs;
+
+	info = kzalloc(sizeof(struct uio_info), GFP_KERNEL);
+	if (!info)
+		return -ENOMEM;
+
+	regs = platform_get_resource(dev, IORESOURCE_MEM, 0);
+	if (!regs)
+		goto out_free;
+
+	info->mem[0].addr = regs->start;
+	if (!info->mem[0].addr)
+		goto out_free;
+
+	info->mem[0].size = regs->end - regs->start + 1;
+	info->mem[0].internal_addr = ioremap(regs->start, info->mem[0].size);
+	if (!info->mem[0].internal_addr)
+		goto out_free;
+
+	info->mem[0].memtype = UIO_MEM_PHYS;
+
+	info->name = "smx-ce";
+	info->version = "0.03";
+	info->irq = platform_get_irq(dev, 0);
+	info->irq_flags = IRQF_SHARED;
+	info->handler = smx_handler;
+
+	platform_set_drvdata(dev, info);
+
+	if (uio_register_device(&dev->dev, info))
+		goto out_unmap;
+
+	return 0;
+out_unmap:
+	iounmap(info->mem[0].internal_addr);
+out_free:
+	kfree(info);
+
+	return -ENODEV;
+}
+
+static int __devexit smx_ce_remove(struct platform_device *dev)
+{
+	struct uio_info *info = platform_get_drvdata(dev);
+
+	uio_unregister_device(info);
+	platform_set_drvdata(dev, NULL);
+	iounmap(info->mem[0].internal_addr);
+
+	kfree(info);
+
+	return 0;
+}
+
+static struct platform_driver smx_ce_driver = {
+	.probe		= smx_ce_probe,
+	.remove		= __devexit_p(smx_ce_remove),
+	.driver		= {
+		.name	= "smx-ce",
+		.owner	= THIS_MODULE,
+	},
+};
+
+static int __init smx_ce_init_module(void)
+{
+	return platform_driver_register(&smx_ce_driver);
+}
+module_init(smx_ce_init_module);
+
+static void __exit smx_ce_exit_module(void)
+{
+	platform_driver_unregister(&smx_ce_driver);
+}
+module_exit(smx_ce_exit_module);
+
+MODULE_LICENSE("GPL v2");
+MODULE_AUTHOR("Ben Nizette <[email protected]>");