uio: User IRQ Mode V2

This is V2 of the UIO "User IRQ Mode" patch.

In User IRQ Mode the kernel space interrupt handler simply 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 by the User IRQ Mode since the generic in-kernel interrupt handler will disable the shared interrupt 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]>

Signed-off-by Uwe Kleine-König <[email protected]>

---

Changes since V1
- use disable_irq_nosync() in interrupt handler, thanks Alan!

Changes since Uwe's last version:
- flags should be unsigned long
- simplify uio_userirq_handler()

Similar code has been posted some time ago as:
"[PATCH] uio_pdrv: Unique IRQ Mode" "[PATCH 00/03][RFC] Reusable UIO Platform Driver".

Needs "[PATCH 0/1] UIO: Add a write() function to enable/disable interrupts"

 include/linux/uio_driver.h |   26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

include/linux/uio_driver.h +26 -0

@@ -17,6 +17,7 @@
 #include <linux/module.h>
 #include <linux/fs.h>
 #include <linux/interrupt.h>
+#include <linux/bitops.h>
 
 struct uio_map;
 

@@ -63,6 +64,7 @@ struct uio_info {
 	long			irq;
 	unsigned long		irq_flags;
 	void			*priv;
+	unsigned long		flags;
 	irqreturn_t (*handler)(int irq, struct uio_info *dev_info);
 	int (*mmap)(struct uio_info *info, struct vm_area_struct *vma);
 	int (*open)(struct uio_info *info, struct inode *inode);

@@ -92,4 +94,28 @@ extern void uio_event_notify(struct uio_
 #define UIO_MEM_LOGICAL	2
 #define UIO_MEM_VIRTUAL 3
 
+/* defines for uio_info->flags */
+#define UIO_FLAGS_IRQDISABLED 0
+
+static inline irqreturn_t uio_userirq_handler(int irq, struct uio_info *info)
+{
+	if (!test_and_set_bit(UIO_FLAGS_IRQDISABLED, &info->flags))
+		disable_irq_nosync(info->irq);
+
+	return IRQ_HANDLED;
+}
+
+static inline int uio_userirq_irqcontrol(struct uio_info *info, s32 irq_on)
+{
+	if (irq_on) {
+		if (test_and_clear_bit(UIO_FLAGS_IRQDISABLED, &info->flags))
+			enable_irq(info->irq);
+	} else {
+		if (!test_and_set_bit(UIO_FLAGS_IRQDISABLED, &info->flags))
+			disable_irq(info->irq);
+	}
+
+	return 0;
+}
+
 #endif /* _LINUX_UIO_DRIVER_H_ */

Quoted 1 line · Magnus Damm

This is V2 of the UIO "User IRQ Mode" patch.

We already have Uwe's uio_pdrv. It allows generic handling of UIO platform devices by defining struct uio_info and the irq handler together with struct platform_device in board support. An irqcontrol() function can optionally be defined if needed.

Your idea takes this one step further. You say we can even omit the irq handler and the irqcontrol() function if we know that the platform devices's irq is not shared. We can then replace both functions with generic ones using disable_irq_nosync(). As a consequence, the userspace part of the driver must reenable the interrupt using write().

As this is a special version of uio_pdrv, it should be implemented in a similar way. I suggest calling it uio_pdrv_genirq because the difference to uio_pdrv is that irqs are handled in a generic way in the kernel (as I already said, I find the term "User IRQ mode" misleading).

uio_pdrv_genirq would be quite similar to uio_pdrv, with the following differences:

- Its probe() function should fail if either (*handler) or (*irqcontrol) is not NULL, because obviously the user didn't get the idea.

- It should instead fill in the generic functions from your patch below. These can be implemented in uio_pdrv_genirq.c, no need to patch them into the UIO core header file.

- The Kconfig help text should state clearly that this only works for non-shared interrupts. Maybe it should be disabled on x86.

One of your first implementations of that idea patched it into uio_pdrv. I didn't like this because it adds stuff that doesn't work on every platform to something that does. Although uio_pdrv and uio_pdrv_genirq would share some code, I find it justified to have two separate implementation files, as both differ in platform dependencies and possible use cases. Having two separate drivers clearly shows a user which choices he has. Trying to save a few lines by putting it all in one file would mainly add confusion and #ifdefs.

Implementing it this way, we also have the advantage that we don't have to change any existing UIO core files (aside from adding the "flags" element to struct uio_info, which is OK for me).

If you implemented it like this, uio_pdrv_genirq would become a part of the UIO framework, and I'd explain it in the docs (I need to describe uio_pdrv there anyway).

Would this approach be acceptable to you? If yes, please send a patch, I think it can be merged soon.

Sorry I didn't have that idea earlier. I didn't feel well rejecting your implementations without having something better to offer. I really hope we can settle it this way.

Thanks for your contributions and patience,