Message-ID: <20080609040926.GA9492@linux-sh.org>
Date:	Mon, 9 Jun 2008 13:09:26 +0900
From:	Paul Mundt <lethal@...ux-sh.org>
To:	"Hans J. Koch" <hjk@...utronix.de>
Cc:	Magnus Damm <magnus.damm@...il.com>, linux-kernel@...r.kernel.org,
	Uwe.Kleine-Koenig@...i.com, gregkh@...e.de,
	akpm@...ux-foundation.org, tglx@...utronix.de
Subject: Re: [PATCH] uio_pdrv: Unique IRQ Mode

On Sun, Jun 08, 2008 at 10:54:57PM +0200, Hans J. Koch wrote:
> On Sun, Jun 08, 2008 at 07:19:25PM +0900, Magnus Damm wrote:
> > >>
> > >> So your main objection against this patch is that you cannot use it
> > >> with shared interrupts?
> > >
> > > I think I've explained my objections detailed enough.
> > 
> > It's still unclear to me. Please make a brief summary of your objections.
> 
> The objection is that your code offers no advantages. What can we do
> with your patch applied that we cannot do with uio_pdrv alone?
> 
Re-use it across similar devices, without endless duplication for one.
This point has been re-iterated multiple times in these threads, yet
seems to be completely ignored each time it is brought up. There is zero
point in reproducing the same code over and over and over again for each
CPU variant, especially when the existing UIO framework is a pretty good
match for the problem.

This is behaviour that we both need and wish to take advantage of through
UIO. You constantly advocate uio_pdrv as the one-stop solution, which
might even fit this case if the unique IRQ mode was factored in to it. If
you're not able to live with that, then a separate driver is necessary.

> > > If it's a device within the SoC, you won't use UIO for that. If you did,
> > > your platform would depend on certain userspace software which is simply
> > > crap. And devices outside the SoC are board specific.
> > 
> > Why wouldn't we use UIO for device within the Soc? 
> 
> Can't you read? I've answered that in the lines above your question.
> 
> > I've been doing
> > that for quite some time now.
> 
> Really? I haven't seen any such driver yet. IMO, support for everything
> inside a SoC should be completely within the kernel, I wouldn't do it
> with UIO. But it's up to the arch maintainer to decide that. Did you ask
> him?
> 
Yes, and this was the conclusion we came to. There is simply no in-kernel
infrastructure for dealing with the class of devices that are being
exposed to userspace. A small amount of refcounting and management glue
in the kernel is necessary, but the rest is userspace's problem. It's
also hardly architecture-specific, anyone with a complex SoC containing
equally complex IP blocks where no in-kernel infrastructure exists that
still needs to do basic accounting on the kernel side is going to be
faced with this situation.

UIO is generally a pretty good match for this problem, and I'd really
rather not have something buried in my architecture directory to work
around this because the subsystem people decide to be resistant to other
approaches.

> Once more (for the last time): The technical argument against your patch
> is that it offers no advantages. It makes other code more complicated,
> less obvious, and less consistent. This might be OK if we had big
> advantages in return, but this isn't the case.

Great, so lets throw an ifdef around it and be done with it. Some people
need this functionality, while others do not. The only way this can be
perceived as confusing is if it's default-enabled, which then also
requires the semantics to be documented. Having said that, there are
plenty of places in the kernel where we flip between IRQ and polling mode
based on IRQ specifier automatically, so it's not obvious that this sort
of use would qualify as a source of confusion.

> Furthermore, your approach works only on certain platforms. But that
> doesn't matter anymore, because the above is already enough to NAK your
> patch. We won't add code just because it could be done.
> 
I'm not sure what your point is. Your uio_pdrv approach only works on
certain platforms too, so what? We add code because it solves a
particular problem, not for the hell of it.

> If I post a patch, it is my job to make it clear what advantages we have
> if we apply it. And no, nothing is unclear with your patch.

The advantages are primarly around reusability. UIO offers a fairly clean
interface to work with, and the only thing that needs to be addressed is
how to deal with the "unique" IRQ mode. For the blocks Magnus is
exposing, they all have this quality, and none of them have any place in
the kernel outside of the bare management bits. While Magnus could easily
run off and create yet another user interface for these things, it would
be nice to have this solved in a more generic fashion. I'm not sure
what's confusing about any of this or why you don't see these reasons as
an advantage.

All of the code you've wanted to see has been posted in previous threads.

> > > Unfortunately, I'm one of the two UIO maintainers, so I feel obliged to
> > > review your patch and give my opinion. That doesn't mean I'm
> > > the big boss who makes the final decision. I can be critized and
> > > overridden. If Greg loves your patch and merges it, fine. Try it.
> > 
> > Maybe I will. =)
> 
> Did you notice that in this thread nobody spoke up to support your
> patch?
> 
It's difficult to jump in and support a patch when it's unclear what the
technical arguments against it are. Usually if there's a need for a
driver and no technical arguments against it, the problem works itself
out. Whether we go with this patch or the "other" driver approach is
immaterial, if you aren't going to allow uio_pdrv to be modified, then
please stop peddling it as the solution to the given problem.
--
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