On Fri, 27 Feb 2009 13:50:32 -0800
David Brownell [off-list ref] wrote:

On Friday 27 February 2009, Felipe Balbi wrote:

quoted

quoted

quoted

+static irqreturn_t powerbutton_irq(int irq, void *dev_id)
+{
+______int err;
+______u8 value;
+
+#ifdef CONFIG_LOCKDEP
+______/* WORKAROUND for lockdep forcing IRQF_DISABLED on us, which
+______ * we don't want and can't tolerate. __Although it might be
+______ * friendlier not to borrow this thread context...
+______ */
+______local_irq_enable();
+#endif
+
+______err = twl4030_i2c_read_u8(TWL4030_MODULE_PM_MASTER, &value,
+______________________________________________________ __STS_HW_CONDITIONS);

And right there is the reason we can't tolerate IRQF_DISABLED:
this IRQ handler must run in a thread, since it needs to make
sleeping calls through the I2C stack.  (Typically using high
speed I2C -- 2.6 MHz or more -- so it's not pokey slow; but
this IRQ handler thread must still sleep.)

quoted

quoted

quoted

+______...

Tell us some more about this lockdep thing ;)

See kernel/irq/manage.c ... it forces IRQF_DISABLED on.

#ifdef CONFIG_LOCKDEP
	/*
	 * Lockdep wants atomic interrupt handlers:
	 */
	irqflags |= IRQF_DISABLED;
#endif

the comment is useless and the changelog ("[PATCH] lockdep: core") says
nothing about why this is here.  Great.

That periodically hiddes locking bugs, because it makes
debug kernels (with lockdep) act *very* differently
from normal ones "in the field" (no lockdep).

I suppose we could just take that out, see if we can find out why it
was added.

It also prevents some drivers from working with lockdep.
Two that come immediatly to mind are the AT91 and OMAP1
MMC/SD drivers.  (Though I suppose they might work if
they grow an #ifdef like above.)


The root cause is that the lock dependency analysis is
currently making a troublesome simplifying assumption:
all IRQ handlers disable IRQs.  At this point I think
it's fair to say most do NOT disable IRQs.

Peter Zijlstra was really hoping to Tom-Sawyer a fix
to this issue out of someone, but I think he gave up
on that approach a while back.  :)

I'd still like to see the appended patch merge, so
that developers at least get a heads-up when lockdep
introduces adds such gremlins to system testing.  Or,
various drivers could depend on !LOCKDEP ... but that
would only help (a little) *after* bugs get tracked
down to "root cause == lockdep", wasting much time.

I never noticed that lockdep was doing this.  Yes, it is pretty
regrettable.

quoted

David Brownell can comment better about it, that came from him when we
were converting twl4030 driver(s) to a more readable form :-)

If you take a look at all twl4030's children, you'll all of them needed
that.

Dave, could you comment, please ?

There are actually two issues.  The lockdep issue is
one ... the above #ifdef suffices to work around it
for all the TWL4030 (family) IRQs.

The other is that Linux needs real support for threaded
interrupts.  Almost every I2C (or SPI) device that raises
an IRQ needs its IRQ handler to run in a thread, and most
of them have the same type of workqueue-based hack to
get such a thread.  (Some others have bugs instead...)

Obviously, if any threaded IRQ handler grabs a mutex,
but lockdep has disabled IRQs, trouble ensues...

What's going on here?  hardirq handlers cannot take mutexes and cannot
sleep, period.  Enabling local interrupts might shut up some warning,
but won't fix the deadlocks which that warning is there to warn us
about.

Confused.

I suppose so, if we can't just stop doing this.