Message-ID: <20080610061121.GA22814@digi.com>
Date:	Tue, 10 Jun 2008 08:11:21 +0200
From:	Uwe Kleine-König <Uwe.Kleine-Koenig@...i.com>
To:	"Hans J. Koch" <hjk@...utronix.de>
CC:	Magnus Damm <magnus.damm@...il.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"gregkh@...e.de" <gregkh@...e.de>,
	"akpm@...ux-foundation.org" <akpm@...ux-foundation.org>,
	"lethal@...ux-sh.org" <lethal@...ux-sh.org>,
	"tglx@...utronix.de" <tglx@...utronix.de>
Subject: Re: [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@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists