linux-kernel - Re: [PATCH] uio_pdrv: Unique IRQ Mode

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <aec7e5c30806080303i6d412d89o177e665bc3e8f38a@mail.gmail.com>
Date:	Sun, 8 Jun 2008 19:03:39 +0900
From:	"Magnus Damm" <magnus.damm@...il.com>
To:	"Hans J. Koch" <hjk@...utronix.de>
Cc:	"Uwe Kleine-König" <Uwe.Kleine-Koenig@...i.com>,
	linux-kernel@...r.kernel.org, gregkh@...e.de,
	akpm@...ux-foundation.org, lethal@...ux-sh.org, tglx@...utronix.de
Subject: Re: [PATCH] uio_pdrv: Unique IRQ Mode

On Fri, Jun 6, 2008 at 7:04 PM, Hans J. Koch <hjk@...utronix.de> wrote:
> On Fri, Jun 06, 2008 at 11:55:30AM +0900, Magnus Damm wrote:
>> On Thu, Jun 5, 2008 at 3:49 PM, Uwe Kleine-König
>> <Uwe.Kleine-Koenig@...i.com> wrote:
>> > Magnus Damm wrote:
>> >> ... "Unique IRQ Mode". ...
>> > BTW, I wouldn't call it "Unique IRQ Mode" because the non-shared irq is
>> > only a result from automatically disabling the irq.  IMHO something like
>> > "No IRQ Handler Mode" is more suitable.
>>
>> That may be a good idea. Any name is fine with me.
>>
>> Hopefully a better name will make people less confused. =)
>
> I didn't criticize the name. My objection is that you want to introduce
> an optional special handling for cases where the interrupt line is not
> shared, e.g. a GPIO line. In that case, the handler _could_ look like
> this:

First of all, I don't really see how this is related to GPIO. The
hardware blocks I'm exporting to user space are media accelerator
blocks - for instance providing hardware color space conversion. Any
type of device can be exported using this technique as long as it is
memory mapped and it comes with a unique interrupt.

> static irqreturn_t my_handler(int irq, struct uio_info *info)
> {
>        irq_disable(MY_GPIO_LINE);
>        return IRQ_HANDLED;
> }

Why did you remove the atomic_set()? I suggest that you try out the
code, or maybe even look at the irq_enable()/irq_disable()
implementation in kernel/irq/manage.c. Hint: the tricky part is
"desc->depth" in __enable_irq(). Basically, you shouldn't enable the
the interrupt more times than you disable it. So we need to keep track
of things and prevent user space from mucking up the depth for us.

> This solution is only second best, if possible, the irq should be
> properly acknowledged within the chip, but it could be done like this.

Of course the interrupt should be acknowledged at some point. It's
just a question of when and where you do that. I can't see the reason
of having device specific UIO code in kernel space when you can have a
generic kernel space driver and let user space do everything.

> Note that this doesn't work on every platform, it assumes that each GPIO
> line has its own irq number. (Did you ever think about fixing Kconfig so
> that this option is disabled on platforms where it is not possible or
> not sensible to do this?)

Is MY_GPIO_LINE a Kconfig option? Using the "irq" parameter makes more
sense for us since we want to have multiple platform UIO instances up
and running simultaneously.

Would you like me to wrap the above lines in #ifdefs?

> You now suggest that if somebody doesn't fill in an irq handler, we
> should make the above the default. This would save somebody the trouble
> to add the above 5 lines to the 30 lines of board/platform support code
> he has to write anyway. That's the only gain, and that is not enough.

But the code in this patch is reusable. Any platform that has a unique
interrupt assigned to a memory mapped device can just export it to
user space as a regular platform driver. No other kernel space code
needed.

Maybe 5 lines doesn't matter to you, but we want to use this for many
different devices. Maybe we'll start with 5 devices and work our way
from there.

We can of course just do this somewhere under the arch/sh tree, but
why should we make the code architecture specific when it's something
that many architectures can make use of?

> Giving it a different name doesn't make it better.

So exactly what makes it better?

Thank you.

/ magnus
--
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/