uio: add ioctl callback

PATCH In review Neil Armstrong 3 participants 6 messages Nov 12, 2008 Userspace I/O (UIO)

lore

Share & tools

lore

Neil Armstrong

author

2008-11-12 15:40 UTC #0 lore raw

PATCH 1/1 uio: add ioctl callback

Add an ioctl callback for uio devices.

Signed-off-by Neil Armstrong <[email protected]>

---

drivers/uio/uio.c +0 -0

index 2d2440c..38044fb 100644
@@ -385,6 +385,18 @@ static unsigned int uio_poll(struct file *filep,

poll_table *wait)

     return 0;

}

+static int uio_ioctl(struct inode *inode, struct file *filep,
+ unsigned int cmd, unsigned long arg)
+{
+ struct uio_listener *listener = filep->private_data;
+ struct uio_device *idev = listener->dev;
+
+ if (idev->info->ioctl)
+ return idev->info->ioctl(idev->info, cmd, arg);
+
+ return -ENOSYS;
+}
+
static ssize_t uio_read(struct file *filep, char __user *buf,
size_t count, loff_t *ppos)
{
@@ -590,6 +602,7 @@ static const struct file_operations uio_fops = {
.mmap = uio_mmap,
.poll = uio_poll,
.fasync = uio_fasync,
+ .ioctl = uio_ioctl,
};

static int uio_major_init(void)

include/linux/uio_driver.h +2 -0

index cdf338d..e44e89b 100644
@@ -68,6 +68,8 @@ struct uio_info {
     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);
+    int (*ioctl)(struct uio_info *info, unsigned int cmd,
+            unsigned long arg);
 };
 
 extern int __must_check

begin:vcard
fn:Neil Armstrong
n:Armstrong;Neil
org:Neotion;Neotion Sophia Antipolis
adr:;;;Sophia Antipolis;;;France
email;internet:[email protected]
title:Embedded Linux Software Engineer
tel;cell:0667474169
note:PGP 0x1166F485
x-mozilla-html:FALSE
url:http://www.neotion.com
version:2.1
end:vcard

Hans J. Koch

2008-11-12 15:49 UTC #1 lore raw

Quoted 1 line · Neil Armstrong

Add an ioctl callback for uio devices.

Hi Neil,
sorry, but we don't want to have ioctl() for UIO. UIO devices should be completely controllable through their mapped memory. Additional functions can be added by creating custom sysfs files. To switch the interrupt on and off, we have the irqcontrol hook in the write() function. I cannot imagine a case where ioctl() could be useful.

You didn't mention why you want ioctl(). If you tell us the problem, we can probably find a solution that uses the standard UIO interface.

Thanks,
Hans

Quoted 64 lines

Signed-off-by Neil Armstrong <[email protected]>

---

drivers/uio/uio.c +12 -0

index 2d2440c..38044fb 100644
@@ -385,6 +385,18 @@ static unsigned int uio_poll(struct file *filep,
 poll_table *wait)
     return 0;
 }
 
+static int uio_ioctl(struct inode *inode, struct file *filep,
+            unsigned int cmd, unsigned long arg)
+{
+    struct uio_listener *listener = filep->private_data;
+    struct uio_device *idev = listener->dev;
+
+    if (idev->info->ioctl)
+        return idev->info->ioctl(idev->info, cmd, arg);
+
+    return -ENOSYS;
+}
+
 static ssize_t uio_read(struct file *filep, char __user *buf,
             size_t count, loff_t *ppos)

{

drivers/uio/uio.c +1 -0

@@ -590,6 +602,7 @@ static const struct file_operations uio_fops = {
     .mmap        = uio_mmap,
     .poll        = uio_poll,
     .fasync        = uio_fasync,
+    .ioctl        = uio_ioctl,
 };
 
 static int uio_major_init(void)

include/linux/uio_driver.h +2 -0

index cdf338d..e44e89b 100644
@@ -68,6 +68,8 @@ struct uio_info {
     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);
+    int (*ioctl)(struct uio_info *info, unsigned int cmd,
+            unsigned long arg);
 };
 
 extern int __must_check

Neil Armstrong

author

2008-11-12 16:00 UTC #2 lore raw

Hi Hans,

We need an ioctl callback because we need to query some values only available when the irq handler is running.
For example, we have 3 types of reasons why the irq is triggered and these bits are no more available when the irq is cleared.

The cleanest way to have this very specific information is to have a dirty old ioctl returning this data.

Thanks for your time.

Neil

Quoted 12 lines · Hans J. Koch

Quoted 2 lines · Neil Armstrong
Add an ioctl callback for uio devices.
Hi Neil,
sorry, but we don't want to have ioctl() for UIO. UIO devices should be completely controllable through their mapped memory. Additional functions can be added by creating custom sysfs files. To switch the interrupt on and off, we have the irqcontrol hook in the write() function. I cannot imagine a case where ioctl() could be useful.

Quoted 76 lines

You didn't mention why you want ioctl(). If you tell us the problem, we can probably find a solution that uses the standard UIO interface.

Thanks,
Hans

Quoted 66 lines

Signed-off-by Neil Armstrong <[email protected]>

---

drivers/uio/uio.c +12 -0

index 2d2440c..38044fb 100644
@@ -385,6 +385,18 @@ static unsigned int uio_poll(struct file *filep,
 poll_table *wait)
     return 0;
 }
 
+static int uio_ioctl(struct inode *inode, struct file *filep,
+            unsigned int cmd, unsigned long arg)
+{
+    struct uio_listener *listener = filep->private_data;
+    struct uio_device *idev = listener->dev;
+
+    if (idev->info->ioctl)
+        return idev->info->ioctl(idev->info, cmd, arg);
+
+    return -ENOSYS;
+}
+
 static ssize_t uio_read(struct file *filep, char __user *buf,
             size_t count, loff_t *ppos)

{

drivers/uio/uio.c +1 -0

@@ -590,6 +602,7 @@ static const struct file_operations uio_fops = {
     .mmap        = uio_mmap,
     .poll        = uio_poll,
     .fasync        = uio_fasync,
+    .ioctl        = uio_ioctl,
 };
 
 static int uio_major_init(void)

include/linux/uio_driver.h +2 -0

index cdf338d..e44e89b 100644
@@ -68,6 +68,8 @@ struct uio_info {
     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);
+    int (*ioctl)(struct uio_info *info, unsigned int cmd,
+            unsigned long arg);
 };
 
 extern int __must_check

Greg KH

2008-11-12 16:18 UTC #3 lore raw

Quoted 9 lines · Neil Armstrong

Hi Hans,
We need an ioctl callback because we need to query some values only available when the irq handler is running.
For example, we have 3 types of reasons why the irq is triggered and these bits are no more available when the irq is cleared.
The cleanest way to have this very specific information is to have a dirty old ioctl returning this data.

Do you also have a patch for your UIO driver that implements this functionality?

I really don't think we want to add an ioctl callback, perhaps you could just use a sysfs file for this?

thanks,

greg k-h

Hans J. Koch

2008-11-12 16:23 UTC #4 lore raw

In reply to Neil Armstrong

Quoted 6 lines · Neil Armstrong

Hi Hans,
We need an ioctl callback because we need to query some values only available when the irq handler is running.
For example, we have 3 types of reasons why the irq is triggered and these bits are no more available when the irq is cleared.

Ah, that one. That's why we invented the irqcontrol hook. In case of such broken hardware, you need to mask the irq in the kernel without touching the register containing those volatile bits. On a system where you can be sure the irq is not shared, you can simply use disable_irq().

If the irq might be shared, you need to find something else. PCI cards, for example, often have a register within their PCI bridge that contains irq mask bits (that's how it is done in uio_cif.c).

Userspace can then reenable the irq by writing the value 1 as a signed 32-bit int to /dev/uioX. You need to implement an irqcontrol() function in your kernel driver that does the right thing (e.g. call enable_irq() in the first example).

Quoted 3 lines

The cleanest way to have this very specific information is to have a dirty old ioctl returning this data.

The cleanest way would be to throw such hardware into the trash bin :-)

A chip where the irq mask register cannot be written without destroying the irq status register is simply broken and bad design.

Thanks,
Hans

Neil Armstrong

author

2008-11-12 16:44 UTC #5 lore raw

Hi,

I should have specified that I work on a SoC, which does not follow strict rules like PCI devices.
From a hardware designer point of view, this is a "good" design especially for this kind of device. (DVB Conditionnal Access Slave Shared Memory)
If the code was in the kernel space, we could do some processing in irq mode like other devices of the SoC.

Thanks for your time and comments, I will see the irqcontrol solution as an alternative.

Just forget my patch since it's only applicable to very specific devices.

Neil

Quoted 37 lines · Hans J. Koch

Quoted 7 lines · Neil Armstrong
Hi Hans,
We need an ioctl callback because we need to query some values only available when the irq handler is running.
For example, we have 3 types of reasons why the irq is triggered and these bits are no more available when the irq is cleared.
Ah, that one. That's why we invented the irqcontrol hook. In case of such broken hardware, you need to mask the irq in the kernel without touching the register containing those volatile bits. On a system where you can be sure the irq is not shared, you can simply use disable_irq().
If the irq might be shared, you need to find something else. PCI cards, for example, often have a register within their PCI bridge that contains irq mask bits (that's how it is done in uio_cif.c).
Userspace can then reenable the irq by writing the value 1 as a signed 32-bit int to /dev/uioX. You need to implement an irqcontrol() function in your kernel driver that does the right thing (e.g. call enable_irq() in the first example).
Quoted 3 lines
The cleanest way to have this very specific information is to have a dirty old ioctl returning this data.
The cleanest way would be to throw such hardware into the trash bin :-)
A chip where the irq mask register cannot be written without destroying the irq status register is simply broken and bad design.
Thanks,
Hans