Re: [PATCH v1 1/4] max3100: added raise_threaded_irq

[Thomas Gleixner]

Christian,

On Fri, 16 Apr 2010, christian pellegrin wrote:

> Hi,
> 
> On Fri, Apr 16, 2010 at 1:22 AM, Thomas Gleixner <tglx@xxxxxxxxxxxxx> wrote:
> >
> >> raise_threaded_irq schedules the execution of an interrupt thread
> >
> > I really have a hard time to understand _WHY_ we want to have that
> > function.
> >
> .....
> >
> > Can you please explain, what you are trying to achieve and why it
> > can't be done with the existing interfaces ?
> >
> 
> The idea was that by using this function we just need one kind of
> deferred work (interrupt threads) instead of two (for example
> interrupt threads and workqueues) in some situations. This is very
> handy with devices that do transmission and reception at the same
> time, for example many SPI devices. The user case is the max3100 UART
> on SPI driver. The same SPI instruction both receives and sends chars.
> So when we need to send a char we just start the interrupt thread
> instead of having another kind of deferred work doing the job. This
> greatly simplifies locking and avoids duplication of functionality
> (otherwise we must have an interrupt thread that does reception and a
> workqueue that does sending and receiving for example) because
> everything is done in just one point. The move from worqueues to
> interrupt threads was motivated by the much smaller latency under load
> of the latter because they are scheduled as RT processes. I hope this
> doesn't sound like a terrible abuse of threaded interrupts. Let me
> know before I try to fix other problems you mentioned.

Thanks for the explanation. Now, that makes a lot of sense and I can
see that it removes a lot of serialization issues and duplicated code
pathes.

So what you want is a mechanism to "inject" interrupts by
software. 

I wonder whether we should restrict this mechanism to threaded
handlers or just implement it in the following way:

int irq_inject(unsigned int irq)
{
        struct irq_desc *desc = irq_to_desc(irq);

	if (!desc)
		return -EINVAL;
	
	local_irq_disable();
        desc->handle_irq(irq, desc);
        local_irq_enable();
	return 0;
}

That would call the real interupt code path as it is called from the
arch/*/kernel/irq.c:entry code and take care of all serialization
issues.

The drawback is that it will increase the irq statistics, but I think
that's really a pure cosmetic problem.

That requires that the primary interrupt handler code knows about the
software "interrupt" event, but that's easy to solve. So the primary
handler would just return IRQ_WAKE_THREAD as it would do for a real
hardware triggered interrupt. But as a goodie that would work for non
threaded interrupts as well.

There is another thing which needs some care: 

This will not work out of the box when the irq is nested into some
demultiplexing thread handler (IRQF_NESTED_THREAD).

I'm too tried to look at this now, but I don't see a real showstopper
there.

Thanks,

	tglx
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/