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

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

[Hans J. Koch]

At the moment, I've got two examples of hardware where the UIO interrupt
handling has problems:

1) A PCI card with several internal interrupt sources that can activate
the single IRQ line of the card. The card's PCI bridge has an irq
acknowledge register that can be mapped to userspace, but setting a bit
in it would be a read-modify-write operation which is racy from
userspace.

2) An FPGA that has several internal interrupt sources OR'ed to one
interrupt line that is connected to a GPIO of an ARM processor. The chip
only has a single IRQ register where different bits show the status of
the different interrupt sources. To acknowledge an IRQ, I have to clear
the respective bit, which would leave the userspace part of the driver
with a cleared register, unable to determine the source of the
interrupt. My only chance is to disable the GPIO interrupt, but this
leads to the problem that userspace cannot turn it on again.

The patch that I'll send in response to this mail solves these problems by
adding a write() function for /dev/uioX. Userspace can write a 32-bit
int value to /dev/uioX. This value will usually be 0 or 1 to disable or
enable the device's interrupts. The kernel part of the driver can
implement an irqcontrol() hook that is called by the UIO core.

Magnus Damm also ran into this problem when developing a generic UIO
platform handler and came up with a different solution:

http://lkml.org/lkml/2008/5/21/60

However, extending the UIO specs by adding a write function seems to be
a more generic approach. It allows enabling _and_ disabling of
interrupts. For devices that don't need this functionality, nothing
changes. They can simply ignore the new irqcontrol() pointer, and UIO
behaves as it always did. My patch is completely compatible with
existing UIO drivers, no changes to drivers are required.

Thanks to Jan Altenberg for testing this, and to Magnus Damm for telling
me that I'm not the only one who has these problems ;-)

Thanks,
Hans

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

[Hans J. Koch]

Sometimes it is necessary to enable/disable the interrupt of a UIO device
from the userspace part of the driver. With this patch, the UIO kernel driver
can implement an "irqcontrol()" function that does this. Userspace can write
an s32 value to /dev/uioX (usually 0 or 1 to turn the irq off or on). The
UIO core will then call the driver's irqcontrol function.

Signed-off-by: Hans J. Koch <hjk@linutronix.de>
---
 Documentation/DocBook/uio-howto.tmpl |   40 ++++++++++++++++++++++++++++++++++-
 drivers/uio/uio.c                    |   26 ++++++++++++++++++++++
 include/linux/uio_driver.h           |    2 +
 3 files changed, 67 insertions(+), 1 deletion(-)

Index: linux-2.6.26-rc/include/linux/uio_driver.h
===================================================================
--- linux-2.6.26-rc.orig/include/linux/uio_driver.h	2008-05-22 20:22:57.000000000 +0200
+++ linux-2.6.26-rc/include/linux/uio_driver.h	2008-05-22 20:23:12.000000000 +0200
@@ -53,6 +53,7 @@
  * @mmap:		mmap operation for this uio device
  * @open:		open operation for this uio device
  * @release:		release operation for this uio device
+ * @irqcontrol:		disable/enable irqs when 0/1 is written to /dev/uioX
  */
 struct uio_info {
 	struct uio_device	*uio_dev;
@@ -66,6 +67,7 @@
 	int (*mmap)(struct uio_info *info, struct vm_area_struct *vma);
 	int (*open)(struct uio_info *info, struct inode *inode);
 	int (*release)(struct uio_info *info, struct inode *inode);
+	int (*irqcontrol)(struct uio_info *info, s32 irq_on);
 };
 
 extern int __must_check
Index: linux-2.6.26-rc/drivers/uio/uio.c
===================================================================
--- linux-2.6.26-rc.orig/drivers/uio/uio.c	2008-05-22 20:23:07.000000000 +0200
+++ linux-2.6.26-rc/drivers/uio/uio.c	2008-05-22 20:23:12.000000000 +0200
@@ -420,6 +420,31 @@
 	return retval;
 }
 
+static ssize_t uio_write(struct file *filep, const char __user *buf,
+			size_t count, loff_t *ppos)
+{
+	struct uio_listener *listener = filep->private_data;
+	struct uio_device *idev = listener->dev;
+	ssize_t retval;
+	s32 irq_on;
+
+	if (idev->info->irq == UIO_IRQ_NONE)
+		return -EIO;
+
+	if (count != sizeof(s32))
+		return -EINVAL;
+
+	if (copy_from_user(&irq_on, buf, count))
+		return -EFAULT;
+
+	if (!idev->info->irqcontrol)
+		return -ENOSYS;
+
+	retval = idev->info->irqcontrol(idev->info, irq_on);
+
+	return retval ? retval : sizeof(s32);
+}
+
 static int uio_find_mem_index(struct vm_area_struct *vma)
 {
 	int mi;
@@ -539,6 +564,7 @@
 	.open		= uio_open,
 	.release	= uio_release,
 	.read		= uio_read,
+	.write		= uio_write,
 	.mmap		= uio_mmap,
 	.poll		= uio_poll,
 	.fasync		= uio_fasync,
Index: linux-2.6.26-rc/Documentation/DocBook/uio-howto.tmpl
===================================================================
--- linux-2.6.26-rc.orig/Documentation/DocBook/uio-howto.tmpl	2008-05-22 20:22:57.000000000 +0200
+++ linux-2.6.26-rc/Documentation/DocBook/uio-howto.tmpl	2008-05-22 20:23:12.000000000 +0200
@@ -30,6 +30,12 @@
 
 <revhistory>
 	<revision>
+	<revnumber>0.5</revnumber>
+	<date>2008-05-22</date>
+	<authorinitials>hjk</authorinitials>
+	<revremark>Added description of write() function.</revremark>
+	</revision>
+	<revision>
 	<revnumber>0.4</revnumber>
 	<date>2007-11-26</date>
 	<authorinitials>hjk</authorinitials>
@@ -64,7 +70,7 @@
 <?dbhtml filename="copyright.html"?>
 <title>Copyright and License</title>
 <para>
-      Copyright (c) 2006 by Hans-JÃŒrgen Koch.</para>
+      Copyright (c) 2006-2008 by Hans-JÃŒrgen Koch.</para>
 <para>
 This documentation is Free Software licensed under the terms of the
 GPL version 2.
@@ -189,6 +195,30 @@
 	represents the total interrupt count. You can use this number
 	to figure out if you missed some interrupts.
 	</para>
+	<para>
+	For some hardware that has more than one interrupt source internally,
+	but not seperate IRQ mask and status registers, there might be
+	situations where userspace cannot determine what the interrupt source
+	was if the kernel handler disables them by writing to the chip's IRQ
+	register. In such a case, the kernel has to disable the IRQ completely
+	to leave the chip's register untouched. Now the userspace part can
+	determine the cause of the interrupt, but it cannot re-enable
+	interrupts. Another cornercase are chips where re-enabling interrupts
+	is a read-modify-write operation to a combined IRQ status/acknowledge
+	register. This would be racy if a new interrupt occured
+	simultaneously.
+	</para>
+	<para>
+	To address these problems, UIO also implements a write() function. It
+	is normally not used and can be ignored for hardware that has only a
+	single interrupt source or has seperate IRQ mask and status registers.
+	If you need it, however, a write to <filename>/dev/uioX</filename>
+	will call the <function>irqcontrol()</function> function implemented
+	by the driver. You have to write a 32-bit value that is usually either
+	0 or 1 do disable or enable interrupts. If a driver does not implement
+	<function>irqcontrol()</function>, <function>write()</function> will
+	return with <varname>-ENOSYS</varname>.
+	</para>
 
 	<para>
 	To handle interrupts properly, your custom kernel module can
@@ -362,6 +392,14 @@
 <function>open()</function>, you will probably also want a custom
 <function>release()</function> function.
 </para></listitem>
+
+<listitem><para>
+<varname>int (*irqcontrol)(struct uio_info *info, s32 irq_on)
+</varname>: Optional. If you need to be able to enable or disable
+interrupts from userspace by writing to <filename>/dev/uioX</filename>,
+you can implement this function. The parameter <varname>irq_on</varname>
+will be 0 to disable interrupts and 1 to enable them.
+</para></listitem>
 </itemizedlist>
 
 <para>

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

[Tom Spink]

2008/5/22 Hans J. Koch <hjk@linutronix.de>:
> Sometimes it is necessary to enable/disable the interrupt of a UIO device
> from the userspace part of the driver. With this patch, the UIO kernel driver
> can implement an "irqcontrol()" function that does this. Userspace can write
> an s32 value to /dev/uioX (usually 0 or 1 to turn the irq off or on). The
> UIO core will then call the driver's irqcontrol function.
>
> Signed-off-by: Hans J. Koch <hjk@linutronix.de>

<snip>

Hi,

I wonder if it would be better to implement this as an ioctl, rather
than a write to the device.  Writing to a device is a pretty generic
thing, and this patch would tie that up to specifically controlling
interrupts.  An ioctl would be more appropriate, IMO, as you are
issuing a controlling command, i.e. disable or enable interrupts.

By the way, I have absolutely no idea how the UIO driver works, other
than reading http://lwn.net/Articles/232575/

-- 
Tom Spink

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

[Hans J. Koch]

On Thu, May 22, 2008 at 08:47:16PM +0100, Tom Spink wrote:

Hi Tom,

> 2008/5/22 Hans J. Koch <hjk@linutronix.de>:
> > Sometimes it is necessary to enable/disable the interrupt of a UIO device
> > from the userspace part of the driver. With this patch, the UIO kernel driver
> > can implement an "irqcontrol()" function that does this. Userspace can write
> > an s32 value to /dev/uioX (usually 0 or 1 to turn the irq off or on). The
> > UIO core will then call the driver's irqcontrol function.
> >
> > Signed-off-by: Hans J. Koch <hjk@linutronix.de>
> 
> <snip>
> 
> Hi,
> 
> I wonder if it would be better to implement this as an ioctl,

No way. We don't want to introduce new ioctls.

> rather
> than a write to the device.  Writing to a device is a pretty generic
> thing, and this patch would tie that up to specifically controlling
> interrupts.  

UIO userspace drivers do their whole work by accessing the device's
memory directly. The purpose of the kernel part is mainly

1) allow this memory to be mapped
2) handle interrupts

We have an mmap() implementation for 1) and a read() implementation to
wait for interrupts. Now we add write to enable/disable interrupts,
which completes 2). Looks clean to me.

> An ioctl would be more appropriate, IMO, as you are
> issuing a controlling command, i.e. disable or enable interrupts.
> 
> By the way, I have absolutely no idea how the UIO driver works, other
> than reading http://lwn.net/Articles/232575/

You could read the docs that come with the kernel sources:
Documentation/DocBook/uio_howto

Thanks,
Hans

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

[Tom Spink]

2008/5/22 Hans J. Koch <hjk@linutronix.de>:
> On Thu, May 22, 2008 at 08:47:16PM +0100, Tom Spink wrote:
>
> Hi Tom,
>
>> 2008/5/22 Hans J. Koch <hjk@linutronix.de>:
>> > Sometimes it is necessary to enable/disable the interrupt of a UIO device
>> > from the userspace part of the driver. With this patch, the UIO kernel driver
>> > can implement an "irqcontrol()" function that does this. Userspace can write
>> > an s32 value to /dev/uioX (usually 0 or 1 to turn the irq off or on). The
>> > UIO core will then call the driver's irqcontrol function.
>> >
>> > Signed-off-by: Hans J. Koch <hjk@linutronix.de>
>>
>> <snip>
>>
>> Hi,
>>
>> I wonder if it would be better to implement this as an ioctl,
>
> No way. We don't want to introduce new ioctls.

Why not?  A typical usage scenario would then be:

fd = open("/dev/uio0", O_RDONLY);
...
ioctl(fd, UIO_IOC_SETIRQ, 0);
...
ioctl(fd, UIO_IOC_SETIRQ, 1);
...
close(fd);

The added benefit is that the code becomes less complex, as you don't
have to check buffer sizes and copy the integer from userspace.  You
just implement an ioctl handle on the uio char device, reducing the
code to something like this (not compiled or tested in any way, and
missing the definition of UIO_IOC_SETIRQ):

diff --git a/drivers/uio/uio.c b/drivers/uio/uio.c
index 55cc7b8..9edc7c0 100644
--- a/drivers/uio/uio.c
+++ b/drivers/uio/uio.c
@@ -534,11 +534,31 @@ static int uio_mmap(struct file *filep, struct
vm_area_struct *vma)
        }
 }

+static int uio_ioctl(struct inode *inode, struct file *filp, unsigned
int cmd, unsigned long arg)
+{
+       struct uio_listener *listener = filp->private_data;
+       struct uio_device *idev = listener->dev;
+
+       switch (cmd) {
+       case UIO_IOC_SETIRQ:
+               if (idev->info->irq == UIO_IRQ_NONE)
+                       return -EIO;
+
+               if (idev->info->irqcontrol)
+                       return idev->info->irqcontrol(idev->info, arg);
+               else
+                       return -ENOSYS;
+       }
+
+       return -ENOTTY;
+}
+
 static const struct file_operations uio_fops = {
        .owner          = THIS_MODULE,
        .open           = uio_open,
        .release        = uio_release,
        .read           = uio_read,
+       .ioctl          = uio_ioctl,
        .mmap           = uio_mmap,
        .poll           = uio_poll,
        .fasync         = uio_fasync,

>
>> rather
>> than a write to the device.  Writing to a device is a pretty generic
>> thing, and this patch would tie that up to specifically controlling
>> interrupts.
>
> UIO userspace drivers do their whole work by accessing the device's
> memory directly. The purpose of the kernel part is mainly
>
> 1) allow this memory to be mapped
> 2) handle interrupts
>
> We have an mmap() implementation for 1) and a read() implementation to
> wait for interrupts. Now we add write to enable/disable interrupts,
> which completes 2). Looks clean to me.

Okay, I understand that.  Fair point, but what happens if later write
needs to do something else?

>
>> An ioctl would be more appropriate, IMO, as you are
>> issuing a controlling command, i.e. disable or enable interrupts.
>>
>> By the way, I have absolutely no idea how the UIO driver works, other
>> than reading http://lwn.net/Articles/232575/
>
> You could read the docs that come with the kernel sources:
> Documentation/DocBook/uio_howto

Thanks!  That document certainly helps.

> Thanks,
> Hans

-- 
Tom Spink

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

[Uwe Kleine-König]

Hello Tom,

Tom Spink wrote:
> The added benefit is that the code becomes less complex, as you don't
> have to check buffer sizes and copy the integer from userspace.
AFAIK this is wrong.  You need to copy the integer from userspace in
uio_ioctl.  Actually it's a value coming from user space, so you need to
do it somewhere.

Best regards
Uwe

-- 
Uwe Kleine-König, Software Engineer
Digi International GmbH Branch Breisach, Küferstrasse 8, 79206 Breisach, Germany
Tax: 315/5781/0242 / VAT: DE153662976 / Reg. Amtsgericht Dortmund HRB 13962

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

[Uwe Kleine-König]

Hello Hans,

> -      Copyright (c) 2006 by Hans-JÃŒrgen Koch.</para>
> +      Copyright (c) 2006-2008 by Hans-JÃŒrgen Koch.</para>
This looks wrong.

	ukleinek@zentaur:~$ echo Hans-JÃŒrgen | iconv -t ISO8859-15
	Hans-Jürgen

Best regards
Uwe

-- 
Uwe Kleine-König, Software Engineer
Digi International GmbH Branch Breisach, Küferstrasse 8, 79206 Breisach, Germany
Tax: 315/5781/0242 / VAT: DE153662976 / Reg. Amtsgericht Dortmund HRB 13962

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

[Hans J. Koch]

Am Fri, 23 May 2008 07:55:27 +0200
schrieb Uwe Kleine-König <Uwe.Kleine-Koenig@digi.com>:

> Hello Hans,
> 
> > -      Copyright (c) 2006 by Hans-JÃŒrgen Koch.</para>
> > +      Copyright (c) 2006-2008 by Hans-JÃŒrgen Koch.</para>
> This looks wrong.

DocBook XML is supposed to be UTF-8.

Thanks,
Hans

> 
> 	ukleinek@zentaur:~$ echo Hans-JÃŒrgen | iconv -t ISO8859-15
> 	Hans-Jürgen
> 
> Best regards
> Uwe
> 

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

[Hans J. Koch]

Am Fri, 23 May 2008 07:41:15 +0200
schrieb Uwe Kleine-König <Uwe.Kleine-Koenig@digi.com>:

> Hello Tom,
> 
> Tom Spink wrote:
> > The added benefit is that the code becomes less complex, as you
> > don't have to check buffer sizes and copy the integer from
> > userspace.
> AFAIK this is wrong.  You need to copy the integer from userspace in
> uio_ioctl.  Actually it's a value coming from user space, so you need
> to do it somewhere.

True. Also note that this is not type-safe. All ioctl calls blindly
trust userspace to pass in correct data. This has to be tolerated for
ancient well-known filesystem ioctls, because you'd break almost all of
userspace if you changed that, but we certainly don't want to add new
stuff to this mess.

Thanks,
Hans

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

[Uwe Kleine-König]

Hans J. Koch wrote:
> Am Fri, 23 May 2008 07:55:27 +0200
> schrieb Uwe Kleine-König <Uwe.Kleine-Koenig@digi.com>:
> 
> > Hello Hans,
> > 
> > > -      Copyright (c) 2006 by Hans-JÃŒrgen Koch.</para>
> > > +      Copyright (c) 2006-2008 by Hans-JÃŒrgen Koch.</para>
> > This looks wrong.
> 
> DocBook XML is supposed to be UTF-8.
Correct.  But still the problem is real.  The headers of your mail claim
the content to be encoded in UTF-8, but actually it's latin1.  So I
cannot apply the patch you sent with git am without hand editing.

Best regards
Uwe

-- 
Uwe Kleine-König, Software Engineer
Digi International GmbH Branch Breisach, Küferstrasse 8, 79206 Breisach, Germany
Tax: 315/5781/0242 / VAT: DE153662976 / Reg. Amtsgericht Dortmund HRB 13962

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

[Hans J. Koch]

Am Fri, 23 May 2008 11:10:09 +0200
schrieb Uwe Kleine-König <Uwe.Kleine-Koenig@digi.com>:

> Hans J. Koch wrote:
> > Am Fri, 23 May 2008 07:55:27 +0200
> > schrieb Uwe Kleine-König <Uwe.Kleine-Koenig@digi.com>:
> > 
> > > Hello Hans,
> > > 
> > > > -      Copyright (c) 2006 by Hans-JÃŒrgen Koch.</para>
> > > > +      Copyright (c) 2006-2008 by Hans-JÃŒrgen Koch.</para>
> > > This looks wrong.
> > 
> > DocBook XML is supposed to be UTF-8.
> Correct.  But still the problem is real.  The headers of your mail
> claim the content to be encoded in UTF-8, but actually it's latin1.
> So I cannot apply the patch you sent with git am without hand editing.

Try the version below.

Thanks,
Hans

> 
> Best regards
> Uwe
> 
From: Hans J. Koch <hjk@linutronix.de>
To: linux-kernel@vger.kernel.org
Cc: Greg Kroah-Hartman <gregkh@suse.de>,
	Jan Altenberg <jan.altenberg@linutronix.de>,
	Thomas Gleixner <tglx@linutronix.de>,
	Uwe Kleine-König <Uwe.Kleine-Koenig@digi.com>,
	Magnus Damm <magnus.damm@gmail.com>
Subject: UIO: Add write function to allow irq masking
Date: Thu, 22 May 2008 20:43:09 +0200

Sometimes it is necessary to enable/disable the interrupt of a UIO device
from the userspace part of the driver. With this patch, the UIO kernel driver
can implement an "irqcontrol()" function that does this. Userspace can write
an s32 value to /dev/uioX (usually 0 or 1 to turn the irq off or on). The
UIO core will then call the driver's irqcontrol function.

Signed-off-by: Hans J. Koch <hjk@linutronix.de>
---
 Documentation/DocBook/uio-howto.tmpl |   40 ++++++++++++++++++++++++++++++++++-
 drivers/uio/uio.c                    |   26 ++++++++++++++++++++++
 include/linux/uio_driver.h           |    2 +
 3 files changed, 67 insertions(+), 1 deletion(-)

Index: linux-2.6.26-rc/include/linux/uio_driver.h
===================================================================
--- linux-2.6.26-rc.orig/include/linux/uio_driver.h	2008-05-22 20:22:57.000000000 +0200
+++ linux-2.6.26-rc/include/linux/uio_driver.h	2008-05-22 20:23:12.000000000 +0200
@@ -53,6 +53,7 @@
  * @mmap:		mmap operation for this uio device
  * @open:		open operation for this uio device
  * @release:		release operation for this uio device
+ * @irqcontrol:		disable/enable irqs when 0/1 is written to /dev/uioX
  */
 struct uio_info {
 	struct uio_device	*uio_dev;
@@ -66,6 +67,7 @@
 	int (*mmap)(struct uio_info *info, struct vm_area_struct *vma);
 	int (*open)(struct uio_info *info, struct inode *inode);
 	int (*release)(struct uio_info *info, struct inode *inode);
+	int (*irqcontrol)(struct uio_info *info, s32 irq_on);
 };
 
 extern int __must_check
Index: linux-2.6.26-rc/drivers/uio/uio.c
===================================================================
--- linux-2.6.26-rc.orig/drivers/uio/uio.c	2008-05-22 20:23:07.000000000 +0200
+++ linux-2.6.26-rc/drivers/uio/uio.c	2008-05-22 20:23:12.000000000 +0200
@@ -420,6 +420,31 @@
 	return retval;
 }
 
+static ssize_t uio_write(struct file *filep, const char __user *buf,
+			size_t count, loff_t *ppos)
+{
+	struct uio_listener *listener = filep->private_data;
+	struct uio_device *idev = listener->dev;
+	ssize_t retval;
+	s32 irq_on;
+
+	if (idev->info->irq == UIO_IRQ_NONE)
+		return -EIO;
+
+	if (count != sizeof(s32))
+		return -EINVAL;
+
+	if (copy_from_user(&irq_on, buf, count))
+		return -EFAULT;
+
+	if (!idev->info->irqcontrol)
+		return -ENOSYS;
+
+	retval = idev->info->irqcontrol(idev->info, irq_on);
+
+	return retval ? retval : sizeof(s32);
+}
+
 static int uio_find_mem_index(struct vm_area_struct *vma)
 {
 	int mi;
@@ -539,6 +564,7 @@
 	.open		= uio_open,
 	.release	= uio_release,
 	.read		= uio_read,
+	.write		= uio_write,
 	.mmap		= uio_mmap,
 	.poll		= uio_poll,
 	.fasync		= uio_fasync,
Index: linux-2.6.26-rc/Documentation/DocBook/uio-howto.tmpl
===================================================================
--- linux-2.6.26-rc.orig/Documentation/DocBook/uio-howto.tmpl	2008-05-22 20:22:57.000000000 +0200
+++ linux-2.6.26-rc/Documentation/DocBook/uio-howto.tmpl	2008-05-23 11:57:23.000000000 +0200
@@ -30,6 +30,12 @@
 
 <revhistory>
 	<revision>
+	<revnumber>0.5</revnumber>
+	<date>2008-05-22</date>
+	<authorinitials>hjk</authorinitials>
+	<revremark>Added description of write() function.</revremark>
+	</revision>
+	<revision>
 	<revnumber>0.4</revnumber>
 	<date>2007-11-26</date>
 	<authorinitials>hjk</authorinitials>
@@ -64,7 +70,7 @@
 <?dbhtml filename="copyright.html"?>
 <title>Copyright and License</title>
 <para>
-      Copyright (c) 2006 by Hans-Jürgen Koch.</para>
+      Copyright (c) 2006-2008 by Hans-Jürgen Koch.</para>
 <para>
 This documentation is Free Software licensed under the terms of the
 GPL version 2.
@@ -189,6 +195,30 @@
 	represents the total interrupt count. You can use this number
 	to figure out if you missed some interrupts.
 	</para>
+	<para>
+	For some hardware that has more than one interrupt source internally,
+	but not seperate IRQ mask and status registers, there might be
+	situations where userspace cannot determine what the interrupt source
+	was if the kernel handler disables them by writing to the chip's IRQ
+	register. In such a case, the kernel has to disable the IRQ completely
+	to leave the chip's register untouched. Now the userspace part can
+	determine the cause of the interrupt, but it cannot re-enable
+	interrupts. Another cornercase are chips where re-enabling interrupts
+	is a read-modify-write operation to a combined IRQ status/acknowledge
+	register. This would be racy if a new interrupt occured
+	simultaneously.
+	</para>
+	<para>
+	To address these problems, UIO also implements a write() function. It
+	is normally not used and can be ignored for hardware that has only a
+	single interrupt source or has seperate IRQ mask and status registers.
+	If you need it, however, a write to <filename>/dev/uioX</filename>
+	will call the <function>irqcontrol()</function> function implemented
+	by the driver. You have to write a 32-bit value that is usually either
+	0 or 1 do disable or enable interrupts. If a driver does not implement
+	<function>irqcontrol()</function>, <function>write()</function> will
+	return with <varname>-ENOSYS</varname>.
+	</para>
 
 	<para>
 	To handle interrupts properly, your custom kernel module can
@@ -362,6 +392,14 @@
 <function>open()</function>, you will probably also want a custom
 <function>release()</function> function.
 </para></listitem>
+
+<listitem><para>
+<varname>int (*irqcontrol)(struct uio_info *info, s32 irq_on)
+</varname>: Optional. If you need to be able to enable or disable
+interrupts from userspace by writing to <filename>/dev/uioX</filename>,
+you can implement this function. The parameter <varname>irq_on</varname>
+will be 0 to disable interrupts and 1 to enable them.
+</para></listitem>
 </itemizedlist>
 
 <para>

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

[Uwe Kleine-König]

Hello Hans,

Hans J. Koch wrote:
> Am Fri, 23 May 2008 11:10:09 +0200
> schrieb Uwe Kleine-König <Uwe.Kleine-Koenig@digi.com>:
> 
> > Hans J. Koch wrote:
> > > Am Fri, 23 May 2008 07:55:27 +0200
> > > schrieb Uwe Kleine-König <Uwe.Kleine-Koenig@digi.com>:
> > > 
> > > > Hello Hans,
> > > > 
> > > > > -      Copyright (c) 2006 by Hans-JÃŒrgen Koch.</para>
> > > > > +      Copyright (c) 2006-2008 by Hans-JÃŒrgen Koch.</para>
> > > > This looks wrong.
> > > 
> > > DocBook XML is supposed to be UTF-8.
> > Correct.  But still the problem is real.  The headers of your mail
> > claim the content to be encoded in UTF-8, but actually it's latin1.
> > So I cannot apply the patch you sent with git am without hand editing.
> 
> Try the version below.
This one is better---I can apply it.

> +	if (copy_from_user(&irq_on, buf, count))
> +		return -EFAULT;
> +
> +	if (!idev->info->irqcontrol)
> +		return -ENOSYS;
I would swap these two.  copy_from_user is more expensive than testing
idev->info->irqcontrol.  (Is it really?)  Anyhow only fetching a value
from userspace if you really need it looks more clean to me.

Otherwise the patch looks fine.

Uwe


-- 
Uwe Kleine-König, Software Engineer
Digi International GmbH Branch Breisach, Küferstrasse 8, 79206 Breisach, Germany
Tax: 315/5781/0242 / VAT: DE153662976 / Reg. Amtsgericht Dortmund HRB 13962

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

[Tom Spink]

2008/5/23 Uwe Kleine-König <Uwe.Kleine-Koenig@digi.com>:
> Hello Tom,

Hi Uwe,

> Tom Spink wrote:
>> The added benefit is that the code becomes less complex, as you don't
>> have to check buffer sizes and copy the integer from userspace.
> AFAIK this is wrong.  You need to copy the integer from userspace in
> uio_ioctl.  Actually it's a value coming from user space, so you need to
> do it somewhere.

Not really in this case.  It's not a *pointer* to a value in
userspace, so you don't need to copy anything.  If it was being used
to point to a memory location holding a value, then yes, it would need
to be copied across.  But in this case, it's just being used to pass
across 1 or 0.

For example, look at the drivers/char/tty_ioctl.c file, and see how
they use arg as a simple value.  arg doesn't have to be a pointer to
userspace.

> Best regards
> Uwe

-- 
Tom Spink

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

[Hans J. Koch]

Am Fri, 23 May 2008 12:56:04 +0200
schrieb Uwe Kleine-König <Uwe.Kleine-Koenig@digi.com>:

> Hello Hans,
> 
> Hans J. Koch wrote:
> > Am Fri, 23 May 2008 11:10:09 +0200
> > schrieb Uwe Kleine-König <Uwe.Kleine-Koenig@digi.com>:
> > 
> > > Hans J. Koch wrote:
> > > > Am Fri, 23 May 2008 07:55:27 +0200
> > > > schrieb Uwe Kleine-König <Uwe.Kleine-Koenig@digi.com>:
> > > > 
> > > > > Hello Hans,
> > > > > 
> > > > > > -      Copyright (c) 2006 by Hans-JÃŒrgen Koch.</para>
> > > > > > +      Copyright (c) 2006-2008 by Hans-JÃŒrgen Koch.</para>
> > > > > This looks wrong.
> > > > 
> > > > DocBook XML is supposed to be UTF-8.
> > > Correct.  But still the problem is real.  The headers of your mail
> > > claim the content to be encoded in UTF-8, but actually it's
> > > latin1. So I cannot apply the patch you sent with git am without
> > > hand editing.
> > 
> > Try the version below.
> This one is better---I can apply it.

OK. Was a problem with my mail client.

> 
> > +	if (copy_from_user(&irq_on, buf, count))
> > +		return -EFAULT;
> > +
> > +	if (!idev->info->irqcontrol)
> > +		return -ENOSYS;
> I would swap these two.  copy_from_user is more expensive than testing
> idev->info->irqcontrol.  (Is it really?)  Anyhow only fetching a value
> from userspace if you really need it looks more clean to me.

Agreed. See updated version below.

> 
> Otherwise the patch looks fine.

Thanks for your review! Could you add your Signed-off-by?

Hans

From: Hans J. Koch <hjk@linutronix.de>
To: linux-kernel@vger.kernel.org
Cc: Greg Kroah-Hartman <gregkh@suse.de>,
	Jan Altenberg <jan.altenberg@linutronix.de>,
	Thomas Gleixner <tglx@linutronix.de>,
	Uwe Kleine-König <Uwe.Kleine-Koenig@digi.com>,
	Magnus Damm <magnus.damm@gmail.com>
Subject: UIO: Add write function to allow irq masking
Date: Thu, Fri, 23 May 2008 13:50:14 +0200

Sometimes it is necessary to enable/disable the interrupt of a UIO device
from the userspace part of the driver. With this patch, the UIO kernel driver
can implement an "irqcontrol()" function that does this. Userspace can write
an s32 value to /dev/uioX (usually 0 or 1 to turn the irq off or on). The
UIO core will then call the driver's irqcontrol function.

Signed-off-by: Hans J. Koch <hjk@linutronix.de>
---
 Documentation/DocBook/uio-howto.tmpl |   40 ++++++++++++++++++++++++++++++++++-
 drivers/uio/uio.c                    |   26 ++++++++++++++++++++++
 include/linux/uio_driver.h           |    2 +
 3 files changed, 67 insertions(+), 1 deletion(-)

Index: linux-2.6.26-rc/include/linux/uio_driver.h
===================================================================
--- linux-2.6.26-rc.orig/include/linux/uio_driver.h	2008-05-22 20:22:57.000000000 +0200
+++ linux-2.6.26-rc/include/linux/uio_driver.h	2008-05-22 20:23:12.000000000 +0200
@@ -53,6 +53,7 @@
  * @mmap:		mmap operation for this uio device
  * @open:		open operation for this uio device
  * @release:		release operation for this uio device
+ * @irqcontrol:		disable/enable irqs when 0/1 is written to /dev/uioX
  */
 struct uio_info {
 	struct uio_device	*uio_dev;
@@ -66,6 +67,7 @@
 	int (*mmap)(struct uio_info *info, struct vm_area_struct *vma);
 	int (*open)(struct uio_info *info, struct inode *inode);
 	int (*release)(struct uio_info *info, struct inode *inode);
+	int (*irqcontrol)(struct uio_info *info, s32 irq_on);
 };
 
 extern int __must_check
Index: linux-2.6.26-rc/drivers/uio/uio.c
===================================================================
--- linux-2.6.26-rc.orig/drivers/uio/uio.c	2008-05-22 20:23:07.000000000 +0200
+++ linux-2.6.26-rc/drivers/uio/uio.c	2008-05-23 13:49:40.000000000 +0200
@@ -420,6 +420,31 @@
 	return retval;
 }
 
+static ssize_t uio_write(struct file *filep, const char __user *buf,
+			size_t count, loff_t *ppos)
+{
+	struct uio_listener *listener = filep->private_data;
+	struct uio_device *idev = listener->dev;
+	ssize_t retval;
+	s32 irq_on;
+
+	if (idev->info->irq == UIO_IRQ_NONE)
+		return -EIO;
+
+	if (count != sizeof(s32))
+		return -EINVAL;
+
+	if (!idev->info->irqcontrol)
+		return -ENOSYS;
+
+	if (copy_from_user(&irq_on, buf, count))
+		return -EFAULT;
+
+	retval = idev->info->irqcontrol(idev->info, irq_on);
+
+	return retval ? retval : sizeof(s32);
+}
+
 static int uio_find_mem_index(struct vm_area_struct *vma)
 {
 	int mi;
@@ -539,6 +564,7 @@
 	.open		= uio_open,
 	.release	= uio_release,
 	.read		= uio_read,
+	.write		= uio_write,
 	.mmap		= uio_mmap,
 	.poll		= uio_poll,
 	.fasync		= uio_fasync,
Index: linux-2.6.26-rc/Documentation/DocBook/uio-howto.tmpl
===================================================================
--- linux-2.6.26-rc.orig/Documentation/DocBook/uio-howto.tmpl	2008-05-22 20:22:57.000000000 +0200
+++ linux-2.6.26-rc/Documentation/DocBook/uio-howto.tmpl	2008-05-23 11:57:23.000000000 +0200
@@ -30,6 +30,12 @@
 
 <revhistory>
 	<revision>
+	<revnumber>0.5</revnumber>
+	<date>2008-05-22</date>
+	<authorinitials>hjk</authorinitials>
+	<revremark>Added description of write() function.</revremark>
+	</revision>
+	<revision>
 	<revnumber>0.4</revnumber>
 	<date>2007-11-26</date>
 	<authorinitials>hjk</authorinitials>
@@ -64,7 +70,7 @@
 <?dbhtml filename="copyright.html"?>
 <title>Copyright and License</title>
 <para>
-      Copyright (c) 2006 by Hans-Jürgen Koch.</para>
+      Copyright (c) 2006-2008 by Hans-Jürgen Koch.</para>
 <para>
 This documentation is Free Software licensed under the terms of the
 GPL version 2.
@@ -189,6 +195,30 @@
 	represents the total interrupt count. You can use this number
 	to figure out if you missed some interrupts.
 	</para>
+	<para>
+	For some hardware that has more than one interrupt source internally,
+	but not seperate IRQ mask and status registers, there might be
+	situations where userspace cannot determine what the interrupt source
+	was if the kernel handler disables them by writing to the chip's IRQ
+	register. In such a case, the kernel has to disable the IRQ completely
+	to leave the chip's register untouched. Now the userspace part can
+	determine the cause of the interrupt, but it cannot re-enable
+	interrupts. Another cornercase are chips where re-enabling interrupts
+	is a read-modify-write operation to a combined IRQ status/acknowledge
+	register. This would be racy if a new interrupt occured
+	simultaneously.
+	</para>
+	<para>
+	To address these problems, UIO also implements a write() function. It
+	is normally not used and can be ignored for hardware that has only a
+	single interrupt source or has seperate IRQ mask and status registers.
+	If you need it, however, a write to <filename>/dev/uioX</filename>
+	will call the <function>irqcontrol()</function> function implemented
+	by the driver. You have to write a 32-bit value that is usually either
+	0 or 1 do disable or enable interrupts. If a driver does not implement
+	<function>irqcontrol()</function>, <function>write()</function> will
+	return with <varname>-ENOSYS</varname>.
+	</para>
 
 	<para>
 	To handle interrupts properly, your custom kernel module can
@@ -362,6 +392,14 @@
 <function>open()</function>, you will probably also want a custom
 <function>release()</function> function.
 </para></listitem>
+
+<listitem><para>
+<varname>int (*irqcontrol)(struct uio_info *info, s32 irq_on)
+</varname>: Optional. If you need to be able to enable or disable
+interrupts from userspace by writing to <filename>/dev/uioX</filename>,
+you can implement this function. The parameter <varname>irq_on</varname>
+will be 0 to disable interrupts and 1 to enable them.
+</para></listitem>
 </itemizedlist>
 
 <para>

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

[Uwe Kleine-König]

Hello Tom,

> > Tom Spink wrote:
> >> The added benefit is that the code becomes less complex, as you don't
> >> have to check buffer sizes and copy the integer from userspace.
> > AFAIK this is wrong.  You need to copy the integer from userspace in
> > uio_ioctl.  Actually it's a value coming from user space, so you need to
> > do it somewhere.
> 
> Not really in this case.  It's not a *pointer* to a value in
> userspace, so you don't need to copy anything.  If it was being used
> to point to a memory location holding a value, then yes, it would need
> to be copied across.  But in this case, it's just being used to pass
> across 1 or 0.
ah, OK, you're right.  Thanks for correcting my correction :-)

Uwe

-- 
Uwe Kleine-König, Software Engineer
Digi International GmbH Branch Breisach, Küferstrasse 8, 79206 Breisach, Germany
Tax: 315/5781/0242 / VAT: DE153662976 / Reg. Amtsgericht Dortmund HRB 13962

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

[Tom Spink]

2008/5/23 Uwe Kleine-König <Uwe.Kleine-Koenig@digi.com>:
> Hello Tom,
>
>> > Tom Spink wrote:
>> >> The added benefit is that the code becomes less complex, as you don't
>> >> have to check buffer sizes and copy the integer from userspace.
>> > AFAIK this is wrong.  You need to copy the integer from userspace in
>> > uio_ioctl.  Actually it's a value coming from user space, so you need to
>> > do it somewhere.
>>
>> Not really in this case.  It's not a *pointer* to a value in
>> userspace, so you don't need to copy anything.  If it was being used
>> to point to a memory location holding a value, then yes, it would need
>> to be copied across.  But in this case, it's just being used to pass
>> across 1 or 0.
> ah, OK, you're right.  Thanks for correcting my correction :-)

Heh, no problem.  My initial idea was just a thought anyway, just to
maintain a bit of extensibility if .write is ever needed for something
else. :-)

> Uwe

-- 
Tom Spink

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

[Uwe Kleine-König]

Hello Hans,

> > Otherwise the patch looks fine.
> 
> Thanks for your review! Could you add your Signed-off-by?
I don't consider my feed back valueable enough for a Signed-off-by, but
you can get an ...

> From: Hans J. Koch <hjk@linutronix.de>
> To: linux-kernel@vger.kernel.org
> Cc: Greg Kroah-Hartman <gregkh@suse.de>,
> 	Jan Altenberg <jan.altenberg@linutronix.de>,
> 	Thomas Gleixner <tglx@linutronix.de>,
> 	Uwe Kleine-König <Uwe.Kleine-Koenig@digi.com>,
> 	Magnus Damm <magnus.damm@gmail.com>
> Subject: UIO: Add write function to allow irq masking
> Date: Thu, Fri, 23 May 2008 13:50:14 +0200
> 
> Sometimes it is necessary to enable/disable the interrupt of a UIO device
> from the userspace part of the driver. With this patch, the UIO kernel driver
> can implement an "irqcontrol()" function that does this. Userspace can write
> an s32 value to /dev/uioX (usually 0 or 1 to turn the irq off or on). The
> UIO core will then call the driver's irqcontrol function.
> 
> Signed-off-by: Hans J. Koch <hjk@linutronix.de>
Acked-by: Uwe Kleine-König <Uwe.Kleine-Koenig@digi.com>,

Best regards
Uwe

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

[Hans J. Koch]

Am Fri, 23 May 2008 13:00:17 +0100
schrieb "Tom Spink" <tspink@gmail.com>:

> My initial idea was just a thought anyway, just to
> maintain a bit of extensibility if .write is ever needed for something
> else. :-)

Hi Tom,
thanks for your contribution, but for me it's just the other way round:
I'm glad write() gets a defined purpose before people do something
stupid with it. It's good to remember that all data exchange with the
device has to be done through the mapped memory. If this is not
possible, the hardware is no candidate for a UIO driver.

BTW, I wait for the first UIO driver which abuses this write()
function to write many different values to trigger different actions.
I wonder if I should restrict write() to the value 0 and 1...

Thanks,
Hans
 

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

[Tom Spink]

2008/5/23 Hans J. Koch <hjk@linutronix.de>:
> Am Fri, 23 May 2008 13:00:17 +0100
> schrieb "Tom Spink" <tspink@gmail.com>:
>
>> My initial idea was just a thought anyway, just to
>> maintain a bit of extensibility if .write is ever needed for something
>> else. :-)
>
> Hi Tom,
> thanks for your contribution, but for me it's just the other way round:
> I'm glad write() gets a defined purpose before people do something
> stupid with it. It's good to remember that all data exchange with the
> device has to be done through the mapped memory. If this is not
> possible, the hardware is no candidate for a UIO driver.
>
> BTW, I wait for the first UIO driver which abuses this write()
> function to write many different values to trigger different actions.
> I wonder if I should restrict write() to the value 0 and 1...
>
> Thanks,
> Hans

Hi Hans,

Thanks for your explanation.  Another thing, I noticed then, is that
in your return statement, you blindly return the the value of
irqcontrol if it's non-zero, and if it's zero, then the length of the
data written.  However, if irqcontrol returns a value that's > 0, it
could potentially confuse writers.  I guess it's up to the implementer
of irqcontrol to ensure they stick to -EXXX and 0, but it's just a
thought (while you were on the subject of input validation!)

-- 
Tom Spink

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

[Hans J. Koch]

Am Fri, 23 May 2008 13:20:50 +0100
schrieb "Tom Spink" <tspink@gmail.com>:

> 2008/5/23 Hans J. Koch <hjk@linutronix.de>:
> > Am Fri, 23 May 2008 13:00:17 +0100
> > schrieb "Tom Spink" <tspink@gmail.com>:
> >
> >> My initial idea was just a thought anyway, just to
> >> maintain a bit of extensibility if .write is ever needed for
> >> something else. :-)
> >
> > Hi Tom,
> > thanks for your contribution, but for me it's just the other way
> > round: I'm glad write() gets a defined purpose before people do
> > something stupid with it. It's good to remember that all data
> > exchange with the device has to be done through the mapped memory.
> > If this is not possible, the hardware is no candidate for a UIO
> > driver.
> >
> > BTW, I wait for the first UIO driver which abuses this write()
> > function to write many different values to trigger different
> > actions. I wonder if I should restrict write() to the value 0 and
> > 1...
> >
> > Thanks,
> > Hans
> 
> Hi Hans,
> 
> Thanks for your explanation.  Another thing, I noticed then, is that
> in your return statement, you blindly return the the value of
> irqcontrol if it's non-zero, and if it's zero, then the length of the
> data written.  However, if irqcontrol returns a value that's > 0, it
> could potentially confuse writers.  I guess it's up to the implementer
> of irqcontrol to ensure they stick to -EXXX and 0

True. And it's not even worth fixing this by checking retval<0, because
if irqcontrol returns values>0, it's broken anyway. It is supposed to
return zero if successful, and negative errors otherwise. This is
common practice in the kernel, and people frequently use if (ret)... to
detect errors without checking if ret is a legal negative error number.

Thanks,
Hans

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

[Randy Dunlap]

On Fri, 23 May 2008 13:55:57 +0200 Hans J. Koch wrote:

>  Documentation/DocBook/uio-howto.tmpl |   40 ++++++++++++++++++++++++++++++++++-
>  drivers/uio/uio.c                    |   26 ++++++++++++++++++++++
>  include/linux/uio_driver.h           |    2 +
>  3 files changed, 67 insertions(+), 1 deletion(-)
> 
> Index: linux-2.6.26-rc/Documentation/DocBook/uio-howto.tmpl
> ===================================================================
> --- linux-2.6.26-rc.orig/Documentation/DocBook/uio-howto.tmpl	2008-05-22 20:22:57.000000000 +0200
> +++ linux-2.6.26-rc/Documentation/DocBook/uio-howto.tmpl	2008-05-23 11:57:23.000000000 +0200
> @@ -30,6 +30,12 @@
>  
>  <revhistory>
>  	<revision>
> +	<revnumber>0.5</revnumber>
> +	<date>2008-05-22</date>
> +	<authorinitials>hjk</authorinitials>
> +	<revremark>Added description of write() function.</revremark>
> +	</revision>
> +	<revision>
>  	<revnumber>0.4</revnumber>
>  	<date>2007-11-26</date>
>  	<authorinitials>hjk</authorinitials>
> @@ -64,7 +70,7 @@
>  <?dbhtml filename="copyright.html"?>
>  <title>Copyright and License</title>
>  <para>
> -      Copyright (c) 2006 by Hans-Jürgen Koch.</para>
> +      Copyright (c) 2006-2008 by Hans-Jürgen Koch.</para>
>  <para>
>  This documentation is Free Software licensed under the terms of the
>  GPL version 2.
> @@ -189,6 +195,30 @@
>  	represents the total interrupt count. You can use this number
>  	to figure out if you missed some interrupts.
>  	</para>
> +	<para>
> +	For some hardware that has more than one interrupt source internally,
> +	but not seperate IRQ mask and status registers, there might be

	        separate

> +	situations where userspace cannot determine what the interrupt source
> +	was if the kernel handler disables them by writing to the chip's IRQ
> +	register. In such a case, the kernel has to disable the IRQ completely
> +	to leave the chip's register untouched. Now the userspace part can
> +	determine the cause of the interrupt, but it cannot re-enable
> +	interrupts. Another cornercase are chips where re-enabling interrupts

	                               is

> +	is a read-modify-write operation to a combined IRQ status/acknowledge
> +	register. This would be racy if a new interrupt occured

	                                                occurred

> +	simultaneously.
> +	</para>
> +	<para>
> +	To address these problems, UIO also implements a write() function. It
> +	is normally not used and can be ignored for hardware that has only a
> +	single interrupt source or has seperate IRQ mask and status registers.

	                               separate
["There's <a rat> in separate."]

> +	If you need it, however, a write to <filename>/dev/uioX</filename>
> +	will call the <function>irqcontrol()</function> function implemented
> +	by the driver. You have to write a 32-bit value that is usually either
> +	0 or 1 do disable or enable interrupts. If a driver does not implement

	       to

> +	<function>irqcontrol()</function>, <function>write()</function> will
> +	return with <varname>-ENOSYS</varname>.
> +	</para>

---
~Randy

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

[Leon Woestenberg]

Hello,

On Fri, May 23, 2008 at 1:55 PM, Hans J. Koch <hjk@linutronix.de> wrote:
> +static ssize_t uio_write(struct file *filep, const char __user *buf,
> +                       size_t count, loff_t *ppos)
> +{
> +       struct uio_listener *listener = filep->private_data;
> +       struct uio_device *idev = listener->dev;
> +       ssize_t retval;
> +       s32 irq_on;
> +
> +       if (idev->info->irq == UIO_IRQ_NONE)
> +               return -EIO;
> +
> +       if (count != sizeof(s32))
> +               return -EINVAL;
> +
> +       if (!idev->info->irqcontrol)
> +               return -ENOSYS;
> +
> +       if (copy_from_user(&irq_on, buf, count))
> +               return -EFAULT;
> +
> +       retval = idev->info->irqcontrol(idev->info, irq_on);
> +
> +       return retval ? retval : sizeof(s32);
> +}
> +

Shouldn't this be more future-proof, what if we need to abuse write()
for something else in the future?

I would suggest a check for ppos to be 0 (zero) as well, just to be
sure and future-proof and backwards-safe.

Regards,
-- 
Leon

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

[Hans J. Koch]

On Fri, May 23, 2008 at 10:44:42PM +0200, Leon Woestenberg wrote:
> Hello,
> 
> On Fri, May 23, 2008 at 1:55 PM, Hans J. Koch <hjk@linutronix.de> wrote:
> > +static ssize_t uio_write(struct file *filep, const char __user *buf,
> > +                       size_t count, loff_t *ppos)
> > +{
> > +       struct uio_listener *listener = filep->private_data;
> > +       struct uio_device *idev = listener->dev;
> > +       ssize_t retval;
> > +       s32 irq_on;
> > +
> > +       if (idev->info->irq == UIO_IRQ_NONE)
> > +               return -EIO;
> > +
> > +       if (count != sizeof(s32))
> > +               return -EINVAL;
> > +
> > +       if (!idev->info->irqcontrol)
> > +               return -ENOSYS;
> > +
> > +       if (copy_from_user(&irq_on, buf, count))
> > +               return -EFAULT;
> > +
> > +       retval = idev->info->irqcontrol(idev->info, irq_on);
> > +
> > +       return retval ? retval : sizeof(s32);
> > +}
> > +
> 
> Shouldn't this be more future-proof, what if we need to abuse write()
> for something else in the future?

We don't. I'm thinking about letting the function fail if irq_on is not
0 or 1, just to stop any ideas of abusing write().

read() and write() only deal with irq handling, all data exchange with the
device is done through mapped memory.

> 
> I would suggest a check for ppos to be 0 (zero) as well, just to be
> sure and future-proof and backwards-safe.

write() is only for enabling/disabling irqs, there's only one possible
value of count, and we don't have a seek function. So why check ppos?

Thanks,
Hans

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

[Hans-Jürgen Koch]

Am Fri, 23 May 2008 11:36:50 -0700
schrieb Randy Dunlap <randy.dunlap@oracle.com>:

>
> > +	For some hardware that has more than one interrupt source internally,
> > +	but not seperate IRQ mask and status registers, there might be
> 
> 	        separate
> 
> > +	situations where userspace cannot determine what the interrupt source
> > +	was if the kernel handler disables them by writing to the chip's IRQ
> > +	register. In such a case, the kernel has to disable the IRQ completely
> > +	to leave the chip's register untouched. Now the userspace part can
> > +	determine the cause of the interrupt, but it cannot re-enable
> > +	interrupts. Another cornercase are chips where re-enabling interrupts
> 
> 	                               is
> 
> > +	is a read-modify-write operation to a combined IRQ status/acknowledge
> > +	register. This would be racy if a new interrupt occured
> 
> 	                                                occurred
> 
> > +	simultaneously.
> > +	</para>
> > +	<para>
> > +	To address these problems, UIO also implements a write() function. It
> > +	is normally not used and can be ignored for hardware that has only a
> > +	single interrupt source or has seperate IRQ mask and status registers.
> 
> 	                               separate
> ["There's <a rat> in separate."]

I'll certainly remember that one :-)

> 
> > +	If you need it, however, a write to <filename>/dev/uioX</filename>
> > +	will call the <function>irqcontrol()</function> function implemented
> > +	by the driver. You have to write a 32-bit value that is usually either
> > +	0 or 1 do disable or enable interrupts. If a driver does not implement
> 
> 	       to
> 
> > +	<function>irqcontrol()</function>, <function>write()</function> will
> > +	return with <varname>-ENOSYS</varname>.
> > +	</para>
> 
> ---
> ~Randy

Thanks a lot for reviewing this! Corrected patch below.

Hans

From: Hans J. Koch <hjk@linutronix.de>
To: linux-kernel@vger.kernel.org
Cc: Greg Kroah-Hartman <gregkh@suse.de>,
	Jan Altenberg <jan.altenberg@linutronix.de>,
	Thomas Gleixner <tglx@linutronix.de>,
	Uwe Kleine-König <Uwe.Kleine-Koenig@digi.com>,
	Magnus Damm <magnus.damm@gmail.com>
Subject: UIO: Add write function to allow irq masking
Date: Thu, Fri, 23 May 2008 13:50:14 +0200

Sometimes it is necessary to enable/disable the interrupt of a UIO device
from the userspace part of the driver. With this patch, the UIO kernel driver
can implement an "irqcontrol()" function that does this. Userspace can write
an s32 value to /dev/uioX (usually 0 or 1 to turn the irq off or on). The
UIO core will then call the driver's irqcontrol function.

Signed-off-by: Hans J. Koch <hjk@linutronix.de>
---
 Documentation/DocBook/uio-howto.tmpl |   40 ++++++++++++++++++++++++++++++++++-
 drivers/uio/uio.c                    |   26 ++++++++++++++++++++++
 include/linux/uio_driver.h           |    2 +
 3 files changed, 67 insertions(+), 1 deletion(-)

Index: linux-2.6.26-rc/include/linux/uio_driver.h
===================================================================
--- linux-2.6.26-rc.orig/include/linux/uio_driver.h	2008-05-22 20:22:57.000000000 +0200
+++ linux-2.6.26-rc/include/linux/uio_driver.h	2008-05-22 20:23:12.000000000 +0200
@@ -53,6 +53,7 @@
  * @mmap:		mmap operation for this uio device
  * @open:		open operation for this uio device
  * @release:		release operation for this uio device
+ * @irqcontrol:		disable/enable irqs when 0/1 is written to /dev/uioX
  */
 struct uio_info {
 	struct uio_device	*uio_dev;
@@ -66,6 +67,7 @@
 	int (*mmap)(struct uio_info *info, struct vm_area_struct *vma);
 	int (*open)(struct uio_info *info, struct inode *inode);
 	int (*release)(struct uio_info *info, struct inode *inode);
+	int (*irqcontrol)(struct uio_info *info, s32 irq_on);
 };
 
 extern int __must_check
Index: linux-2.6.26-rc/drivers/uio/uio.c
===================================================================
--- linux-2.6.26-rc.orig/drivers/uio/uio.c	2008-05-22 20:23:07.000000000 +0200
+++ linux-2.6.26-rc/drivers/uio/uio.c	2008-05-23 13:49:40.000000000 +0200
@@ -420,6 +420,31 @@
 	return retval;
 }
 
+static ssize_t uio_write(struct file *filep, const char __user *buf,
+			size_t count, loff_t *ppos)
+{
+	struct uio_listener *listener = filep->private_data;
+	struct uio_device *idev = listener->dev;
+	ssize_t retval;
+	s32 irq_on;
+
+	if (idev->info->irq == UIO_IRQ_NONE)
+		return -EIO;
+
+	if (count != sizeof(s32))
+		return -EINVAL;
+
+	if (!idev->info->irqcontrol)
+		return -ENOSYS;
+
+	if (copy_from_user(&irq_on, buf, count))
+		return -EFAULT;
+
+	retval = idev->info->irqcontrol(idev->info, irq_on);
+
+	return retval ? retval : sizeof(s32);
+}
+
 static int uio_find_mem_index(struct vm_area_struct *vma)
 {
 	int mi;
@@ -539,6 +564,7 @@
 	.open		= uio_open,
 	.release	= uio_release,
 	.read		= uio_read,
+	.write		= uio_write,
 	.mmap		= uio_mmap,
 	.poll		= uio_poll,
 	.fasync		= uio_fasync,
Index: linux-2.6.26-rc/Documentation/DocBook/uio-howto.tmpl
===================================================================
--- linux-2.6.26-rc.orig/Documentation/DocBook/uio-howto.tmpl	2008-05-22 20:22:57.000000000 +0200
+++ linux-2.6.26-rc/Documentation/DocBook/uio-howto.tmpl	2008-05-24 00:33:48.000000000 +0200
@@ -30,6 +30,12 @@
 
 <revhistory>
 	<revision>
+	<revnumber>0.5</revnumber>
+	<date>2008-05-22</date>
+	<authorinitials>hjk</authorinitials>
+	<revremark>Added description of write() function.</revremark>
+	</revision>
+	<revision>
 	<revnumber>0.4</revnumber>
 	<date>2007-11-26</date>
 	<authorinitials>hjk</authorinitials>
@@ -64,7 +70,7 @@
 <?dbhtml filename="copyright.html"?>
 <title>Copyright and License</title>
 <para>
-      Copyright (c) 2006 by Hans-Jürgen Koch.</para>
+      Copyright (c) 2006-2008 by Hans-Jürgen Koch.</para>
 <para>
 This documentation is Free Software licensed under the terms of the
 GPL version 2.
@@ -189,6 +195,30 @@
 	represents the total interrupt count. You can use this number
 	to figure out if you missed some interrupts.
 	</para>
+	<para>
+	For some hardware that has more than one interrupt source internally,
+	but not separate IRQ mask and status registers, there might be
+	situations where userspace cannot determine what the interrupt source
+	was if the kernel handler disables them by writing to the chip's IRQ
+	register. In such a case, the kernel has to disable the IRQ completely
+	to leave the chip's register untouched. Now the userspace part can
+	determine the cause of the interrupt, but it cannot re-enable
+	interrupts. Another cornercase is chips where re-enabling interrupts
+	is a read-modify-write operation to a combined IRQ status/acknowledge
+	register. This would be racy if a new interrupt occurred
+	simultaneously.
+	</para>
+	<para>
+	To address these problems, UIO also implements a write() function. It
+	is normally not used and can be ignored for hardware that has only a
+	single interrupt source or has separate IRQ mask and status registers.
+	If you need it, however, a write to <filename>/dev/uioX</filename>
+	will call the <function>irqcontrol()</function> function implemented
+	by the driver. You have to write a 32-bit value that is usually either
+	0 or 1 to disable or enable interrupts. If a driver does not implement
+	<function>irqcontrol()</function>, <function>write()</function> will
+	return with <varname>-ENOSYS</varname>.
+	</para>
 
 	<para>
 	To handle interrupts properly, your custom kernel module can
@@ -362,6 +392,14 @@
 <function>open()</function>, you will probably also want a custom
 <function>release()</function> function.
 </para></listitem>
+
+<listitem><para>
+<varname>int (*irqcontrol)(struct uio_info *info, s32 irq_on)
+</varname>: Optional. If you need to be able to enable or disable
+interrupts from userspace by writing to <filename>/dev/uioX</filename>,
+you can implement this function. The parameter <varname>irq_on</varname>
+will be 0 to disable interrupts and 1 to enable them.
+</para></listitem>
 </itemizedlist>
 
 <para>

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

[Leon Woestenberg]

Hello,

On Sat, May 24, 2008 at 12:43 AM, Hans J. Koch <hjk@linutronix.de> wrote:
> On Fri, May 23, 2008 at 10:44:42PM +0200, Leon Woestenberg wrote:
>>
>> Shouldn't this be more future-proof, what if we need to abuse write()
>> for something else in the future?
>
> We don't. I'm thinking about letting the function fail if irq_on is not
> 0 or 1, just to stop any ideas of abusing write().
>
We don't want to be future-proof?

With kernel UIO and userspace driver in seperate source repositories,
expect serious API drift in the longer term. I.e. the UIO interface
must be backwards and forwards proof IMHO.

> read() and write() only deal with irq handling, all data exchange with the
> device is done through mapped memory.
>
*Currently*, read() and write() only deal with irq handling.

In the future you might want to add a second control. I cannot think
of what that should be now, much like it was not foreseen a write()
call was needed.

>> I would suggest a check for ppos to be 0 (zero) as well, just to be
>> sure and future-proof and backwards-safe.
>
> write() is only for enabling/disabling irqs, there's only one possible
> value of count, and we don't have a seek function. So why check ppos?
>
So that *if* we have a second write()able location (again, for
something I cannot foresee now), you at least check that the userspace
proper wants to enable/disable the interrupt.

AFAIK, POSIX pwrite() does not require a seek() implementation in the
driver, but will come in with a different ppos.

Idea and patch looks fine, I just wanted to bring this up so that it
is considered.

Regards,
-- 
Leon

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

[Greg KH]

On Fri, May 23, 2008 at 01:55:57PM +0200, Hans J. Koch wrote:
> Sometimes it is necessary to enable/disable the interrupt of a UIO device
> from the userspace part of the driver. With this patch, the UIO kernel driver
> can implement an "irqcontrol()" function that does this. Userspace can write
> an s32 value to /dev/uioX (usually 0 or 1 to turn the irq off or on). The
> UIO core will then call the driver's irqcontrol function.

Why not just a new sysfs file for the uio device, irq_enabled, or
something like that?  That way our main read/write path is left alone.

thanks,

greg k-h

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

[Hans J. Koch]

On Fri, May 23, 2008 at 09:43:54PM -0700, Greg KH wrote:
> On Fri, May 23, 2008 at 01:55:57PM +0200, Hans J. Koch wrote:
> > Sometimes it is necessary to enable/disable the interrupt of a UIO device
> > from the userspace part of the driver. With this patch, the UIO kernel driver
> > can implement an "irqcontrol()" function that does this. Userspace can write
> > an s32 value to /dev/uioX (usually 0 or 1 to turn the irq off or on). The
> > UIO core will then call the driver's irqcontrol function.
> 
> Why not just a new sysfs file for the uio device, irq_enabled, or
> something like that?  That way our main read/write path is left alone.

Hi Greg,
this is in a fast path, so I'm not sure if a sysfs file is not too much
overhead. Special devices in embedded boards sometimes generate a
considerable irq load, and I think it might be problem to do a sysfs
write operation a few thousand times per second.

Thanks,
Hans

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

[Thomas Gleixner]

On Fri, 23 May 2008, Greg KH wrote:

> On Fri, May 23, 2008 at 01:55:57PM +0200, Hans J. Koch wrote:
> > Sometimes it is necessary to enable/disable the interrupt of a UIO device
> > from the userspace part of the driver. With this patch, the UIO kernel driver
> > can implement an "irqcontrol()" function that does this. Userspace can write
> > an s32 value to /dev/uioX (usually 0 or 1 to turn the irq off or on). The
> > UIO core will then call the driver's irqcontrol function.
> 
> Why not just a new sysfs file for the uio device, irq_enabled, or
> something like that?  That way our main read/write path is left alone.

It makes a certain amount of sense to use write. You hold the device
file descriptor anyway for the read (wait for interrupt) operation,
so using the same file descriptor is not a too bad idea:

    while (!stop) {

        /* wait for interrupt */
	read(fd);

	do_stuff();

	/*reenable interrupt */
	write(fd);
    }

I thought about using a sysfs entry for a while, but looking at the
actual use case made the write() solution a more natural choice.

Thanks,

	tglx

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

[Tom Spink]

2008/5/24 Thomas Gleixner <tglx@linutronix.de>:
> On Fri, 23 May 2008, Greg KH wrote:
>
>> On Fri, May 23, 2008 at 01:55:57PM +0200, Hans J. Koch wrote:
>> > Sometimes it is necessary to enable/disable the interrupt of a UIO device
>> > from the userspace part of the driver. With this patch, the UIO kernel driver
>> > can implement an "irqcontrol()" function that does this. Userspace can write
>> > an s32 value to /dev/uioX (usually 0 or 1 to turn the irq off or on). The
>> > UIO core will then call the driver's irqcontrol function.
>>
>> Why not just a new sysfs file for the uio device, irq_enabled, or
>> something like that?  That way our main read/write path is left alone.

Hi,

> It makes a certain amount of sense to use write. You hold the device
> file descriptor anyway for the read (wait for interrupt) operation,
> so using the same file descriptor is not a too bad idea:

What do you think about my ioctl idea, earlier in the thread?

>    while (!stop) {
>
>        /* wait for interrupt */
>        read(fd);
>
>        do_stuff();
>
>        /*reenable interrupt */
>        write(fd);
>    }

So, instead of write, you'd use ioctl(fd, ...).

> I thought about using a sysfs entry for a while, but looking at the
> actual use case made the write() solution a more natural choice.

I thought ioctl would be more natural, as [en,dis]abling interrupts is
a "controlling" operation :-)

> Thanks,
>
>        tglx

-- 
Tom Spink

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

[Thomas Gleixner]

On Sat, 24 May 2008, Tom Spink wrote:
> 2008/5/24 Thomas Gleixner <tglx@linutronix.de>:
> > It makes a certain amount of sense to use write. You hold the device
> > file descriptor anyway for the read (wait for interrupt) operation,
> > so using the same file descriptor is not a too bad idea:
> 
> What do you think about my ioctl idea, earlier in the thread?

I think it's a pretty bad idea.
 
> >    while (!stop) {
> >
> >        /* wait for interrupt */
> >        read(fd);
> >
> >        do_stuff();
> >
> >        /*reenable interrupt */
> >        write(fd);
> >    }
> 
> So, instead of write, you'd use ioctl(fd, ...).

And what's the actual gain ?

> > I thought about using a sysfs entry for a while, but looking at the
> > actual use case made the write() solution a more natural choice.
> 
> I thought ioctl would be more natural, as [en,dis]abling interrupts is
> a "controlling" operation :-)

Oh no. We are not going to open the bottomless pit of ioctls in
UIO. Once we have an ioctl channel in place we have the same mess
which we want to avoid in the first place.

Also when a driver needs more than the obvious interrupt wait /
control functions (which are pretty symetric btw.) aside of the
mmapped access to the device then it does not belong into the category
of an UIO driver.

Thanks,

	tglx

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

[Tom Spink]

2008/5/24 Thomas Gleixner <tglx@linutronix.de>:
> On Sat, 24 May 2008, Tom Spink wrote:
>> 2008/5/24 Thomas Gleixner <tglx@linutronix.de>:
>> > It makes a certain amount of sense to use write. You hold the device
>> > file descriptor anyway for the read (wait for interrupt) operation,
>> > so using the same file descriptor is not a too bad idea:
>>
>> What do you think about my ioctl idea, earlier in the thread?
>
> I think it's a pretty bad idea.

<grin>

>
>> >    while (!stop) {
>> >
>> >        /* wait for interrupt */
>> >        read(fd);
>> >
>> >        do_stuff();
>> >
>> >        /*reenable interrupt */
>> >        write(fd);
>> >    }
>>
>> So, instead of write, you'd use ioctl(fd, ...).
>
> And what's the actual gain ?

Simpler implementation, simpler use and future-proofing (in the sense
that ->write is no longer tied to this operation)

>
>> > I thought about using a sysfs entry for a while, but looking at the
>> > actual use case made the write() solution a more natural choice.
>>
>> I thought ioctl would be more natural, as [en,dis]abling interrupts is
>> a "controlling" operation :-)
>
> Oh no. We are not going to open the bottomless pit of ioctls in
> UIO. Once we have an ioctl channel in place we have the same mess
> which we want to avoid in the first place.
>
> Also when a driver needs more than the obvious interrupt wait /
> control functions (which are pretty symetric btw.) aside of the
> mmapped access to the device then it does not belong into the category
> of an UIO driver.

Fair enough :-) symmetry is good.  This is pretty much the response I
got from Hans.

> Thanks,
>
>        tglx

-- 
Tom Spink

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

[Greg KH]

On Sun, May 25, 2008 at 12:22:20AM +0200, Thomas Gleixner wrote:
> On Fri, 23 May 2008, Greg KH wrote:
> 
> > On Fri, May 23, 2008 at 01:55:57PM +0200, Hans J. Koch wrote:
> > > Sometimes it is necessary to enable/disable the interrupt of a UIO device
> > > from the userspace part of the driver. With this patch, the UIO kernel driver
> > > can implement an "irqcontrol()" function that does this. Userspace can write
> > > an s32 value to /dev/uioX (usually 0 or 1 to turn the irq off or on). The
> > > UIO core will then call the driver's irqcontrol function.
> > 
> > Why not just a new sysfs file for the uio device, irq_enabled, or
> > something like that?  That way our main read/write path is left alone.
> 
> It makes a certain amount of sense to use write. You hold the device
> file descriptor anyway for the read (wait for interrupt) operation,
> so using the same file descriptor is not a too bad idea:
> 
>     while (!stop) {
> 
>         /* wait for interrupt */
> 	read(fd);
> 
> 	do_stuff();
> 
> 	/*reenable interrupt */
> 	write(fd);
>     }
> 
> I thought about using a sysfs entry for a while, but looking at the
> actual use case made the write() solution a more natural choice.

Ok, that makes sense, I can accept this.

Anyone care to respin the patches with the latest updates and send them
to me?

thanks,

greg k-h

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

[Uwe Kleine-König]

Hello Hans-Jürgen,

> Sometimes it is necessary to enable/disable the interrupt of a UIO device
> from the userspace part of the driver. With this patch, the UIO kernel driver
> can implement an "irqcontrol()" function that does this. Userspace can write
> an s32 value to /dev/uioX (usually 0 or 1 to turn the irq off or on). The
> UIO core will then call the driver's irqcontrol function.
IMHO it would make sense to demand that irqcontrol() is idempotent and
then call irqcontrol(ON) before blocking in read and poll.

Best regards
Uwe

-- 
Uwe Kleine-König, Software Engineer
Digi International GmbH Branch Breisach, Küferstrasse 8, 79206 Breisach, Germany
Tax: 315/5781/0242 / VAT: DE153662976 / Reg. Amtsgericht Dortmund HRB 13962

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

[Thomas Gleixner]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 700 bytes --]

On Wed, 4 Jun 2008, Uwe Kleine-König wrote:
> Hello Hans-Jürgen,
> 
> > Sometimes it is necessary to enable/disable the interrupt of a UIO device
> > from the userspace part of the driver. With this patch, the UIO kernel driver
> > can implement an "irqcontrol()" function that does this. Userspace can write
> > an s32 value to /dev/uioX (usually 0 or 1 to turn the irq off or on). The
> > UIO core will then call the driver's irqcontrol function.
> IMHO it would make sense to demand that irqcontrol() is idempotent and
> then call irqcontrol(ON) before blocking in read and poll.

We thought about that, but it is racy. Also explicit control is way
better than automagic hackery.

Thanks,
	tglx