lkml.org 
[lkml]   [2008]   [Jun]   [10]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH] uio_pdrv: Unique IRQ Mode
Hello Hans,

> > > > - 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.
> > You didn't understand what I want. (Probably because I choosed a poor
> > wording.)
> >
> > IMHO it should be asserted that irqs are on before waiting for the irq
> > in poll and read. So I suggest to call irqcontrol(ON) before doing so.
> > This should allow to work with that kind of hardware, right?
>
> Yes. But userspace can simply write() a 1 to /dev/uioX to achieve the
> same result. This would clearly show what's happening. Remember, this is
> only needed for certain (broken) hardware. If we hide some automagic irq
> enabling in the kernel, it'll become less obvious and might even have
> some bad side effects. I want to avoid this kind of trickery, especially
> as it is not needed. Userspace should use write() to control irqs. It's
> like this with any normal UIO driver, and we shouldn't have a different
> handling in uio_pdrv.
> Think of a chip that's directly connected to the bus on some embedded
> board. You use uio_pdrv to handle it. Then the very same chip appears on
> a PCI card in a normal PC. You write a normal UIO driver for it. The
> userspace part of both drivers could be exactly the same. But if
> uio_pdrv automagically reenabled the irq, we would need different
> handling in userspace, without reasons obvious to the user.
Note that my intention is to enable irqs in uio.c, not uio_pdrv.c. So
you could still use the same driver for a PCI card and similar a memory
mapped chip.

Probably I should show some code, but I think I won't have time today to
do so and then I will be in vacation for two weeks. So this has to
wait.

> > > > 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.
> > Taking Magnus' patch there is a default irqcontrol() function that does
> > the right thing in this case. This should probably go to uio_pdrv.c.
>
> Just doing irq_disable() limits it to irqs that are not shared. If there
> was a huge advantage, I'd think about it. But as it is, I'll never
> accept that. Magnus' patch is not needed, not even by himself.
I don't suggest to *use* that function per default, just provide it and
allow board support to use it as a call back.

> > > 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.
> > I think the benefit is to add some code to uio_pdrv and/or uio and in
> > turn save some code in board support code.
>
> Yes, but the savings (if any) are small compared with the disadvantages.
Currently I don't see any disadvantages. IMHO we should wait on a new
version of Magnus' patch. Then we can discuss this more effective
referering to code.

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

\
 
 \ /
  Last update: 2008-06-10 08:15    [from the cache]
©2003-2014 Jasper Spaans. Advertise on this site