Prev: (none)
Next: irq: support IRQ_NESTED_THREAD with non-threaded interrupt handlers
From: Esben Haabendal on 5 Jun 2010 10:00
I have a board with an I2C PCA9535 chip with two PHY interrupt lines
hooked up to. The pca953x driver calls set_irq_nested_thread on all
irq's on initialization. The PHY driver then calls request_irq, and has
no idea that it should actually be using a threaded handler.

With this patch, the PHY driver is able to work in this scenario
without changes (and so should any other driver using request_irq).

/Esben

On Fri, Jun 4, 2010 at 11:46 PM, Thomas Gleixner <tglx(a)linutronix.de> wrote:
> On Fri, 4 Jun 2010, Esben Haabendal wrote:
>
>> set_irq_nested_thread() might be called by interrupt controller to
>> supported nested irq thread handling, and with this change, drivers
>> with non-threaded irq handlers can still use the irqs.
>
> What's the problem you are trying to solve ? The above changelog is
> not very useful ?
>
> Thanks,
>
> � � � �tglx
>
>> Signed-off-by: Esben Haabendal <eha(a)doredevelopment.dk>
>> ---
>> �kernel/irq/chip.c � | � �9 ++++++++-
>> �kernel/irq/manage.c | � �4 +---
>> �2 files changed, 9 insertions(+), 4 deletions(-)
>>
>> diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c
>> index b7091d5..8202993 100644
>> --- a/kernel/irq/chip.c
>> +++ b/kernel/irq/chip.c
>> @@ -405,7 +405,14 @@ void handle_nested_irq(unsigned int irq)
>> � � � desc->status |= IRQ_INPROGRESS;
>> � � � raw_spin_unlock_irq(&desc->lock);
>>
>> - � � action_ret = action->thread_fn(action->irq, action->dev_id);
>> + � � if (desc->status & IRQ_NESTED_THREAD && action->thread_fn)
>> + � � � � � � action_ret = action->thread_fn(action->irq, action->dev_id);
>> + � � else {
>> + � � � � � � local_irq_disable();
>> + � � � � � � action_ret = action->handler(action->irq, action->dev_id);
>> + � � � � � � local_irq_enable();
>> + � � }
>> +
>> � � � if (!noirqdebug)
>> � � � � � � � note_interrupt(irq, desc, action_ret);
>>
>> diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
>> index 3164ba7..e7bca04 100644
>> --- a/kernel/irq/manage.c
>> +++ b/kernel/irq/manage.c
>> @@ -681,10 +681,8 @@ __setup_irq(unsigned int irq, struct irq_desc *desc, struct irqaction *new)
>> � � � �* Check whether the interrupt nests into another interrupt
>> � � � �* thread.
>> � � � �*/
>> - � � nested = desc->status & IRQ_NESTED_THREAD;
>> + � � nested = (desc->status & IRQ_NESTED_THREAD) && new->thread_fn;
>> � � � if (nested) {
>> - � � � � � � if (!new->thread_fn)
>> - � � � � � � � � � � return -EINVAL;
>> � � � � � � � /*
>> � � � � � � � �* Replace the primary handler which was provided from
>> � � � � � � � �* the driver for non nested interrupt handling by the
>> --
>> 1.7.1
>>
>>
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo(a)vger.kernel.org
> More majordomo info at �http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at �http://www.tux.org/lkml/
>



--
Esben Haabendal, Senior Software Consultant
Dor�Development ApS, Ved Stranden 1, 9560 Hadsund, DK-Denmark
Phone: +45 51 92 53 93, E-mail: eha(a)doredevelopment.dk
WWW: http://www.doredevelopment.dk
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Esben Haabendal on 5 Jun 2010 13:00
On Sat, Jun 5, 2010 at 4:10 PM, Marc Zyngier <maz(a)misterjones.org> wrote:
> On Sat, 5 Jun 2010 15:56:01 +0200
> Esben Haabendal <esbenhaabendal(a)gmail.com> wrote:
>
>> I have a board with an I2C PCA9535 chip with two PHY interrupt lines
>> hooked up to. The pca953x driver calls set_irq_nested_thread on all
>> irq's on initialization. The PHY driver then calls request_irq, and has
>> no idea that it should actually be using a threaded handler.
>>
>> With this patch, the PHY driver is able to work in this scenario
>> without changes (and so should any other driver using request_irq).
>
> You may want to give request_any_context_irq() a try (available since the
> latest merge window). It still requires your driver to be changed, but it
> should then work in both threaded and non-threaded cases.

The problem is not in "my" driver, but in the phy driver framework in this
particular case.

What is the plan here, should all drivers change from request_any_context_irq()
at some point in time?

/Esben
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Esben Haabendal on 5 Jun 2010 13:10
On Sat, Jun 5, 2010 at 5:33 PM, Thomas Gleixner <tglx(a)linutronix.de> wrote:
> On Sat, 5 Jun 2010, Marc Zyngier wrote:
>> You may want to give request_any_context_irq() a try (available since the
>> latest merge window). It still requires your driver to be changed, but it
>> should then work in both threaded and non-threaded cases.
>
> And it nicely annotates that somebody looked at the driver in
> question. That's the rule of least surprise and does not impose checks
> on the fast path.

What in particular should I be looking for in a driver before changing
from request_irq() to request_any_context_irq() ?

As for not checking in the fast path, it should be noted that this is "only"
in handle_nested_irq(), which is only used in few interupt controller
drivers, all of which I assume are generally not considered very "fast".

Unless all interrupt handlers should be rewritten to be able to in both
thread and interrupt context, I fail to se the conflict between the patch
proposed and the work being done on request_any_context_irq().

/Esben
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Esben Haabendal on 5 Jun 2010 15:40
>> What is the plan here, should all drivers change from
>> request_any_context_irq() at some point in time?
>
> All? Probably not. Only the few where the need arises, and after carefully
> checking that you're not introducing a horrible race condition somewhere
> (threaded handlers are running with interrupts enabled...).

Yes, and that is exactly one of the issues I wanted to address with the patch.
If you have a driver which uses an interrupt handler, which is designed
and tested in interrupt context, you really have to take care before turning
it into a threaded interrupt handler.

But if you have a slow interrupt controller which uses nested threaded
interrupt handler, then it should be safe to let the interrupt handler
run within the interrupt controllers thread, but with interrupts disabled.

Yes, it is propably better to rewrite the handler to be able to run
threaded, but the same could be said without the slow interrupt
handler. So unless all interrupt handlers should be rewritten
as threaded handlers, I don't see why it should not be possible
to just run a non-threaded interrupt handler, in a threaded
interrupt controllers thread, but with interrupts disabled.

Carefully checking for horrible race condition in a driver that you
really did not otherwise needed to work on is asking for
quite a bit, and I fear that this approach is doomed to actually
cause the horrible race conditions, because the checking
will be done by people without sufficient insight in the driver
in question.

/Esben
--
Esben Haabendal, Senior Software Consultant
Dor�Development ApS, Ved Stranden 1, 9560 Hadsund, DK-Denmark
Phone: +45 51 92 53 93, E-mail: eha(a)doredevelopment.dk
WWW: http://www.doredevelopment.dk
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Esben Haabendal on 5 Jun 2010 15:50
On Sat, Jun 5, 2010 at 8:52 PM, Thomas Gleixner <tglx(a)linutronix.de> wrote:
> Esben,
>
> On Sat, 5 Jun 2010, Esben Haabendal wrote:
>
>> On Sat, Jun 5, 2010 at 5:33 PM, Thomas Gleixner <tglx(a)linutronix.de> wrote:
>> > On Sat, 5 Jun 2010, Marc Zyngier wrote:
>> >> You may want to give request_any_context_irq() a try (available since the
>> >> latest merge window). It still requires your driver to be changed, but it
>> >> should then work in both threaded and non-threaded cases.
>> >
>> > And it nicely annotates that somebody looked at the driver in
>> > question. That's the rule of least surprise and does not impose checks
>> > on the fast path.
>>
>> What in particular should I be looking for in a driver before changing
>> from request_irq() to request_any_context_irq() ?
>
> Whether the irq handler relies on interrupts being disabled is the
> most important thing. There are other constraints like
> enable_irq/disable_irq calls from the driver code, which are not
> allowed to run in atomic context for interrupt hanging of a i2c irq
> controller.

Yes, I were hit by this also.

The phy interrupt handler calls disable_irq, which does not work
with an i2c irq controller (ie. the genirc buslock mutex), and I specifically
got rid of the buslock for pca9535 driver (powerpc only, though).

This is really a drawback of the genirq buslock IMHO.

>> As for not checking in the fast path, it should be noted that this is "only"
>> in handle_nested_irq(), which is only used in few interupt controller
>> drivers, all of which I assume are generally not considered very "fast".
>
> Fair enough. Still I fundamentaly dislike the automagic handling of
> this and especially the irq disabled portion of it.

Yes, it would be better to not have irq disabled.
But not really much worse than with other interrupt controllers,
where the handler would be running in interrupt context anyway.

So until all handlers are rewritten to run threaded, I believe
something like the proposed patch will give value to people
with i2c irq controllers. Hopefully, not to many people are unlucky
enough to have this, but anyway...

>> Unless all interrupt handlers should be rewritten to be able to in both
>> thread and interrupt context, I fail to se the conflict between the patch
>> proposed and the work being done on request_any_context_irq().
>
> It's not a question of conflict. It's a question of semantics.
>
> We had and still have enough surprises in preempt-rt where we force
> thread all handlers which were caused by various assumptions in the
> handler code. I really prefer that the system yells in such a case
> instead of letting run people into hard to debug problems silently.

I fully appreciate that. And for exactly that reason (combined with
a tight timeschedule), I really just need to have the phy interrupt handler
running unchanged with the i2c irq controller.

/Esben
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
 |  Next  |  Last
Pages: 1 2
Prev: (none)
Next: irq: support IRQ_NESTED_THREAD with non-threaded interrupt handlers