Message-ID: <20080521092533.GB29607@digi.com>
Date:	Wed, 21 May 2008 11:25:33 +0200
From:	Uwe Kleine-König <Uwe.Kleine-Koenig@...i.com>
To:	Magnus Damm <magnus.damm@...il.com>
CC:	"Hans J. Koch" <hjk@...utronix.de>, <linux-kernel@...r.kernel.org>,
	<lethal@...ux-sh.org>, <gregkh@...e.de>, <linux-sh@...r.kernel.org>
Subject: Re: [PATCH 00/03][RFC] Reusable UIO Platform Driver

Magnus Damm wrote:
> Hi Uwe!
> 
> Thanks for your email.
> 
> On Wed, May 21, 2008 at 3:49 PM, Uwe Kleine-König
> <Uwe.Kleine-Koenig@...i.com> wrote:
> > Magnus Damm wrote:
> >> The uio_pdrv driver doesn't do what I need at this point, though I may
> >> be able to extend it with the following:
> >> - Interrupt enable/disable code
> >> - Physically contiguous memory support
> >>
> >> The interrupt code may be placed in the board/cpu code, but I need to
> >> share that code between multiple UIO driver instances. We want to use
> >> the same UIO driver for many different processor models and hardware
> >> blocks.
> > What about adding uio_platform_handler (with a different name) to
> > uio_pdrv.c?
> 
> I'm not sure if it will help. What would such function do? Please explain.
Just add irq_disabled to struct uio_platdata and define

	irqreturn_t uio_pdrv_disirq(int irq, struct uio_info *dev_info)
	{
		struct uio_platdata *pdata = container_of(dev_info, struct uio_platdata, uio_info);

		disable_irq(irq);
		pdata->irq_disabled = 1;
		return IRQ_HANDLED;
	}
	EXPORT_SYMBOL(uio_pdrv_disirq);

	void uio_pdrv_enirq(struct uio_info *dev_info)
	{
		...
	}
	EXPORT_SYMBOL(uio_pdrv_enirq);

and then you can do

	info->handler = uio_pdrv_disirq;
	info->enable_irq = uio_pdrv_enirq;

in the arch specific code.  I just realize that you need to compile UIO
statically then :-(

IMHO something like prep_read_poll is a better name than enable_irq for
the new member of uio_info.

> > OTOH I don't see why you want to disable the irq.  Can you describe the
> > reason?
> 
> Most UIO kernel drivers today contain hardware device specific code to
> acknowledge interrupts. In other words, most UIO interrupt handlers
> touches some device specific bits so the interrupt gets deasserted.
> 
> My uio_platform driver handles interrupts in a different way. The
> kernel UIO driver is not aware of the hardware device specific method
> to acknowledge the interrupt, instead it simply disables the interrupt
> and notifies user space which instead will acknowledge the interrupt.
> Next time a read() or poll() call gets made, the interrupt is enabled
> again.
> 
> This allows us to export a hardware device to user space and allow
> user space to handle interrupts without knowing in kernel space how to
> acknowledge interrupts.
OK, got it.  The down-side is that you can only get a single interrupt
between two calls to read() (or poll()).  So you might or might not
loose information.  And you might run into problems if your device or
your interrupt goes berserk as your handler always returns IRQ_HANDLED.
With a functional handler you can rely on existing mechanisms in the
kernel.
 
> >> To be frank, I have my doubts in adding an extra forwarding-only
> >> platform layer on top of UIO compared to using uio_register_device()
> >> directly from the board code. I like that the platform layer is using
> >> struct resource and handles resource ranges for us automatically, but
> >> wouldn't it make more sense to extend the UIO core to always use
> >> struct resource instead of struct uio_mem? I'd be happy to help out -
> >> just point me in the right direction.
> > That alone doesn't help.  You need a struct device to register a uio
> > device.  So a platform device is the straight forward approach.
> 
> I don't mind that you are using platform devices. Actually, I think
> platform devices are great. We use them for all sorts of things on the
> SuperH architecture. I'm trying to suggest that maybe it's a good idea
> to change the UIO core code to use struct resource instead of struct
> uio_mem. Or maybe that's not a good idea, I'm not sure.
struct resource alone doesn't provide enough information.  At least
memtype is needed.  And you don't need the pointers *parent, *sibling,
*child, so in my eyes it's fine to have a dedicated structure for uio.
 
> >> >> The interrupt handling code in uio_platform assumes the device is the
> >> >> only user of the assigned interrupt.
> >> >
> >> > Uwe's approach doesn't have this limitation.
> >>
> >> True, but the uio_pdrv driver is choosing to not deal with interrupts
> >> at all. I'd like to have shared interrupt handling code. With my
> >> driver, you just feed it io memory window parameters and an interrupt
> >> number and off you go. No need for any callbacks.
> > In my eyes this isn't completly correct.  Just the way you specify your
> > handler is a bit different.  You can pass a handler via platform data
> > with my driver, too.
> 
> I don't want to pass any handler. All devices share the same interrupt
> handler, the only thing that differs between multiple uio_platform
> devices on one system is memory window information and irq number.
See above.  That would be the cost to share code with "my" driver.

All in all I'm not conviced that it's a good idea to use the irq_disable
trick to save acking in kernel space.  This doesn't need to stop you
doing it that way of course.

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