[lkml]   [2008]   [Jun]   [10]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
Messages in this thread
    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
    > > > >
    > > > > (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

    > > > > 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 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
    More majordomo info at
    Please read the FAQ at

     \ /
      Last update: 2008-06-10 08:15    [W:0.030 / U:0.332 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site