uio: uio_pdrv_genirq V2

This patch is V2 of uio_pdrv_genirq.c, a platform driver for UIO with generic IRQ handling code. This driver is very similar to the regular UIO platform driver, but is only suitable for devices that are connected to the interrupt controller using unique interrupt lines.

The uio_pdrv_genirq driver includes generic interrupt handling code which disables the serviced interrupt in the interrupt controller and makes the user space driver responsible for acknowledging the interrupt in the device and reenabling the interrupt in the interrupt controller.

Shared interrupts are not supported since the in-kernel interrupt handler will disable the interrupt line in the interrupt controller, and in a shared interrupt configuration this will stop other devices from delivering interrupts.

Signed-off-by Magnus Damm <[email protected]>

---

Changes since V1
- allow user space to enable _and_ disable the interrupt
- replaced bitops with spinlock + irq_disabled variable
- renamed pdata variable name to priv

 drivers/uio/Kconfig           |   13 ++
 drivers/uio/Makefile          |    1
 drivers/uio/uio_pdrv_genirq.c |  186 +++++++++++++++++++++++++++++++++++++++++
 3 files changed, 200 insertions(+)

drivers/uio/Kconfig 2008-07-09 17:10:20.000000000 +0900 renamed from drivers/uio/Kconfig +13 -0

@@ -33,6 +33,19 @@ config UIO_PDRV
 
          If you don't know what to do here, say N.
 
+config UIO_PDRV_GENIRQ
+       tristate "Userspace I/O platform driver with generic IRQ handling"
+       help
+         Platform driver for Userspace I/O devices, including generic
+         interrupt handling code. Shared interrupts are not supported.
+
+         This kernel driver requires that the matching userspace driver
+         handles interrupts in a special way. Userspace is responsible
+         for acknowledging the hardware device if needed, and re-enabling
+         interrupts in the interrupt controller using the write() syscall.
+
+         If you don't know what to do here, say N.
+
 config UIO_SMX
        tristate "SMX cryptengine UIO interface"
        depends on UIO

drivers/uio/Makefile 2008-07-09 17:10:20.000000000 +0900 renamed from drivers/uio/Makefile +1 -0

@@ -1,4 +1,5 @@
 obj-$(CONFIG_UIO)      += uio.o
 obj-$(CONFIG_UIO_CIF)  += uio_cif.o
 obj-$(CONFIG_UIO_PDRV) += uio_pdrv.o
+obj-$(CONFIG_UIO_PDRV_GENIRQ)  += uio_pdrv_genirq.o
 obj-$(CONFIG_UIO_SMX)  += uio_smx.o

drivers/uio/uio_pdrv_genirq.c 2008-07-10 12:24:52.000000000 +0900 new file +52 -0

@@ -0,0 +1,186 @@
+/*
+ * drivers/uio/uio_pdrv_genirq.c
+ *
+ * Userspace I/O platform driver with generic IRQ handling code.
+ *
+ * Copyright (C) 2008 Magnus Damm
+ *
+ * Based on uio_pdrv.c by Uwe Kleine-Koenig,
+ * Copyright (C) 2008 by Digi International Inc.
+ * All rights reserved.
+ *
+ * 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/platform_device.h>
+#include <linux/uio_driver.h>
+#include <linux/spinlock.h>
+#include <linux/interrupt.h>
+#include <linux/stringify.h>
+
+#define DRIVER_NAME "uio_pdrv_genirq"
+
+struct uio_pdrv_genirq_platdata {
+       struct uio_info *uioinfo;
+       spinlock_t lock;
+       int irq_disabled;
+};
+
+static irqreturn_t uio_pdrv_genirq_handler(int irq, struct uio_info *dev_info)
+{
+       struct uio_pdrv_genirq_platdata *priv = dev_info->priv;
+       unsigned long flags;
+
+       /* Just disable the interrupt in the interrupt controller, and
+        * remember the state so we can allow user space to enable it later.
+        */
+       spin_lock_irqsave(&priv->lock, flags);
+       if (!priv->irq_disabled) {
+               disable_irq_nosync(irq);
+               priv->irq_disabled = 1;
+       }
+       spin_unlock_irqrestore(&priv->lock, flags);
+       return IRQ_HANDLED;
+}
+
+static int uio_pdrv_genirq_irqcontrol(struct uio_info *dev_info, s32 irq_on)
+{
+       struct uio_pdrv_genirq_platdata *priv = dev_info->priv;
+       unsigned long flags;
+

drivers/uio/uio_pdrv_genirq.c 2008-07-10 12:24:52.000000000 +0900 new file +4 -0

+
+       priv->irq_disabled = !irq_on;
+       spin_unlock_irqrestore(&priv->lock, flags);
+       return 0;

drivers/uio/uio_pdrv_genirq.c line 62

Quoted 3 lines

drivers/uio/uio_pdrv_genirq.c new file +2 -0

+       else if (!irq_on && !priv->irq_disabled)
+               disable_irq(dev_info->irq);

I'm not sure if this is a problem on SMP. Should you use disable_irq_nosync here, too? Probably it's OK.

Quoted 5 lines

drivers/uio/uio_pdrv_genirq.c line 62

Quoted 3 lines

drivers/uio/uio_pdrv_genirq.c new file +2 -0

+       else if (!irq_on && !priv->irq_disabled)
+               disable_irq(dev_info->irq);

I'm not sure if this is a problem on SMP. Should you use disable_irq_nosync here, too? Probably it's OK.

That one will also deadlock.

The easiest fix is probably to use test_and_set and friends for each I/O operation.

           You would then not need the lock to protect ->irq_disabled.

Propogating that throughout means your user space has to handle the case of an IRQ arriving after disable returns but would be a fair bit saner I think ?

Quoted 3 lines · Alan Cox

Quoted 3 lines

Quoted 3 lines

drivers/uio/uio_pdrv_genirq.c new file +2 -0

+       else if (!irq_on && !priv->irq_disabled)
+               disable_irq(dev_info->irq);

Quoted 4 lines

Quoted 2 lines

I'm not sure if this is a problem on SMP. Should you use disable_irq_nosync here, too? Probably it's OK.

That one will also deadlock.

Can you explain why? I think irqcontrol is only called in task context. I only see one possible deadlock and that's disable_irq being called while the irq is IRQ_INPROGRESS on the same cpu. I'm always willing to learn.

I think I didn't understand you right here, with the lock this can happen, too, doesn't it?

Quoted 4 lines · On Thu, 10 Jul 2008 12:30:36 +0200 Uwe Kleine-König…

Quoted 3 lines · Alan Cox

Quoted 3 lines

Quoted 3 lines

drivers/uio/uio_pdrv_genirq.c new file +2 -0

+       else if (!irq_on && !priv->irq_disabled)
+               disable_irq(dev_info->irq);

Quoted 8 lines

Quoted 4 lines

Quoted 2 lines

I'm not sure if this is a problem on SMP. Should you use disable_irq_nosync here, too? Probably it's OK.

That one will also deadlock.

Can you explain why? I think irqcontrol is only called in task context. I only see one possible deadlock and that's disable_irq being called while the irq is IRQ_INPROGRESS on the same cpu. I'm always willing to learn.

CPU0 (UIO IRQ) CPU1 (irqcontrol)
take IRQ
take spin lock
spin on spinlock
disable_irq (blocks)

Quoted 7 lines · Alan Cox

Quoted 5 lines

drivers/uio/uio_pdrv_genirq.c line 62

Quoted 3 lines

drivers/uio/uio_pdrv_genirq.c new file +2 -0

+       else if (!irq_on && !priv->irq_disabled)
+               disable_irq(dev_info->irq);

I'm not sure if this is a problem on SMP. Should you use disable_irq_nosync here, too? Probably it's OK.

That one will also deadlock.

Can you explain why? I think irqcontrol is only called in task context. I only see one possible deadlock and that's disable_irq being called while the irq is IRQ_INPROGRESS on the same cpu. I'm always willing to learn.

Quoted 2 lines

The easiest fix is probably to use test_and_set and friends for each I/O operation.

Actually using spinlock + irq_disabled variable is new in V2 of this patch. Don't know why this changed, though.

Quoted 27 lines

drivers/uio/uio_pdrv_genirq.c new file +27 -0

+
+static irqreturn_t uio_pdrv_genirq_handler(int irq, struct uio_info *dev_info)
+{
+       struct uio_pdrv_genirq_platdata *priv = dev_info->priv;
+       unsigned long flags;
+
+       /* Just disable the interrupt in the interrupt controller, and
+        * remember the state so we can allow user space to enable it later.
+        */
+       spin_lock_irqsave(&priv->lock, flags);
+       if (!priv->irq_disabled) {
+               disable_irq_nosync(irq);
+               priv->irq_disabled = 1;
+       }
+       spin_unlock_irqrestore(&priv->lock, flags);
+       return IRQ_HANDLED;
+}
+
+static int uio_pdrv_genirq_irqcontrol(struct uio_info *dev_info, s32 irq_on)
+{
+       struct uio_pdrv_genirq_platdata *priv = dev_info->priv;
+       unsigned long flags;
+
+       /* Allow user space to enable and disable the interrupt
+        * in the interrupt controller, but keep track of the
+        * state to prevent per-irq depth damage.
+        */

drivers/uio/uio_pdrv_genirq.c line 62

Quoted 6 lines

drivers/uio/uio_pdrv_genirq.c new file +6 -0

+
+       spin_lock_irqsave(&priv->lock, flags);
+       if (irq_on && priv->irq_disabled)
+               enable_irq(dev_info->irq);
+       else if (!irq_on && !priv->irq_disabled)
+               disable_irq(dev_info->irq);

I'm not sure if this is a problem on SMP. Should you use disable_irq_nosync here, too? Probably it's OK.

Quoted 5 lines

drivers/uio/uio_pdrv_genirq.c new file +4 -0

+
+       priv->irq_disabled = !irq_on;
+       spin_unlock_irqrestore(&priv->lock, flags);
+       return 0;

drivers/uio/uio_pdrv_genirq.c line 144

Quoted 10 lines

drivers/uio/uio_pdrv_genirq.c new file +8 -0

+}
 
 
+       ret = uio_register_device(&pdev->dev, priv->uioinfo);
+       if (ret) {
+               dev_err(&pdev->dev, "unable to register uio device\n");
+               goto bad1;
+       }
+
+       platform_set_drvdata(pdev, priv);

This should probably go before uio_register_device. (Uups, this is an issue for uio_pdrv, too.)

Quoted 7 lines

Quoted 7 lines

drivers/uio/uio_pdrv_genirq.c new file +7 -0

+       ret = uio_register_device(&pdev->dev, priv->uioinfo);
+       if (ret) {
+               dev_err(&pdev->dev, "unable to register uio device\n");
+               goto bad1;
+       }
+
+       platform_set_drvdata(pdev, priv);

Quoted 2 lines

This should probably go before uio_register_device. (Uups, this is an issue for uio_pdrv, too.)

Yes, because uio_register_device will enable the irq, and you might arrive in the handler without having your platform data in place.

Hi,

Quoted 7 lines · Hans J. Koch

Quoted 7 lines

Quoted 7 lines

drivers/uio/uio_pdrv_genirq.c new file +7 -0

+       ret = uio_register_device(&pdev->dev, priv->uioinfo);
+       if (ret) {
+               dev_err(&pdev->dev, "unable to register uio device\n");
+               goto bad1;
+       }
+
+       platform_set_drvdata(pdev, priv);

Quoted 5 lines

Quoted 2 lines

This should probably go before uio_register_device. (Uups, this is an issue for uio_pdrv, too.)

Yes, because uio_register_device will enable the irq, and you might arrive in the handler without having your platform data in place.

How do we want to fix that? Should I sent a new version that also includes the memory-leak fix and copyright specification? Or another incremental patch?

Quoted 7 lines · Uwe Kleine-König

Quoted 7 lines

drivers/uio/uio_pdrv_genirq.c new file +7 -0

+       ret = uio_register_device(&pdev->dev, priv->uioinfo);
+       if (ret) {
+               dev_err(&pdev->dev, "unable to register uio device\n");
+               goto bad1;
+       }
+
+       platform_set_drvdata(pdev, priv);

Quoted 2 lines

This should probably go before uio_register_device. (Uups, this is an issue for uio_pdrv, too.)

Yes, because uio_register_device will enable the irq, and you might arrive in the handler without having your platform data in place.

Quoted 8 lines · Hans J. Koch

Quoted 7 lines · Uwe Kleine-König

Quoted 7 lines

drivers/uio/uio_pdrv_genirq.c new file +7 -0

+       ret = uio_register_device(&pdev->dev, priv->uioinfo);
+       if (ret) {
+               dev_err(&pdev->dev, "unable to register uio device\n");
+               goto bad1;
+       }
+
+       platform_set_drvdata(pdev, priv);

Quoted 5 lines

Quoted 2 lines

This should probably go before uio_register_device. (Uups, this is an issue for uio_pdrv, too.)

Yes, because uio_register_device will enable the irq, and you might arrive in the handler without having your platform data in place.

I'd say that this is a non-issue.

Only uio_pdrv_genirq_remove() is using the platform data. This remove() method won't be called if uio_pdrv_genirq_probe() returns failure. The UIO subsystem is using uioinfo->priv which of course is set up correctly already.