Re: [PATCH 01/03] uio: Add enable_irq() callback

[Magnus Damm]

Hi Hans,

On Fri, May 23, 2008 at 5:18 AM, Hans J. Koch <hjk@xxxxxxxxxxxxx> wrote:
> On Wed, May 21, 2008 at 08:58:24PM +0900, Magnus Damm wrote:
>> On Tue, May 20, 2008 at 7:51 PM, Magnus Damm <magnus.damm@xxxxxxxxx> wrote:
>> > Add enable_irq() callback to struct uio_info. This callback is needed by
>> > the uio_platform driver so interrupts can be enabled before blocking.
>>
>> We can most likely use a single uio platform driver if this patch is
>> acceptable. Any NAKs?
>
> Yes. Your approach only allows enabling interrupts, but not disabling
> them. And I don't like that it is not possible for generic userspace
> tools to find out if a UIO device has this auto-irq-enabling capability
> or not.

Thanks for your effort. I understand that you need to enable and
disable interrupts from user space, but that's a bit different from
what I want to do. I just want interrupts to be enabled before I do
read() or poll().

Also, adding the capability of disabling and enabling interrupts from
user space seems a bit error prone to me. Mainly since user space then
needs to know that the interrupt handler in kernel space can cope with
such changes. Not such a clean interface IMO. OTOH you may need that
to cope with some broken hardware.

But why does user space need to know if the auto-irq-enabling function
is there or not? If the user is interfacing to the wrong kernel UIO
driver with wrong behavior then he has obviously done something wrong.
Knowing if auto-irq-enabling is there from user space isn't going to
save users from themselves. They can and will mix and match things in
all sorts of wrong ways anyway.

> I just posted a patch that allows enabling _and_ disabling of irqs from
> userspace by writing 0 or 1 to /dev/uioX. I've CCed you, could you
> please test? If this doesn't do what you need, please let me know.

I'm sure your patch or the ioctl suggestion both allow re-enabling
interrupts from user space. That's great, but both of them add extra
syscall overhead compared to my suggestion. They also make the user
space interface in user space part of the driver a bit more
complicated.

I do understand that you don't want to mess up your UIO kernel
callbacks by introducing just merging new ones all the time. OTOH, my
patch is just a few lines. Is introducing one extra syscall good
enough performance wise?

Thanks for your help!

/ magnus
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/