Re: [PATCH] uio_pdrv: Unique IRQ Mode

["Hans J. Koch" <hjk@linutronix.de>]

On Mon, Jun 09, 2008 at 09:57:01AM +0200, Uwe Kleine-König wrote:
> Hello Hans,
> 
> > Did you notice that in this thread nobody spoke up to support your
> > patch?
> Actually I like what the patch tries to achieve.  I'd like to have it a
> bit more explicit tough:
> 
> - Provide the irq disabling handler in uio_pdrv.c (or even uio.c) with a
>   prototype in an adequate header.  Then the platforms that want this
>   kind of handling can request it explicitly.

You could provide an irqcontrol() function in uio_pdrv that calls a function
defined in board support. If no function is defined there, it returns
-ENOSYS. That would be consistent behaviour and not limited to
non-shared interrupts. Note that this requires the add-write-function
patch I recently posted.

> 
> - Don't use this handler automatically.
> 
> - Provide the function named uio_pdrv_unique_irqcontrol in Magnus' patch
>   in uio_pdrv.c and in an adequate header.

Why invent a new name? The approach above works with all kinds of irqs on
all platforms.

> 
> - Either rely on userspace to enable the irq before reading/polling or
>   assert that in kernel space.  See also
>   http://thread.gmane.org/gmane.linux.kernel/684683/focus=689635
>   (I asked tglx about the race condition via irc, but without a response
>   so far.)

There are two problems:
1) If the hardware is designed in such a broken way that userspace needs
a read-modify-write operation on a combined irq mask/status register to
re-enable the irq, then this is racy against a new interrupt that occurs
simultaneously. We have seen this on two devices so far.

2) If we wanted to make sure the interrupt is enabled in read() and
poll(), we would have the problem that userspace usually calls poll()
and then read() immediately afterwards. This would enable the irq twice,
which can lead to two interrupts being seen in some cases.

For both reasons, we decided that introducing the write() function to
enable and disable irqs is the best solution. Greg already added that
patch to his tree, so it should appear in one of the next kernels.

>   Currently the former is done, but if we decide to let it as it is, I'd
>   like to have it documented.  (I.e. something like:  "Before
>   polling/reading /dev/uioX assert that irqs are enabled.")

We cannot do this, at least not in a clean way.

> 
> The last point is a bit independent from that mode, but applies to
> devices that have a irqcontrol function in general.
> 
> Apart from the general things above, I'd change a few things in the
> implementation:
> 
>  - call dev_info->irqcontrol(OFF) in the handler (instead of
>    disable_irq()) and demand that calling this is idempotent.
>    With this change it isn't uio_pdrv specific any more and could go to
>    uio.c.

Why should we want to do this? You save five lines of irq handler code
by introducing the need for an irqcontrol() function.
I already said that in the discussion with Magnus, I don't see any
advantage in this. Magnus cannot tell me either, he just keeps telling
me "but we can do it" over and over again.
With the modifications mentioned above, it would be a little better, but
I still don't see what we really gain. Your uio_pdrv is a nice and clean
thing, I don't want to add code that makes it less obvious just to save
five lines of irq handler code in some cornercases (or nothing at all,
if we need an irqcontrol() function instead).

Thanks,
Hans