irq: support IRQ_NESTED_THREAD with non-threaded interrupt handlers
RFC PATCH 1/1 irq: support IRQ_NESTED_THREAD with non-threaded interrupt handlers
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.
--- kernel/irq/chip.c | 9 ++++++++- kernel/irq/manage.c | 4 +--- 2 files changed, 9 insertions(+), 4 deletions(-)
kernel/irq/chip.c +8 -1
@@ -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);
kernel/irq/manage.c +1 -3
@@ -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
Signature · 1 line
1.7.1
Quoted 3 lines · 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.
What's the problem you are trying to solve ? The above changelog is not very useful ?
Thanks,
tglx
Quoted 47 lines
Signed-off-by Esben Haabendal <[email protected]>--- kernel/irq/chip.c | 9 ++++++++- kernel/irq/manage.c | 4 +--- 2 files changed, 9 insertions(+), 4 deletions(-)kernel/irq/chip.c +8
-1@@ -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);kernel/irq/manage.c +1
-3@@ -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 Signature · 1 line
1.7.1
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
Quoted 66 lines · Thomas Gleixner
Quoted 3 lines · 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.What's the problem you are trying to solve ? The above changelog is not very useful ?
Thanks,
tglx
Quoted 43 lines
Signed-off-by Esben Haabendal <[email protected]>---
kernel/irq/chip.c | 9 ++++++++-
kernel/irq/manage.c | 4 +---2 files changed, 9 insertions(+), 4 deletions(-)kernel/irq/chip.c +8
-1@@ -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);kernel/irq/manage.c +1
-4@@ -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 -- Quoted 4 lines
1.7.1
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Signature · 4 lines
Esben Haabendal, Senior Software Consultant DoréDevelopment ApS, Ved Stranden 1, 9560 Hadsund, DK-Denmark Phone: +45 51 92 53 93, E-mail: [email protected] WWW: http://www.doredevelopment.dk
Quoted 7 lines · On Sat, 5 Jun 2010 15:56:01 +0200 Esben Haabendal <[email protected]>…
I have a board with an I2C PCA9535 chip with two PHY interrupt lines hooked up to. The pca953x driver calls
set_irq_nested_threadon all irq's on initialization. The PHY driver then callsrequest_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.
Signature · 3 lines
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.
Quoted 8 lines · Marc Zyngier
Quoted 5 lines · On Sat, 5 Jun 2010 15:56:01 +0200 Esben Haabendal <[email protected]>…
I have a board with an I2C PCA9535 chip with two PHY interrupt lines hooked up to. The pca953x driver calls
set_irq_nested_threadon all irq's on initialization. The PHY driver then callsrequest_irq, and has no idea that it should actually be using a threaded handler.
Quoted 6 lines
Quoted 2 lines
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
Quoted 8 lines · Thomas Gleixner
Quoted 3 lines · Marc Zyngier
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
Esben,
Quoted 6 lines · Esben Haabendal
Quoted 5 lines · Thomas Gleixner
Quoted 3 lines · Marc Zyngier
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.
Quoted 6 lines
Quoted 3 lines
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()torequest_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.
Quoted 3 lines
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.
Quoted 3 lines
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
Quoted 14 lines · Thomas Gleixner
Esben,
Quoted 10 lines · Esben Haabendal
Quoted 8 lines · Thomas Gleixner
Quoted 3 lines · Marc Zyngier
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.
Quoted 8 lines
Quoted 2 lines
What in particular should I be looking for in a driver before changing from
request_irq()torequest_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.
Quoted 6 lines
Quoted 3 lines
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...
Quoted 10 lines
Quoted 3 lines
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
Quoted 24 lines · Esben Haabendal
Quoted 14 lines · Thomas Gleixner
Esben,
Quoted 10 lines · Esben Haabendal
Quoted 8 lines · Thomas Gleixner
Quoted 3 lines · Marc Zyngier
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.
Quoted 8 lines
Quoted 2 lines
What in particular should I be looking for in a driver before changing from
request_irq()torequest_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.
Quoted 5 lines
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.
Quoted 1 line
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.
Quoted 10 lines
Quoted 6 lines
Quoted 3 lines
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.
Quoted 4 lines
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.
Quoted 6 lines
Quoted 6 lines
Quoted 3 lines
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.
Quoted 8 lines
Quoted 4 lines
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
Quoted 5 lines · Thomas Gleixner
Quoted 3 lines
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.
Quoted 4 lines
Quoted 1 line
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.
Quoted 2 lines
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?
Quoted 7 lines
Quoted 4 lines
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,
Quoted 12 lines
Quoted 12 lines
Quoted 11 lines
Quoted 4 lines
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.
Quoted 7 lines
Quoted 3 lines
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.
Quoted 2 lines
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().
Quoted 3 lines
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.
Quoted 3 lines
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
Signature · 4 lines
Esben Haabendal, Senior Software Consultant DoréDevelopment ApS, Ved Stranden 1, 9560 Hadsund, DK-Denmark Phone: +45 51 92 53 93, E-mail: [email protected] WWW: http://www.doredevelopment.dk
Quoted 9 lines · Esben Haabendal
Quoted 5 lines · Thomas Gleixner
Quoted 3 lines
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 :)
Quoted 3 lines
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_chipfunction. 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.
Quoted 5 lines
Quoted 2 lines
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.
Quoted 5 lines
Quoted 3 lines
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.
Quoted 8 lines
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.
Quoted 9 lines
Quoted 2 lines
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.
Quoted 15 lines
Quoted 3 lines
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 withdisable_irq_nosync()callingchip_bus_lock()andchip_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.
Quoted 5 lines
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 !
Quoted 6 lines
Quoted 3 lines
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_irqin 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
Quoted 11 lines · Thomas Gleixner
Quoted 3 lines
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_chipfunction. 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_chipfunctions withirq_desc->lockheld 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.
Quoted 2 lines
So the
irq_chipfunctions 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.
Quoted 3 lines
After releasing
desc->lockthe 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.
Quoted 6 lines
Therefor we take the buslock before we take
desc->lockand release
it after unlockingdesc->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?
Quoted 2 lines
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 ;-)
Quoted 9 lines
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 :-)
Quoted 7 lines
Quoted 5 lines
Quoted 2 lines
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).
Quoted 4 lines
Quoted 4 lines
Quoted 3 lines
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.
Quoted 8 lines
Quoted 1 line
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.
Quoted 11 lines
Quoted 8 lines
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 :-)
Quoted 11 lines
Quoted 9 lines
Quoted 2 lines
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...
Quoted 6 lines
Quoted 5 lines
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.
Quoted 8 lines
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?
Quoted 8 lines
Quoted 6 lines
Quoted 3 lines
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_irqin 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?
Quoted 1 line
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/
Quoted 2 lines
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
Quoted 24 lines · Esben Haabendal
Quoted 11 lines · Thomas Gleixner
Quoted 3 lines
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_chipfunction. 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_chipfunctions withirq_desc->lockheld 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.Quoted 2 lines
So the
irq_chipfunctions modify driver local state, which might be
modified by non irq related code as well.
Quoted 8 lines
Which is not done here.
Theirq_chipfunctions 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.
Quoted 6 lines
The handler is implemented in
drivers/net/phy/phy.cand is used by all phy drivers indrivers/net/phy/Quoted 2 lines
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.
Quoted 6 lines
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.
Quoted 5 lines
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
Quoted 2 lines · Thomas Gleixner
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 ofloade= d
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 function= s
that are not allowed to be called in that context. But now I know :-)
/Esben
--=20
Esben Haabendal, Senior Software Consultant Dor=E9Development ApS, Ved Stranden 1, 9560 Hadsund, DK-Denmark
Phone: +45 51 92 53 93, E-mail: [email protected]
WWW: http://www.doredevelopment.dk
Quoted 5 lines · Esben Haabendal
Quoted 2 lines · Thomas Gleixner
Maybe you understand now, why I was pretty sure upfront, that your approach was wrong even without knowing all the gory details ? :)
Quoted 8 lines
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.
Quoted 3 lines
Oh, I still think that the
disable_irq_nosyncdocumentaiton 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
Quoted 15 lines · Thomas Gleixner
Quoted 5 lines · Esben Haabendal
Quoted 2 lines · Thomas Gleixner
Maybe you understand now, why I was pretty sure upfront, that your approach was wrong even without knowing all the gory details ? :)
Quoted 8 lines
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.
Quoted 4 lines
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.
Quoted 5 lines
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.
Quoted 3 lines
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
Quoted 6 lines
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.
Quoted 6 lines
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.
Quoted 4 lines
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
Quoted 7 lines · Esben Haabendal
Quoted 2 lines · Thomas Gleixner
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
Quoted 14 lines · Thomas Gleixner
Quoted 7 lines · Esben Haabendal
Quoted 2 lines · Thomas Gleixner
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 thein_atomic()check is your fiddling with the nested irq dispatcher.
Quoted 7 lines
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
Quoted 8 lines · Marc Zyngier
Quoted 5 lines · On Sat, 5 Jun 2010 15:56:01 +0200 Esben Haabendal <[email protected]>…
I have a board with an I2C PCA9535 chip with two PHY interrupt lines hooked up to. The pca953x driver calls
set_irq_nested_threadon all irq's on initialization. The PHY driver then callsrequest_irq, and has no idea that it should actually be using a threaded handler.
Quoted 6 lines
Quoted 2 lines
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
Quoted 12 lines · On Sat, 5 Jun 2010 18:53:53 +0200 Esben Haabendal <[email protected]>…
Quoted 11 lines · Marc Zyngier
Quoted 7 lines · On Sat, 5 Jun 2010 15:56:01 +0200 Esben Haabendal <[email protected]>…
I have a board with an I2C PCA9535 chip with two PHY interrupt lines hooked up to. The pca953x driver calls
set_irq_nested_threadon all irq's on initialization. The PHY driver then callsrequest_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).
Quoted 6 lines
Quoted 3 lines
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 ;-).
Quoted 2 lines
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.
Signature · 3 lines
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.
Quoted 6 lines
Quoted 2 lines
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
Signature · 4 lines
Esben Haabendal, Senior Software Consultant DoréDevelopment ApS, Ved Stranden 1, 9560 Hadsund, DK-Denmark Phone: +45 51 92 53 93, E-mail: [email protected] WWW: http://www.doredevelopment.dk