[RFC][PATCH] irq: support IRQ_NESTED_THREAD with non-threaded interrupt handlers

[RFC][PATCH] irq: support IRQ_NESTED_THREAD with non-threaded interrupt handlers

[Esben Haabendal]

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.

Signed-off-by: Esben Haabendal <eha@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

Re: [RFC][PATCH] irq: support IRQ_NESTED_THREAD with non-threaded interrupt handlers

[Thomas Gleixner]

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@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
> 
> 
> 

Re: [RFC][PATCH] irq: support IRQ_NESTED_THREAD with non-threaded interrupt handlers

[Esben Haabendal]

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@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@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@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@doredevelopment.dk
WWW: http://www.doredevelopment.dk

Re: [RFC][PATCH] irq: support IRQ_NESTED_THREAD with non-threaded interrupt handlers

[Marc Zyngier]

On Sat, 5 Jun 2010 15:56:01 +0200
Esben Haabendal <esbenhaabendal@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.

	M.
-- 
Enforcement officers may use a hand-held detection device to measure both
the direction and the strength of a TV signal. This makes it easy for us
to locate TV receiving equipment in even the hardest-to-reach places.

Re: [RFC][PATCH] irq: support IRQ_NESTED_THREAD with non-threaded interrupt handlers

[Thomas Gleixner]

On Sat, 5 Jun 2010, Marc Zyngier wrote:

> On Sat, 5 Jun 2010 15:56:01 +0200
> Esben Haabendal <esbenhaabendal@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.

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.

Thanks,

	tglx

Re: [RFC][PATCH] irq: support IRQ_NESTED_THREAD with non-threaded interrupt handlers

[Esben Haabendal]

On Sat, Jun 5, 2010 at 5:33 PM, Thomas Gleixner <tglx@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

Re: [RFC][PATCH] irq: support IRQ_NESTED_THREAD with non-threaded interrupt handlers

[Thomas Gleixner]

Esben,

On Sat, 5 Jun 2010, Esben Haabendal wrote:

> On Sat, Jun 5, 2010 at 5:33 PM, Thomas Gleixner <tglx@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.

> 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.
 
> 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.

The sanity check there makes the problem entirely obvious and forces
people to look at the driver for the following reasons:

 - the code has been audited for thread safety

 - the annotation of request_any_context_irq() documents that the
   audit has been done.

 - the annotation of request_any_context_irq() makes other people who
   are changing the code aware of the fact that it _is_ used in
   threaded context on some hardware.

 - in some drivers a cleanup can be done based on the threaded code,
   e.g. for drivers which use their own worker threads or rely heavily
   on workqueues.

Thanks,

	tglx

Re: [RFC][PATCH] irq: support IRQ_NESTED_THREAD with non-threaded interrupt handlers

[Esben Haabendal]

On Sat, Jun 5, 2010 at 8:52 PM, Thomas Gleixner <tglx@linutronix.de> wrote:
> Esben,
>
> On Sat, 5 Jun 2010, Esben Haabendal wrote:
>
>> On Sat, Jun 5, 2010 at 5:33 PM, Thomas Gleixner <tglx@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

Re: [RFC][PATCH] irq: support IRQ_NESTED_THREAD with non-threaded interrupt handlers

[Thomas Gleixner]

On Sat, 5 Jun 2010, Esben Haabendal wrote:
> On Sat, Jun 5, 2010 at 8:52 PM, Thomas Gleixner <tglx@linutronix.de> wrote:
> > Esben,
> >
> > On Sat, 5 Jun 2010, Esben Haabendal wrote:
> >
> >> On Sat, Jun 5, 2010 at 5:33 PM, Thomas Gleixner <tglx@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).

And that's the fundamentally wrong approach.
 
> This is really a drawback of the genirq buslock IMHO.

No, it avoids races even on UP. It's there for a reason and there was
a huge discussion about this last year.

Just because you have not hit the problem yet does not make it non
existant.

> >> 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.

That's a totally different playground.
 
> 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...

No it does not, as it just encourages people to disable buslocks and
do simliar shitty crap. 
 
> >> 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.

Yeah, and at the same time you rip out the buslock. Why are you not
sending this patch as well ? Simply because you know that it is
utterly wrong and a horrible hack. 

But at the same time you want to sell me a patch which rips out the
prevention mechanism for your hack. 

The time you spent to fiddle with the generic code and ripping out the
buslock is probably more than it had taken to make the PHY driver
thread safe.

The buslock is there for a reason and if you can't use the code with
the disable/enable_irq() in the atomic context, then this needs to be
fixed there if you need to run the irq handler in thread context.

Sorry, no dice.

Thanks,

	tglx

Re: [RFC][PATCH] irq: support IRQ_NESTED_THREAD with non-threaded interrupt handlers

[Esben Haabendal]

On Sat, Jun 5, 2010 at 10:14 PM, Thomas Gleixner <tglx@linutronix.de> wrote:
>> 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).
>
> And that's the fundamentally wrong approach.

Well, given you haven't seen what I have done there, I don't see how you
can be so sure ;-)

The only reason for the buslock in the pca9535 driver is to set the
direction (ie. input) for interrupt pins.  On powerpc, I do this in the map()
irq_chip function. So I don't see the need for buslock on powerpc.

>> This is really a drawback of the genirq buslock IMHO.
>
> No, it avoids races even on UP. It's there for a reason and there was
> a huge discussion about this last year.

I am not talking about it in general, but for the pca953x driver in particular.
The only thing that is done in the unlock is to set direction, and I don't see
the point in not doing this in earlier.  In most situations, it will
likely be set
even eariler, given that default direction is input, and that it is not uncommon
to set the direction appropriately early in the board init phase.

> Just because you have not hit the problem yet does not make it non
> existant.

Which particular problem should I be on the lookout for in pca953x
driver?

>> 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...
>
> No it does not, as it just encourages people to disable buslocks and
> do simliar shitty crap.

Hold your horses, no reason to call it shitty when you haven't even seen
it. I didn't try to start any kind of general duscission for/against genirq
buslock.

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.
>
> Yeah, and at the same time you rip out the buslock. Why are you not
> sending this patch as well ? Simply because you know that it is
> utterly wrong and a horrible hack.

No, it was send at the same time, but to the linuxppc-dev.  I do not
see it as utterly wrong, and I hope you will give it a look with an open
mind, not just assuming that it is shitty crap, utterly wrong or horrible
hack even before reading it, thanks ;-)

http://patchwork.ozlabs.org/patch/54717/

It is a longer patch, and I expect that it could be improved quite a
bit, but I really don't see it as a fundanmentally shitty.

> But at the same time you want to sell me a patch which rips out the
> prevention mechanism for your hack.

Which patch are you refering to?

The patch we are discussing here does not rip out any thing,

It simply adds support for using existing dirvers together
with handle_nested_irq().

> The time you spent to fiddle with the generic code and ripping out the
> buslock is probably more than it had taken to make the PHY driver
> thread safe.

Maybe, but I am not so sure.

The phy driver calls disable_irq_nosync(), which is documented as
"This function may be called from IRQ context."
And with disable_irq_nosync() calling chip_bus_lock() and
chip_bus_sync_unlock(), which definitely is not meant
for IRQ context, I got the understanding that it was not
the phy driver that needed fixing, but mroe the generic
buslock stuff, which I did not have the guts to touch, as
I understand it would be easy to get wrong.

So maybe you can help me out here, is disable_irq_nosync()
to be improved here?  It does not seem to be fair to
document that it can be called in IRQ context, and then
have it designed to blow up if done so in combination
with a genirq buslock driver.

> The buslock is there for a reason and if you can't use the code with
> the disable/enable_irq() in the atomic context, then this needs to be
> fixed there if you need to run the irq handler in thread context.

How can I disable_irq in interrupt context, with the interrupt handled
by a genirq buslock driver?

/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@doredevelopment.dk
WWW: http://www.doredevelopment.dk

Re: [RFC][PATCH] irq: support IRQ_NESTED_THREAD with non-threaded interrupt handlers

[Thomas Gleixner]

On Sun, 6 Jun 2010, Esben Haabendal wrote:

> On Sat, Jun 5, 2010 at 10:14 PM, Thomas Gleixner <tglx@linutronix.de> wrote:
> >> 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).
> >
> > And that's the fundamentally wrong approach.
> 
> Well, given you haven't seen what I have done there, I don't see how you
> can be so sure ;-)

See below :)
 
> The only reason for the buslock in the pca9535 driver is to set the
> direction (ie. input) for interrupt pins.  On powerpc, I do this in the map()
> irq_chip function. So I don't see the need for buslock on powerpc.

What's so magic about powerpc. Does it magically serialize stuff ?

The buslock is there for a reason:

  The generic irq code calls irq_chip functions with irq_desc->lock
  held and interrupts disabled. So the driver _CANNOT_ access the I2C
  bus because that's blocking.

  So the irq_chip functions modify driver local state, which might be
  modified by non irq related code as well.

  After releasing desc->lock the genirq code calls the bus unlock
  function which does the I2C work and releases the buslock after it's
  done.

  Therefor we take the buslock before we take desc->lock and release
  it after unlocking desc->lock.

So your removal of buslock removes the protection of the genirq
operation, which is guaranteed to be serialized against other
callers. The driver local state is not longer protected.

That's totally unrelated to powerpc and UP, it's simply plain
wrong. And that's why I knew w/o looking at your patch. :)

You can argue to me in circles, that it is not a problem on your
particular system. I don't care at all. It does not matter as it
violates the semantics and might blow up in other usage scenarios
badly. Hint: think SMP

And that's why we have request_any_context_irq() in the first place
and want people to realize that the driver which ends up on a I2C irq
controller (which is one of the most braindead ideas hardware folks
came up with ever) needs to be audited and probably modified.

> > Just because you have not hit the problem yet does not make it non
> > existant.
> 
> Which particular problem should I be on the lookout for in pca953x
> driver?

See above.
 
> > Yeah, and at the same time you rip out the buslock. Why are you not
> > sending this patch as well ? Simply because you know that it is
> > utterly wrong and a horrible hack.
> 
> No, it was send at the same time, but to the linuxppc-dev.  I do not

You should have CC'ed me as well on that to give me the full context,
so I would have told you in the first reply why it is [pick your
favourite out of my arsenal of swear word combos] but in a more
moderate way as I would have noticed right away that you did not
realize the implications and semantics of buslock and the reasons why
you need to look at the innocent PHY driver.

> see it as utterly wrong, and I hope you will give it a look with an open
> mind, not just assuming that it is shitty crap, utterly wrong or horrible
> hack even before reading it, thanks ;-)
> 
> http://patchwork.ozlabs.org/patch/54717/
> 
> It is a longer patch, and I expect that it could be improved quite a
> bit, but I really don't see it as a fundanmentally shitty.

I do (even before looking at it). Simply because it breaks the
driver. See above.

> > But at the same time you want to sell me a patch which rips out the
> > prevention mechanism for your hack.
> 
> Which patch are you refering to?
> 
> The patch we are discussing here does not rip out any thing,
>
> It simply adds support for using existing dirvers together
> with handle_nested_irq().

It's ripping out the prevention of what you are trying to do.
 
> > The time you spent to fiddle with the generic code and ripping out the
> > buslock is probably more than it had taken to make the PHY driver
> > thread safe.
> 
> Maybe, but I am not so sure.
> 
> The phy driver calls disable_irq_nosync(), which is documented as
> "This function may be called from IRQ context."
> And with disable_irq_nosync() calling chip_bus_lock() and
> chip_bus_sync_unlock(), which definitely is not meant
> for IRQ context, I got the understanding that it was not
> the phy driver that needed fixing, but mroe the generic
> buslock stuff, which I did not have the guts to touch, as
> I understand it would be easy to get wrong.
> 
> So maybe you can help me out here, is disable_irq_nosync()
> to be improved here?  It does not seem to be fair to
> document that it can be called in IRQ context, and then
> have it designed to blow up if done so in combination
> with a genirq buslock driver.

The documentation says: It may be called from irq context.

That means it's safe to call it from the irq handler itself, contrary
to disable_irq() which would deadlock. When your handler is running in
thread context then this applies as well.

Maybe the documentation is a bit unclear about that. I'm happy to
accept a patch which improves it !
 
> > The buslock is there for a reason and if you can't use the code with
> > the disable/enable_irq() in the atomic context, then this needs to be
> > fixed there if you need to run the irq handler in thread context.
> 
> How can I disable_irq in interrupt context, with the interrupt handled
> by a genirq buslock driver?

See above.

Btw, which phy driver are you talking about ? 

Calling irq_disable_nosync() from the irq handler needs a damned good
reason and in most cases it pops up the red "Hack Alert" sign.

Thanks,

	tglx

Re: [RFC][PATCH] irq: support IRQ_NESTED_THREAD with non-threaded interrupt handlers

[Esben Haabendal]

On Mon, 2010-06-07 at 00:08 +0200, Thomas Gleixner wrote:

> > The only reason for the buslock in the pca9535 driver is to set the
> > direction (ie. input) for interrupt pins.  On powerpc, I do this in the map()
> > irq_chip function. So I don't see the need for buslock on powerpc.
> 
> What's so magic about powerpc. Does it magically serialize stuff ?
> 
> The buslock is there for a reason:
> 
>   The generic irq code calls irq_chip functions with irq_desc->lock
>   held and interrupts disabled. So the driver _CANNOT_ access the I2C
>   bus because that's blocking.

Which I don't see a need for. As I mentioned, the only I2C access done
at this point is the write to direction register, in case new irq's
requires switching a pin from output to input.

This can be done on irq_chip->map() on powerpc, without requiring
other drivers to know about it.

> So the irq_chip functions modify driver local state, which might be
>   modified by non irq related code as well.

Which is not done here.
The irq_chip functions modify the following driver local state
variables:
  irq_mask (mask/unmask)
  irq_trig_fall (set_type)
  irq_trig_raise (set_type)

They are not (to be) modified by other functions.

> After releasing desc->lock the genirq code calls the bus unlock
>   function which does the I2C work and releases the buslock after it's
>   done.

See above. No I2C work is needed.

> Therefor we take the buslock before we take desc->lock and release
>   it after unlocking desc->lock.
> 
> So your removal of buslock removes the protection of the genirq
> operation, which is guaranteed to be serialized against other
> callers. The driver local state is not longer protected.

But I don't see what I need to protect against. No other
code is touching driver local state that is also touched by
the genirq operations.  Or what am I overseeing here?

> That's totally unrelated to powerpc and UP, it's simply plain
> wrong. And that's why I knew w/o looking at your patch. :)

Well, it seems to me that you have a few wrong assumptions about
what the genirq operations in the patched pca953x driver is
doing, which might warrant a second look at my patch ;-)

> You can argue to me in circles, that it is not a problem on your
> particular system. I don't care at all. It does not matter as it
> violates the semantics and might blow up in other usage scenarios
> badly. Hint: think SMP
> 
> And that's why we have request_any_context_irq() in the first place
> and want people to realize that the driver which ends up on a I2C irq
> controller (which is one of the most braindead ideas hardware folks
> came up with ever) needs to be audited and probably modified.

I totally agree with the last part. I have asked (and convinced) the
hardware people to change that for next revision :-)

> > > Just because you have not hit the problem yet does not make it non
> > > existant.
> > 
> > Which particular problem should I be on the lookout for in pca953x
> > driver?
> 
> See above.

Ok, but I believe that these issues have been dealt with by removing the
need to do I2C work in the irq_chip functions, and by restricting the
driver local state to be only touched by irq_chip functions.  Unless I
am overlooking something more, I fail to see the need for the buslock
in this case (not the general case). 

> > > Yeah, and at the same time you rip out the buslock. Why are you not
> > > sending this patch as well ? Simply because you know that it is
> > > utterly wrong and a horrible hack.
> > 
> > No, it was send at the same time, but to the linuxppc-dev.  I do not
> 
> You should have CC'ed me as well on that to give me the full context,
> so I would have told you in the first reply why it is [pick your
> favourite out of my arsenal of swear word combos] but in a more
> moderate way as I would have noticed right away that you did not
> realize the implications and semantics of buslock and the reasons why
> you need to look at the innocent PHY driver.

Sorry about that.

But your explanations of the buslock in this thread
fits very well with my understanding prior to posting here, so I might
not be as mis-guided in my attempt.

> > see it as utterly wrong, and I hope you will give it a look with an open
> > mind, not just assuming that it is shitty crap, utterly wrong or horrible
> > hack even before reading it, thanks ;-)
> > 
> > http://patchwork.ozlabs.org/patch/54717/
> > 
> > It is a longer patch, and I expect that it could be improved quite a
> > bit, but I really don't see it as a fundanmentally shitty.
> 
> I do (even before looking at it). Simply because it breaks the
> driver. See above.

Well, I would be very interested in finding out in more detail where it
is broken, as I have replied above to the concerns for I2C work and
driver local state race conditions.  I am very open about having
overlooked something, but I would prefer to know exactly what is wrong.
If nothing else, just to be a bit better of next time I enter this
area :-)

> > > But at the same time you want to sell me a patch which rips out the
> > > prevention mechanism for your hack.
> > 
> > Which patch are you refering to?
> > 
> > The patch we are discussing here does not rip out any thing,
> >
> > It simply adds support for using existing dirvers together
> > with handle_nested_irq().
> 
> It's ripping out the prevention of what you are trying to do.

Still, the patch handle_nested_thread() does not rip anything out.
The pca953x.c patch does, but I see it as two different things.

Am I right that you are NAK'ing this patch due to your concerns about
the pca953x.c patch?  If so and if I can convince you that the pca953x.c
patch actually might be heading in a sane direction, you will be
willing to look at this patch again?

Of-course, if anyone implemented an interrupt controller in a thread_fn,
but without I2C or any other need for buslock, the patch proposed here
could make sense.  I don't know about any such driver though... 

> > So maybe you can help me out here, is disable_irq_nosync()
> > to be improved here?  It does not seem to be fair to
> > document that it can be called in IRQ context, and then
> > have it designed to blow up if done so in combination
> > with a genirq buslock driver.
> 
> The documentation says: It may be called from irq context.
> 
> That means it's safe to call it from the irq handler itself, contrary
> to disable_irq() which would deadlock. When your handler is running in
> thread context then this applies as well.
> 
> Maybe the documentation is a bit unclear about that. I'm happy to
> accept a patch which improves it !

But when does it make sense to call functions which are not valid in irq
context from a function which are allowed to be called from irq context?
At least, the principles of least surprise is violated.

Should the documentation be amended with a not on disable_irq(_nosync),
that it must not be called from irq context on interrupts which are
handled by genirq buslock enabled drivers? 

> > > The buslock is there for a reason and if you can't use the code with
> > > the disable/enable_irq() in the atomic context, then this needs to be
> > > fixed there if you need to run the irq handler in thread context.
> > 
> > How can I disable_irq in interrupt context, with the interrupt handled
> > by a genirq buslock driver?
> 
> See above.

Where above?  You mean that I can call disable_irq_nosync() from irq
context?  But it calls chip_bus_lock() which calls mutex_lock().  How
is that going to work?

> Btw, which phy driver are you talking about ? 

The handler is implemented in drivers/net/phy/phy.c and is used by all
phy drivers in drivers/net/phy/

> Calling irq_disable_nosync() from the irq handler needs a damned good
> reason and in most cases it pops up the red "Hack Alert" sign.

Hehe, ok :-D

It might be the root to all my trouble here, so I would be very happy to
find a solution for it.

The problem is that MDIO communication is often just as painstakingly
slow as I2C, so the phy irq handler disables the irq and schedules a
work, which then will do the MDIO communication, and thus ack the irq,
and finally re-enable the irq.

Hints on how to fix that would be appreciated.

/Esben

Re: [RFC][PATCH] irq: support IRQ_NESTED_THREAD with non-threaded interrupt handlers

[Thomas Gleixner]

On Mon, 7 Jun 2010, Esben Haabendal wrote:
> On Mon, 2010-06-07 at 00:08 +0200, Thomas Gleixner wrote:
> 
> > > The only reason for the buslock in the pca9535 driver is to set the
> > > direction (ie. input) for interrupt pins.  On powerpc, I do this in the map()
> > > irq_chip function. So I don't see the need for buslock on powerpc.
> > 
> > What's so magic about powerpc. Does it magically serialize stuff ?
> > 
> > The buslock is there for a reason:
> > 
> >   The generic irq code calls irq_chip functions with irq_desc->lock
> >   held and interrupts disabled. So the driver _CANNOT_ access the I2C
> >   bus because that's blocking.
> 
> Which I don't see a need for. As I mentioned, the only I2C access done
> at this point is the write to direction register, in case new irq's
> requires switching a pin from output to input.
> 
> This can be done on irq_chip->map() on powerpc, without requiring
> other drivers to know about it.
> 
> > So the irq_chip functions modify driver local state, which might be
> >   modified by non irq related code as well.
> 
> Which is not done here.
> The irq_chip functions modify the following driver local state
> variables:
>   irq_mask (mask/unmask)
>   irq_trig_fall (set_type)
>   irq_trig_raise (set_type)
> 
> They are not (to be) modified by other functions.

That does not matter. You remove even the serialization of these
functions and the guarantee of higher level functions, that the
changed state has hit the hardware _BEFORE_ something else can change
it.

Thats what the buslock/unlock mechanism protects, and it _IS_ not
going to change, even if it could work on some UP configurations for
whatever reason. Neither is the sanity check for nested handlers going
away.

Can we please stop it here and solve the problem where it needs to be
solved? See below.

> The handler is implemented in drivers/net/phy/phy.c and is used by all
> phy drivers in drivers/net/phy/
> 
> > Calling irq_disable_nosync() from the irq handler needs a damned good
> > reason and in most cases it pops up the red "Hack Alert" sign.
> 
> Hehe, ok :-D
> 
> It might be the root to all my trouble here, so I would be very happy to
> find a solution for it.
> 
> The problem is that MDIO communication is often just as painstakingly

It's not about slow. MDIO access needs thread context.

> slow as I2C, so the phy irq handler disables the irq and schedules a
> work, which then will do the MDIO communication, and thus ack the irq,
> and finally re-enable the irq.
> 
> Hints on how to fix that would be appreciated.

Yes, the driver should be converted to threaded interrupts as MDIO
access needs thread context, but when the driver was written, there
were no threaded handlers available.

So it does nothing in the interrupt than disabling the irq line and
scheduling work. The real functions are done in thread context and
that's why it needs to disable the irq line.

That's where threaded interrupt comes handy, because threaded
interrupts do that already with the IRQF_ONESHOT flag set for direct
called irqs and in case of nested threaded interrupts, there is no
need at all, because the demux handler serializes interrupts already.

The only problem in the non nested case is, that we currently do not
allow ONESHOT for shared interrupts as this might hold off the other
device on that IRQ line for too long, but looking at this driver it
must be done anyway via the disable_irq_nosync() call in the interrupt
handler. And it needs careful synchronization across the shared
interrupts.

We can remove that restriction with an IRQF_FORCE_ONESHOT flag, which
annotates such usecases and is easy to grep for (ab)users. We have
patches in -rt already to handle that for shared interrupts, I just
need to polish them a bit.

That would remove a lot of ugly code in such drivers which is
necessary: the disable_irq_nosync() call, synchronizing with
workqueues etc.

And in your case it would remove _TWO_ I2C transfers, which is
definitely a huge performance win.

You don't have to wait for the genirq changes as in your case you can
remove the IRQF_SHARED flag until the genirq changes are available.

Maybe you understand now, why I was pretty sure upfront, that your
approach was wrong even without knowing all the gory details ? :)

Thanks,

	tglx

Re: [RFC][PATCH] irq: support IRQ_NESTED_THREAD with non-threaded interrupt handlers

[Esben Haabendal]

On Mon, Jun 7, 2010 at 5:06 PM, Thomas Gleixner <tglx@linutronix.de> wrote:

> Maybe you understand now, why I was pretty sure upfront, that your
> approach was wrong even without knowing all the gory details ? :)

I understand.  There is a better solution, which is to use threaded
interrupts where needed.

But I must confess that I am disappointed that you still fail to see
how the pca953x
patch actually eliminates the need for serialization. But I don't
think there is much
point in going on about that.

The phy driver should be rewritten to use a threaded handler, it will
solve my particular problem.
And in the meantime, I have been promised to get the phy interrupts ofloaded
to real CPU interrupt lines :-)

Oh, I still think that the disable_irq_nosync documentaiton is misleading.
Functions that are allowed in a particular context should not call functions
that are not allowed to be called in that context. But now I know :-)

/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@doredevelopment.dk
WWW: http://www.doredevelopment.dk

Re: [RFC][PATCH] irq: support IRQ_NESTED_THREAD with non-threaded interrupt handlers

[Thomas Gleixner]

On Mon, 7 Jun 2010, Esben Haabendal wrote:
> On Mon, Jun 7, 2010 at 5:06 PM, Thomas Gleixner <tglx@linutronix.de> wrote:
> 
> > Maybe you understand now, why I was pretty sure upfront, that your
> > approach was wrong even without knowing all the gory details ? :)
> 
> I understand.  There is a better solution, which is to use threaded
> interrupts where needed.
> 
> But I must confess that I am disappointed that you still fail to see
> how the pca953x
> patch actually eliminates the need for serialization. But I don't
> think there is much
> point in going on about that.

I return the favour of being disappointed that you are still failing
to accept that there is a fundamental difference between "it works in
a particular case" and semantical correctness under all
circumstances.

There is no place for "it works in a particular case" in a kernel
which runs on a gazillion of different devices unless you want to
create a complete unmaintainable mess. The only way to avoid this is
to be strict about semantics even if it seems unsensible for a
particular case.

Also I explained it to you in great length, that your patch violates
the semantical guarantees of existing functions, but you ignore that
completely.

It's Open Source, go wild and change it for your particular case - but
don't expect that I accept patches to the generic irq code which will
cause _me_ predictable trouble as I have to deal with the
fallout. Neither expect, that I ack patches to users of that code
which are semantically wrong.

Just for the record, the pca953x driver is broken vs. the local state
protection anyway as the GPIO functions are not serialized against the
interrupt controller functions. There is no magic which prevents that,
neither on your system nor on any other. The GPIO function should take
buslock when modifying local state and that's one of the reasons why
buslock it is there and cannot go away.

I'm anticipating that you are going to tell me that it does not matter
on your particular powerpc combined with your usage pattern (i.e no
GPIO users). And I tell you right away, that I'm not interested in
this answer for well explained reasons.

> Oh, I still think that the disable_irq_nosync documentaiton is misleading.
> Functions that are allowed in a particular context should not call functions
> that are not allowed to be called in that context. But now I know :-)

As I said before: I agree and I'm happy to accept a patch to fix that
documentation problem.

Thanks,

	tglx

Re: [RFC][PATCH] irq: support IRQ_NESTED_THREAD with non-threaded interrupt handlers

[Esben Haabendal]

On Tue, 2010-06-08 at 01:18 +0200, Thomas Gleixner wrote: 
> On Mon, 7 Jun 2010, Esben Haabendal wrote:
> > On Mon, Jun 7, 2010 at 5:06 PM, Thomas Gleixner <tglx@linutronix.de> wrote:
> > 
> > > Maybe you understand now, why I was pretty sure upfront, that your
> > > approach was wrong even without knowing all the gory details ? :)
> > 
> > I understand.  There is a better solution, which is to use threaded
> > interrupts where needed.
> > 
> > But I must confess that I am disappointed that you still fail to see
> > how the pca953x
> > patch actually eliminates the need for serialization. But I don't
> > think there is much
> > point in going on about that.
> 
> I return the favour of being disappointed that you are still failing
> to accept that there is a fundamental difference between "it works in
> a particular case" and semantical correctness under all
> circumstances.

No reason to do that.  I must have misunderstood you then.
I understand that the general case, I just never got the point
where you understood what I did that (I believe) makes it
actually work (not just for now, but really work), in my
particular case.

> There is no place for "it works in a particular case" in a kernel
> which runs on a gazillion of different devices unless you want to
> create a complete unmaintainable mess. The only way to avoid this is
> to be strict about semantics even if it seems unsensible for a
> particular case.

I won't argue against.  Not at all.

> Also I explained it to you in great length, that your patch violates
> the semantical guarantees of existing functions, but you ignore that
> completely.

And I answered you several times, that I understand your point,
but 


> It's Open Source, go wild and change it for your particular case - but
> don't expect that I accept patches to the generic irq code which will
> cause _me_ predictable trouble as I have to deal with the
> fallout. Neither expect, that I ack patches to users of that code
> which are semantically wrong.
> 
> Just for the record, the pca953x driver is broken vs. the local state
> protection anyway as the GPIO functions are not serialized against the
> interrupt controller functions. There is no magic which prevents that,
> neither on your system nor on any other. The GPIO function should take
> buslock when modifying local state and that's one of the reasons why
> buslock it is there and cannot go away.

Ok, so the genirq buslock should be held in fx.
pca953x_gpio_direction_input and pca953x_gpio_direction_output ?
That makes sense in combination with the currently implemented
bus_sync_unlock.

But I still fail to see why it is needed, or even desired, that the
setting of direction should be related to the genirq code at all.

I see it as a sane seperation to let the actual gpio code handle
the direction setting, and the irq code to handle the (in software)
irq mask.  Driver and/or board code should be sane enough to set
pca953x gpio's as input if/when interrupts from them are needed.
And they should do this in advance.  And by doing it this way,
there should not be any hardware state to serialize, and there
actually is no pca953x driver local state either, as the pca953x_chip
members are either exclusively managed by gpio or irq code.

> I'm anticipating that you are going to tell me that it does not matter
> on your particular powerpc combined with your usage pattern (i.e no
> GPIO users). And I tell you right away, that I'm not interested in
> this answer for well explained reasons.

No, I don't tie it to my particular usage pattern. I tell you that
I don't need it (serialization) on powerpc because my pca953x powerpc
patch have removed the need for shared local state between gpio and irq
code.

Of-course, for the pca953x driver to be acceptable, it would
be nice (or more likely, required) to unify it for other archs
also, I just don't see how to do that.

That said, I will proceed with the request_any_context_irq() in the phy
driver instead :-)  Case closed for now.

/Esben

Re: [RFC][PATCH] irq: support IRQ_NESTED_THREAD with non-threaded interrupt handlers

[Thomas Gleixner]

On Mon, 7 Jun 2010, Esben Haabendal wrote:

> On Mon, Jun 7, 2010 at 5:06 PM, Thomas Gleixner <tglx@linutronix.de> wrote:
> 
> > Maybe you understand now, why I was pretty sure upfront, that your
> > approach was wrong even without knowing all the gory details ? :)
> 
> I understand.  There is a better solution, which is to use threaded
> interrupts where needed.

FWIW, it just occured to me, that the only reason why the
disable_irq_nosysnc() trips up on the in_atomic() check is your
fiddling with the nested irq dispatcher. 

If you just would have changed the phy driver to
request_irq_any_context() it would have simply worked out of the box,
without any need to remove buslock and changing genirq code at all.

That would not give you the advantage of getting rid of the two
additional I2C transfers, but that's nn optimzation not a functional
requirement.

Thanks,

	tglx

Re: [RFC][PATCH] irq: support IRQ_NESTED_THREAD with non-threaded interrupt handlers

[Esben Haabendal]

On Tue, 2010-06-08 at 08:58 +0200, Thomas Gleixner wrote: 
> On Mon, 7 Jun 2010, Esben Haabendal wrote:
> 
> > On Mon, Jun 7, 2010 at 5:06 PM, Thomas Gleixner <tglx@linutronix.de> wrote:
> > 
> > > Maybe you understand now, why I was pretty sure upfront, that your
> > > approach was wrong even without knowing all the gory details ? :)
> > 
> > I understand.  There is a better solution, which is to use threaded
> > interrupts where needed.
> 
> FWIW, it just occured to me, that the only reason why the
> disable_irq_nosysnc() trips up on the in_atomic() check is your
> fiddling with the nested irq dispatcher. 
> 
> If you just would have changed the phy driver to
> request_irq_any_context() it would have simply worked out of the box,
> without any need to remove buslock and changing genirq code at all.
> 
> That would not give you the advantage of getting rid of the two
> additional I2C transfers, but that's nn optimzation not a functional
> requirement.

Now, that is good news. Thanks!

/Esben

Re: [RFC][PATCH] irq: support IRQ_NESTED_THREAD with non-threaded interrupt handlers

[Esben Haabendal]

On Sat, Jun 5, 2010 at 4:10 PM, Marc Zyngier <maz@misterjones.org> wrote:
> On Sat, 5 Jun 2010 15:56:01 +0200
> Esben Haabendal <esbenhaabendal@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

Re: [RFC][PATCH] irq: support IRQ_NESTED_THREAD with non-threaded interrupt handlers

[Marc Zyngier]

On Sat, 5 Jun 2010 18:53:53 +0200
Esben Haabendal <esbenhaabendal@gmail.com> wrote:

> On Sat, Jun 5, 2010 at 4:10 PM, Marc Zyngier <maz@misterjones.org> wrote:
> > On Sat, 5 Jun 2010 15:56:01 +0200
> > Esben Haabendal <esbenhaabendal@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.

To me, any driver/subsystem/whatever qualifies as "yours" if you're taking
care of it ;-).

> 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...).

	M.
-- 
Enforcement officers may use a hand-held detection device to measure both
the direction and the strength of a TV signal. This makes it easy for us
to locate TV receiving equipment in even the hardest-to-reach places.

Re: [RFC][PATCH] irq: support IRQ_NESTED_THREAD with non-threaded interrupt handlers

[Esben Haabendal]

>> 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@doredevelopment.dk
WWW: http://www.doredevelopment.dk