Message-ID: <20080609123204.GA27803@digi.com>
Date:	Mon, 9 Jun 2008 14:32:04 +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

Hi Hans,

Hans J. Koch wrote:
> 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.
I didn't check, but I think this is what is happening just now, though
with a different implementation: board support passed the uio_info which
might or might not include a irqcontrol() function.  This is given
unchanged to uio_register.  Assuming that writing without an
irqcontrol() function yields -ENOSYS we're already there.
 
> > - 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.
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?
 
> 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.
OK, for this case a pending flag would be needed.  (This doesn't mean I
suggest to implement it that way.)  I'll think about that a bit.

> 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.
We cannot document it in a clean way?  (Probably not, I assume "this"
still refers to "enable the irq in read and poll"?)
 
> > 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.

> 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.  In fact this is similar to
the whole uio_pdrv driver.  Each platform could implement it without
much hassle itself.  But having all that in one central place makes it
easier for most people.

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