UIO: Add a write() function to enable/disable interrupts
Subsystems (2)
cover UIO: Add a write() function to enable/disable interrupts
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
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.
--- Documentation/DocBook/uio-howto.tmpl | 40 ++++++++++++++++++++++++++++++++++- drivers/uio/uio.c | 26 ++++++++++++++++++++++ include/linux/uio_driver.h | 2 + 3 files changed, 67 insertions(+), 1 deletion(-)
include/linux/uio_driver.h +2 -0
@@ -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
drivers/uio/uio.c +26 -0
@@ -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,
Documentation/DocBook/uio-howto.tmpl +39 -1
@@ -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>
2008/5/22 Hans J. Koch <[email protected]>:
Quoted 7 lines
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 <[email protected]>
<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/
Signature · 1 line
Tom Spink
On Thu, May 22, 2008 at 08:47:16PM +0100, Tom Spink wrote:
Hi Tom,
Quoted 7 lines
2008/5/22 Hans J. Koch <[email protected]>:
Quoted 6 lines
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.
Quoted 7 lines
Quoted 1 line
Signed-off-by Hans J. Koch <[email protected]><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.
Quoted 4 lines
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.
Quoted 5 lines
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
2008/5/22 Hans J. Koch <[email protected]>:
Quoted 13 lines · Tom Spink
Hi Tom,
Quoted 9 lines
2008/5/22 Hans J. Koch <[email protected]>:
Quoted 7 lines
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 <[email protected]>
Quoted 7 lines
Quoted 5 lines
<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):
drivers/uio/uio.c +0 -0
@@ -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,Quoted 9 lines
Quoted 5 lines
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
Quoted 6 lines
1) allow this memory to be mapped
2) handle interruptsWe have an
mmap()implementation for 1) and aread()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?
Quoted 9 lines
Quoted 6 lines
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.
Quoted 2 lines
Thanks,
Hans
Signature · 1 line
Tom Spink
Hello Tom,
Quoted 2 lines · Tom Spink
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
Signature · 3 lines
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
Am Fri, 23 May 2008 07:41:15 +0200
schrieb Uwe Kleine-König <[email protected]>:
Quoted 9 lines
Hello Tom,
Quoted 3 lines · Tom Spink
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
2008/5/23 Uwe Kleine-König <[email protected]>:
Quoted 1 line
Hello Tom,
Hi Uwe,
Quoted 6 lines
Quoted 2 lines · Tom Spink
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.
Quoted 2 lines
Best regards
Uwe
Signature · 1 line
Tom Spink
Hello Tom,
Quoted 7 lines
Quoted 6 lines
Quoted 2 lines · Tom Spink
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.
Quoted 5 lines
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
Signature · 3 lines
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
2008/5/23 Uwe Kleine-König <[email protected]>:
Quoted 9 lines
Hello Tom,
Quoted 7 lines
Quoted 6 lines
Quoted 2 lines · Tom Spink
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.
Quoted 6 lines
Quoted 5 lines
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. :-)
Quoted 1 line
Uwe
Signature · 1 line
Tom Spink
Am Fri, 23 May 2008 13:00:17 +0100
schrieb "Tom Spink" <[email protected]>:
Quoted 3 lines
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
2008/5/23 Hans J. Koch <[email protected]>:
Quoted 14 lines
Am Fri, 23 May 2008 13:00:17 +0100
schrieb "Tom Spink" <[email protected]>:Quoted 3 lines
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 gladwrite()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.
Quoted 6 lines
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 restrictwrite()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!)
Signature · 1 line
Tom Spink
Am Fri, 23 May 2008 13:20:50 +0100
schrieb "Tom Spink" <[email protected]>:
Quoted 24 lines
2008/5/23 Hans J. Koch <[email protected]>:
Quoted 15 lines
Am Fri, 23 May 2008 13:00:17 +0100
schrieb "Tom Spink" <[email protected]>:Quoted 3 lines
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 gladwrite()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.Quoted 7 lines
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 restrictwrite()to the value 0 and 1...Thanks,
Hans
Quoted 8 lines
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
-EXXXand 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
Hello Hans,
Documentation/DocBook/uio-howto.tmpl line 73
Quoted 2 lines
Documentation/DocBook/uio-howto.tmpl +1
-1- 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
Signature · 3 lines
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
Am Fri, 23 May 2008 07:55:27 +0200
schrieb Uwe Kleine-König <[email protected]>:
Quoted 5 lines
Hello Hans,
Documentation/DocBook/uio-howto.tmpl line 73
Quoted 2 lines
Documentation/DocBook/uio-howto.tmpl +1
-1- 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
Quoted 7 lines
ukleinek@zentaur:~$ echo Hans-JÃŒrgen | iconv -t ISO8859-15 Hans-JürgenBest regards
Uwe
Quoted 10 lines · Hans J. Koch
Am Fri, 23 May 2008 07:55:27 +0200
schrieb Uwe Kleine-König <[email protected]>:Quoted 5 lines
Hello Hans,
Documentation/DocBook/uio-howto.tmpl line 73
Quoted 2 lines
Documentation/DocBook/uio-howto.tmpl +1
-1- 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
Signature · 3 lines
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
Am Fri, 23 May 2008 11:10:09 +0200
schrieb Uwe Kleine-König <[email protected]>:
Quoted 6 lines
Quoted 5 lines · Hans J. Koch
Am Fri, 23 May 2008 07:55:27 +0200
schrieb Uwe Kleine-König <[email protected]>:Quoted 2 lines
Hello Hans,
Quoted 8 lines
Quoted 5 lines
Quoted 3 lines
Review
Quoted 2 lines
(quoted diff) +1
-1- 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
Quoted 4 lines
Best regards
Uwe
From: Hans J. Koch <[email protected]>
To: [email protected]
Cc: Greg Kroah-Hartman <[email protected]>,
Jan Altenberg <[email protected]>, Thomas Gleixner <[email protected]>, Uwe Kleine-König <[email protected]>, Magnus Damm <[email protected]>
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.
--- Documentation/DocBook/uio-howto.tmpl | 40 ++++++++++++++++++++++++++++++++++- drivers/uio/uio.c | 26 ++++++++++++++++++++++ include/linux/uio_driver.h | 2 + 3 files changed, 67 insertions(+), 1 deletion(-)
include/linux/uio_driver.h +2 -0
@@ -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
drivers/uio/uio.c +26 -0
@@ -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,
Documentation/DocBook/uio-howto.tmpl +39 -1
@@ -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>
Hello Hans,
Quoted 11 lines · Hans J. Koch
Am Fri, 23 May 2008 11:10:09 +0200
schrieb Uwe Kleine-König <[email protected]>:Quoted 8 lines
Quoted 7 lines · Hans J. Koch
Am Fri, 23 May 2008 07:55:27 +0200
schrieb Uwe Kleine-König <[email protected]>:Quoted 4 lines
Hello Hans,
Quoted 2 lines
(quoted diff) +1
-1- Copyright (c) 2006 by Hans-JÃŒrgen Koch.</para> + Copyright (c) 2006-2008 by Hans-JÃŒrgen Koch.</para>
Quoted 8 lines
Quoted 6 lines
Quoted 3 lines
Quoted 1 line
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.
drivers/uio/uio.c line 441
Quoted 5 lines
drivers/uio/uio.c +5
-0+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
Signature · 3 lines
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
Am Fri, 23 May 2008 12:56:04 +0200
schrieb Uwe Kleine-König <[email protected]>:
Quoted 16 lines
Hello Hans,
Quoted 13 lines · Hans J. Koch
Am Fri, 23 May 2008 11:10:09 +0200
schrieb Uwe Kleine-König <[email protected]>:Quoted 10 lines
Quoted 9 lines · Hans J. Koch
Am Fri, 23 May 2008 07:55:27 +0200
schrieb Uwe Kleine-König <[email protected]>:Quoted 5 lines
Hello Hans,
Review
Quoted 2 lines
(quoted diff) +1
-1- Copyright (c) 2006 by Hans-JÃŒrgen Koch.</para> + Copyright (c) 2006-2008 by Hans-JÃŒrgen Koch.</para>This looks wrong.
Quoted 8 lines
Quoted 7 lines
Quoted 5 lines
Quoted 1 line
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.
Quoted 9 lines
Review
Quoted 6 lines
(quoted diff) +5
-0+ if (copy_from_user(&irq_on, buf, count)) + return -EFAULT; + + if (!idev->info->irqcontrol) + return -ENOSYS;I would swap these two.
copy_from_useris more expensive than testingidev->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.
Quoted 2 lines
Otherwise the patch looks fine.
Thanks for your review! Could you add your Signed-off-by?
Hans
From: Hans J. Koch <[email protected]>
To: [email protected]
Cc: Greg Kroah-Hartman <[email protected]>,
Jan Altenberg <[email protected]>, Thomas Gleixner <[email protected]>, Uwe Kleine-König <[email protected]>, Magnus Damm <[email protected]>
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.
--- Documentation/DocBook/uio-howto.tmpl | 40 ++++++++++++++++++++++++++++++++++- drivers/uio/uio.c | 26 ++++++++++++++++++++++ include/linux/uio_driver.h | 2 + 3 files changed, 67 insertions(+), 1 deletion(-)
include/linux/uio_driver.h +2 -0
@@ -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
drivers/uio/uio.c +26 -0
@@ -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,
Documentation/DocBook/uio-howto.tmpl +39 -1
@@ -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>
Uwe Kleine-König
Acked-byHello Hans,
Quoted 3 lines
Quoted 1 line
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 ...
Quoted 10 lines
From: Hans J. Koch <[email protected]>
To: [email protected]
Cc: Greg Kroah-Hartman <[email protected]>,
Jan Altenberg <[email protected]>, Thomas Gleixner <[email protected]>, Uwe Kleine-König <[email protected]>, Magnus Damm <[email protected]>
Subject: UIO: Add write function to allow irq masking
Date: Thu, Fri, 23 May 2008 13:50:14 +0200
Quoted 7 lines
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 <[email protected]>
Best regards
Uwe
Quoted 32 lines · 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
===================================================================Documentation/DocBook/uio-howto.tmpl +7
-1@@ -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.
Documentation/DocBook/uio-howto.tmpl line 200
Quoted 6 lines
Documentation/DocBook/uio-howto.tmpl +3
-0@@ -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
Documentation/DocBook/uio-howto.tmpl line 206
Quoted 6 lines
Documentation/DocBook/uio-howto.tmpl +6
-0+ 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
Documentation/DocBook/uio-howto.tmpl line 208
Quoted 2 lines
Documentation/DocBook/uio-howto.tmpl +2
-0+ is a read-modify-write operation to a combined IRQ status/acknowledge + register. This would be racy if a new interrupt occured
occurred
Documentation/DocBook/uio-howto.tmpl line 214
Quoted 6 lines
Documentation/DocBook/uio-howto.tmpl +6
-0+ 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."]
Documentation/DocBook/uio-howto.tmpl line 218
Quoted 4 lines
Documentation/DocBook/uio-howto.tmpl +4
-0+ 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
Documentation/DocBook/uio-howto.tmpl line 221
Quoted 3 lines
Documentation/DocBook/uio-howto.tmpl +3
-0+ <function>irqcontrol()</function>, <function>write()</function> will + return with <varname>-ENOSYS</varname>. + </para>
---
~Randy
Am Fri, 23 May 2008 11:36:50 -0700
schrieb Randy Dunlap <[email protected]>:
Quoted 20 lines
Review
Quoted 3 lines
(quoted diff) +2
-0+ For some hardware that has more than one interrupt source internally, + but not seperate IRQ mask and status registers, there might beseparate
Review
Quoted 6 lines
(quoted diff) +6
-0+ 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 interruptsis
Review
Quoted 2 lines
(quoted diff) +2
-0+ is a read-modify-write operation to a combined IRQ status/acknowledge + register. This would be racy if a new interrupt occuredoccurred
Quoted 9 lines
Review
Quoted 6 lines
(quoted diff) +6
-0+ 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 :-)
Quoted 8 lines
Review
Quoted 5 lines
(quoted diff) +4
-0+ 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 implementto
Quoted 6 lines
Review
Quoted 3 lines
(quoted diff) +3
-0+ <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 <[email protected]>
To: [email protected]
Cc: Greg Kroah-Hartman <[email protected]>,
Jan Altenberg <[email protected]>, Thomas Gleixner <[email protected]>, Uwe Kleine-König <[email protected]>, Magnus Damm <[email protected]>
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.
--- Documentation/DocBook/uio-howto.tmpl | 40 ++++++++++++++++++++++++++++++++++- drivers/uio/uio.c | 26 ++++++++++++++++++++++ include/linux/uio_driver.h | 2 + 3 files changed, 67 insertions(+), 1 deletion(-)
include/linux/uio_driver.h +2 -0
@@ -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
drivers/uio/uio.c +26 -0
@@ -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,
Documentation/DocBook/uio-howto.tmpl +39 -1
@@ -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>
Hello Hans-Jürgen,
Quoted 5 lines
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
Signature · 3 lines
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
Quoted 9 lines · Uwe Kleine-König
Hello Hans-Jürgen,
Quoted 5 lines
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
Hello,
Quoted 15 lines · Hans J. Koch
drivers/uio/uio.c +15
-0+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)
drivers/uio/uio.c line 447
Quoted 10 lines
drivers/uio/uio.c +10
-0+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,
Signature · 1 line
Leon
Quoted 23 lines · Leon Woestenberg
Hello,
Quoted 20 lines · Hans J. Koch
drivers/uio/uio.c +20
-0+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 ; +
Quoted 8 lines
drivers/uio/uio.c line 447
Quoted 5 lines
drivers/uio/uio.c +5
-0+ 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.
Quoted 3 lines
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
Hello,
Quoted 8 lines · Hans J. Koch
Quoted 3 lines · Leon Woestenberg
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_onis not 0 or 1, just to stop any ideas of abusingwrite().
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.
Quoted 3 lines
read()andwrite()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.
Quoted 6 lines
Quoted 2 lines
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,
Signature · 1 line
Leon
Quoted 5 lines · 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.
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
Quoted 9 lines · Greg KH
Quoted 5 lines · 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.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
Quoted 9 lines · Greg KH
Quoted 5 lines · 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.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
2008/5/24 Thomas Gleixner <[email protected]>:
Quoted 11 lines · Greg KH
Quoted 10 lines · Hans J. Koch
Quoted 5 lines
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,
Quoted 3 lines
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?
Quoted 10 lines
while (!stop) {/* wait for interrupt */ read(fd); do_stuff(); /*reenable interrupt */ write(fd);}
So, instead of write, you'd use ioctl(fd, ...).
Quoted 2 lines
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 :-)
Quoted 3 lines
Thanks,
tglx
Signature · 1 line
Tom Spink
Quoted 6 lines · Tom Spink
2008/5/24 Thomas Gleixner <[email protected]>:
Quoted 3 lines
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.
Quoted 5 lines
Quoted 5 lines
while (!stop) {/* wait for interrupt */ read(fd);
Quoted 7 lines
Quoted 5 lines
do_stuff(); /*reenable interrupt */ write(fd);}
So, instead of write, you'd use
ioctl(fd, ...).
And what's the actual gain ?
Quoted 5 lines
Quoted 2 lines
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
2008/5/24 Thomas Gleixner <[email protected]>:
Quoted 9 lines · Tom Spink
Quoted 6 lines
2008/5/24 Thomas Gleixner <[email protected]>:
Quoted 3 lines
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>
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
while (!stop) {/* wait for interrupt */ read(fd); do_stuff();
Quoted 7 lines
Quoted 5 lines
Quoted 3 lines
/*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)
Quoted 7 lines
Quoted 6 lines
Quoted 3 lines
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 :-)
Quoted 8 lines
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.
Quoted 3 lines
Thanks,
tglx
Signature · 1 line
Tom Spink
Quoted 27 lines · Thomas Gleixner
Quoted 9 lines · Greg KH
Quoted 5 lines · 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.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); }
Quoted 2 lines
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