From: Thomas Gleixner on
On Fri, 4 Jun 2010, Esben Haabendal wrote:

> set_irq_nested_thread() might be called by interrupt controller to
> supported nested irq thread handling, and with this change, drivers
> with non-threaded irq handlers can still use the irqs.

What's the problem you are trying to solve ? The above changelog is
not very useful ?

Thanks,

tglx

> Signed-off-by: Esben Haabendal <eha(a)doredevelopment.dk>
> ---
> kernel/irq/chip.c | 9 ++++++++-
> kernel/irq/manage.c | 4 +---
> 2 files changed, 9 insertions(+), 4 deletions(-)
>
> diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c
> index b7091d5..8202993 100644
> --- a/kernel/irq/chip.c
> +++ b/kernel/irq/chip.c
> @@ -405,7 +405,14 @@ void handle_nested_irq(unsigned int irq)
> desc->status |= IRQ_INPROGRESS;
> raw_spin_unlock_irq(&desc->lock);
>
> - action_ret = action->thread_fn(action->irq, action->dev_id);
> + if (desc->status & IRQ_NESTED_THREAD && action->thread_fn)
> + action_ret = action->thread_fn(action->irq, action->dev_id);
> + else {
> + local_irq_disable();
> + action_ret = action->handler(action->irq, action->dev_id);
> + local_irq_enable();
> + }
> +
> if (!noirqdebug)
> note_interrupt(irq, desc, action_ret);
>
> diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
> index 3164ba7..e7bca04 100644
> --- a/kernel/irq/manage.c
> +++ b/kernel/irq/manage.c
> @@ -681,10 +681,8 @@ __setup_irq(unsigned int irq, struct irq_desc *desc, struct irqaction *new)
> * Check whether the interrupt nests into another interrupt
> * thread.
> */
> - nested = desc->status & IRQ_NESTED_THREAD;
> + nested = (desc->status & IRQ_NESTED_THREAD) && new->thread_fn;
> if (nested) {
> - if (!new->thread_fn)
> - return -EINVAL;
> /*
> * Replace the primary handler which was provided from
> * the driver for non nested interrupt handling by the
> --
> 1.7.1
>
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Thomas Gleixner on
On Sat, 5 Jun 2010, Marc Zyngier wrote:

> On Sat, 5 Jun 2010 15:56:01 +0200
> Esben Haabendal <esbenhaabendal(a)gmail.com> wrote:
>
> > I have a board with an I2C PCA9535 chip with two PHY interrupt lines
> > hooked up to. The pca953x driver calls set_irq_nested_thread on all
> > irq's on initialization. The PHY driver then calls request_irq, and has
> > no idea that it should actually be using a threaded handler.
> >
> > With this patch, the PHY driver is able to work in this scenario
> > without changes (and so should any other driver using request_irq).
>
> You may want to give request_any_context_irq() a try (available since the
> latest merge window). It still requires your driver to be changed, but it
> should then work in both threaded and non-threaded cases.

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Thomas Gleixner on
Esben,

On Sat, 5 Jun 2010, Esben Haabendal wrote:

> On Sat, Jun 5, 2010 at 5:33 PM, Thomas Gleixner <tglx(a)linutronix.de> wrote:
> > On Sat, 5 Jun 2010, Marc Zyngier wrote:
> >> You may want to give request_any_context_irq() a try (available since the
> >> latest merge window). It still requires your driver to be changed, but it
> >> should then work in both threaded and non-threaded cases.
> >
> > And it nicely annotates that somebody looked at the driver in
> > question. That's the rule of least surprise and does not impose checks
> > on the fast path.
>
> What in particular should I be looking for in a driver before changing
> from request_irq() to request_any_context_irq() ?

Whether the irq handler relies on interrupts being disabled is the
most important thing. There are other constraints like
enable_irq/disable_irq calls from the driver code, which are not
allowed to run in atomic context for interrupt hanging of a i2c irq
controller.

> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Thomas Gleixner on
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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Thomas Gleixner on
On Mon, 7 Jun 2010, Esben Haabendal wrote:
> On Mon, Jun 7, 2010 at 5:06 PM, Thomas Gleixner <tglx(a)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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/