[PATCH 1/2] input: misc: add twl4030-pwrbutton driver

[PATCH 1/2] input: misc: add twl4030-pwrbutton driver

[Felipe Balbi]

From: Felipe Balbi <felipe.balbi@nokia.com>

This is part of the twl4030 multifunction device driver.

With this driver we add support for reporting KEY_POWER
events via the input layer.

Cc: David Brownell <dbrownell@users.sourceforge.net>
Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Cc: Samuel Ortiz <sameo@openedhand.com>
Signed-off-by: Felipe Balbi <felipe.balbi@nokia.com>
---
I guess this part should go via input tree if Sam is ok with it.

 drivers/input/misc/Kconfig             |    4 +
 drivers/input/misc/Makefile            |    1 +
 drivers/input/misc/twl4030-pwrbutton.c |  147 ++++++++++++++++++++++++++++++++
 3 files changed, 152 insertions(+), 0 deletions(-)
 create mode 100644 drivers/input/misc/twl4030-pwrbutton.c

diff --git a/drivers/input/misc/Kconfig b/drivers/input/misc/Kconfig
index 67e5553..9667b50 100644
--- a/drivers/input/misc/Kconfig
+++ b/drivers/input/misc/Kconfig
@@ -193,6 +193,10 @@ config INPUT_CM109
 	  To compile this driver as a module, choose M here: the module will be
 	  called cm109.
 
+config INPUT_TWL4030_PWRBUTTON
+	tristate "TWL4030 Power button Driver"
+	depends on TWL4030_CORE
+
 config INPUT_UINPUT
 	tristate "User level driver support"
 	help
diff --git a/drivers/input/misc/Makefile b/drivers/input/misc/Makefile
index bb62e6e..2fabcdb 100644
--- a/drivers/input/misc/Makefile
+++ b/drivers/input/misc/Makefile
@@ -19,6 +19,7 @@ obj-$(CONFIG_INPUT_YEALINK)		+= yealink.o
 obj-$(CONFIG_INPUT_CM109)		+= cm109.o
 obj-$(CONFIG_HP_SDC_RTC)		+= hp_sdc_rtc.o
 obj-$(CONFIG_INPUT_UINPUT)		+= uinput.o
+obj-$(CONFIG_INPUT_TWL4030_PWRBUTTON)	+= twl4030-pwrbutton.o
 obj-$(CONFIG_INPUT_APANEL)		+= apanel.o
 obj-$(CONFIG_INPUT_SGI_BTNS)		+= sgi_btns.o
 obj-$(CONFIG_INPUT_PCF50633_PMU)	+= pcf50633-input.o
diff --git a/drivers/input/misc/twl4030-pwrbutton.c b/drivers/input/misc/twl4030-pwrbutton.c
new file mode 100644
index 0000000..676d789
--- /dev/null
+++ b/drivers/input/misc/twl4030-pwrbutton.c
@@ -0,0 +1,147 @@
+/**
+ * drivers/i2c/chips/twl4030-pwrbutton.c
+ *
+ * Driver for sending triton2 power button event to input-layer
+ *
+ * Copyright (C) 2008-2009 Nokia Corporation
+ *
+ * Written by Peter De Schrijver <peter.de-schrijver@nokia.com>
+ *
+ * This file is subject to the terms and conditions of the GNU General
+ * Public License. See the file "COPYING" in the main directory of this
+ * archive for more details.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
+ */
+
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/kernel.h>
+#include <linux/errno.h>
+#include <linux/input.h>
+#include <linux/interrupt.h>
+#include <linux/platform_device.h>
+#include <linux/i2c/twl4030.h>
+
+#define PWR_PWRON_IRQ (1 << 0)
+
+#define STS_HW_CONDITIONS 0xf
+
+static struct input_dev *powerbutton_dev;
+static struct device *dbg_dev;
+
+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);
+	if (!err)  {
+		input_report_key(powerbutton_dev, KEY_POWER,
+				 value & PWR_PWRON_IRQ);
+	} else {
+		dev_err(dbg_dev, "twl4030: i2c error %d while reading TWL4030"
+			" PM_MASTER STS_HW_CONDITIONS register\n", err);
+	}
+
+	return IRQ_HANDLED;
+}
+
+static int __devinit twl4030_pwrbutton_probe(struct platform_device *pdev)
+{
+	int err = 0;
+	int irq = platform_get_irq(pdev, 0);
+
+	dbg_dev = &pdev->dev;
+
+	/* PWRBTN == PWRON */
+	err = request_irq(irq, powerbutton_irq,
+			IRQF_TRIGGER_FALLING | IRQF_TRIGGER_RISING,
+			"twl4030-pwrbutton", NULL);
+	if (err < 0) {
+		dev_dbg(&pdev->dev, "Can't get IRQ for pwrbutton: %d\n", err);
+		goto out;
+	}
+
+	powerbutton_dev = input_allocate_device();
+	if (!powerbutton_dev) {
+		dev_dbg(&pdev->dev, "Can't allocate power button\n");
+		err = -ENOMEM;
+		goto free_irq_and_out;
+	}
+
+	powerbutton_dev->evbit[0] = BIT_MASK(EV_KEY);
+	powerbutton_dev->keybit[BIT_WORD(KEY_POWER)] = BIT_MASK(KEY_POWER);
+	powerbutton_dev->name = "triton2-pwrbutton";
+
+	err = input_register_device(powerbutton_dev);
+	if (err) {
+		dev_dbg(&pdev->dev, "Can't register power button: %d\n", err);
+		goto free_input_dev;
+	}
+
+	dev_info(&pdev->dev, "triton2 power button driver initialized\n");
+
+	return 0;
+
+
+free_input_dev:
+	input_free_device(powerbutton_dev);
+free_irq_and_out:
+	free_irq(TWL4030_PWRIRQ_PWRBTN, NULL);
+out:
+	return err;
+}
+
+static int __devexit twl4030_pwrbutton_remove(struct platform_device *pdev)
+{
+	int irq = platform_get_irq(pdev, 0);
+
+	free_irq(irq, NULL);
+	input_unregister_device(powerbutton_dev);
+	input_free_device(powerbutton_dev);
+
+	return 0;
+}
+
+struct platform_driver twl4030_pwrbutton_driver = {
+	.probe		= twl4030_pwrbutton_probe,
+	.remove		= twl4030_pwrbutton_remove,
+	.driver		= {
+		.name	= "twl4030-pwrbutton",
+		.owner	= THIS_MODULE,
+	},
+};
+
+static int __init twl4030_pwrbutton_init(void)
+{
+	return platform_driver_register(&twl4030_pwrbutton_driver);
+}
+module_init(twl4030_pwrbutton_init);
+
+static void __exit twl4030_pwrbutton_exit(void)
+{
+	platform_driver_unregister(&twl4030_pwrbutton_driver);
+}
+module_exit(twl4030_pwrbutton_exit);
+
+MODULE_DESCRIPTION("Triton2 Power Button");
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Peter De Schrijver");
+
-- 
1.6.2.rc0.61.g5cd12

[PATCH 2/2] mfd: twl4030: add twl4030-pwrbutton as our child

[Felipe Balbi]

From: Felipe Balbi <felipe.balbi@nokia.com>

Make that twl4030-pwrbutton.c driver probe with current
child creation api for twl4030.

Cc: Samuel Ortiz <sameo@openedhand.com>
Cc: David Brownell <dbrownell@users.sourceforge.net>
Signed-off-by: Felipe Balbi <felipe.balbi@nokia.com>
---
This part is better off going via mfd.

 drivers/mfd/twl4030-core.c |   13 +++++++++++++
 1 files changed, 13 insertions(+), 0 deletions(-)

diff --git a/drivers/mfd/twl4030-core.c b/drivers/mfd/twl4030-core.c
index 68826f1..c86bc3b 100644
--- a/drivers/mfd/twl4030-core.c
+++ b/drivers/mfd/twl4030-core.c
@@ -101,6 +101,12 @@
 #define twl_has_usb()	false
 #endif
 
+#if defined(CONFIG_INPUT_TWL4030_PWRBUTTON) \
+	|| defined(CONFIG_INPUT_TWL4030_PWBUTTON_MODULE)
+#define twl_has_pwrbutton()	true
+#else
+#define twl_has_pwrbutton()	false
+#endif
 
 /* Triton Core internal information (BEGIN) */
 
@@ -526,6 +532,13 @@ add_children(struct twl4030_platform_data *pdata, unsigned long features)
 		usb_transceiver = child;
 	}
 
+	if (twl_has_pwrbutton()) {
+		child = add_child(1, "twl4030_pwrbutton",
+				NULL, 0, true, pdata->irq_base + 8 + 0, 0);
+		if (IS_ERR(child))
+			return PTR_ERR(child);
+	}
+
 	if (twl_has_regulator()) {
 		/*
 		child = add_regulator(TWL4030_REG_VPLL1, pdata->vpll1);
-- 
1.6.2.rc0.61.g5cd12

Re: [PATCH 2/2] mfd: twl4030: add twl4030-pwrbutton as our child

[Andrew Morton]

On Fri, 27 Feb 2009 21:28:03 +0200
Felipe Balbi <me@felipebalbi.com> wrote:

> From: Felipe Balbi <felipe.balbi@nokia.com>
> 
> Make that twl4030-pwrbutton.c driver probe with current
> child creation api for twl4030.
> 
> Cc: Samuel Ortiz <sameo@openedhand.com>
> Cc: David Brownell <dbrownell@users.sourceforge.net>
> Signed-off-by: Felipe Balbi <felipe.balbi@nokia.com>
> ---
> This part is better off going via mfd.
> 
>  drivers/mfd/twl4030-core.c |   13 +++++++++++++
>  1 files changed, 13 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/mfd/twl4030-core.c b/drivers/mfd/twl4030-core.c
> index 68826f1..c86bc3b 100644
> --- a/drivers/mfd/twl4030-core.c
> +++ b/drivers/mfd/twl4030-core.c
> @@ -101,6 +101,12 @@
>  #define twl_has_usb()	false
>  #endif
>  
> +#if defined(CONFIG_INPUT_TWL4030_PWRBUTTON) \
> +	|| defined(CONFIG_INPUT_TWL4030_PWBUTTON_MODULE)

OK, this is "wrong".  The core shouldn't need to know about specific
clients.

> +#define twl_has_pwrbutton()	true
> +#else
> +#define twl_has_pwrbutton()	false
> +#endif
>  
>  /* Triton Core internal information (BEGIN) */
>  
> @@ -526,6 +532,13 @@ add_children(struct twl4030_platform_data *pdata, unsigned long features)
>  		usb_transceiver = child;
>  	}
>  
> +	if (twl_has_pwrbutton()) {
> +		child = add_child(1, "twl4030_pwrbutton",
> +				NULL, 0, true, pdata->irq_base + 8 + 0, 0);
> +		if (IS_ERR(child))
> +			return PTR_ERR(child);
> +	}
> +
>  	if (twl_has_regulator()) {
>  		/*
>  		child = add_regulator(TWL4030_REG_VPLL1, pdata->vpll1);

The client module should register itself with the core, rather than the core
registering the client.

What has gone wrong here?

Re: [PATCH 2/2] mfd: twl4030: add twl4030-pwrbutton as our child

[David Brownell]

On Friday 27 February 2009, Andrew Morton wrote:
> > --- a/drivers/mfd/twl4030-core.c
> > +++ b/drivers/mfd/twl4030-core.c
> > @@ -101,6 +101,12 @@
> >  #define twl_has_usb()        false
> >  #endif
> >  
> > +#if defined(CONFIG_INPUT_TWL4030_PWRBUTTON) \
> > +     || defined(CONFIG_INPUT_TWL4030_PWBUTTON_MODULE)
> 
> OK, this is "wrong".  The core shouldn't need to know about specific
> clients.

This is a pretty standard idiom:  only create the device
nodes a system actually uses.  Applied comprehensively,
the kernel footprint shrinks ... supporting a device can
require a lot of ancillary infrastructure, which may not
need to be compiled in.


> > +#define twl_has_pwrbutton()  true
> > +#else
> > +#define twl_has_pwrbutton()  false
> > +#endif
> >  
> >  /* Triton Core internal information (BEGIN) */
> >  
> > @@ -526,6 +532,13 @@ add_children(struct twl4030_platform_data *pdata, unsigned long features)
> >               usb_transceiver = child;
> >       }
> >  
> > +     if (twl_has_pwrbutton()) {
> > +             child = add_child(1, "twl4030_pwrbutton",
> > +                             NULL, 0, true, pdata->irq_base + 8 + 0, 0);
> > +             if (IS_ERR(child))
> > +                     return PTR_ERR(child);
> > +     }
> > +
> >       if (twl_has_regulator()) {
> >               /*
> >               child = add_regulator(TWL4030_REG_VPLL1, pdata->vpll1);
> 
> The client module should register itself with the core, rather than the core
> registering the client.
> 
> What has gone wrong here?

Not much I can see.  It's registering a platform_device,
but only if it could be used on this system.  Quite a lot
of OMAP3 boards don't hook up this power button.

Maybe it should also verify that *this* board supports
a power button.  A boolean flag in the twl4030 platform
data would suffice, for multi-board kernels.

- Dave
 

Re: [PATCH 2/2] mfd: twl4030: add twl4030-pwrbutton as our child

[Felipe Balbi]

On Fri, Feb 27, 2009 at 01:58:46PM -0800, David Brownell wrote:
> On Friday 27 February 2009, Andrew Morton wrote:
> > > --- a/drivers/mfd/twl4030-core.c
> > > +++ b/drivers/mfd/twl4030-core.c
> > > @@ -101,6 +101,12 @@
> > >  #define twl_has_usb()        false
> > >  #endif
> > >  
> > > +#if defined(CONFIG_INPUT_TWL4030_PWRBUTTON) \
> > > +     || defined(CONFIG_INPUT_TWL4030_PWBUTTON_MODULE)
> > 
> > OK, this is "wrong".  The core shouldn't need to know about specific
> > clients.
> 
> This is a pretty standard idiom:  only create the device
> nodes a system actually uses.  Applied comprehensively,
> the kernel footprint shrinks ... supporting a device can
> require a lot of ancillary infrastructure, which may not
> need to be compiled in.
> 
> 
> > > +#define twl_has_pwrbutton()  true
> > > +#else
> > > +#define twl_has_pwrbutton()  false
> > > +#endif
> > >  
> > >  /* Triton Core internal information (BEGIN) */
> > >  
> > > @@ -526,6 +532,13 @@ add_children(struct twl4030_platform_data *pdata, unsigned long features)
> > >               usb_transceiver = child;
> > >       }
> > >  
> > > +     if (twl_has_pwrbutton()) {
> > > +             child = add_child(1, "twl4030_pwrbutton",
> > > +                             NULL, 0, true, pdata->irq_base + 8 + 0, 0);
> > > +             if (IS_ERR(child))
> > > +                     return PTR_ERR(child);
> > > +     }
> > > +
> > >       if (twl_has_regulator()) {
> > >               /*
> > >               child = add_regulator(TWL4030_REG_VPLL1, pdata->vpll1);
> > 
> > The client module should register itself with the core, rather than the core
> > registering the client.
> > 
> > What has gone wrong here?
> 
> Not much I can see.  It's registering a platform_device,
> but only if it could be used on this system.  Quite a lot
> of OMAP3 boards don't hook up this power button.
> 
> Maybe it should also verify that *this* board supports
> a power button.  A boolean flag in the twl4030 platform
> data would suffice, for multi-board kernels.

something like the following should be enough then:

============== cut here ===================
diff --git a/drivers/mfd/twl4030-core.c b/drivers/mfd/twl4030-core.c
index c86bc3b..714073f 100644
--- a/drivers/mfd/twl4030-core.c
+++ b/drivers/mfd/twl4030-core.c
@@ -532,7 +532,7 @@ add_children(struct twl4030_platform_data *pdata, unsigned long features)
                usb_transceiver = child;
        }
 
-       if (twl_has_pwrbutton()) {
+       if (twl_has_pwrbutton() && pdata->pwrbutton) {
                child = add_child(1, "twl4030_pwrbutton",
                                NULL, 0, true, pdata->irq_base + 8 + 0, 0);
                if (IS_ERR(child))
diff --git a/include/linux/i2c/twl4030.h b/include/linux/i2c/twl4030.h
index 8137f66..277451e 100644
--- a/include/linux/i2c/twl4030.h
+++ b/include/linux/i2c/twl4030.h
@@ -293,6 +293,8 @@ struct twl4030_platform_data {
        struct regulator_init_data              *vaux3;
        struct regulator_init_data              *vaux4;
 
+       bool                                    pwrbutton;
+
        /* REVISIT more to come ... _nothing_ should be hard-wired */
 };
 

-- 
balbi

Re: [PATCH 2/2] mfd: twl4030: add twl4030-pwrbutton as our child

[Andrew Morton]

On Fri, 27 Feb 2009 13:58:46 -0800
David Brownell <david-b@pacbell.net> wrote:

> On Friday 27 February 2009, Andrew Morton wrote:
> > > --- a/drivers/mfd/twl4030-core.c
> > > +++ b/drivers/mfd/twl4030-core.c
> > > @@ -101,6 +101,12 @@
> > > __#define twl_has_usb()________________false
> > > __#endif
> > > __
> > > +#if defined(CONFIG_INPUT_TWL4030_PWRBUTTON) \
> > > +__________|| defined(CONFIG_INPUT_TWL4030_PWBUTTON_MODULE)
> > 
> > OK, this is "wrong". __The core shouldn't need to know about specific
> > clients.
> 
> This is a pretty standard idiom:

That doesn't make it right.

>  only create the device
> nodes a system actually uses.

That happens automatically if the nodes are made when the client
registers itself with the core.

> > 
> > What has gone wrong here?
> 
> Not much I can see.  It's registering a platform_device,
> but only if it could be used on this system.

Again, that all gets fixed if this is done the right way around.  Run
your probe function.  If the hardware is there, register with the core
and all the nodes appear.  If the hardware is not present: bale.

The design of the whole subsystem appears to be upside down :(

Re: [PATCH 2/2] mfd: twl4030: add twl4030-pwrbutton as our child

[David Brownell]

On Friday 27 February 2009, Andrew Morton wrote:
> > > 
> > > What has gone wrong here?
> > 
> > Not much I can see.  It's registering a platform_device,
> > but only if it could be used on this system.
> 
> Again, that all gets fixed if this is done the right way around.  Run
> your probe function.  If the hardware is there, register with the core
> and all the nodes appear.  If the hardware is not present: bale.

Right.  If there's no button hooked up to its "power button"
signal, don't register that "twl4030-pwrbutton" node.


> The design of the whole subsystem appears to be upside down :(

You seem to be missing something.  Is it only the lack of that
tweak Felipe sent?

Re: [PATCH 2/2] mfd: twl4030: add twl4030-pwrbutton as our child

[Andrew Morton]

On Fri, 27 Feb 2009 15:20:56 -0800
David Brownell <david-b@pacbell.net> wrote:

> On Friday 27 February 2009, Andrew Morton wrote:
> > > > 
> > > > What has gone wrong here?
> > > 
> > > Not much I can see. __It's registering a platform_device,
> > > but only if it could be used on this system.
> > 
> > Again, that all gets fixed if this is done the right way around. __Run
> > your probe function. __If the hardware is there, register with the core
> > and all the nodes appear. __If the hardware is not present: bale.
> 
> Right.  If there's no button hooked up to its "power button"
> signal, don't register that "twl4030-pwrbutton" node.
> 
> 
> > The design of the whole subsystem appears to be upside down :(
> 
> You seem to be missing something.  Is it only the lack of that
> tweak Felipe sent?

I generally just delete unchangelogged patches.

<goes back and finds it>


That doesn't address the problem at all.  A function called
"add_children" just shouldn't exist.  The general kernel design is for
client drivers to register themselves with the core, so the core does
not have any hard-wired knowledge of any client drivers.

IOW, twl4030_pwrbutton_probe() should call into twl4030-core,
registering the powerbutton driver.

Re: [PATCH 2/2] mfd: twl4030: add twl4030-pwrbutton as our child

[Trilok Soni]

Hi Andrew/Samuel,

On Sat, Feb 28, 2009 at 5:12 AM, Andrew Morton<akpm@linux-foundation.org> wrote:
> On Fri, 27 Feb 2009 15:20:56 -0800
> David Brownell <david-b@pacbell.net> wrote:
>
>> On Friday 27 February 2009, Andrew Morton wrote:
>> > > >
>> > > > What has gone wrong here?
>> > >
>> > > Not much I can see. __It's registering a platform_device,
>> > > but only if it could be used on this system.
>> >
>> > Again, that all gets fixed if this is done the right way around. __Run
>> > your probe function. __If the hardware is there, register with the core
>> > and all the nodes appear. __If the hardware is not present: bale.
>>
>> Right.  If there's no button hooked up to its "power button"
>> signal, don't register that "twl4030-pwrbutton" node.
>>
>>
>> > The design of the whole subsystem appears to be upside down :(
>>
>> You seem to be missing something.  Is it only the lack of that
>> tweak Felipe sent?
>
> I generally just delete unchangelogged patches.
>
> <goes back and finds it>
>
>
> That doesn't address the problem at all.  A function called
> "add_children" just shouldn't exist.  The general kernel design is for
> client drivers to register themselves with the core, so the core does
> not have any hard-wired knowledge of any client drivers.
>
> IOW, twl4030_pwrbutton_probe() should call into twl4030-core,
> registering the powerbutton driver.
>

Reviving this very old thread.

Andrew,

It seems that every driver under "mfd" is following the more or less
approach like twl4030, where the core driver is creating sub-device
driver's platform devices based on either some conditions like
platform data for that sub-device driver is available or not, or
sub-device array with it's "name" and "id" is passed from arch/ board
files.

Samuel,

I don't see any further discussion of points raised by Andrew in this driver.


-- 
---Trilok Soni
http://triloksoni.wordpress.com
http://www.linkedin.com/in/triloksoni

Re: [PATCH 2/2] mfd: twl4030: add twl4030-pwrbutton as our child

[David Brownell]

On Wednesday 22 July 2009, Trilok Soni wrote:
> > That doesn't address the problem at all.  A function called
> > "add_children" just shouldn't exist.  The general kernel design is for
> > client drivers to register themselves with the core, so the core does
> > not have any hard-wired knowledge of any client drivers.

The general kernel design also involves bus drivers
(consider PCI or USB) enumerating their child devices
and then *adding* those children ... that's what is
done here.  The devices supported by that system get
added, and then the driver model code can bind each
device to its driver.


> > IOW, twl4030_pwrbutton_probe() should call into twl4030-core,
> > registering the powerbutton driver.

It calls to the driver model to register that driver,
which then sees that the device is registered.

- Dave


> 
> Reviving this very old thread.
> 

Re: [PATCH 1/2] input: misc: add twl4030-pwrbutton driver

[Andrew Morton]

On Fri, 27 Feb 2009 21:28:02 +0200
Felipe Balbi <me@felipebalbi.com> wrote:

> +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);
> +	if (!err)  {
> +		input_report_key(powerbutton_dev, KEY_POWER,
> +				 value & PWR_PWRON_IRQ);
> +	} else {
> +		dev_err(dbg_dev, "twl4030: i2c error %d while reading TWL4030"
> +			" PM_MASTER STS_HW_CONDITIONS register\n", err);
> +	}
> +
> +	return IRQ_HANDLED;
> +}

Tell us some more about this lockdep thing ;)

Re: [PATCH 1/2] input: misc: add twl4030-pwrbutton driver

[Felipe Balbi]

On Fri, Feb 27, 2009 at 12:33:44PM -0800, Andrew Morton wrote:
> On Fri, 27 Feb 2009 21:28:02 +0200
> Felipe Balbi <me@felipebalbi.com> wrote:
> 
> > +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);
> > +	if (!err)  {
> > +		input_report_key(powerbutton_dev, KEY_POWER,
> > +				 value & PWR_PWRON_IRQ);
> > +	} else {
> > +		dev_err(dbg_dev, "twl4030: i2c error %d while reading TWL4030"
> > +			" PM_MASTER STS_HW_CONDITIONS register\n", err);
> > +	}
> > +
> > +	return IRQ_HANDLED;
> > +}
> 
> Tell us some more about this lockdep thing ;)

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 ?

-- 
balbi

Re: lockdep and threaded IRQs (was: [PATCH 1/2] input: misc: add twl4030-pwrbutton driver)

[David Brownell]

On Friday 27 February 2009, Felipe Balbi wrote:
> 
> > > +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.)


> > > +   ...
> > 
> > Tell us some more about this lockdep thing ;)

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

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

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.


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

I've lost track of the status of the threaded IRQ stuff,
but the last proposal I saw from Thomas Gleixner looked
like it omitted IRQ chaining support that TWL4030 type
chips need.  See drivers/mfd/twl4030-irq.c for what
AFAIK is the only in-tree example of an irq_chip where
the guts of the irq_chip methods themselves must run in
threads (to mask/ack/... IRQs using i2c registers).
Presumably the threaded IRQ support will offer cleaner
ways to handle such stuff.

- Dave


======== CUT HERE
From: David Brownell <dbrownell@users.sourceforge.net>

When lockdep turns on IRQF_DISABLED, emit a warning to make it
easier to track down problems this introduces in drivers that
expect handlers to run with IRQs enabled.

Signed-off-by: David Brownell <dbrownell@users.sourceforge.net>
---
 kernel/irq/manage.c |    6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -689,7 +689,11 @@ int request_irq(unsigned int irq, irq_ha
 	/*
 	 * Lockdep wants atomic interrupt handlers:
 	 */
-	irqflags |= IRQF_DISABLED;
+	if (!(irqflags & IRQF_DISABLED)) {
+		pr_warning("IRQ %d/%s: lockdep sets IRQF_DISABLED\n",
+				irq, devname);
+		irqflags |= IRQF_DISABLED;
+	}
 #endif
 	/*
 	 * Sanity-check: shared interrupts must pass in a real dev-ID,

Re: lockdep and threaded IRQs (was: [PATCH 1/2] input: misc: add twl4030-pwrbutton driver)

[Andrew Morton]

On Fri, 27 Feb 2009 13:50:32 -0800
David Brownell <david-b@pacbell.net> wrote:

> On Friday 27 February 2009, Felipe Balbi wrote:
> > 
> > > > +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.)
> 
> 
> > > > +______...
> > > 
> > > 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.

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

> From: David Brownell <dbrownell@users.sourceforge.net>
> 
> When lockdep turns on IRQF_DISABLED, emit a warning to make it
> easier to track down problems this introduces in drivers that
> expect handlers to run with IRQs enabled.
> 
> Signed-off-by: David Brownell <dbrownell@users.sourceforge.net>
> ---
>  kernel/irq/manage.c |    6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> --- a/kernel/irq/manage.c
> +++ b/kernel/irq/manage.c
> @@ -689,7 +689,11 @@ int request_irq(unsigned int irq, irq_ha
>  	/*
>  	 * Lockdep wants atomic interrupt handlers:
>  	 */
> -	irqflags |= IRQF_DISABLED;
> +	if (!(irqflags & IRQF_DISABLED)) {
> +		pr_warning("IRQ %d/%s: lockdep sets IRQF_DISABLED\n",
> +				irq, devname);
> +		irqflags |= IRQF_DISABLED;
> +	}
>  #endif

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

Re: lockdep and threaded IRQs (was: ...)

[David Brownell]

On Friday 27 February 2009, Andrew Morton wrote:
> 
> > There are actually two issues.  The lockdep issue is
> > one ... the above #ifdef suffices to work around it
> > for all the TWL4030 (family) IRQs.

(And other similar cases, of course, including those
MMC cases that crap out in the ARM memory management
code when lockdep wrongly disables 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.  

But these handlers are *NOT* running in hardirq context;
so such thought doesn't apply.  The hardirq bit of the
IRQ-handling activity completed quite a bit earlier.

Being able to sleep and grab mutexes is one of the main
reasons for wanting threaded IRQ handlers.  For common
(embedded) I2C hardware, if threaded IRQ handlers can't
sleep they're completely useless.

And something that may eventually matter for mainline:
in RT contexts, thread priority applies to mutex grabbing,
and helps manage priority inversion.  That's equivalent
to sleeping.

This stuff just pokes at some annoying current gaps in the
IRQ framework.  I'll be glad when eventually there's no
need to work around those weaknesses ... that is, when
real threaded IRQ support is available.


> Enabling local interrupts might shut up some warning, 
> but won't fix the deadlocks which that warning is there to warn us
> about.

Wrong; all such warnings are false, in these cases.

- Dave

Re: lockdep and threaded IRQs (was: ...)

[Andrew Morton]

On Fri, 27 Feb 2009 15:18:57 -0800
David Brownell <david-b@pacbell.net> wrote:

> On Friday 27 February 2009, Andrew Morton wrote:
> > 
> > > There are actually two issues. __The lockdep issue is
> > > one ... the above #ifdef suffices to work around it
> > > for all the TWL4030 (family) IRQs.
> 
> (And other similar cases, of course, including those
> MMC cases that crap out in the ARM memory management
> code when lockdep wrongly disables 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. __
> 
> But these handlers are *NOT* running in hardirq context;
> so such thought doesn't apply.

But you just told me that this code (which is an IRQ handler) needs to
sleep and take sleeping locks.

Let us start again.  Why does this function:

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);
	if (!err)  {
		input_report_key(powerbutton_dev, KEY_POWER,
				 value & PWR_PWRON_IRQ);
	} else {
		dev_err(dbg_dev, "twl4030: i2c error %d while reading TWL4030"
			" PM_MASTER STS_HW_CONDITIONS register\n", err);
	}

	return IRQ_HANDLED;
}

Which is connected up via this statement:

	err = request_irq(irq, powerbutton_irq,
			IRQF_TRIGGER_FALLING | IRQF_TRIGGER_RISING,
			"twl4030-pwrbutton", NULL);

reenable local interrupts?

Re: lockdep and threaded IRQs (was: ...)

[Andrew Morton]

On Fri, 27 Feb 2009 15:32:04 -0800
Andrew Morton <akpm@linux-foundation.org> wrote:

> Why does this function:
> 
> 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);
> 	if (!err)  {
> 		input_report_key(powerbutton_dev, KEY_POWER,
> 				 value & PWR_PWRON_IRQ);
> 	} else {
> 		dev_err(dbg_dev, "twl4030: i2c error %d while reading TWL4030"
> 			" PM_MASTER STS_HW_CONDITIONS register\n", err);
> 	}
> 
> 	return IRQ_HANDLED;
> }
> 
> Which is connected up via this statement:
> 
> 	err = request_irq(irq, powerbutton_irq,
> 			IRQF_TRIGGER_FALLING | IRQF_TRIGGER_RISING,
> 			"twl4030-pwrbutton", NULL);
> 
> reenable local interrupts?

ah, OK, twl4030_i2c_read_u8() does i2c I/O.

Can't do that.  If some random process currently holds
mutex_lock(&twl->xfer_lock) and an interrupt occurs then this interrupt
handler will try to acquire mutex_lock(&twl->xfer_lock).  Deadlock.

Re: lockdep and threaded IRQs (was: ...)

[David Brownell]

On Friday 27 February 2009, Andrew Morton wrote:
> On Fri, 27 Feb 2009 15:32:04 -0800
> Andrew Morton <akpm@linux-foundation.org> wrote:
> 
> > Why does this function:
> > 
> > static irqreturn_t powerbutton_irq(int irq, void *dev_id)
> > {
> > 	... threaded irq handler body elided ...
> > }
> > 
> > Which is connected up via this statement:
> > 
> > 	err = request_irq(irq, powerbutton_irq,
> > 			IRQF_TRIGGER_FALLING | IRQF_TRIGGER_RISING,
> > 			"twl4030-pwrbutton", NULL);
> > 
> > reenable local interrupts?

Because threaded IRQ handlers are, well, threaded.  And all
the twl4030 IRQ handlers are threaded -- must be.  But when
CONFIG_LOCKDEP is enabled, it goofs such handlers ... as well
as a bunch of other perfectly functional driver code.

In the absense of the lockdep IRQF_DISABLED goofage, the IRQs
are properly dispatched -- IRQs stay enabled while these
handlers run, all the relevant locking invariants are obeyed.


> ah, OK, twl4030_i2c_read_u8() does i2c I/O.
> 
> Can't do that. 

Threaded IRQ handlers *can* do that.  That's the point.

Now, if you were to say "keep waiting a few more years
until some threaded IRQ framework finally merges" ...
the question comes up, "What to do in the meanwhile".
(Ditto, "well, we've been waiting a long time now to
see those threaded IRQs, what's up with them?")

"Nothing" is not an option.  The "something" being done
here is a reasonably clean approach, and doesn't call
for any surgery to kernel/irq/* ... the *only* problem
is the lockdep bug, which causes trouble for a variety
of other drivers too.


> If some random process currently holds 
> mutex_lock(&twl->xfer_lock) and an interrupt occurs then this interrupt
> handler will try to acquire mutex_lock(&twl->xfer_lock).  Deadlock.

No, no, no.  *THREADED IRQ HANDLER* at work here.  Bzzt.

Threaded IRQ handler.

The relevant mutexes are *never* accessed outside of a
thread context.  Not by this code.  Not by any other code.

Re: lockdep and threaded IRQs (was: ...)

[Andrew Morton]

On Fri, 27 Feb 2009 18:30:36 -0800 David Brownell <david-b@pacbell.net> wrote:

> > ah, OK, twl4030_i2c_read_u8() does i2c I/O.
> > 
> > Can't do that. 
> 
> Threaded IRQ handlers *can* do that.  That's the point.
> 
> Now, if you were to say "keep waiting a few more years
> until some threaded IRQ framework finally merges" ...
> the question comes up, "What to do in the meanwhile".
> (Ditto, "well, we've been waiting a long time now to
> see those threaded IRQs, what's up with them?")

I don't have a clue what you're talking about.  Where is this "threaded
irq handler" implementation of which you speak?  File and line?

Re: lockdep and threaded IRQs (was: ...)

[David Brownell]

On Friday 27 February 2009, Andrew Morton wrote:
> On Fri, 27 Feb 2009 18:30:36 -0800 David Brownell <david-b@pacbell.net> wrote:
> 
> > > ah, OK, twl4030_i2c_read_u8() does i2c I/O.
> > > 
> > > Can't do that. 
> > 
> > Threaded IRQ handlers *can* do that.  That's the point.
> > 
> > Now, if you were to say "keep waiting a few more years
> > until some threaded IRQ framework finally merges" ...
> > the question comes up, "What to do in the meanwhile".
> > (Ditto, "well, we've been waiting a long time now to
> > see those threaded IRQs, what's up with them?")
> 
> I don't have a clue what you're talking about.  Where is this "threaded
> irq handler" implementation of which you speak?  File and line?

You quoted the entirety of the *handler* a few messages ago
in this thread, where you asked the question which you
answer "ah, OK..." above.

Now, where is that being set up as being threaded?  I
referenced that a message or two earlier:

  drivers/mfd/twl4030-irq.c

Where you'll observe twl_init_irq() at line 688 setting
up the thread and the Primary IRQ Handler (PIH) dispatch.
That's pretty much bog-standard chained IRQ setup code,
except that it chains through a thread.

When an IRQ comes in, handle_twl4030_pih() acks and masks
that top level IRQ.  Then it wakes twl4030_irq_thread(),
which issues I2C operations to read the IRQ status from
the chip ... first PIH to find out which SIH modules are
raising an IRQ, then SIH to dispatch that status.  Then
handle_irq() from that thread to invoke the handler in
that thread context; it will issue more I2C ops.

And the lockdep thing kicks in through handle_irq(),
where the IRQ handler wrongly gets invoked with the
IRQs disabled -- iff lockdep is enabled.  Otherwise,
that IRQ thread is just like any other thread.

- Dave

Re: lockdep and threaded IRQs (was: ...)

[Andrew Morton]

On Fri, 27 Feb 2009 20:46:50 -0800 David Brownell <david-b@pacbell.net> wrote:

>   drivers/mfd/twl4030-irq.c
> 
> Where you'll observe twl_init_irq() at line 688 setting
> up the thread and the Primary IRQ Handler (PIH) dispatch.
> That's pretty much bog-standard chained IRQ setup code,
> except that it chains through a thread.

OK, that's clever.  I never knew that anyone was doing that.  afaict
MFD is the only such place...

Yes, it's regrettable that it's a private-to-mfd implementation.  I
expect a lot of i2c clients (at least) would like this.

> When an IRQ comes in, handle_twl4030_pih() acks and masks
> that top level IRQ.  Then it wakes twl4030_irq_thread(),
> which issues I2C operations to read the IRQ status from
> the chip ... first PIH to find out which SIH modules are
> raising an IRQ, then SIH to dispatch that status.  Then
> handle_irq() from that thread to invoke the handler in
> that thread context; it will issue more I2C ops.

yup.

> And the lockdep thing kicks in through handle_irq(),
> where the IRQ handler wrongly gets invoked with the
> IRQs disabled -- iff lockdep is enabled.  Otherwise,
> that IRQ thread is just like any other thread.

OK.

Perhaps it would be somewhat less dirty to do something like

--- a/kernel/irq/manage.c~a
+++ a/kernel/irq/manage.c
@@ -689,7 +689,8 @@ int request_irq(unsigned int irq, irq_ha
 	/*
 	 * Lockdep wants atomic interrupt handlers:
 	 */
-	irqflags |= IRQF_DISABLED;
+	if (!(irqflags & IRQF_NO_LOCKDEP_HACK))
+		irqflags |= IRQF_DISABLED;
 #endif
 	/*
 	 * Sanity-check: shared interrupts must pass in a real dev-ID,
_

Re: lockdep and threaded IRQs (was: ...)

[David Brownell]

On Friday 27 February 2009, Andrew Morton wrote:
> On Fri, 27 Feb 2009 20:46:50 -0800 David Brownell <david-b@pacbell.net> wrote:
> 
> >   drivers/mfd/twl4030-irq.c
> > 
> > Where you'll observe twl_init_irq() at line 688 setting
> > up the thread and the Primary IRQ Handler (PIH) dispatch.
> > That's pretty much bog-standard chained IRQ setup code,
> > except that it chains through a thread.
> 
> OK, that's clever.  I never knew that anyone was doing that.  afaict
> MFD is the only such place...

This is the Mark II code, which can look clever
because it uses genirq better than the fairly
ugly Mark I code (and doesn't clone the same
logic into three separate drivers).

What's unique about this particular code is that
it chains those IRQs, instead of creating its own
little irq core (like menelaus.c in that directory)
or only needing to handle one IRQ (as with RTC
alarm drivers).


> Yes, it's regrettable that it's a private-to-mfd implementation.

More like:  private-to-twl4030.  If a more generic
version (Mark III) comes along at some point, I'd
hope switching would be easy...


> I expect a lot of i2c clients (at least) would like this.

I2C, SPI, maybe some more ... yes.  RTC alarm handling,
as one fairly routine example.


> Perhaps it would be somewhat less dirty to do something like

Yeah, that #ifdef is pretty ugly.

Thomas sent some threaded IRQ patches around sometime
last fall, which handled the top level dispatch but
ISTR not the chaining part.  This kind of thing would
be appropriate for the chained IRQs.  Maybe the flag
would better be named IRQF_THREADED.

(However, this flavor hack wouldn't address the lockdep
breakage for drivers like the AT91 and OMAP1 MMC hosts.)

- Dave


> --- a/kernel/irq/manage.c~a
> +++ a/kernel/irq/manage.c
> @@ -689,7 +689,8 @@ int request_irq(unsigned int irq, irq_ha
>  	/*
>  	 * Lockdep wants atomic interrupt handlers:
>  	 */
> -	irqflags |= IRQF_DISABLED;
> +	if (!(irqflags & IRQF_NO_LOCKDEP_HACK))
> +		irqflags |= IRQF_DISABLED;
>  #endif
>  	/*
>  	 * Sanity-check: shared interrupts must pass in a real dev-ID,
> 

Re: lockdep and threaded IRQs (was: ...)

[Thomas Gleixner]

On Fri, 27 Feb 2009, David Brownell wrote:
>   drivers/mfd/twl4030-irq.c
> 
> Where you'll observe twl_init_irq() at line 688 setting
> up the thread and the Primary IRQ Handler (PIH) dispatch.
> That's pretty much bog-standard chained IRQ setup code,
> except that it chains through a thread.

So this would be the perfect candidate to test the generic threaded
code I posted a few days ago ?

Thanks,

	tglx

Re: lockdep and threaded IRQs (was: ...)

[David Brownell]

On Saturday 28 February 2009, Thomas Gleixner wrote:
> On Fri, 27 Feb 2009, David Brownell wrote:
> >   drivers/mfd/twl4030-irq.c
> > 
> > Where you'll observe twl_init_irq() at line 688 setting
> > up the thread and the Primary IRQ Handler (PIH) dispatch.
> > That's pretty much bog-standard chained IRQ setup code,
> > except that it chains through a thread.
> 
> So this would be the perfect candidate to test the generic threaded
> code I posted a few days ago ?

Could be.  URL?

Re: lockdep and threaded IRQs (was: ...)

[Thomas Gleixner]

On Sat, 28 Feb 2009, David Brownell wrote:
> On Saturday 28 February 2009, Thomas Gleixner wrote:
> > On Fri, 27 Feb 2009, David Brownell wrote:
> > >   drivers/mfd/twl4030-irq.c
> > > 
> > > Where you'll observe twl_init_irq() at line 688 setting
> > > up the thread and the Primary IRQ Handler (PIH) dispatch.
> > > That's pretty much bog-standard chained IRQ setup code,
> > > except that it chains through a thread.
> > 
> > So this would be the perfect candidate to test the generic threaded
> > code I posted a few days ago ?
> 
> Could be.  URL?

http://lkml.org/lkml/2009/2/26/146

	tglx
 

Re: lockdep and threaded IRQs (was: ...)

[David Brownell]

On Saturday 28 February 2009, Thomas Gleixner wrote:
> > > > 
> > > > Where you'll observe twl_init_irq() at line 688 setting
> > > > up the thread and the Primary IRQ Handler (PIH) dispatch.
> > > > That's pretty much bog-standard chained IRQ setup code,
> > > > except that it chains through a thread.
> > > 
> > > So this would be the perfect candidate to test the generic threaded
> > > code I posted a few days ago ?
> > 
> > Could be.  URL?
> 
> http://lkml.org/lkml/2009/2/26/146

Quick report:

 - patch 2/4 didn't apply, mainline uses GFP_ATOMIC in that
   memory allocation not GFP_KERNEL; obvious fix.

 - patch 4/4 also didn't apply, 5/15 hunks failed ... looks
   like IRQ affinity stuff.  free_irq() changes need rework,
   the others look to have obvious fixes.

Got a version that applies to mainline GIT?


At a quick glance it looks like these patches don't cover
set_irq_chained_handler(), which would be trouble since
__setup_irq() doesn't get called in those cases.

They should however handle simpler cases, like I2C devices
that only expose one IRQ instead of needing to demux several
dozen IRQs going to different drivers and subsystems.

And, not touching lockdep, the original ugliness will still
be needed (re-enabling IRQs in threaded handlers).

- Dave

Re: lockdep and threaded IRQs (was: ...)

[Thomas Gleixner]

On Sat, 28 Feb 2009, David Brownell wrote:
> Got a version that applies to mainline GIT?

http://tglx.de/~tglx/patches.tar.bz2 
 
> At a quick glance it looks like these patches don't cover
> set_irq_chained_handler(), which would be trouble since
> __setup_irq() doesn't get called in those cases.

Hmm, I did not think about chained handlers where the demux handler
needs to run in a thread as well. Usually demux handlers just have a
fast path kicking the particular real handlers.

> They should however handle simpler cases, like I2C devices
> that only expose one IRQ instead of needing to demux several
> dozen IRQs going to different drivers and subsystems.
> 
> And, not touching lockdep, the original ugliness will still
> be needed (re-enabling IRQs in threaded handlers).

Err ? The threaded handlers run with interrupts enabled.

static int irq_thread(void *data)
{
        ...

        while (!kthread_should_stop()) {
                set_current_state(TASK_INTERRUPTIBLE);
                if (irq_thread_should_run(action) && current->irqaction) {
                        __set_current_state(TASK_RUNNING);
                        action->handler(action->irq, action->dev_id);
                } else
                        schedule();
        }

Thanks,

	tglx

Re: lockdep and threaded IRQs (was: ...)

[Thomas Gleixner]

On Sat, 28 Feb 2009, Thomas Gleixner wrote:

> On Sat, 28 Feb 2009, David Brownell wrote:
> > Got a version that applies to mainline GIT?
> 
> http://tglx.de/~tglx/patches.tar.bz2 
>  
> > At a quick glance it looks like these patches don't cover
> > set_irq_chained_handler(), which would be trouble since
> > __setup_irq() doesn't get called in those cases.
> 
> Hmm, I did not think about chained handlers where the demux handler
> needs to run in a thread as well. Usually demux handlers just have a
> fast path kicking the particular real handlers.

I looked at the code in twl4030-irq.c and I don't think that the demux
handler needs to be setup as a chained handler at all.

Just register it as a standard handler and let it wakeup the other irq
threads or is there a particular requirement that the demuxed handlers
need to run in the same thread context as the demux handler itself ?

Thanks,

	tglx

Re: lockdep and threaded IRQs (was: ...)

[David Brownell]

On Saturday 28 February 2009, Thomas Gleixner wrote:
> > needs to run in a thread as well. Usually demux handlers just have a
> > fast path kicking the particular real handlers.
> 
> I looked at the code in twl4030-irq.c and I don't think that the demux
> handler needs to be setup as a chained handler at all. 
> 
> Just register it as a standard handler and let it wakeup the other irq
> threads

Not clear what you mean by that.  Did I miss a "wakeup an
irq thread" operation?  You've coupled the task to the
irqaction, not to the irq_desc.


> or is there a particular requirement that the demuxed handlers 
> need to run in the same thread context as the demux handler itself ?

The toplevel IRQ shouldn't be re-enabled until all the chained
handlers have actually handled their events ... some of those
events are level triggered too.  That needs synchronization, and
the simplest synch is to just process those demuxed events in
the same thread.

The TWL4030 family chips are a little bit quirky with respect
to IRQ handling (PIH vs SIH, SIH_CTRL.COR inconsistency, etc),
but they aren't the only chips with such requirements for their
internal IRQ controllers.

- Dave

Re: lockdep and threaded IRQs (was: ...)

[David Brownell]

On Saturday 28 February 2009, Thomas Gleixner wrote:
> On Sat, 28 Feb 2009, David Brownell wrote:
> > Got a version that applies to mainline GIT?
> 
> http://tglx.de/~tglx/patches.tar.bz2 

Got it, thanks.


> > At a quick glance it looks like these patches don't cover
> > set_irq_chained_handler(), which would be trouble since
> > __setup_irq() doesn't get called in those cases.
> 
> Hmm, I did not think about chained handlers where the demux handler
> needs to run in a thread as well. Usually demux handlers just have a
> fast path kicking the particular real handlers.

That can't work when the demux needs to access state across
I2C in order to see which "real" handlers to kick.  :)


> > They should however handle simpler cases, like I2C devices
> > that only expose one IRQ instead of needing to demux several
> > dozen IRQs going to different drivers and subsystems.
> > 
> > And, not touching lockdep, the original ugliness will still
> > be needed (re-enabling IRQs in threaded handlers).
> 
> Err ? The threaded handlers run with interrupts enabled.

Hmm, I'll have a closer look.  You changed handle_IRQ_event()
which is where the relevant IRQF_DISABLED test kicks in.  In
your updated code, that pokes any quick_check_handler() and
then maybe pokes a per-IRQ thread.

That seems to presume a hardirq-to-taskirq handoff.  But the
problem case is taskirq-to-taskirq chaining, through e.g.
what set_irq_chip_and_handler() provided.  (Details not very
amenable to brief emails, just UTSL.)

Thing is, I'm not sure a per-IRQ thread can work easily with
that chaining.  The chained IRQs can need to be handled before
the top-level IRQ gets re-enabled.  That's why the twl4030-irq
code uses just one taskirq thread for all incoming events.

(Which of course is rarely more than one at a time, so there's
little reason not to share that task between the demuxing code
and the events being demuxed.  Interrupts that need processing
via I2C/SPI/etc are more or less by definition not frequent or
performance-critical.)

- Dave

Re: lockdep and threaded IRQs (was: ...)

[Thomas Gleixner]

On Sat, 28 Feb 2009, David Brownell wrote:
> That seems to presume a hardirq-to-taskirq handoff.  But the
> problem case is taskirq-to-taskirq chaining, through e.g.
> what set_irq_chip_and_handler() provided.  (Details not very
> amenable to brief emails, just UTSL.)
> 
> Thing is, I'm not sure a per-IRQ thread can work easily with
> that chaining.  The chained IRQs can need to be handled before
> the top-level IRQ gets re-enabled.  That's why the twl4030-irq
> code uses just one taskirq thread for all incoming events.

This can be solved by a completion as well.
 
> (Which of course is rarely more than one at a time, so there's
> little reason not to share that task between the demuxing code
> and the events being demuxed.  Interrupts that need processing
> via I2C/SPI/etc are more or less by definition not frequent or
> performance-critical.)

Then all we need to provide in the generic code is a function which
does not go through the handle_IRQ_event() logic and calls the action
handler directly. Not rocket science to do that and better than using
a facility which is designed to run in hardirq context and expect that
it works in thread context without complaints.

Thanks,

	tglx

Re: lockdep and threaded IRQs (was: ...)

[David Brownell]

On Sunday 01 March 2009, Thomas Gleixner wrote:
> On Sat, 28 Feb 2009, David Brownell wrote:
> > That seems to presume a hardirq-to-taskirq handoff.  But the
> > problem case is taskirq-to-taskirq chaining, through e.g.
> > what set_irq_chip_and_handler() provided.  (Details not very
> > amenable to brief emails, just UTSL.)
> > 
> > Thing is, I'm not sure a per-IRQ thread can work easily with
> > that chaining.  The chained IRQs can need to be handled before
> > the top-level IRQ gets re-enabled.  That's why the twl4030-irq
> > code uses just one taskirq thread for all incoming events.
> 
> This can be solved by a completion as well.

One of many potential solutions; it's probably a better
use case for a counting semaphore though, especially if
they were all going in parallel.  And of course there's
the issue of where that synch code lives...

Though I still don't see any real issue with keeping it
simple and serializing them without creating new threads.
In terms of resource consumption, that simple solution is
clearly superior.


> > (Which of course is rarely more than one at a time, so there's
> > little reason not to share that task between the demuxing code
> > and the events being demuxed.  Interrupts that need processing
> > via I2C/SPI/etc are more or less by definition not frequent or
> > performance-critical.)
> 
> Then all we need to provide in the generic code is a function which
> does not go through the handle_IRQ_event() logic and calls the action
> handler directly.

That is, something to replace handle_simple_irq() and
handle_edge_irq() flow handlers?  (irq_desc.handle_irq)


> Not rocket science to do that and better than using 
> a facility which is designed to run in hardirq context and expect that
> it works in thread context without complaints.

The main "complaint" is the pre-existing lockdep breakage.  :)

The need to call irq_desc.handle_irq() with IRQs disabled is a
bit strange, but not really a problem; and it ensures consistent
locking for the irq_desc statistics and flag updates.

- Dave

Re: lockdep and threaded IRQs (was: ...)

[Peter Zijlstra]

On Sun, 2009-03-01 at 14:54 -0800, David Brownell wrote:
> The main "complaint" is the pre-existing lockdep breakage.  :)

Right, I just send a patch to fix that, by simply fixing !lockdep to do
as lockdep does :-)

IRQF_DISABLED is bonkers, we should simply always disable interrupts for
interrupt handlers.

The only problem you have is your of your own tinkering.

Re: lockdep and threaded IRQs (was: ...)

[David Brownell]

On Monday 02 March 2009, Peter Zijlstra wrote:
> IRQF_DISABLED is bonkers,

Hmm, after all the work that's been done to get Linux
to the point where *most* drivers run without IRQs
enabled ... that sentiment surprises me.

And I suspect it would surprise most driver developers.


> we should simply always disable interrupts for 
> interrupt handlers.

That would be why you have refused to fix the bug
in lockdep, whereby it forcibly enables that flag?

I've been wondering for some months now why you've
left that bug unfixed.

Re: lockdep and threaded IRQs (was: ...)

[Peter Zijlstra]

On Mon, 2009-03-02 at 13:04 -0800, David Brownell wrote:
> On Monday 02 March 2009, Peter Zijlstra wrote:
> > IRQF_DISABLED is bonkers,
> 
> Hmm, after all the work that's been done to get Linux
> to the point where *most* drivers run without IRQs
> enabled ... that sentiment surprises me.
> 
> And I suspect it would surprise most driver developers.

How so?, its the natural extension of that work.

> > we should simply always disable interrupts for 
> > interrupt handlers.
> 
> That would be why you have refused to fix the bug
> in lockdep, whereby it forcibly enables that flag?
> 
> I've been wondering for some months now why you've
> left that bug unfixed.

Because running irq handlers with irqs enabled it plain silly.

Except it turns out there is some really broken ass hardware out there.
But supposedly IDE PIO could be done from a threaded handler.

Re: lockdep and threaded IRQs (was: ...)

[Andrew Morton]

On Mon, 02 Mar 2009 22:16:57 +0100
Peter Zijlstra <a.p.zijlstra@chello.nl> wrote:

> > > we should simply always disable interrupts for 
> > > interrupt handlers.
> > 
> > That would be why you have refused to fix the bug
> > in lockdep, whereby it forcibly enables that flag?
> > 
> > I've been wondering for some months now why you've
> > left that bug unfixed.
> 
> Because running irq handlers with irqs enabled it plain silly.

Do we get to revert irqstacks?

Re: lockdep and threaded IRQs (was: ...)

[David Brownell]

On Monday 02 March 2009, Peter Zijlstra wrote:
> On Mon, 2009-03-02 at 13:04 -0800, David Brownell wrote:
> > On Monday 02 March 2009, Peter Zijlstra wrote:
> > > IRQF_DISABLED is bonkers,
> > 
> > Hmm, after all the work that's been done to get Linux
> > to the point where *most* drivers run without IRQs

Typo:  "drivers run *with* IRQs enabled".  Not "without".
Should be evident from context.


> > enabled ... that sentiment surprises me.
> > 
> > And I suspect it would surprise most driver developers.
> 
> How so?, its the natural extension of that work.

Not the work to shrink the amount of time IRQ latencies
by shrinking the amount of time IRQs are disabled by
IRQ handlers.


> > > we should simply always disable interrupts for 
> > > interrupt handlers.
> > 
> > That would be why you have refused to fix the bug
> > in lockdep, whereby it forcibly enables that flag?
> > 
> > I've been wondering for some months now why you've
> > left that bug unfixed.
> 
> Because running irq handlers with irqs enabled it plain silly.

Not if you have hardware-prioritized IRQs, which are
fairly common in some environments ... handling an IRQ
for high priority device A needn't interfere with the
handler for lower priority device B, and the system
overall can work better.

Not if you need to shrink IRQ latencies by minimizing
irqs-off critical sections everywhere ... IRQ handlers
being common offenders for keeping IRQs off too long.

Not when IRQs can be disabled selectively around the
real critical sections ... so drivers can leave IRQs
enabled except in those brief sections.
 

Re: lockdep and threaded IRQs (was: ...)

[Peter Zijlstra]

On Mon, 2009-03-02 at 13:37 -0800, David Brownell wrote:

> > How so?, its the natural extension of that work.
> 
> Not the work to shrink the amount of time IRQ latencies
> by shrinking the amount of time IRQs are disabled by
> IRQ handlers.

Ugh, that's done by pushing work out of the hardirq context, not by
doing silly things like enabling irqs from hardirq context.

> > > > we should simply always disable interrupts for 
> > > > interrupt handlers.
> > > 
> > > That would be why you have refused to fix the bug
> > > in lockdep, whereby it forcibly enables that flag?
> > > 
> > > I've been wondering for some months now why you've
> > > left that bug unfixed.
> > 
> > Because running irq handlers with irqs enabled it plain silly.
> 
> Not if you have hardware-prioritized IRQs, which are
> fairly common in some environments ... handling an IRQ
> for high priority device A needn't interfere with the
> handler for lower priority device B, and the system
> overall can work better.
> 
> Not if you need to shrink IRQ latencies by minimizing
> irqs-off critical sections everywhere ... IRQ handlers
> being common offenders for keeping IRQs off too long.
> 
> Not when IRQs can be disabled selectively around the
> real critical sections ... so drivers can leave IRQs
> enabled except in those brief sections.

Yeah, and who gets to debug the stack overflow?

Hardware with irq prio levels can do a prio mask and use a stack per
prio level.

Re: lockdep and threaded IRQs (was: ...)

[David Brownell]

On Monday 02 March 2009, Peter Zijlstra wrote:
> > > How so?, its the natural extension of that work.
> > 
> > Not the work to shrink the amount of time IRQ latencies
> > by shrinking the amount of time IRQs are disabled by
> > IRQ handlers.
> 
> Ugh, that's done by pushing work out of the hardirq context,

That's one of many techniques currently used.

Tradeoffs don't always favor larger driver updates
and re-validation though.  Sometimes it's simpler
to just leverage the reality that "hardirq context"
does not require using IRQF_DISABLED.


> not by doing silly things like enabling irqs from hardirq context.

Somehow I'm certain you have NOT analysed every one of the
thousands of IRQ handlers in various Linux drivers to know
with certainty that's not the reason IRQ_DISABLED is cleared.

There are also *other* reasons to leave IRQ_DISABLED clear.

- Dave

Re: lockdep and threaded IRQs (was: ...)

[Peter Zijlstra]

On Mon, 2009-03-02 at 14:09 -0800, David Brownell wrote:
> On Monday 02 March 2009, Peter Zijlstra wrote:
> > > > How so?, its the natural extension of that work.
> > > 
> > > Not the work to shrink the amount of time IRQ latencies
> > > by shrinking the amount of time IRQs are disabled by
> > > IRQ handlers.
> > 
> > Ugh, that's done by pushing work out of the hardirq context,
> 
> That's one of many techniques currently used.
> 
> Tradeoffs don't always favor larger driver updates
> and re-validation though.  Sometimes it's simpler
> to just leverage the reality that "hardirq context"
> does not require using IRQF_DISABLED.

Simpler doesn't mean its a good idea, it opens the door to stack
overruns for one.

Its very unfortunate that people think its a good idea.

> > not by doing silly things like enabling irqs from hardirq context.
> 
> Somehow I'm certain you have NOT analysed every one of the
> thousands of IRQ handlers in various Linux drivers to know
> with certainty that's not the reason IRQ_DISABLED is cleared.
> 
> There are also *other* reasons to leave IRQ_DISABLED clear.

There might be reasons to do so, that doesn't mean its a good idea and
the desired thing to do.

I state that every !IRQF_DISABLED usage is a bug, either due to broken
hardware or broken drivers.

Re: lockdep and threaded IRQs (was: ...)

[David Brownell]

On Monday 02 March 2009, Peter Zijlstra wrote:
> I state that every !IRQF_DISABLED usage is a bug, either
> due to broken hardware or broken drivers.

That's a novel position.  You do realize that removing that
capability breaks drivers?

But if that's what is keeping you from fixing the lockdep bug,
why haven't you submitted patches to remove IRQF_DISABLED from
the kernel, and update all the drivers relying on IRQs being
enabled when their handlers run?

At least that would get the appropriate level of discussion.

- Dave

Re: lockdep and threaded IRQs (was: ...)

[Peter Zijlstra]

On Mon, 2009-03-02 at 14:40 -0800, David Brownell wrote:
> On Monday 02 March 2009, Peter Zijlstra wrote:
> > I state that every !IRQF_DISABLED usage is a bug, either
> > due to broken hardware or broken drivers.
> 
> That's a novel position.  You do realize that removing that
> capability breaks drivers?

Then we fix them.

> But if that's what is keeping you from fixing the lockdep bug,
> why haven't you submitted patches to remove IRQF_DISABLED from
> the kernel, and update all the drivers relying on IRQs being
> enabled when their handlers run?

I did so today. Just didn't realize things actually relied on it since
lockdep turned them off and my system has been working fine.

Your driver needs threaded interrupts, Thomas is working on that now,
and I saw a conversion of your driver to use that.

IDE PIO can hopefully also be converted to threaded interrupts.

After that I'll post patches to remove IRQF_DISABLED and provide a
another flag to quick-'fix' other iffy drivers.

Once such drivers are found we can work on proper fixes for them.

Re: lockdep and threaded IRQs (was: ...)

[David Brownell]

On Monday 02 March 2009, Peter Zijlstra wrote:
> > But if that's what is keeping you from fixing the lockdep bug,
> > why haven't you submitted patches to remove IRQF_DISABLED from
> > the kernel, and update all the drivers relying on IRQs being
> > enabled when their handlers run?
> 
> I did so today. Just didn't realize things actually relied on it since
> lockdep turned them off and my system has been working fine.

That patch did no such thing.  It added a BUG_ON(),
which has nothing to do with removing IRQF_DISABLED.


> Your driver needs threaded interrupts, Thomas is working on that now,
> and I saw a conversion of your driver to use that.

Thomas hasn't yet touched the issue of how to chain such IRQs
though ... I consider his v2 patches a decent start, with some
limitations that could be attributed to an x86 focus.


> IDE PIO can hopefully also be converted to threaded interrupts.

I have worked with ARMs with IDE support.  That's become
rare in new chips though, even for CF cards; it needs too
many signal wires.

- Dave

Re: lockdep and threaded IRQs (was: ...)

[Peter Zijlstra]

On Mon, 2009-03-02 at 15:29 -0800, David Brownell wrote:

> That patch did no such thing.  It added a BUG_ON(),
> which has nothing to do with removing IRQF_DISABLED.

http://lkml.org/lkml/2009/3/2/33

Re: lockdep and threaded IRQs

[David Miller]

From: Peter Zijlstra <a.p.zijlstra@chello.nl>
Date: Mon, 02 Mar 2009 23:19:31 +0100

> I state that every !IRQF_DISABLED usage is a bug, either due to broken
> hardware or broken drivers.

We'll send you the bill to have everyone's hardware
replaced :-)

Re: lockdep and threaded IRQs

[Andrew Morton]

On Mon, 02 Mar 2009 14:46:47 -0800 (PST)
David Miller <davem@davemloft.net> wrote:

> From: Peter Zijlstra <a.p.zijlstra@chello.nl>
> Date: Mon, 02 Mar 2009 23:19:31 +0100
> 
> > I state that every !IRQF_DISABLED usage is a bug, either due to broken
> > hardware or broken drivers.
> 
> We'll send you the bill to have everyone's hardware
> replaced :-)

yes, but with what?

No matter how fast all our interrupt handlers are, running them with
local interrupts disabled has to worsen the worst-case interrupt
latency.

I don't see how removing !IRQF_DISABLED improves the kernel - in fact
there's a latency argument for making !IRQF_DISABLED the default.

Re: lockdep and threaded IRQs

[Peter Zijlstra]

On Mon, 2009-03-02 at 14:57 -0800, Andrew Morton wrote:
> On Mon, 02 Mar 2009 14:46:47 -0800 (PST)
> David Miller <davem@davemloft.net> wrote:
> 
> > From: Peter Zijlstra <a.p.zijlstra@chello.nl>
> > Date: Mon, 02 Mar 2009 23:19:31 +0100
> > 
> > > I state that every !IRQF_DISABLED usage is a bug, either due to broken
> > > hardware or broken drivers.
> > 
> > We'll send you the bill to have everyone's hardware
> > replaced :-)
> 
> yes, but with what?
> 
> No matter how fast all our interrupt handlers are, running them with
> local interrupts disabled has to worsen the worst-case interrupt
> latency.
> 
> I don't see how removing !IRQF_DISABLED improves the kernel - in fact
> there's a latency argument for making !IRQF_DISABLED the default.

On preempt-rt all we do in the hardirq path is mask the interrupt line
and wake up a thread. That's the extreme end of low latency interrupts.

Arguably there is a middle way that works for !-rt.

However, striving to enable interrupts in all interrupt handlers is
asking for stack overruns. Interrupt nesting just isn't really helpful.

Re: lockdep and threaded IRQs

[Peter Zijlstra]

On Mon, 2009-03-02 at 14:46 -0800, David Miller wrote:
> From: Peter Zijlstra <a.p.zijlstra@chello.nl>
> Date: Mon, 02 Mar 2009 23:19:31 +0100
> 
> > I state that every !IRQF_DISABLED usage is a bug, either due to broken
> > hardware or broken drivers.
> 
> We'll send you the bill to have everyone's hardware
> replaced :-)

I'm not saying to remove support for such stuff, as long as we clearly
annotate that its due to broken ass hardware we can leave a IRQF_ENABLED
thingy in there.

Preferably such drivers would be converted to threaded interrupts, but I
thought Alan mentioned an IDE chipset that was so broken even that would
be impossible (could not mask the IRQ for it would corrupt stuff).

The thing I am strongly opposing though, is keeping interrupts enabled
for regular drivers on sane hardware.

Re: lockdep and threaded IRQs (was: ...)

[Alan Cox]

> Because running irq handlers with irqs enabled it plain silly.

It makes enormous sense to run with the ability to mask *just* the IRQ
that occurred and not others so that long lasting interrupts. 

There is another reason not to do this of course - the real time work
makes it essentially irrelevant by doing the job right and reducing the
entire problem space to software priority management.
 
> Except it turns out there is some really broken ass hardware out there.
> But supposedly IDE PIO could be done from a threaded handler.

Only if you support reliable masking of a single IRQ *after* the IRQ
handler returns. The real time patches can do that for some chipsets but
on the PC at least you are into the realms of deepest darkest APIC
weirdness and that requires Ingo's pointy hat and wand..

Alan

Re: lockdep and threaded IRQs (was: ...)

[David Brownell]

On Saturday 28 February 2009, Thomas Gleixner wrote:
> > Got a version that applies to mainline GIT?
> 
> http://tglx.de/~tglx/patches.tar.bz2

Very lightly tested patch appended ... it works
with the RTC IRQs, and there's no reason to think
any of the other not-chained-any-longer IRQs would
fail.  This doesn't change anything the irqthread
does, just how it's created and accessed.

- Dave


=====
Minimalist patch to twl4030-irq.c to use the new threaded IRQ
stuff.  For review, not merge; the original irq thread isn't
properly formatted any more -- to make the patch be as small as
possible.

This makes creating the IRQ thread a bit simpler, but the lack
of irq chaining support makes /proc/interrupts be wrong.  The
quickcheck code path necessarily grew longer than the previous
irq_desc.handle() code path.

---
 drivers/mfd/twl4030-irq.c |   88 +++++++++++---------------------------------
 1 file changed, 23 insertions(+), 65 deletions(-)

--- a/drivers/mfd/twl4030-irq.c
+++ b/drivers/mfd/twl4030-irq.c
@@ -172,46 +172,23 @@ static const struct sih sih_modules[6] =
 
 static unsigned twl4030_irq_base;
 
-static struct completion irq_event;
-
 /*
- * This thread processes interrupts reported by the Primary Interrupt Handler.
+ * Threaded IRQ handler, processing interrupts reported by
+ * the Primary Interrupt Handler module.
  */
-static int twl4030_irq_thread(void *data)
+static irqreturn_t twl4030_pih_handler(int irq, void *unused)
 {
-	long irq = (long)data;
-	struct irq_desc *desc = irq_to_desc(irq);
-	static unsigned i2c_errors;
-	const static unsigned max_i2c_errors = 100;
-
-	if (!desc) {
-		pr_err("twl4030: Invalid IRQ: %ld\n", irq);
-		return -EINVAL;
-	}
-
-	current->flags |= PF_NOFREEZE;
-
-	while (!kthread_should_stop()) {
 		int ret;
 		int module_irq;
 		u8 pih_isr;
 
-		/* Wait for IRQ, then read PIH irq status (also blocking) */
-		wait_for_completion_interruptible(&irq_event);
-
+		ret = IRQ_NONE;
 		ret = twl4030_i2c_read_u8(TWL4030_MODULE_PIH, &pih_isr,
 					  REG_PIH_ISR_P1);
 		if (ret) {
 			pr_warning("twl4030: I2C error %d reading PIH ISR\n",
 					ret);
-			if (++i2c_errors >= max_i2c_errors) {
-				printk(KERN_ERR "Maximum I2C error count"
-						" exceeded.  Terminating %s.\n",
-						__func__);
-				break;
-			}
-			complete(&irq_event);
-			continue;
+			goto done;
 		}
 
 		/* these handlers deal with the relevant SIH irq status */
@@ -225,7 +202,7 @@ static int twl4030_irq_thread(void *data
 				if (!d) {
 					pr_err("twl4030: Invalid SIH IRQ: %d\n",
 					       module_irq);
-					return -EINVAL;
+					continue;
 				}
 
 				/* These can't be masked ... always warn
@@ -234,44 +211,31 @@ static int twl4030_irq_thread(void *data
 				if (d->status & IRQ_DISABLED)
 					note_interrupt(module_irq, d,
 							IRQ_NONE);
-				else
+				else {
 					d->handle_irq(module_irq, d);
+					ret = IRQ_HANDLED;
+				}
 			}
 		}
 		local_irq_enable();
 
-		desc->chip->unmask(irq);
-	}
-
-	return 0;
+done:
+	/* unmask the IRQ; it retriggers as needed */
+	enable_irq(irq);
+	return ret;
 }
 
 /*
- * handle_twl4030_pih() is the desc->handle method for the twl4030 interrupt.
- * This is a chained interrupt, so there is no desc->action method for it.
  * Now we need to query the interrupt controller in the twl4030 to determine
  * which module is generating the interrupt request.  However, we can't do i2c
  * transactions in interrupt context, so we must defer that work to a kernel
- * thread.  All we do here is acknowledge and mask the interrupt and wakeup
+ * thread.  All we do here is mask the active-low interrupt and wakeup
  * the kernel thread.
  */
-static void handle_twl4030_pih(unsigned int irq, irq_desc_t *desc)
-{
-	/* Acknowledge, clear *AND* mask the interrupt... */
-	desc->chip->ack(irq);
-	complete(&irq_event);
-}
-
-static struct task_struct *start_twl4030_irq_thread(long irq)
+static irqreturn_t twl4030_pih_quickcheck(int irq, void *unused)
 {
-	struct task_struct *thread;
-
-	init_completion(&irq_event);
-	thread = kthread_run(twl4030_irq_thread, (void *)irq, "twl4030-irq");
-	if (!thread)
-		pr_err("twl4030: could not create irq %ld thread!\n", irq);
-
-	return thread;
+	disable_irq(irq);
+	return IRQ_WAKE_THREAD;
 }
 
 /*----------------------------------------------------------------------*/
@@ -691,7 +655,6 @@ int twl_init_irq(int irq_num, unsigned i
 
 	int			status;
 	int			i;
-	struct task_struct	*task;
 
 	/*
 	 * Mask and clear all TWL4030 interrupts since initially we do
@@ -733,18 +696,13 @@ int twl_init_irq(int irq_num, unsigned i
 		goto fail;
 	}
 
-	/* install an irq handler to demultiplex the TWL4030 interrupt */
-	task = start_twl4030_irq_thread(irq_num);
-	if (!task) {
-		pr_err("twl4030: irq thread FAIL\n");
-		status = -ESRCH;
-		goto fail;
-	}
-
-	set_irq_data(irq_num, task);
-	set_irq_chained_handler(irq_num, handle_twl4030_pih);
+	/* FIXME should be a "chained" handler else irq statistics lie */
+	status = request_threaded_irq(irq_num,
+			twl4030_pih_handler, twl4030_pih_quickcheck,
+			IRQF_THREADED, "twl4030", NULL);
 
-	return status;
+	if (status == 0)
+		return status;
 
 fail:
 	for (i = irq_base; i < irq_end; i++)

Re: lockdep and threaded IRQs

[Stefan Richter]

David Brownell wrote:
> Now, where is that being set up as being threaded?  I
> referenced that a message or two earlier:
> 
>   drivers/mfd/twl4030-irq.c
> 
> Where you'll observe twl_init_irq() at line 688 setting
> up the thread and the Primary IRQ Handler (PIH) dispatch.
> That's pretty much bog-standard chained IRQ setup code,
> except that it chains through a thread.
> 
> When an IRQ comes in, handle_twl4030_pih() acks and masks
> that top level IRQ.  Then it wakes twl4030_irq_thread(),
> which issues I2C operations to read the IRQ status from
> the chip ... first PIH to find out which SIH modules are
> raising an IRQ, then SIH to dispatch that status.  Then
> handle_irq() from that thread to invoke the handler in
> that thread context; it will issue more I2C ops.
> 
> And the lockdep thing kicks in through handle_irq(),
> where the IRQ handler wrongly gets invoked with the
> IRQs disabled -- iff lockdep is enabled.  Otherwise,
> that IRQ thread is just like any other thread.

Ah, so there /is/ a threaded IRQ handler implementation in the mainline,
down in some driver framework...

Why don't these drivers simply use <linux/workqueue.h>?
-- 
Stefan Richter
-=====-==--= --=- ===--
http://arcgraph.de/sr/

Re: lockdep and threaded IRQs

[David Brownell]

On Saturday 28 February 2009, Stefan Richter wrote:
> Ah, so there /is/ a threaded IRQ handler implementation in
> the mainline, down in some driver framework...

One that's a bit more powerful than the recent patch
proposal, I'd say, since it chains.

 
> Why don't these drivers simply use <linux/workqueue.h>?

Most do.  In fact, that one *does* ... what's your point?

If I hadn't know that more generic threaded IRQ support
was already on the way, I might have done more with that
particular driver than just massive cleanup.  I stopped
short of some changes that it seemed genirq should obviate
before too long.  As always, more cleanup *could* be done.

- Dave

Re: lockdep and threaded IRQs

[Stefan Richter]

David Brownell wrote:
>> Why don't these drivers simply use <linux/workqueue.h>?
> 
> Most do.  In fact, that one *does* ... what's your point?

Workqueue jobs are rarely seen doing
#ifdef CONFIG_LOCKDEP
	local_irq_enable();
#endif

> If I hadn't know that more generic threaded IRQ support
> was already on the way, I might have done more with that
> particular driver than just massive cleanup.

OK, makes sense.
-- 
Stefan Richter
-=====-==--= --=- ===--
http://arcgraph.de/sr/

Re: lockdep and threaded IRQs (was: ...)

[Peter Zijlstra]

On Fri, 2009-02-27 at 15:18 -0800, David Brownell wrote:
> 
> This stuff just pokes at some annoying current gaps in the
> IRQ framework.  I'll be glad when eventually there's no
> need to work around those weaknesses ... that is, when
> real threaded IRQ support is available.

Its unfortunate that you prefer these dinky little hacks over helping
out providing whatever infrastructure you need.

There's plenty good reasons for mandating that irq handlers run with
irqs disabled, if you need threaded handlers -- that's fine, but then
teach the generic code about them.

What you do _NOT_ do is hack your way around things, that's not how
Linux works, and by doing that you make the world a slightly worse place
for everyone.

Re: lockdep and threaded IRQs (was: ...)

[David Brownell]

On Monday 02 March 2009, Peter Zijlstra wrote:
> On Fri, 2009-02-27 at 15:18 -0800, David Brownell wrote:
> > 
> > This stuff just pokes at some annoying current gaps in the
> > IRQ framework.  I'll be glad when eventually there's no
> > need to work around those weaknesses ... that is, when
> > real threaded IRQ support is available.
> 
> Its unfortunate that you prefer these dinky little hacks over
> helping out providing whatever infrastructure you need.

That's an unfair and unreasonable criticism.  You don't
know what I "prefer", or how much time I have to "help"
with.  Plus, you discount the work involved in actually
tracking down root causes ... and your evaluation of at
least this issue is biased.

What's unfortunate is that you prefer not to fix that
IRQF_DISABLED bug in lockdep, which you co-"maintain".
When running with lockdep, that bug (a) introduces bugs
in some drivers and (b) hides bugs in others.  You've
rejected even a minimal warning fix, to help minimize
the amount of time developers waste on (a) and (b).

Attacking folk for having to cope with such bugs escalates
things beyond "unfortunate".  If lockdep is "maintained",
your response should be fixing that lockdep bug.  Once
that's done, all workarounds for that bug can be removed.


> There's plenty good reasons for mandating that irq handlers run with
> irqs disabled, if you need threaded handlers -- that's fine, but then
> teach the generic code about them.

There are many ways to "teach" such things, of course,
and the learning goes both ways ... the initial set of
perceived requirements changes when 

And since there have been folk at work on such issues for
some time, I'm not sure why you think I shouldn't give
them a fair chance to deliver, and help improve those
(long anticipated) solutions.  There's a saying that
"too many cooks spoil the broth"; and in software, it's
well known that extra developers aren't necessarily any
kind of help.


> What you do _NOT_ do is hack your way around things, that's not how
> Linux works, and by doing that you make the world a slightly worse place
> for everyone.

Linux has lots of bug workarounds and odd constraints, like
any other large system does.  The "world" is full of them;
DNA-coded machinery is felt to consist *mostly* of them.
(And many folk argue that monoculture is dangerous...)

The sign of a bad workaround is that it sits in core code
and impacts everything using it.  Like, oh, that lockdep
bug, preventing various systems from ever using it because
they rely on mechanisms broken by that bug.


On the other hand, the main valid complaint about one-line
workarounds is needing them at all.  Boiling down a system
misbehavior to a one-line workaround *IS* a way to help
make the (kernel) world a better place, by fingerpointing
a real issue ... setting trail markers for a better way.

In this case there's a real bug in the lockdep code.

The recent threaded IRQ proposal moves some support down
a layer (into genirq) and generalizes it a bit, but some
drivers (e.g. I2C ones) have threaded IRQs for a long time
now -- out of necessity, not just performance tweaking.

- Dave

Re: lockdep and threaded IRQs (was: ...)

[Peter Zijlstra]

On Mon, 2009-03-02 at 14:10 -0800, David Brownell wrote:

> What's unfortunate is that you prefer not to fix that
> IRQF_DISABLED bug in lockdep, which you co-"maintain".
> When running with lockdep, that bug (a) introduces bugs
> in some drivers and (b) hides bugs in others.  You've
> rejected even a minimal warning fix, to help minimize
> the amount of time developers waste on (a) and (b).

I've come to the conclusion that the only technically sound solution is
to do as I proposed today, utterly eliminate !IRQF_DISABLED handlers.

Apparently you had enough time to come up with the creative genirq abuse
of twl4030, I think that with a similar effort you could have
implemented generic threaded irq stuff like proposed by Thomas.

> Attacking folk for having to cope with such bugs escalates
> things beyond "unfortunate".  If lockdep is "maintained",
> your response should be fixing that lockdep bug.  Once
> that's done, all workarounds for that bug can be removed.

I state there is no lockdep bug in this respect. The bug is trying to
enable interrupts from hardirq context and running code that assumes
hardirq context from task context.

Re: lockdep and threaded IRQs (was: ...)

[David Brownell]

On Monday 02 March 2009, Peter Zijlstra wrote:
> On Mon, 2009-03-02 at 14:10 -0800, David Brownell wrote:
> 
> > What's unfortunate is that you prefer not to fix that
> > IRQF_DISABLED bug in lockdep, which you co-"maintain".
> > When running with lockdep, that bug (a) introduces bugs
> > in some drivers and (b) hides bugs in others.  You've
> > rejected even a minimal warning fix, to help minimize
> > the amount of time developers waste on (a) and (b).
> 
> I've come to the conclusion that the only technically sound solution is
> to do as I proposed today, utterly eliminate !IRQF_DISABLED handlers.

As you announced today.  If you truly believe that, then
you should at least submit a warning patch for 2.6.29-rc
("driver X isn't setting IRQF_DISABLED, reimplement!")
with a Documentation/feature-removal-schedule.txt plan
for removing that mechanism.  And maybe updating the
handling of IRQF_SHARED at the same time... most shared
IRQs don't want IRQF_DISABLED.  And ... surely more,
even just to start phasing out such a longstanding
mechanism.

(Or more likely, start a real LKML discussion on that
specific topic.  I suspect it won't complete in time
to merge such a patch with any sort of consensus.)


> Apparently you had enough time to come up with the creative genirq abuse
> of twl4030, I think that with a similar effort you could have
> implemented generic threaded irq stuff like proposed by Thomas.

Actually, I made time to clean that code up a lot;
I didn't come up with that original stuff at all.

Why would you think otherwise, after reading the
copyrights at the top of twl4030-irq.c ??

And when I was doing that cleanup, the Official Plan
was that Thomas' code would be ready for merge into
at least the -next tree soon.  I don't know about
you, but stepping all over his work didn't seem like
it would be at all constructive.  Having a significant
driver be ready to try using it ... seemed like a
more productive approach.

 
> > Attacking folk for having to cope with such bugs escalates
> > things beyond "unfortunate".  If lockdep is "maintained",
> > your response should be fixing that lockdep bug.  Once
> > that's done, all workarounds for that bug can be removed.
> 
> I state there is no lockdep bug in this respect. The bug is trying to
> enable interrupts from hardirq context and running code that assumes
> hardirq context from task context.

Well, I state that you're wrong about those points...

Regardless of what either of us states, the truth of
the matter is that there are drivers that don't work
with CONFIG_LOCKDEP and the *only* reason is the code
removed by the (untested) patchlet below:  semantics
of irqs change when lockdep is enabled.  (That is, your
characterization above is incorrect.)  I call that a
lockdep bug; you don't want to accept that label.

Are those incorrect lockdep assumptions really so hard
to fix?  Where do they surface?  I scanned lockdep.c
briefly, and it talks about "hardirq" in several places.
Is it conflating that with "irqs disabled"?  The two
concepts should be distinct.

- Dave


---
 kernel/irq/manage.c |    6 ------
 1 file changed, 6 deletions(-)

--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -816,12 +816,6 @@ int request_irq_quickcheck(unsigned int 
 				"guaranteed on shared IRQs\n",
 				irq, devname);
 
-#ifdef CONFIG_LOCKDEP
-	/*
-	 * Lockdep wants atomic interrupt handlers:
-	 */
-	irqflags |= IRQF_DISABLED;
-#endif
 	/*
 	 * Sanity-check: shared interrupts must pass in a real dev-ID,
 	 * otherwise we'll have trouble later trying to figure out

Re: lockdep and threaded IRQs (was: ...)

[Ingo Molnar]

* David Brownell <david-b@pacbell.net> wrote:

> On Monday 02 March 2009, Peter Zijlstra wrote:
> > On Mon, 2009-03-02 at 14:10 -0800, David Brownell wrote:
> > 
> > > What's unfortunate is that you prefer not to fix that
> > > IRQF_DISABLED bug in lockdep, which you co-"maintain".
> > > When running with lockdep, that bug (a) introduces bugs
> > > in some drivers and (b) hides bugs in others.  You've
> > > rejected even a minimal warning fix, to help minimize
> > > the amount of time developers waste on (a) and (b).
> > 
> > I've come to the conclusion that the only technically sound solution is
> > to do as I proposed today, utterly eliminate !IRQF_DISABLED handlers.
> 
> As you announced today.  If you truly believe that, then
> you should at least submit a warning patch for 2.6.29-rc
> ("driver X isn't setting IRQF_DISABLED, reimplement!")

i have changed the BUG_ON() to a WARN_ONCE() message so the 
warning is in place now.

> with a Documentation/feature-removal-schedule.txt plan for 
> removing that mechanism. [...]

you are misunderstanding the workings and purpose of 
feature-removal-schedule.txt. It is mainly used for 
functionality that is user-visible. It is sometimes used for 
functionality that a subsystem has exported to a lot of drivers 
consciously and which is being removed.

It is never used for a single driver finding a core kernel 
symbol and abusing it in a way that was never intended. Abuse of 
kernel internals by kernel code was never a 'feature'. Just 
because you find a symbol (which is not even exported to 
drivers) does not mean you can use it like that.

If you want to work on genirq threaded IRQ handlers them please 
check out and test the threaded IRQ handlers patches that are 
being worked on at lkml. See:

[patch 0/4] genirq: add infrastructure for threaded interrupt handlers V2

	Ingo

Re: lockdep and threaded IRQs (was: ...)

[David Brownell]

On Monday 02 March 2009, Ingo Molnar wrote:
> If you want to work on genirq threaded IRQ handlers them please 
> check out and test the threaded IRQ handlers patches that are 
> being worked on at lkml. See:
> 
> [patch 0/4] genirq: add infrastructure for threaded interrupt handlers V2

I did check them out, as noted earlier in this thread.

The significant omission is lack of support for chaining
such threads.  Example, an I2C device that exposes
several dozen IRQs with mask/ack/... operations that
require I2C access.

I'm not sure what Thomas intends to do with that issue,
if anything.  It does touch on messy bits of genirq.

Those V2 patches do look to handle simple cases well,
of the flavor that's often handled today by creating
a singlethreaded workqueue in the driver.  I think
it's good to have such support, but that's not enough
to handle the hardware I've come across.

- Dave

Re: lockdep and threaded IRQs (was: ...)

[Ingo Molnar]

* David Brownell <david-b@pacbell.net> wrote:

> On Monday 02 March 2009, Ingo Molnar wrote:
> > If you want to work on genirq threaded IRQ handlers them please 
> > check out and test the threaded IRQ handlers patches that are 
> > being worked on at lkml. See:
> > 
> > [patch 0/4] genirq: add infrastructure for threaded interrupt handlers V2
> 
> I did check them out, as noted earlier in this thread.
> 
> The significant omission is lack of support for chaining
> such threads.  Example, an I2C device that exposes
> several dozen IRQs with mask/ack/... operations that
> require I2C access.

Well, those are rarely used, embedded-only constructs - the main 
focus of IRQ threading patches are the more common patterns.

Since you care about them - could you please send patches on top 
of the IRQ threading patches to add support for them?

Thanks,

	Ingo

Re: lockdep and threaded IRQs (was: ...)

[David Brownell]

On Monday 02 March 2009, Ingo Molnar wrote:
> > 
> > The significant omission is lack of support for chaining
> > such threads.  Example, an I2C device that exposes
> > several dozen IRQs with mask/ack/... operations that
> > require I2C access.
> 
> Well, those are rarely used, embedded-only constructs - the main 
> focus of IRQ threading patches are the more common patterns.

Yes, mostly for embedded, where "system bus" more likely
means I2C than PCI.


> Since you care about them - could you please send patches on top 
> of the IRQ threading patches to add support for them?

I'll look at that, and try to prepare something on top
of the version of the threading patches that gets into
the -next tree.  I got the impression there was going
to be a v3 of those patches soonish...

I expect there will be two basic parts of that work:

 - One to cope with the upcoming change to handle_irq(),
   insisting that it live in hardirq context instead of
   just an irqs-off context (and thereby preventing use
   of standard chaining calls in irq threads, sigh).

 - Another to set up a chaining thread, since chain
   setup bypasses setup_irq() and friends.

That latter might touch what the v2 patches added,
since I'd want it to share code.

- Dave

p.s. Note that those changes would still leave the
     lockdep bug around ... it will still be breaking
     various drivers that use normal IRQs, by forcibly
     enabling IRQF_DISABLED.

Re: lockdep and threaded IRQs (was: ...)

[Ingo Molnar]

* David Brownell <david-b@pacbell.net> wrote:

> On Monday 02 March 2009, Ingo Molnar wrote:
> > > 
> > > The significant omission is lack of support for chaining
> > > such threads.  Example, an I2C device that exposes
> > > several dozen IRQs with mask/ack/... operations that
> > > require I2C access.
> > 
> > Well, those are rarely used, embedded-only constructs - the main 
> > focus of IRQ threading patches are the more common patterns.
> 
> Yes, mostly for embedded, where "system bus" more likely
> means I2C than PCI.
> 
> 
> > Since you care about them - could you please send patches on top 
> > of the IRQ threading patches to add support for them?
> 
> I'll look at that, and try to prepare something on top
> of the version of the threading patches that gets into
> the -next tree.  I got the impression there was going
> to be a v3 of those patches soonish...

Great! We'll sort out any conflicts so dont worry about that - 
you can pick up v2 just fine and post patches.

> I expect there will be two basic parts of that work:
> 
>  - One to cope with the upcoming change to handle_irq(),
>    insisting that it live in hardirq context instead of
>    just an irqs-off context (and thereby preventing use
>    of standard chaining calls in irq threads, sigh).
> 
>  - Another to set up a chaining thread, since chain
>    setup bypasses setup_irq() and friends.

If you mean to push the chaining bits into the IRQ thread too, i 
think the chaining bits actually should never be threaded. Is 
there a good reason to do that? It's not like they will really 
be preemptible (preempting a chaining thread would mean the 
whole demuxing chain is held up => bad).

> That latter might touch what the v2 patches added,
> since I'd want it to share code.

Sure.

> 
> - Dave
> 
> p.s. Note that those changes would still leave the
>      lockdep bug around ... it will still be breaking
>      various drivers that use normal IRQs, by forcibly
>      enabling IRQF_DISABLED.

it's not a bug - and i think Peter explained that already. It's 
not really breaking things either - we've had this for more than 
2 years.

	Ingo

Re: lockdep and threaded IRQs (was: ...)

[David Brownell]

On Monday 02 March 2009, Ingo Molnar wrote:
> > 
> > p.s. Note that those changes would still leave the
> >      lockdep bug around ... it will still be breaking
> >      various drivers that use normal IRQs, by forcibly
> >      enabling IRQF_DISABLED.
> 
> it's not a bug - and i think Peter explained that already. 

No.  But I did get a non-response that didn't include any
explanation, and relied totally on unfounded assertions
combined with the presumption that someday IRQF_DISABLED
will be forced on in all drivers.


> It's  
> not really breaking things either - we've had this for more than 
> 2 years.

I happened across two MMC host adapter drivers which it
broke.  They preceded the patch whereby lockdep changes
request_irq() semantics.  The drivers break only with
lockdep ... so platforms relying on those can't use lockdep.
No reason to think there aren't more such bugs lurking.

We also wasted quite a few months tracking down USB bugs
that were masked by that patch ... the indeterminacy of
IRQF_DISABLED behavior given IRQF_SHARED was hidden by
enabling lockdep.  Led to a lot of totally crapulous oops
traces, I assure you.

So ... it has most certainly broken things.  There has
seemed to be a bit of stick-fingers-in-ears going on, that
prevents hearing about such problems though.

- Dave

Re: lockdep and threaded IRQs (was: ...)

[Peter Zijlstra]

On Mon, 2009-03-02 at 18:37 -0800, David Brownell wrote:
> No.  But I did get a non-response that didn't include any
> explanation, and relied totally on unfounded assertions
> combined with the presumption that someday IRQF_DISABLED
> will be forced on in all drivers.

Enabling IRQs in hardirq context is BAD because:

 - IRQ handler nesting leads to stack overflow
 - It gives the false impression its OK for IRQ handlers to be slow,
   it is _NOT_, as you still generate horrible preemption latency.

Therefore IRQF_DISABLED _will_ be forced on everybody some day soon, and
I'll provide an IRQF_ENABLED for use by broken hardware only (and make a
TAINT flag for that too).

Re: lockdep and threaded IRQs (was: ...)

[Ingo Molnar]

* Peter Zijlstra <a.p.zijlstra@chello.nl> wrote:

> On Mon, 2009-03-02 at 18:37 -0800, David Brownell wrote:
> > No.  But I did get a non-response that didn't include any
> > explanation, and relied totally on unfounded assertions
> > combined with the presumption that someday IRQF_DISABLED
> > will be forced on in all drivers.
> 
> Enabling IRQs in hardirq context is BAD because:
> 
>  - IRQ handler nesting leads to stack overflow
>  - It gives the false impression its OK for IRQ handlers to be slow,
>    it is _NOT_, as you still generate horrible preemption latency.
> 
> Therefore IRQF_DISABLED _will_ be forced on everybody some day 
> soon, and I'll provide an IRQF_ENABLED for use by broken 
> hardware only (and make a TAINT flag for that too).

Basically the problem why !IRQF_DISABLED is bad that if there 
are enough interrupt handlers we can get nesting like this:

  <irq 20>
  <handler runs with irqs enabled>
      <irq 21>
      <handler runs with irqs enabled>
          <irq 22>
          <handler runs with irqs enabled>
              <irq 23>
              <handler runs with irqs enabled>
                  <irq 24>
                  <handler runs with irqs enabled>

Suppose each handler gets interruped while it already used up 
1000 bytes of the stack (conservative estimation - often it's 
more) - the above sequence is already 5000 bytes into the stack.

There is no protection against stack overflow there and such 
bugs can be _very_ hard to trigger and find. If there's a 
sufficient number of devices and a high enough load it can 
trigger spuriously.

Yes, in a few limited embedded environments where you dont have 
more than 3-4 IRQ sources you might decide that it's safe to do 
(or you might decide that you dont care). Also, there's a few 
legacy pieces of hardware with either very long hw access 
latencies or too short buffers. Plus there might be any number 
of other hw factors - or architecture details (such as the use 
of separate per IRQ stacks) that limit IRQ handler parallelism 
in practice.

So we'll have the quirk flag for the weird cases - but these are 
the exceptions that strengthen the general rule. The concept of 
enabling interrupts in a hardirq handler is a no-no on a general 
purpose kernel and no modern driver should make use of it.

I hope this explains why lockdep never supported this case.

	Ingo

Re: lockdep and threaded IRQs (was: ...)

[Alan Cox]

> Therefore IRQF_DISABLED _will_ be forced on everybody some day soon, and
> I'll provide an IRQF_ENABLED for use by broken hardware only (and make a
> TAINT flag for that too).

I don't think you understand how the kernel project works. If everyone
thinks your change is inappropriate it won't get in. You can talk about
forcing things all you like but "force" used that way generally means "new
maintainer required"

Re: lockdep and threaded IRQs (was: ...)

[Ingo Molnar]

* Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:

> > Therefore IRQF_DISABLED _will_ be forced on everybody some 
> > day soon, and I'll provide an IRQF_ENABLED for use by broken 
> > hardware only (and make a TAINT flag for that too).
> 
> I don't think you understand how the kernel project works. If 
> everyone thinks your change is inappropriate it won't get in.

The change that people had a problem with was the immediate 
removal of IRQF_ENABLED, and that's not on the plate anymore.

I dont think anyone offered any example where IRQF_ENABLED is 
used in a healthy way - they are all legacy or special hw quirks 
where we limp along with enabling IRQs in a hacky way.

Furthermore, even these quirky cases can be supported cleanly 
_without_ IRQF_ENABLED: where an IRQ handler can take a long 
time to execute, the handler can be converted to a threaded IRQ 
handler - where it's fine to enable IRQs as there are no stack 
nesting issues.

So there's no real technical problem here.

	Ingo

Re: lockdep and threaded IRQs (was: ...)

[Alan Cox]

> _without_ IRQF_ENABLED: where an IRQ handler can take a long 
> time to execute, the handler can be converted to a threaded IRQ 
> handler - where it's fine to enable IRQs as there are no stack 
> nesting issues.

Only if you can mask the interrupt on the APIC without losing it or
having the APIC throw a fit.
 
> So there's no real technical problem here.

In the long term no - but forcing people to make sudden changes to
critical I/O drivers isn't the right way to do it.

Re: lockdep and threaded IRQs (was: ...)

[Peter Zijlstra]

On Tue, 2009-03-03 at 10:30 +0000, Alan Cox wrote:
> 
> > So there's no real technical problem here.
> 
> In the long term no - but forcing people to make sudden changes to
> critical I/O drivers isn't the right way to do it.

My plan is:
 1) find all drivers that do not use IRQF_DISABLED
 2) add IRQF_LEGACY_ENABLED to those
 3) make IRQF_DISABLED 0 and remove its functionality
    while adding IRQF_LEGACY_ENABLED
 4) make request_irq() print a warning for IRQF_LEGACY_ENABLED
 5) make an actual IRQF_LEGACY_ENABLED irq firing taint the kernel

After that its cleanup time, and I'll try to help out converting some of
these to threaded IRQs where appropriate.

This will not break any driver, nor force sudden change. Stuff will
continue working as expected, just a little more boot noise for those
with crappy drivers/hardware.

Re: lockdep and threaded IRQs (was: ...)

[Ingo Molnar]

* Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:

> > _without_ IRQF_ENABLED: where an IRQ handler can take a long 
> > time to execute, the handler can be converted to a threaded 
> > IRQ handler - where it's fine to enable IRQs as there are no 
> > stack nesting issues.
> 
> Only if you can mask the interrupt on the APIC without losing 
> it or having the APIC throw a fit.

Hm, that reads like the boot IRQ erratas of certain chipsets - 
the APIC could throw a fit essentially locking up the system. 
FYI, we have fixes for that upstream already.

Do you have any description about that problem, which hardware 
it affects, whether it's manufactured today and any (ballpark 
figure) estimation about the Linux installed base on it? Can 
they live with the quirk flag?

> > So there's no real technical problem here.
> 
> In the long term no - but forcing people to make sudden 
> changes to critical I/O drivers isn't the right way to do it.

i think you severely over-estimate the importance and ratio of 
drivers that enable irqs within irq handlers. (Nor does anyone 
want to break them really - we want to have a sane default and 
we want to flag the broken cases as broken.)

The thing is, while you seem to spend precious resources on 
weird legacy cases, we have a _lot_ of everyday systems in 
bugzilla that do not boot or do not work for one reason or 
another. Most of that is not in the weird-hardware category at 
all.

You might also have noticed that over the past 2-3 years the 
term "hard lockup" in regression reports has gone down by about 
an order of magnitude - and much of that can be attributed to 
the lockdep coverage we have in place. So in terms of real 
everyday quality impact on Linux Peter is very, very, very 
efficient.

And frankly, while Peter's patch here needs modifications, as a 
maintainer i prefer Peter as a contributor so much not only 
because he is fantastically productive in terms of fixing 
locking crap all over the kernel, but also because he 
concentrates on the big picture and on the common case and on 
the net effect on Linux instead of just stubbornly concentrating 
on an extreme-0.01% of the hardware space.

So your attack on him is quite misguided and unfair:

>> [..] You can talk about forcing things all you like but 
>> "force" used that way generally means "new maintainer 
>> required" [...]

Btw., Peter submitted a genirq patch and FYI he does not 
maintain the genirq subsystem and never maintained it.

	Ingo

Re: lockdep and threaded IRQs (was: ...)

[Alan Cox]

> Hm, that reads like the boot IRQ erratas of certain chipsets - 
> the APIC could throw a fit essentially locking up the system. 
> FYI, we have fixes for that upstream already.

Good - certainly it used to be the case that masking APIC IRQs and
leaving them masked from the IRQ handled used to do funny things
sometimes.

> i think you severely over-estimate the importance and ratio of 
> drivers that enable irqs within irq handlers. (Nor does anyone 
> want to break them really - we want to have a sane default and 
> we want to flag the broken cases as broken.)

IDE. A lot less people use the IDE stack nowdays but its a big item and
getting it wrong tends to eat your files.


I do object to the attitude shown about "forcing" people. It's a
community project built by a large number of people on a mix of
pragmatic and elegant design balances. Maybe it's just unfortunate choice
of wording but it is the wrong sentiment.

Re: lockdep and threaded IRQs (was: ...)

[Ingo Molnar]

* Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:

> > Hm, that reads like the boot IRQ erratas of certain chipsets 
> > - the APIC could throw a fit essentially locking up the 
> > system. FYI, we have fixes for that upstream already.
> 
> Good - certainly it used to be the case that masking APIC IRQs 
> and leaving them masked from the IRQ handled used to do funny 
> things sometimes.

I think being careful is definitely warranted in the case of 
IDE. I'd not be surprised if not all chipsets are mapped via the 
boot-IRQ quirks: those quirks are opt-in based on PCI IDs - 
those tend to be the quirk mechanisms with the least coverage. 
(The IDs were also derived from enterprise testing of -rt, which 
tends to under-emphasise cheap broken chipsets.)

Plus the erratum you described about doing an IRQ masking 
mid-PIO-transfer confusing the chipset can also be a separate 
standalone bug not permitting an irq-masking based IRQ flow at 
all on such hardware.

So your worries are spot on IMO and are not being dismissed 
forcibly.

> > i think you severely over-estimate the importance and ratio 
> > of drivers that enable irqs within irq handlers. (Nor does 
> > anyone want to break them really - we want to have a sane 
> > default and we want to flag the broken cases as broken.)
> 
> IDE. A lot less people use the IDE stack nowdays but its a big 
> item and getting it wrong tends to eat your files.
> 
> I do object to the attitude shown about "forcing" people. It's 
> a community project built by a large number of people on a mix 
> of pragmatic and elegant design balances. Maybe it's just 
> unfortunate choice of wording but it is the wrong sentiment.

It was in the heat of the argument i think ...

(I do think we need to be somewhat less permissive in terms of 
weird driver practices, but that's just my opinion and not 
'enforced' via any artificial way.)

	Ingo

Re: lockdep and threaded IRQs (was: ...)

[Ingo Molnar]

* Ingo Molnar <mingo@elte.hu> wrote:

> 
> * Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:
> 
> > > _without_ IRQF_ENABLED: where an IRQ handler can take a long 
> > > time to execute, the handler can be converted to a threaded 
> > > IRQ handler - where it's fine to enable IRQs as there are no 
> > > stack nesting issues.
> > 
> > Only if you can mask the interrupt on the APIC without 
> > losing it or having the APIC throw a fit.
> 
> Hm, that reads like the boot IRQ erratas of certain chipsets - 
> the APIC could throw a fit essentially locking up the system. 
> FYI, we have fixes for that upstream already.
> 
> Do you have any description about that problem, which hardware 
> it affects, whether it's manufactured today and any (ballpark 
> figure) estimation about the Linux installed base on it? Can 
> they live with the quirk flag?

btw., i definitely do not say that threaded IRQ handlers will 
work in each an every case (it changes the hardware programming 
pattern and as such it can bring out new erratas) so i 
definitely agree with the argument that the conversion has to be 
careful and case by case.

The plan Peter outlined looks sane. In case you worry about a 
forced removal of irq-enable - you should not.

	Ingo

Re: lockdep and threaded IRQs (was: ...)

[David Brownell]

On Tuesday 03 March 2009, Ingo Molnar wrote:
> i think you severely over-estimate the importance and ratio of 
> drivers that enable irqs within irq handlers. (Nor does anyone 
> want to break them really - we want to have a sane default and 
> we want to flag the broken cases as broken.)

For the record, I've been running for some time now
with a patch that issues a warning for each IRQ that
lockdep forces to use IRQF_DISABLED.

On my x86 systems, pretty much every driver triggers
that warning.  Which makes me think maybe that shoe
is being placed on the wrong foot:  use of IRQF_DISABLED
is the *EXCEPTION* not the rule.  At least on one major
Linux platform...

Re: lockdep and threaded IRQs (was: ...)

[David Brownell]

On Monday 02 March 2009, Ingo Molnar wrote:
> 
> > > Since you care about them - could you please send patches on top 
> > > of the IRQ threading patches to add support for them?
> > 
> > I'll look at that, and try to prepare something on top
> > of the version of the threading patches that gets into
> > the -next tree.  I got the impression there was going
> > to be a v3 of those patches soonish...
> 
> Great! We'll sort out any conflicts so dont worry about that - 
> you can pick up v2 just fine and post patches.

One such patch is about to come, with different $SUBJECT
and trimmed CC list.

Note however that it's completely independent of Thomas'
patches.  It only affects the IRQ dispatch sub-problem;
other bits seem to be needed too.


> If you mean to push the chaining bits into the IRQ thread too, i 
> think the chaining bits actually should never be threaded. Is 
> there a good reason to do that? 

The chaining bits *MUST* be threaded.  The lack of that
support was a key issue with Thomas' patch.  The issue
being that access to the IRQ status registers, as needed
to dispatch the IRQ, is only possible in contexts that
can sleep.  That seems like a good reason.  :)


> It's not like they will really  
> be preemptible (preempting a chaining thread would mean the 
> whole demuxing chain is held up => bad).

See above.  The reason to thread this IRQ handling is
that it can't be done outside of threads ... there's
no other way to access registers via I2C (or SPI, etc).

Accordingly there's no way to avoid preemption ... but
since these IRQs aren't on performance-critical paths,
that's really no bother.

[patch/rfc 0/2] handle_threaded_irq()

[David Brownell]

This is a followup to help address one of the omissions in the
threaded IRQ patches (v2) posted recently.  To recap, those
provided a quickcheck() routine that could wake an irq thread,
and a way to register a handler using that mechanism.

This addresses an orthogonal problem:  given some thread
demultiplexing the subsidiary IRQs reported to some top-level
interrupt, how can that kick in flow handlers chaining to
the subsidiary IRQs' handlers?

The answer here is more or less as suggested by Thomas:
using handle_threaded_irq(), a flow handler which doesn't
use handle_IRQ_event() since that doesn't much like being
called from thread context, at least when lockdep is active.

These two patches:

 - add handle_threaded_irq() flow handler;
 - kick it in for some twl4030 code

Tested on 2.6.29-rc8 code, both with and without the v2
irqthread patches.

[patch/rfc 1/2] GENIRQ: add handle_threaded_irq() flow handler

[David Brownell]

From: David Brownell <dbrownell@users.sourceforge.net>

Define a new flow handler, handle_threaded_irq(), for IRQ threads
to use when chaining IRQs.

Unlike existing flow handlers, handle_simple_irq() and siblings,
this one is used only from sleep-capable contexts.  It always calls
irqaction handlers from that same (shared) sleep-capable context.

This is independent of Thomas' irq threading patchset, and can be
viewed as a complement to it.  This adds support for IRQs whose
handlers must *ONLY* ever run in thread contexts ... instead of
offloading code from hardirq context into a thread.  Another way
this differs is that it doesn't create more kernel threads; it
only leverages an existing thread.

Signed-off-by: David Brownell <dbrownell@users.sourceforge.net>
---
 include/linux/irq.h |    7 ++++-
 kernel/irq/chip.c   |   61 ++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 66 insertions(+), 2 deletions(-)

--- a/include/linux/irq.h
+++ b/include/linux/irq.h
@@ -283,8 +283,8 @@ static inline int irq_balancing_disabled
 extern int handle_IRQ_event(unsigned int irq, struct irqaction *action);
 
 /*
- * Built-in IRQ handlers for various IRQ types,
- * callable via desc->chip->handle_irq()
+ * IRQ flow handlers for various IRQ types, callable via
+ * generic_handle_irq*() or desc->handle_irq()
  */
 extern void handle_level_irq(unsigned int irq, struct irq_desc *desc);
 extern void handle_fasteoi_irq(unsigned int irq, struct irq_desc *desc);
@@ -293,6 +293,9 @@ extern void handle_simple_irq(unsigned i
 extern void handle_percpu_irq(unsigned int irq, struct irq_desc *desc);
 extern void handle_bad_irq(unsigned int irq, struct irq_desc *desc);
 
+/* Flow handler that must only be called from sleeping context */
+extern void handle_threaded_irq(unsigned int irq, struct irq_desc *desc);
+
 /*
  * Monolithic do_IRQ implementation.
  */
--- a/kernel/irq/chip.c
+++ b/kernel/irq/chip.c
@@ -295,6 +295,67 @@ static inline void mask_ack_irq(struct i
 }
 
 /**
+ *	handle_threaded_irq - flow handler reusing current irq thread
+ *	@irq:	the interrupt number
+ *	@desc:	the interrupt description structure for this irq
+ *	Context: irq thread, with IRQs enabled
+ *
+ *	IRQ threads which demultiplex IRQs may use this flow handler
+ *	to chain those demultiplexed IRQs to subsidiary handlers, when
+ *	all that IRQ dispatch logic must run in sleeping contexts.
+ *
+ *	Examples include some multifunction I2C and SPI based devices
+ *	(where access to registers, including ones involved in IRQ
+ *	dispatching, requires sleeping) that have multiple independent
+ *	maskable interupts.
+ *
+ *	The irq thread using this flow handler must handle any ack,
+ *	clear, mask or unmask issues needed.
+ */
+void
+handle_threaded_irq(unsigned int irq, struct irq_desc *desc)
+{
+	struct irqaction *action;
+	irqreturn_t action_ret;
+
+	spin_lock_irq(&desc->lock);
+
+	if (unlikely(desc->status & IRQ_INPROGRESS))
+		goto out_unlock;
+	desc->status &= ~(IRQ_REPLAY | IRQ_WAITING);
+	kstat_incr_irqs_this_cpu(irq, desc);
+
+	action = desc->action;
+	if (unlikely(!action || (desc->status & IRQ_DISABLED)))
+		goto out_unlock;
+
+	desc->status |= IRQ_INPROGRESS;
+	spin_unlock_irq(&desc->lock);
+
+	/* simplified handle_IRQ_event():  no random sampling;
+	 * IRQs are always enabled so action->handler may sleep;
+	 * no hooks for handing off to yet another irq thread.
+	 */
+	action_ret = IRQ_NONE;
+	do {
+		/* REVISIT can we get some explicit knowledge that this
+		 * handler expects to run in thread context?  Maybe an
+		 * IRQF_THREADED check, or a new handler type ...
+		 */
+		action_ret |= action->handler(irq, action->dev_id);
+		action = action->next;
+	} while (action);
+
+	if (!noirqdebug)
+		note_interrupt(irq, desc, action_ret);
+
+	spin_lock_irq(&desc->lock);
+	desc->status &= ~IRQ_INPROGRESS;
+out_unlock:
+	spin_unlock_irq(&desc->lock);
+}
+
+/**
  *	handle_simple_irq - Simple and software-decoded IRQs.
  *	@irq:	the interrupt number
  *	@desc:	the interrupt description structure for this irq

Re: [patch/rfc 1/2] GENIRQ: add handle_threaded_irq() flow handler

[Felipe Balbi]

Hi,

On Wed, Mar 18, 2009 at 03:19:47AM +0100, David Brownell wrote:

[..]

> @@ -295,6 +295,67 @@ static inline void mask_ack_irq(struct i
>  }
>  
>  /**
> + *	handle_threaded_irq - flow handler reusing current irq thread
> + *	@irq:	the interrupt number
> + *	@desc:	the interrupt description structure for this irq
> + *	Context: irq thread, with IRQs enabled
> + *
> + *	IRQ threads which demultiplex IRQs may use this flow handler
> + *	to chain those demultiplexed IRQs to subsidiary handlers, when
> + *	all that IRQ dispatch logic must run in sleeping contexts.
> + *
> + *	Examples include some multifunction I2C and SPI based devices
> + *	(where access to registers, including ones involved in IRQ
> + *	dispatching, requires sleeping) that have multiple independent
> + *	maskable interupts.
> + *
> + *	The irq thread using this flow handler must handle any ack,
> + *	clear, mask or unmask issues needed.
> + */
> +void
> +handle_threaded_irq(unsigned int irq, struct irq_desc *desc)
> +{
> +	struct irqaction *action;
> +	irqreturn_t action_ret;
> +
> +	spin_lock_irq(&desc->lock);
> +
> +	if (unlikely(desc->status & IRQ_INPROGRESS))
> +		goto out_unlock;
> +	desc->status &= ~(IRQ_REPLAY | IRQ_WAITING);
> +	kstat_incr_irqs_this_cpu(irq, desc);
> +
> +	action = desc->action;
> +	if (unlikely(!action || (desc->status & IRQ_DISABLED)))
> +		goto out_unlock;

you say below irqs are always enabled so this branch is something we
never want to happen. How about adding a WARN() then ?

-- 
balbi

Re: [patch/rfc 1/2] GENIRQ: add handle_threaded_irq() flow handler

[David Brownell]

On Wednesday 18 March 2009, Felipe Balbi wrote:
> > +     action = desc->action;
> > +     if (unlikely(!action || (desc->status & IRQ_DISABLED)))
> > +             goto out_unlock;
> 
> you say below irqs are always enabled

Right here they're always disabled by spin_lock_irq().
The "below" follows spin_unlock_irq(), which re-enables
them to traverse that (locked) action list.


> so this branch is something we 
> never want to happen. How about adding a WARN() then ?

When some one says "irqs are enabled" they mean that,
local_irq_disable() or friends have not been called,
so for example a timer or other IRQ could arrive.

The IRQ_DISABLED flag in an IRQ descriptor means
something different:  "don't try *handling* this".

That particular check is used in *ALL* flow handlers.
It guards against things like races in disable_irq()
paths, which could allow an IRQ that was in flight
to arrive "after" the IRQ was disabled.

In the case of an IRQ enable/disable mask sitting
across an I2C bus boundary, it's particularly easy
to see how such a race might happen ... since both
the thread masking the IRQ, and the one handling it,
are subject to preemption and scheduling.

Re: [patch/rfc 1/2] GENIRQ: add handle_threaded_irq() flow handler

[Felipe Balbi]

On Wed, Mar 18, 2009 at 07:31:22PM +0100, David Brownell wrote:
> On Wednesday 18 March 2009, Felipe Balbi wrote:
> > > +     action = desc->action;
> > > +     if (unlikely(!action || (desc->status & IRQ_DISABLED)))
> > > +             goto out_unlock;
> > 
> > you say below irqs are always enabled
> 
> Right here they're always disabled by spin_lock_irq().
> The "below" follows spin_unlock_irq(), which re-enables
> them to traverse that (locked) action list.
> 
> 
> > so this branch is something we 
> > never want to happen. How about adding a WARN() then ?
> 
> When some one says "irqs are enabled" they mean that,
> local_irq_disable() or friends have not been called,
> so for example a timer or other IRQ could arrive.
> 
> The IRQ_DISABLED flag in an IRQ descriptor means
> something different:  "don't try *handling* this".
> 
> That particular check is used in *ALL* flow handlers.
> It guards against things like races in disable_irq()
> paths, which could allow an IRQ that was in flight
> to arrive "after" the IRQ was disabled.
> 
> In the case of an IRQ enable/disable mask sitting
> across an I2C bus boundary, it's particularly easy
> to see how such a race might happen ... since both
> the thread masking the IRQ, and the one handling it,
> are subject to preemption and scheduling.

aha, I see. Thanks for the explanation ;-)

-- 
balbi

[patch/rfc 2/2] twl4030: use new handle_threaded_irq() flow handler

[David Brownell]

From: David Brownell <dbrownell@users.sourceforge.net>

Make the toplevel twl4030 irq dispatch code use the new 
handle_threaded_irq() flow handler.  Also, minor cleanup,
use the newish generic_handle_irq_desc().

Since that flow handler guarantees the IRQ handlers are
called only in a normal (sleeping) thread context, remove
some of the workarounds for the lockdep goofage whereby
it breaks various drivers by forcing IRQF_DISABLED on.
 
Signed-off-by: David Brownell <dbrownell@users.sourceforge.net>
---
 drivers/mfd/twl4030-irq.c     |   15 +++++----------
 drivers/rtc/rtc-twl4030.c     |    8 --------
 drivers/usb/otg/twl4030-usb.c |    8 --------
 3 files changed, 5 insertions(+), 26 deletions(-)

--- a/drivers/mfd/twl4030-irq.c
+++ b/drivers/mfd/twl4030-irq.c
@@ -215,7 +215,6 @@ static int twl4030_irq_thread(void *data
 		}
 
 		/* these handlers deal with the relevant SIH irq status */
-		local_irq_disable();
 		for (module_irq = twl4030_irq_base;
 				pih_isr;
 				pih_isr >>= 1, module_irq++) {
@@ -235,10 +234,9 @@ static int twl4030_irq_thread(void *data
 					note_interrupt(module_irq, d,
 							IRQ_NONE);
 				else
-					d->handle_irq(module_irq, d);
+					generic_handle_irq_desc(module_irq, d);
 			}
 		}
-		local_irq_enable();
 
 		desc->chip->unmask(irq);
 	}
@@ -578,7 +576,7 @@ static inline int sih_read_isr(const str
 }
 
 /*
- * Generic handler for SIH interrupts ... we "know" this is called
+ * Generic handler for SIH interrupts ... we know this is called
  * in task context, with IRQs enabled.
  */
 static void handle_twl4030_sih(unsigned irq, struct irq_desc *desc)
@@ -588,10 +586,7 @@ static void handle_twl4030_sih(unsigned 
 	int isr;
 
 	/* reading ISR acks the IRQs, using clear-on-read mode */
-	local_irq_enable();
 	isr = sih_read_isr(sih);
-	local_irq_disable();
-
 	if (isr < 0) {
 		pr_err("twl4030: %s SIH, read ISR error %d\n",
 			sih->name, isr);
@@ -658,7 +653,7 @@ int twl4030_sih_setup(int module)
 		irq = irq_base + i;
 
 		set_irq_chip_and_handler(irq, &twl4030_sih_irq_chip,
-				handle_edge_irq);
+				handle_threaded_irq);
 		set_irq_chip_data(irq, agent);
 		activate_irq(irq);
 	}
@@ -666,7 +661,7 @@ int twl4030_sih_setup(int module)
 	status = irq_base;
 	twl4030_irq_next += i;
 
-	/* replace generic PIH handler (handle_simple_irq) */
+	/* replace generic PIH handler (handle_threaded_irq) */
 	irq = sih_mod + twl4030_irq_base;
 	set_irq_data(irq, agent);
 	set_irq_chained_handler(irq, handle_twl4030_sih);
@@ -719,7 +714,7 @@ int twl_init_irq(int irq_num, unsigned i
 
 	for (i = irq_base; i < irq_end; i++) {
 		set_irq_chip_and_handler(i, &twl4030_irq_chip,
-				handle_simple_irq);
+				handle_threaded_irq);
 		activate_irq(i);
 	}
 	twl4030_irq_next = i;
--- a/drivers/rtc/rtc-twl4030.c
+++ b/drivers/rtc/rtc-twl4030.c
@@ -325,14 +325,6 @@ static irqreturn_t twl4030_rtc_interrupt
 	int res;
 	u8 rd_reg;
 
-#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
-
 	res = twl4030_rtc_read_u8(&rd_reg, REG_RTC_STATUS_REG);
 	if (res)
 		goto out;
--- a/drivers/usb/otg/twl4030-usb.c
+++ b/drivers/usb/otg/twl4030-usb.c
@@ -517,14 +517,6 @@ static irqreturn_t twl4030_usb_irq(int i
 	struct twl4030_usb *twl = _twl;
 	int status;
 
-#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
-
 	status = twl4030_usb_linkstat(twl);
 	if (status != USB_LINK_UNKNOWN) {
 

Re: lockdep and threaded IRQs (was: ...)

[Thomas Gleixner]

On Mon, 2 Mar 2009, David Brownell wrote:

> On Monday 02 March 2009, Ingo Molnar wrote:
> > If you want to work on genirq threaded IRQ handlers them please 
> > check out and test the threaded IRQ handlers patches that are 
> > being worked on at lkml. See:
> > 
> > [patch 0/4] genirq: add infrastructure for threaded interrupt handlers V2
> 
> I did check them out, as noted earlier in this thread.
> 
> The significant omission is lack of support for chaining
> such threads.  Example, an I2C device that exposes
> several dozen IRQs with mask/ack/... operations that
> require I2C access.

Well, the significant omission is on your side. Instead of talking to
us about the problems and possible shortcomings of the genirq code you
went there and created your private form of abuse and now you are
complaining about that. The lockdep issue is not caused by lockdep,
it's caused by your using code which is designed to run in hardirq
context from a thread context. It does not become more correct just
because it works fine w/o lockdep.

> I'm not sure what Thomas intends to do with that issue,

I can do something about that, when I know about it, but I have just
learned about the details in the last few days.

> if anything.  It does touch on messy bits of genirq.

Which mess are you referring to ? 

The problem you described is straight forward and as I said before
it's not rocket science to provide support for that in the genirq
code. Your use case does not need to use the chained handler setup at
all, it just needs to request the main IRQ as a simple type and handle
the ack/mask/unmask from the two handler parts.

irqreturn_t hardirq_handler()
{
	ack_mask(irq);
	return IRQ_WAKE_THREAD;
}    

irqreturn_t thread_handler()
{
	pending = read_i2c_pending_irqs();

	while (pending) {
	      ....
	      d->handle_irq(module_irq, d);
	}
	unmask(irq);
}

The only change in the generic code which is needed is a new handler
function for the chained irqs "handle_irq_simple_threaded()" which is
designed to handle the calls from thread context.

Thanks,

	tglx

Re: lockdep and threaded IRQs (was: ...)

[David Brownell]

On Tuesday 03 March 2009, Thomas Gleixner wrote:
> > 
> > > [patch 0/4] genirq: add infrastructure for threaded interrupt handlers V2
> > 
> > I did check them out, as noted earlier in this thread.
> > 
> > The significant omission is lack of support for chaining
> > such threads.  Example, an I2C device that exposes
> > several dozen IRQs with mask/ack/... operations that
> > require I2C access.
> 
> Well, the significant omission is on your side.

The facts don't quite match up with that story though ... for
starters, as I've already pointed out in this thread, I didn't
write that code (or even "create a private form of abuse" as 
you put it).  My name isn't even on the copyright. 

I did however clean it up a lot, in hope that such cleanup
would make later updates easier.  Anyone could tell such
updates would be needed.  In fact ...


> Instead of talking to 
> us about the problems and possible shortcomings of the genirq code you
> went there and created your private form of abuse and now you are
> complaining about that.

... I told you about that  *SPECIFIC* driver at the kernel summit,
as something to address with the threaded IRQ infrastructure you
presented at that time.  (The prototype Overo hardware you received
at that time uses this driver, so you could even have tested such
things.  If you went a bit out of your way to do so.)  ISTR cc'ing
you on the IRQ details of that driver a few times around, for that
matter, in case you had some feedback.

Your IRQ threading patches appeared well after this driver went
to mainline.  So I did talk to "us" about those problems, earlier,
but it doesn't seem to have gotten your attention until now.


> The lockdep issue is not caused by lockdep, 
> it's caused by your using code which is designed to run in hardirq
> context from a thread context. It does not become more correct just
> because it works fine w/o lockdep.

No; there are two lockdep symptoms caused by forcing
IRQF_DISABLED on in all cases. I'm repeating myself
again here, again ...

 - one symptom shows up in standard hardirq code, I
   gave the example of two MMC host adapters which
   don't work because of those semantic changes.

 - this driver shows a related symptom, since it needs
   to chain IRQ threads.

You're referring to the second issue.  The code in
question doesn't actually have any dependency on
hardirq context though.


> > I'm not sure what Thomas intends to do with that issue,
> 
> I can do something about that, when I know about it, but I have just
> learned about the details in the last few days.

Well, I did tell you about this earlier.


> > if anything.  It does touch on messy bits of genirq.
> 
> Which mess are you referring to ? 

Assuming all IRQ configuring and dispatching runs with IRQs
disabled.  Your threaded IRQ patches kick in only *after*
dispatching has been done.  So it affects just one of the
three main unusual bits of behavior involved here.

Which mess were you thinking of?  :)


> The problem you described is straight forward and as I said before
> it's not rocket science to provide support for that in the genirq
> code. Your use case does not need to use the chained handler setup at
> all, it just needs to request the main IRQ as a simple type and handle
> the ack/mask/unmask from the two handler parts.

When there is a "main IRQ" that calls the handlers, that's
exactly what chaining involves ...


> The only change in the generic code which is needed is a new handler
> function for the chained irqs "handle_irq_simple_threaded()" which is
> designed to handle the calls from thread context.

I'm not 100% sure that's right; the dispatching is a bit quirky.
That is however where I'll start.

The top level handler (for the PIH module) could easily use a
"handle_irq_simple_threaded()", yes ... but the secondary (SIH)
handlers have a few oddball behaviors including mixes of edge
and level trigger modes. 

- Dave

Re: lockdep and threaded IRQs (was: ...)

[Thomas Gleixner]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 4082 bytes --]

David,

On Wed, 4 Mar 2009, David Brownell wrote:

> On Tuesday 03 March 2009, Thomas Gleixner wrote:
> > > 
> > > > [patch 0/4] genirq: add infrastructure for threaded interrupt handlers V2
> > > 
> > > I did check them out, as noted earlier in this thread.
> > > 
> > > The significant omission is lack of support for chaining
> > > such threads.  Example, an I2C device that exposes
> > > several dozen IRQs with mask/ack/... operations that
> > > require I2C access.
> > 
> > Well, the significant omission is on your side.
> 
> The facts don't quite match up with that story though ... for
> starters, as I've already pointed out in this thread, I didn't
> write that code (or even "create a private form of abuse" as 
> you put it).  My name isn't even on the copyright. 
> 
> I did however clean it up a lot, in hope that such cleanup
> would make later updates easier.  Anyone could tell such
> updates would be needed.  In fact ...

Sorry, did not realize that it was not your design in the first place.
 
> Your IRQ threading patches appeared well after this driver went
> to mainline.  So I did talk to "us" about those problems, earlier,
> but it doesn't seem to have gotten your attention until now.

Fair enough. I did not realize the horror of this chip until now. From
what you told me at KS I figured it would be a halfways straight
forward thing.
 
> You're referring to the second issue.  The code in
> question doesn't actually have any dependency on
> hardirq context though.

Err. handle_IRQ_event was never meant to run in thread context,
neither the handle_TYPE functions.
 
> Assuming all IRQ configuring and dispatching runs with IRQs
> disabled.  Your threaded IRQ patches kick in only *after*
> dispatching has been done.  So it affects just one of the
> three main unusual bits of behavior involved here.
> 
> Which mess were you thinking of?  :)

None, there is no mess in the irq code.
 
> > The problem you described is straight forward and as I said before
> > it's not rocket science to provide support for that in the genirq
> > code. Your use case does not need to use the chained handler setup at
> > all, it just needs to request the main IRQ as a simple type and handle
> > the ack/mask/unmask from the two handler parts.
> 
> When there is a "main IRQ" that calls the handlers, that's
> exactly what chaining involves ...

And how does this rabulistic nit picking help us here ? :)

Again: the chained_handler functionality was never designed to run in
a thread.
 
> > The only change in the generic code which is needed is a new handler
> > function for the chained irqs "handle_irq_simple_threaded()" which is
> > designed to handle the calls from thread context.
> 
> I'm not 100% sure that's right; the dispatching is a bit quirky.
> That is however where I'll start.
> 
> The top level handler (for the PIH module) could easily use a
> "handle_irq_simple_threaded()", yes ... but the secondary (SIH)
> handlers have a few oddball behaviors including mixes of edge
> and level trigger modes. 

I took a closer look at this code and the more I look the more it
confuses me.

You told me that the demux handler runs the secondary handlers in its
thread context, but looking at the code there is a work queue as well.

The mask/unmask functions which are called for the secondary handlers
are just queueing work. I really do not understand the logic here:

    primary interrupt happens
      ->primary thread is woken up

    primary thread runs
      -> primary thread raises secondary irq via 
         generic_handle_irq(irq), which results in:
	 	 desc->handle_irq(irq, desc);
 
The secondary handler has is set to: handle_edge_irq and
handle_edge_irq() does: desc->chip->ack(irq);

But the irqchip, which is set for those secondary irqs has a NULL ack
function. How can this work at all ?

I'm probably missing something and I would appreciate if you could
shed some light on this. An abstract description of the requirements
of the hardware w/o any reference to the current implementation would
be definitely helpful.

Thanks,

	tglx

Re: lockdep and threaded IRQs (was: ...)

[David Brownell]

On Friday 06 March 2009, Thomas Gleixner wrote:
> > > The only change in the generic code which is needed is a new handler
> > > function for the chained irqs "handle_irq_simple_threaded()" which is
> > > designed to handle the calls from thread context.
> > 
> > I'm not 100% sure that's right; the dispatching is a bit quirky.
> > That is however where I'll start.

I just sent a pair of patches for that.

They address only part of the chaining issue ...
dispatching from the top level IRQ task, not how
to set it up so that toplevel IRQ doesn't wrongly
appear in interrupt statistics.


> > The top level handler (for the PIH module) could easily use a
> > "handle_irq_simple_threaded()", yes ... but the secondary (SIH)
> > handlers have a few oddball behaviors including mixes of edge
> > and level trigger modes. 
> 
> I took a closer look at this code and the more I look the more it
> confuses me.

Good thing you didn't see the earlier versions!!  ;)


> You told me that the demux handler runs the secondary handlers in its
> thread context, but looking at the code there is a work queue as well.

The workqueue is for irq_chip methods.

THAT is the fundamental squishy thing here:  when registers
associated with an irq_chip (not just the interrupting device)
are inaccessible except in task/sleeping contexts.

It means that three different things must always run in task
context:  (a) irq_chip methods, (b) IRQ flow handlers, and
(c) IRQ action handlers.


Overstating things somewhat ... your irq threading patches
are best suited for offloading code from hardirq context
into task context -- related to case (c), except here the
state needed by the action handler *can't* be accessed in
hardirq context.  But they don't much help with cases (a)
or (b), where such hardirq context was never relevant in
the first place because the irq management registers aren't
accessible there.

 
> The mask/unmask functions which are called for the secondary handlers
> are just queueing work. I really do not understand the logic here:
> 
>     primary interrupt happens
>       ->primary thread is woken up
> 
>     primary thread runs
>       -> primary thread raises secondary irq via 
>          generic_handle_irq(irq), which results in:
> 	 	 desc->handle_irq(irq, desc);

That's not quite exact; there can be more than one level
of dispatch.  In more detail (it affects answers to your
questions):

 1) Primary thread queries the PIH module (up to 8 IRQ
    status bits) to determine which SIH module(s) raised
    an interrupt.

 2) Then it does a normal dispatch -- desc->handle_irq(),
    maybe wrapped in a convenience function -- to the
    next level, specific to that SIH (not limited to
    just 8 status bits).

 3) Now, depending on how that SIH was set up, one
    of two things will happen:

    A) Dispatch through generic SIH module code; as
       with GPIO and "power" IRQs.  Interrupts for
       MMC card detect, RTC alarm, USB transciver,
       "power button", and a few other things go
       this way.  Dispatched by another indirection
       through a desc->handle_irq() invocation.

    B) Dispatch through non-generic SIH module code.
       That's how the keypad and ADC drivers work
       right now; also the battery charger, but that
       driver isn't used much yet due to HW issues.
  

> The secondary handler has is set to: handle_edge_irq and
> handle_edge_irq() does: desc->chip->ack(irq);

That's the (3A) case above.  I was never certain that
needed to be dispatched with the "edge" flow handler
instead of the "simple" one.  And it turns out that
something like "simple" can work fine, as it currently
does in case (3B).

(The trigger type for those IRQs is typically "edge".
But the "edge" flow handler doesn't seem to be the
best match.)


> But the irqchip, which is set for those secondary irqs has a NULL ack
> function. How can this work at all ?

For the PIH level, this hardware is quite odd:  there
are no ack or mask primitives at all.  At that level,
the only way to clear IRQ status is through the the
child (SIH) level status.

For the SIH level, these chips have a mode you may well
have seen on other hardware.  Reading the SIH IRQ status
register implicitly acks the pending IRQs ... "clear on
read" (COR) mode mentioned in the driver.  So no separate
ack() is needed in that case either.


> I'm probably missing something and I would appreciate if you could
> shed some light on this. An abstract description of the requirements
> of the hardware w/o any reference to the current implementation would
> be definitely helpful.

You're probably trying to avoid reading over 900 pages
of the tps65930 (public flavor of twl4030) technical
reference manual ... ;)

I'm not sure how abstract you want.  I'll hope my words
above -- possibly in conjunction with comments in the
current twl4030-irq.c code -- clarify.

- Dave

Re: lockdep and threaded IRQs (was: ...)

[David Brownell]

On Monday 02 March 2009, Ingo Molnar wrote:
> 
> > > > What's unfortunate is that you prefer not to fix that
> > > > IRQF_DISABLED bug in lockdep, which you co-"maintain".
> > > > When running with lockdep, that bug (a) introduces bugs
> > > > in some drivers and (b) hides bugs in others.  You've
> > > > rejected even a minimal warning fix, to help minimize
> > > > the amount of time developers waste on (a) and (b).
> > > 
> > > I've come to the conclusion that the only technically sound solution is
> > > to do as I proposed today, utterly eliminate !IRQF_DISABLED handlers.
> > 
> > As you announced today.  If you truly believe that, then
> > you should at least submit a warning patch for 2.6.29-rc
> > ("driver X isn't setting IRQF_DISABLED, reimplement!")
> 
> i have changed the BUG_ON() to a WARN_ONCE() message so the 
> warning is in place now.

The patch Peter sent doesn't relate in the least to removing
the IRQF_DISABLED flag though.  Patches addressing that
would be in setup_irq() code paths not IRQ dispatch.


> > with a Documentation/feature-removal-schedule.txt plan for 
> > removing that mechanism. [...]
> 
> you are misunderstanding the workings and purpose of 
> feature-removal-schedule.txt. It is mainly used for 
> functionality that is user-visible.

If by "user" you include "kernel developers", yes;
otherwise, I'd dispute "mainly".  The first several
entries right now relate to kernel interfaces, as
do quite a lot of the others.


> It is sometimes used for  
> functionality that a subsystem has exported to a lot of drivers 
> consciously and which is being removed.

The IRQ framework has very consciously exported IRQF_DISABLED.
That functionality has been around for a very long time; I'm
thinking at least ten years now.

So removing IRQF_DISABLED -- if it's even agreed to be
a good idea, which seems to be a minority opinion so far
on this thread -- is very much the sort of thing one would
expect to appear in that schedule.

Re: lockdep and threaded IRQs (was: ...)

[Ingo Molnar]

* David Brownell <david-b@pacbell.net> wrote:

> On Monday 02 March 2009, Ingo Molnar wrote:
> > 
> > > > > What's unfortunate is that you prefer not to fix that
> > > > > IRQF_DISABLED bug in lockdep, which you co-"maintain".
> > > > > When running with lockdep, that bug (a) introduces bugs
> > > > > in some drivers and (b) hides bugs in others.  You've
> > > > > rejected even a minimal warning fix, to help minimize
> > > > > the amount of time developers waste on (a) and (b).
> > > > 
> > > > I've come to the conclusion that the only technically sound solution is
> > > > to do as I proposed today, utterly eliminate !IRQF_DISABLED handlers.
> > > 
> > > As you announced today.  If you truly believe that, then
> > > you should at least submit a warning patch for 2.6.29-rc
> > > ("driver X isn't setting IRQF_DISABLED, reimplement!")
> > 
> > i have changed the BUG_ON() to a WARN_ONCE() message so the 
> > warning is in place now.
> 
> The patch Peter sent doesn't relate in the least to removing 
> the IRQF_DISABLED flag though.  Patches addressing that would 
> be in setup_irq() code paths not IRQ dispatch.

yes, i referred to the BUG_ON(!irq_irq()) patch.

	Ingo

[PATCH] genirq: assert that irq handlers are indeed run in hardirq context.

[Peter Zijlstra]

On Fri, 2009-02-27 at 15:18 -0800, David Brownell wrote:
> But these handlers are *NOT* running in hardirq context;

Ah, let us stop this tinkering dead in its tracks

Signed-off-by: Peter Zijlstra <a.p.zijlstra@chello.nl>
---
 kernel/irq/handle.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/kernel/irq/handle.c b/kernel/irq/handle.c
index c87d146..b75d73b 100644
--- a/kernel/irq/handle.c
+++ b/kernel/irq/handle.c
@@ -354,6 +354,8 @@ irqreturn_t handle_IRQ_event(unsigned int irq, struct irqaction *action)
 	irqreturn_t ret, retval = IRQ_NONE;
 	unsigned int status = 0;
 
+	BUG_ON(!in_irq());
+
 	if (!(action->flags & IRQF_DISABLED))
 		local_irq_enable_in_hardirq();
 

Re: [PATCH] genirq: assert that irq handlers are indeed run in hardirq context.

[David Brownell]

On Monday 02 March 2009, Peter Zijlstra wrote:
> > But these handlers are *NOT* running in hardirq context;
> 
> Ah, let us stop this tinkering dead in its tracks

How about providing adequate support for threaded IRQs
first, before breaking code that's been working for the
last few years?

This patch fails the "no regressions" test.  And with
no reason given -- not even a bad one.

For what it's worth:  NAK.

- Dave

[tip:irq/genirq] genirq: assert that irq handlers are indeed running in hardirq context

[Peter Zijlstra]

Commit-ID:  3427ce9e6aac783a576c8e2712c323eaddd123a9
Gitweb:     http://git.kernel.org/tip/3427ce9e6aac783a576c8e2712c323eaddd123a9
Author:     Peter Zijlstra <a.p.zijlstra@chello.nl>
AuthorDate: Mon, 2 Mar 2009 16:13:32 +0100
Commit:     Ingo Molnar <mingo@elte.hu>
CommitDate: Mon, 2 Mar 2009 22:59:15 +0100

genirq: assert that irq handlers are indeed running in hardirq context

Make sure the genirq layer handlers are indeed running handlers
in hardirq context. That is the genirq expectation and doing
anything else is broken.

Signed-off-by: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Andrew Morton <akpm@linux-foundation.org>
LKML-Reference: <1236006812.5330.632.camel@laptop>
Signed-off-by: Ingo Molnar <mingo@elte.hu>


---
 kernel/irq/handle.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/kernel/irq/handle.c b/kernel/irq/handle.c
index 3aba8d1..716f4f4 100644
--- a/kernel/irq/handle.c
+++ b/kernel/irq/handle.c
@@ -328,6 +328,8 @@ irqreturn_t handle_IRQ_event(unsigned int irq, struct irqaction *action)
 	irqreturn_t ret, retval = IRQ_NONE;
 	unsigned int status = 0;
 
+	BUG_ON(!in_irq());
+
 	if (!(action->flags & IRQF_DISABLED))
 		local_irq_enable_in_hardirq();
 

[tip:irq/genirq] genirq: assert that irq handlers are indeed running in hardirq context

[Peter Zijlstra]

Commit-ID:  044d408409cc4e1bc75c886e27ca85c270db104c
Gitweb:     http://git.kernel.org/tip/044d408409cc4e1bc75c886e27ca85c270db104c
Author:     Peter Zijlstra <a.p.zijlstra@chello.nl>
AuthorDate: Mon, 2 Mar 2009 16:13:32 +0100
Commit:     Ingo Molnar <mingo@elte.hu>
CommitDate: Tue, 3 Mar 2009 00:05:45 +0100

genirq: assert that irq handlers are indeed running in hardirq context

Make sure the genirq layer handlers are indeed running handlers
in hardirq context. That is the genirq expectation and doing
anything else is broken.

Signed-off-by: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Andrew Morton <akpm@linux-foundation.org>
LKML-Reference: <1236006812.5330.632.camel@laptop>
Signed-off-by: Ingo Molnar <mingo@elte.hu>


---
 kernel/irq/handle.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/kernel/irq/handle.c b/kernel/irq/handle.c
index 3aba8d1..a2ee682 100644
--- a/kernel/irq/handle.c
+++ b/kernel/irq/handle.c
@@ -328,6 +328,8 @@ irqreturn_t handle_IRQ_event(unsigned int irq, struct irqaction *action)
 	irqreturn_t ret, retval = IRQ_NONE;
 	unsigned int status = 0;
 
+	WARN_ONCE(!in_irq(), "BUG: IRQ handler called from non-hardirq context!");
+
 	if (!(action->flags & IRQF_DISABLED))
 		local_irq_enable_in_hardirq();
 

Re: [tip:irq/genirq] genirq: assert that irq handlers are indeed running in hardirq context

[Eric Miao]

On Tue, Mar 3, 2009 at 7:15 AM, Peter Zijlstra <a.p.zijlstra@chello.nl> wrote:
> Commit-ID:  044d408409cc4e1bc75c886e27ca85c270db104c
> Gitweb:     http://git.kernel.org/tip/044d408409cc4e1bc75c886e27ca85c270db104c
> Author:     Peter Zijlstra <a.p.zijlstra@chello.nl>
> AuthorDate: Mon, 2 Mar 2009 16:13:32 +0100
> Commit:     Ingo Molnar <mingo@elte.hu>
> CommitDate: Tue, 3 Mar 2009 00:05:45 +0100
>
> genirq: assert that irq handlers are indeed running in hardirq context
>
> Make sure the genirq layer handlers are indeed running handlers
> in hardirq context. That is the genirq expectation and doing
> anything else is broken.
>
> Signed-off-by: Peter Zijlstra <a.p.zijlstra@chello.nl>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> LKML-Reference: <1236006812.5330.632.camel@laptop>
> Signed-off-by: Ingo Molnar <mingo@elte.hu>
>

OK, now this gave me the warning below and it looks resend_irqs()
and resend_tasklet are somehow found guilty and doing wrong, as
the comment of this commit suggested, yet I'm not sure if this makes
sense:


[   34.728943] ------------[ cut here ]------------
[   34.733573] WARNING: at
/home/ycmiao/work/linux-2.6/kernel/irq/handle.c:366
handle_IRQ_event+0x48/0x160()
[   34.743088] BUG: IRQ[82] handler called from non-hardirq
context!Modules linked in:
[   34.750729] [<c002e958>] (unwind_backtrace+0x0/0xdc) from
[<c00438d0>] (warn_slowpath+0x68/0x8c)
[   34.759520] [<c00438d0>] (warn_slowpath+0x68/0x8c) from
[<c006d100>] (handle_IRQ_event+0x48/0x160)
[   34.768461] [<c006d100>] (handle_IRQ_event+0x48/0x160) from
[<c006eb8c>] (handle_edge_irq+0x14c/0x1c4)
[   34.777747] [<c006eb8c>] (handle_edge_irq+0x14c/0x1c4) from
[<c006e440>] (resend_irqs+0x44/0x78)
[   34.786517] [<c006e440>] (resend_irqs+0x44/0x78) from [<c0048658>]
(tasklet_action+0x7c/0xdc)
[   34.795039] [<c0048658>] (tasklet_action+0x7c/0xdc) from
[<c0048b18>] (__do_softirq+0x60/0xe8)
[   34.803638] [<c0048b18>] (__do_softirq+0x60/0xe8) from [<c0048c88>]
(do_softirq+0x44/0x60)
[   34.811881] [<c0048c88>] (do_softirq+0x44/0x60) from [<c0048d1c>]
(ksoftirqd+0x78/0x168)
[   34.819953] [<c0048d1c>] (ksoftirqd+0x78/0x168) from [<c0058d38>]
(kthread+0x54/0x80)
[   34.827786] [<c0058d38>] (kthread+0x54/0x80) from [<c0046780>]
(do_exit+0x0/0x658)
[   34.835342] [<c0046780>] (do_exit+0x0/0x658) from [<00000000>] (0x0)
[   34.841687] ---[ end trace 26b21608484430d3 ]---



>
> ---
>  kernel/irq/handle.c |    2 ++
>  1 files changed, 2 insertions(+), 0 deletions(-)
>
> diff --git a/kernel/irq/handle.c b/kernel/irq/handle.c
> index 3aba8d1..a2ee682 100644
> --- a/kernel/irq/handle.c
> +++ b/kernel/irq/handle.c
> @@ -328,6 +328,8 @@ irqreturn_t handle_IRQ_event(unsigned int irq, struct irqaction *action)
>        irqreturn_t ret, retval = IRQ_NONE;
>        unsigned int status = 0;
>
> +       WARN_ONCE(!in_irq(), "BUG: IRQ handler called from non-hardirq context!");
> +
>        if (!(action->flags & IRQF_DISABLED))
>                local_irq_enable_in_hardirq();
>
> --
> 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/
>



-- 
Cheers
- eric

Re: [tip:irq/genirq] genirq: assert that irq handlers are indeed running in hardirq context

[Thomas Gleixner]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1236 bytes --]

On Fri, 10 Apr 2009, Eric Miao wrote:
> On Tue, Mar 3, 2009 at 7:15 AM, Peter Zijlstra <a.p.zijlstra@chello.nl> wrote:
> > Commit-ID:  044d408409cc4e1bc75c886e27ca85c270db104c
> > Gitweb:     http://git.kernel.org/tip/044d408409cc4e1bc75c886e27ca85c270db104c
> > Author:     Peter Zijlstra <a.p.zijlstra@chello.nl>
> > AuthorDate: Mon, 2 Mar 2009 16:13:32 +0100
> > Commit:     Ingo Molnar <mingo@elte.hu>
> > CommitDate: Tue, 3 Mar 2009 00:05:45 +0100
> >
> > genirq: assert that irq handlers are indeed running in hardirq context
> >
> > Make sure the genirq layer handlers are indeed running handlers
> > in hardirq context. That is the genirq expectation and doing
> > anything else is broken.
> >
> > Signed-off-by: Peter Zijlstra <a.p.zijlstra@chello.nl>
> > Cc: Andrew Morton <akpm@linux-foundation.org>
> > LKML-Reference: <1236006812.5330.632.camel@laptop>
> > Signed-off-by: Ingo Molnar <mingo@elte.hu>
> >
> 
> OK, now this gave me the warning below and it looks resend_irqs()
> and resend_tasklet are somehow found guilty and doing wrong, as
> the comment of this commit suggested, yet I'm not sure if this makes
> sense:

No, it doesn't. We didn't think about the resend tasklet. I'm going to
fix it.

Thanks,

	tglx

Re: lockdep and threaded IRQs

[Stefan Richter]

David Brownell wrote:
> 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...)

Since when is having an IRQ handler scheduling a workqueue job a hack?
In kernels whose IRQ handlers don't sleep, we don't pretend that they
could; instead we defer sleeping work to a context which can sleep.

Or from another angle:  If a driver requires a kernel with sleeping IRQ
handlers, why submit it for inclusion into a kernel which does not
provide nonatomic context to IRQ handlers?
-- 
Stefan Richter
-=====-==--= --=- ===--
http://arcgraph.de/sr/

Re: lockdep and threaded IRQs

[David Brownell]

On Saturday 28 February 2009, Stefan Richter wrote:
> David Brownell wrote:
> > 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...)
> 
> Since when is having an IRQ handler scheduling a workqueue job a hack?

The only "hack" I recall mentioning was the need to
forcibly re-enable IRQs that lockdep wrongly disabled.


> In kernels whose IRQ handlers don't sleep, we don't pretend that they
> could; instead we defer sleeping work to a context which can sleep.
> 
> Or from another angle:  If a driver requires a kernel with sleeping IRQ
> handlers, why submit it for inclusion into a kernel which does not

(yet)

> provide nonatomic context to IRQ handlers?

Or from yet another angle:  how will progress ever happen
if nobody pushes the boundaries?  :)

We've known for ages that threaded IRQs will eventually
show up in mainline.  Better IMO to plan for that, and
expect minor changes to the drivers which have needed them
all along.

- Dave

Re: [PATCH 1/2] input: misc: add twl4030-pwrbutton driver

[Trilok Soni]

Hi Felipe,

> +++ b/drivers/input/misc/twl4030-pwrbutton.c
> @@ -0,0 +1,147 @@
> +/**
> + * drivers/i2c/chips/twl4030-pwrbutton.c
> + *

Please remove this file path.

> +
> +static struct input_dev *powerbutton_dev;
> +static struct device *dbg_dev;
> +
> +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);
> +       if (!err)  {
> +               input_report_key(powerbutton_dev, KEY_POWER,
> +                                value & PWR_PWRON_IRQ);


input_sync(...) please.

> +       } else {
> +               dev_err(dbg_dev, "twl4030: i2c error %d while reading TWL4030"
> +                       " PM_MASTER STS_HW_CONDITIONS register\n", err);
> +       }
> +

> +static int __devexit twl4030_pwrbutton_remove(struct platform_device *pdev)
> +{
> +       int irq = platform_get_irq(pdev, 0);
> +
> +       free_irq(irq, NULL);
> +       input_unregister_device(powerbutton_dev);
> +       input_free_device(powerbutton_dev);

No need of input_free_device after input_unregister_device.

> +
> +       return 0;
> +}
> +
> +struct platform_driver twl4030_pwrbutton_driver = {
> +       .probe          = twl4030_pwrbutton_probe,
> +       .remove         = twl4030_pwrbutton_remove,

__devexit_p(...)



-- 
---Trilok Soni
http://triloksoni.wordpress.com
http://www.linkedin.com/in/triloksoni

[PATCH] mfd: twl4030: add twl4030-pwrbutton as our child

[Felipe Balbi]

From: Felipe Balbi <felipe.balbi@nokia.com>

Make that twl4030-pwrbutton.c driver probe with current
child creation api for twl4030.

Cc: Samuel Ortiz <sameo@openedhand.com>
Cc: David Brownell <dbrownell@users.sourceforge.net>
Signed-off-by: Felipe Balbi <felipe.balbi@nokia.com>
---
 drivers/input/misc/twl4030-pwrbutton.c |   10 ++++------
 drivers/mfd/twl4030-core.c             |   13 +++++++++++++
 2 files changed, 17 insertions(+), 6 deletions(-)

diff --git a/drivers/input/misc/twl4030-pwrbutton.c b/drivers/input/misc/twl4030-pwrbutton.c
index ba9cbdc..85a46d9 100644
--- a/drivers/input/misc/twl4030-pwrbutton.c
+++ b/drivers/input/misc/twl4030-pwrbutton.c
@@ -1,7 +1,5 @@
 /**
- * drivers/i2c/chips/twl4030-pwrbutton.c
- *
- * Driver for sending triton2 power button event to input-layer
+ * twl4030-pwrbutton.c - TWL4030 Power Button Input Driver
  *
  * Copyright (C) 2008-2009 Nokia Corporation
  *
@@ -55,6 +53,7 @@ static irqreturn_t powerbutton_irq(int irq, void *dev_id)
 	if (!err)  {
 		input_report_key(powerbutton_dev, KEY_POWER,
 				 value & PWR_PWRON_IRQ);
+		input_sync(powerbutton_dev);
 	} else {
 		dev_err(dbg_dev, "twl4030: i2c error %d while reading TWL4030"
 			" PM_MASTER STS_HW_CONDITIONS register\n", err);
@@ -104,7 +103,7 @@ static int __devinit twl4030_pwrbutton_probe(struct platform_device *pdev)
 free_input_dev:
 	input_free_device(powerbutton_dev);
 free_irq_and_out:
-	free_irq(TWL4030_PWRIRQ_PWRBTN, NULL);
+	free_irq(irq, NULL);
 out:
 	return err;
 }
@@ -115,14 +114,13 @@ static int __devexit twl4030_pwrbutton_remove(struct platform_device *pdev)
 
 	free_irq(irq, NULL);
 	input_unregister_device(powerbutton_dev);
-	input_free_device(powerbutton_dev);
 
 	return 0;
 }
 
 struct platform_driver twl4030_pwrbutton_driver = {
 	.probe		= twl4030_pwrbutton_probe,
-	.remove		= twl4030_pwrbutton_remove,
+	.remove		= __devexit_p(twl4030_pwrbutton_remove),
 	.driver		= {
 		.name	= "twl4030-pwrbutton",
 		.owner	= THIS_MODULE,
diff --git a/drivers/mfd/twl4030-core.c b/drivers/mfd/twl4030-core.c
index 68826f1..c86bc3b 100644
--- a/drivers/mfd/twl4030-core.c
+++ b/drivers/mfd/twl4030-core.c
@@ -101,6 +101,12 @@
 #define twl_has_usb()	false
 #endif
 
+#if defined(CONFIG_INPUT_TWL4030_PWRBUTTON) \
+	|| defined(CONFIG_INPUT_TWL4030_PWBUTTON_MODULE)
+#define twl_has_pwrbutton()	true
+#else
+#define twl_has_pwrbutton()	false
+#endif
 
 /* Triton Core internal information (BEGIN) */
 
@@ -526,6 +532,13 @@ add_children(struct twl4030_platform_data *pdata, unsigned long features)
 		usb_transceiver = child;
 	}
 
+	if (twl_has_pwrbutton()) {
+		child = add_child(1, "twl4030_pwrbutton",
+				NULL, 0, true, pdata->irq_base + 8 + 0, 0);
+		if (IS_ERR(child))
+			return PTR_ERR(child);
+	}
+
 	if (twl_has_regulator()) {
 		/*
 		child = add_regulator(TWL4030_REG_VPLL1, pdata->vpll1);
-- 
1.6.2.rc0.61.g5cd12

Re: [PATCH 1/2] input: misc: add twl4030-pwrbutton driver

[Dmitry Torokhov]

Hi Felipe,

On Fri, Feb 27, 2009 at 09:28:02PM +0200, Felipe Balbi wrote:
> From: Felipe Balbi <felipe.balbi@nokia.com>
> 
> This is part of the twl4030 multifunction device driver.
> 
> With this driver we add support for reporting KEY_POWER
> events via the input layer.

...

> 
> +
> +	err = twl4030_i2c_read_u8(TWL4030_MODULE_PM_MASTER, &value,
> +				  STS_HW_CONDITIONS);
> +	if (!err)  {
> +		input_report_key(powerbutton_dev, KEY_POWER,
> +				 value & PWR_PWRON_IRQ);

input_sync() here please.

> +	} else {
> +		dev_err(dbg_dev, "twl4030: i2c error %d while reading TWL4030"
> +			" PM_MASTER STS_HW_CONDITIONS register\n", err);
> +	}
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static int __devinit twl4030_pwrbutton_probe(struct platform_device *pdev)
> +{
> +	int err = 0;
> +	int irq = platform_get_irq(pdev, 0);
> +
> +	dbg_dev = &pdev->dev;
> +
> +	/* PWRBTN == PWRON */
> +	err = request_irq(irq, powerbutton_irq,
> +			IRQF_TRIGGER_FALLING | IRQF_TRIGGER_RISING,
> +			"twl4030-pwrbutton", NULL);

Requesting IRQ before allocating input device seems unsafe. If IRQ
arrives right now we'll go boom.


> +	if (err < 0) {
> +		dev_dbg(&pdev->dev, "Can't get IRQ for pwrbutton: %d\n", err);
> +		goto out;
> +	}
> +
> +	powerbutton_dev = input_allocate_device();
> +	if (!powerbutton_dev) {
> +		dev_dbg(&pdev->dev, "Can't allocate power button\n");
> +		err = -ENOMEM;
> +		goto free_irq_and_out;
> +	}
> +
> +	powerbutton_dev->evbit[0] = BIT_MASK(EV_KEY);
> +	powerbutton_dev->keybit[BIT_WORD(KEY_POWER)] = BIT_MASK(KEY_POWER);
> +	powerbutton_dev->name = "triton2-pwrbutton";

powerbutton_dev->dev.parent = &pdev->dev;

Any chance we could set up phys as well? BUS_I2C seems to be in order as
well.

> +
> +	err = input_register_device(powerbutton_dev);
> +	if (err) {
> +		dev_dbg(&pdev->dev, "Can't register power button: %d\n", err);
> +		goto free_input_dev;
> +	}
> +
> +	dev_info(&pdev->dev, "triton2 power button driver initialized\n");
> +
> +	return 0;
> +
> +
> +free_input_dev:
> +	input_free_device(powerbutton_dev);
> +free_irq_and_out:
> +	free_irq(TWL4030_PWRIRQ_PWRBTN, NULL);
> +out:
> +	return err;
> +}
> +
> +static int __devexit twl4030_pwrbutton_remove(struct platform_device *pdev)
> +{
> +	int irq = platform_get_irq(pdev, 0);
> +
> +	free_irq(irq, NULL);
> +	input_unregister_device(powerbutton_dev);
> +	input_free_device(powerbutton_dev);

Do not call free after unregistering, unregister will drop last
reference and will cause device structure be freed.

-- 
Dmitry

Re: [PATCH 1/2] input: misc: add twl4030-pwrbutton driver

[Felipe Balbi]

On Sat, Feb 28, 2009 at 02:23:03PM -0800, Dmitry Torokhov wrote:
> Hi Felipe,
> 
> On Fri, Feb 27, 2009 at 09:28:02PM +0200, Felipe Balbi wrote:
> > From: Felipe Balbi <felipe.balbi@nokia.com>
> > 
> > This is part of the twl4030 multifunction device driver.
> > 
> > With this driver we add support for reporting KEY_POWER
> > events via the input layer.
> 
> ...

thanks for reviewing, how about the version below:

========== cut here ==========

>From 60c4980bc13b08c73aa5b647cda6ef540ac94939 Mon Sep 17 00:00:00 2001
From: Felipe Balbi <felipe.balbi@nokia.com>
Date: Fri, 27 Feb 2009 21:18:15 +0200
Subject: [PATCH] input: misc: add twl4030-pwrbutton driver

This is part of the twl4030 multifunction device driver.

With this driver we add support for reporting KEY_POWER
events via the input layer.

Cc: David Brownell <dbrownell@users.sourceforge.net>
Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Cc: Samuel Ortiz <sameo@openedhand.com>
Signed-off-by: Felipe Balbi <felipe.balbi@nokia.com>
---
 drivers/input/misc/Kconfig             |    4 +
 drivers/input/misc/Makefile            |    1 +
 drivers/input/misc/twl4030-pwrbutton.c |  147 ++++++++++++++++++++++++++++++++
 3 files changed, 152 insertions(+), 0 deletions(-)
 create mode 100644 drivers/input/misc/twl4030-pwrbutton.c

diff --git a/drivers/input/misc/Kconfig b/drivers/input/misc/Kconfig
index 67e5553..9667b50 100644
--- a/drivers/input/misc/Kconfig
+++ b/drivers/input/misc/Kconfig
@@ -193,6 +193,10 @@ config INPUT_CM109
 	  To compile this driver as a module, choose M here: the module will be
 	  called cm109.
 
+config INPUT_TWL4030_PWRBUTTON
+	tristate "TWL4030 Power button Driver"
+	depends on TWL4030_CORE
+
 config INPUT_UINPUT
 	tristate "User level driver support"
 	help
diff --git a/drivers/input/misc/Makefile b/drivers/input/misc/Makefile
index bb62e6e..2fabcdb 100644
--- a/drivers/input/misc/Makefile
+++ b/drivers/input/misc/Makefile
@@ -19,6 +19,7 @@ obj-$(CONFIG_INPUT_YEALINK)		+= yealink.o
 obj-$(CONFIG_INPUT_CM109)		+= cm109.o
 obj-$(CONFIG_HP_SDC_RTC)		+= hp_sdc_rtc.o
 obj-$(CONFIG_INPUT_UINPUT)		+= uinput.o
+obj-$(CONFIG_INPUT_TWL4030_PWRBUTTON)	+= twl4030-pwrbutton.o
 obj-$(CONFIG_INPUT_APANEL)		+= apanel.o
 obj-$(CONFIG_INPUT_SGI_BTNS)		+= sgi_btns.o
 obj-$(CONFIG_INPUT_PCF50633_PMU)	+= pcf50633-input.o
diff --git a/drivers/input/misc/twl4030-pwrbutton.c b/drivers/input/misc/twl4030-pwrbutton.c
new file mode 100644
index 0000000..cf55583
--- /dev/null
+++ b/drivers/input/misc/twl4030-pwrbutton.c
@@ -0,0 +1,147 @@
+/**
+ * twl4030-pwrbutton.c - TWL4030 Power Button Input Driver
+ *
+ * Copyright (C) 2008-2009 Nokia Corporation
+ *
+ * Written by Peter De Schrijver <peter.de-schrijver@nokia.com>
+ *
+ * This file is subject to the terms and conditions of the GNU General
+ * Public License. See the file "COPYING" in the main directory of this
+ * archive for more details.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
+ */
+
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/kernel.h>
+#include <linux/errno.h>
+#include <linux/input.h>
+#include <linux/interrupt.h>
+#include <linux/platform_device.h>
+#include <linux/i2c/twl4030.h>
+
+#define PWR_PWRON_IRQ (1 << 0)
+
+#define STS_HW_CONDITIONS 0xf
+
+static struct input_dev *powerbutton_dev;
+static struct device *dbg_dev;
+
+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);
+	if (!err)  {
+		input_report_key(powerbutton_dev, KEY_POWER,
+				 value & PWR_PWRON_IRQ);
+		input_sync(powerbutton_dev);
+	} else {
+		dev_err(dbg_dev, "twl4030: i2c error %d while reading TWL4030"
+			" PM_MASTER STS_HW_CONDITIONS register\n", err);
+	}
+
+	return IRQ_HANDLED;
+}
+
+static int __devinit twl4030_pwrbutton_probe(struct platform_device *pdev)
+{
+	int err = 0;
+	int irq = platform_get_irq(pdev, 0);
+
+	dbg_dev = &pdev->dev;
+
+	powerbutton_dev = input_allocate_device();
+	if (!powerbutton_dev) {
+		dev_dbg(&pdev->dev, "Can't allocate power button\n");
+		err = -ENOMEM;
+		goto out;
+	}
+
+	err = request_irq(irq, powerbutton_irq,
+			IRQF_TRIGGER_FALLING | IRQF_TRIGGER_RISING,
+			"twl4030-pwrbutton", NULL);
+	if (err < 0) {
+		dev_dbg(&pdev->dev, "Can't get IRQ for pwrbutton: %d\n", err);
+		goto free_input_dev;
+	}
+
+	powerbutton_dev->evbit[0] = BIT_MASK(EV_KEY);
+	powerbutton_dev->keybit[BIT_WORD(KEY_POWER)] = BIT_MASK(KEY_POWER);
+	powerbutton_dev->name = "triton2-pwrbutton";
+	powerbutton_dev->phys = "twl4030_pwrbutton/input0";
+	powerbutton_dev->dev.parent = &pdev->dev;
+
+	err = input_register_device(powerbutton_dev);
+	if (err) {
+		dev_dbg(&pdev->dev, "Can't register power button: %d\n", err);
+		goto free_irq_and_out;
+	}
+
+	dev_info(&pdev->dev, "triton2 power button driver initialized\n");
+
+	return 0;
+
+
+free_irq_and_out:
+	free_irq(irq, NULL);
+free_input_dev:
+	input_free_device(powerbutton_dev);
+out:
+	return err;
+}
+
+static int __devexit twl4030_pwrbutton_remove(struct platform_device *pdev)
+{
+	int irq = platform_get_irq(pdev, 0);
+
+	free_irq(irq, NULL);
+	input_unregister_device(powerbutton_dev);
+
+	return 0;
+}
+
+struct platform_driver twl4030_pwrbutton_driver = {
+	.probe		= twl4030_pwrbutton_probe,
+	.remove		= __devexit_p(twl4030_pwrbutton_remove),
+	.driver		= {
+		.name	= "twl4030_pwrbutton",
+		.owner	= THIS_MODULE,
+	},
+};
+
+static int __init twl4030_pwrbutton_init(void)
+{
+	return platform_driver_register(&twl4030_pwrbutton_driver);
+}
+module_init(twl4030_pwrbutton_init);
+
+static void __exit twl4030_pwrbutton_exit(void)
+{
+	platform_driver_unregister(&twl4030_pwrbutton_driver);
+}
+module_exit(twl4030_pwrbutton_exit);
+
+MODULE_ALIAS("platform:twl4030_pwrbutton");
+MODULE_DESCRIPTION("Triton2 Power Button");
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Peter De Schrijver");
+
-- 
1.6.2.rc0.61.g5cd12


-- 
balbi

Re: [PATCH 1/2] input: misc: add twl4030-pwrbutton driver

[Dmitry Torokhov]

On Sun, Mar 01, 2009 at 02:30:18AM +0200, Felipe Balbi wrote:
> On Sat, Feb 28, 2009 at 02:23:03PM -0800, Dmitry Torokhov wrote:
> > Hi Felipe,
> > 
> > On Fri, Feb 27, 2009 at 09:28:02PM +0200, Felipe Balbi wrote:
> > > From: Felipe Balbi <felipe.balbi@nokia.com>
> > > 
> > > This is part of the twl4030 multifunction device driver.
> > > 
> > > With this driver we add support for reporting KEY_POWER
> > > events via the input layer.
> > 
> > ...
> 
> thanks for reviewing, how about the version below:
> 

Looks good, couple more items...

>  
> +config INPUT_TWL4030_PWRBUTTON
> +	tristate "TWL4030 Power button Driver"
> +	depends on TWL4030_CORE
> +

Help should be added, at least document module name as we do with
other drivers.

> +
> +static struct input_dev *powerbutton_dev;
> +static struct device *dbg_dev;

Get rid of dbg_dev. The only place it is used in powerbutton_irq and you
can get it from powerbutton_dev->dev.parent now.

> +
> +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...
> +	 */

I would like more verbage explaining that this is a threaded IRQ and
therefore is allowed to sleep and other stuff.

> +	local_irq_enable();
> +#endif
> +
> +	err = twl4030_i2c_read_u8(TWL4030_MODULE_PM_MASTER, &value,
> +				  STS_HW_CONDITIONS);
> +	if (!err)  {
> +		input_report_key(powerbutton_dev, KEY_POWER,
> +				 value & PWR_PWRON_IRQ);
> +		input_sync(powerbutton_dev);
> +	} else {
> +		dev_err(dbg_dev, "twl4030: i2c error %d while reading TWL4030"
> +			" PM_MASTER STS_HW_CONDITIONS register\n", err);
> +	}
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static int __devinit twl4030_pwrbutton_probe(struct platform_device *pdev)
> +{
> +	int err = 0;

No need to initialize.

> +	int irq = platform_get_irq(pdev, 0);
> +
> +	dbg_dev = &pdev->dev;
> +
> +	powerbutton_dev = input_allocate_device();
> +	if (!powerbutton_dev) {
> +		dev_dbg(&pdev->dev, "Can't allocate power button\n");
> +		err = -ENOMEM;
> +		goto out;
> +	}
> +
> +	err = request_irq(irq, powerbutton_irq,
> +			IRQF_TRIGGER_FALLING | IRQF_TRIGGER_RISING,
> +			"twl4030-pwrbutton", NULL);
> +	if (err < 0) {
> +		dev_dbg(&pdev->dev, "Can't get IRQ for pwrbutton: %d\n", err);
> +		goto free_input_dev;
> +	}
> +
> +	powerbutton_dev->evbit[0] = BIT_MASK(EV_KEY);
> +	powerbutton_dev->keybit[BIT_WORD(KEY_POWER)] = BIT_MASK(KEY_POWER);
> +	powerbutton_dev->name = "triton2-pwrbutton";
> +	powerbutton_dev->phys = "twl4030_pwrbutton/input0";

I would like consistency here, if possible. Everywhere we have
twl4030{-|_}pwrbutton except in the name which says "triton2".. What
gives?

> +	powerbutton_dev->dev.parent = &pdev->dev;
> +
> +	err = input_register_device(powerbutton_dev);
> +	if (err) {
> +		dev_dbg(&pdev->dev, "Can't register power button: %d\n", err);
> +		goto free_irq_and_out;
> +	}
> +
> +	dev_info(&pdev->dev, "triton2 power button driver initialized\n");

Loose the message, boot is noisy enough as it is.

-- 
Dmitry

Re: [PATCH 1/2] input: misc: add twl4030-pwrbutton driver

[Felipe Balbi]

On Sat, Feb 28, 2009 at 04:58:01PM -0800, Dmitry Torokhov wrote:
> On Sun, Mar 01, 2009 at 02:30:18AM +0200, Felipe Balbi wrote:
> > On Sat, Feb 28, 2009 at 02:23:03PM -0800, Dmitry Torokhov wrote:
> > > Hi Felipe,
> > > 
> > > On Fri, Feb 27, 2009 at 09:28:02PM +0200, Felipe Balbi wrote:
> > > > From: Felipe Balbi <felipe.balbi@nokia.com>
> > > > 
> > > > This is part of the twl4030 multifunction device driver.
> > > > 
> > > > With this driver we add support for reporting KEY_POWER
> > > > events via the input layer.
> > > 
> > > ...
> > 
> > thanks for reviewing, how about the version below:
> > 
> 
> Looks good, couple more items...

Fixed the extra comments and also got rid of the global input_dev
structure. It could be fetched by passing it to request_irq() and
platform_set/get_drvdata():

==================== cut here ======================

>From 8d424e1775a5c7a1d76bf2ce4907c3d416ff2fb9 Mon Sep 17 00:00:00 2001
From: Felipe Balbi <felipe.balbi@nokia.com>
Date: Fri, 27 Feb 2009 21:18:15 +0200
Subject: [PATCH] input: misc: add twl4030-pwrbutton driver

This is part of the twl4030 multifunction device driver.

With this driver we add support for reporting KEY_POWER
events via the input layer.

Cc: David Brownell <dbrownell@users.sourceforge.net>
Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Cc: Samuel Ortiz <sameo@openedhand.com>
Signed-off-by: Felipe Balbi <felipe.balbi@nokia.com>
---
Thanks to Andrew Morton and Dmitry Torokhov for reviewing this driver.

 drivers/input/misc/Kconfig             |   10 ++
 drivers/input/misc/Makefile            |    1 +
 drivers/input/misc/twl4030-pwrbutton.c |  146 ++++++++++++++++++++++++++++++++
 3 files changed, 157 insertions(+), 0 deletions(-)
 create mode 100644 drivers/input/misc/twl4030-pwrbutton.c

diff --git a/drivers/input/misc/Kconfig b/drivers/input/misc/Kconfig
index 67e5553..6fa9e38 100644
--- a/drivers/input/misc/Kconfig
+++ b/drivers/input/misc/Kconfig
@@ -193,6 +193,16 @@ config INPUT_CM109
 	  To compile this driver as a module, choose M here: the module will be
 	  called cm109.
 
+config INPUT_TWL4030_PWRBUTTON
+	tristate "TWL4030 Power button Driver"
+	depends on TWL4030_CORE
+	help
+	  Say Y here if you want to enable power key reporting via the
+	  TWL4030 family of chips.
+
+	  To compile this driver as a module, choose M here. The module will
+	  be called twl4030_pwrbutton.
+
 config INPUT_UINPUT
 	tristate "User level driver support"
 	help
diff --git a/drivers/input/misc/Makefile b/drivers/input/misc/Makefile
index bb62e6e..2fabcdb 100644
--- a/drivers/input/misc/Makefile
+++ b/drivers/input/misc/Makefile
@@ -19,6 +19,7 @@ obj-$(CONFIG_INPUT_YEALINK)		+= yealink.o
 obj-$(CONFIG_INPUT_CM109)		+= cm109.o
 obj-$(CONFIG_HP_SDC_RTC)		+= hp_sdc_rtc.o
 obj-$(CONFIG_INPUT_UINPUT)		+= uinput.o
+obj-$(CONFIG_INPUT_TWL4030_PWRBUTTON)	+= twl4030-pwrbutton.o
 obj-$(CONFIG_INPUT_APANEL)		+= apanel.o
 obj-$(CONFIG_INPUT_SGI_BTNS)		+= sgi_btns.o
 obj-$(CONFIG_INPUT_PCF50633_PMU)	+= pcf50633-input.o
diff --git a/drivers/input/misc/twl4030-pwrbutton.c b/drivers/input/misc/twl4030-pwrbutton.c
new file mode 100644
index 0000000..7150830
--- /dev/null
+++ b/drivers/input/misc/twl4030-pwrbutton.c
@@ -0,0 +1,146 @@
+/**
+ * twl4030-pwrbutton.c - TWL4030 Power Button Input Driver
+ *
+ * Copyright (C) 2008-2009 Nokia Corporation
+ *
+ * Written by Peter De Schrijver <peter.de-schrijver@nokia.com>
+ * Several fixes by Felipe Balbi <felipe.balbi@nokia.com>
+ *
+ * This file is subject to the terms and conditions of the GNU General
+ * Public License. See the file "COPYING" in the main directory of this
+ * archive for more details.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
+ */
+
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/kernel.h>
+#include <linux/errno.h>
+#include <linux/input.h>
+#include <linux/interrupt.h>
+#include <linux/platform_device.h>
+#include <linux/i2c/twl4030.h>
+
+#define PWR_PWRON_IRQ (1 << 0)
+
+#define STS_HW_CONDITIONS 0xf
+
+static irqreturn_t powerbutton_irq(int irq, void *_pwr)
+{
+	struct input_dev *pwr = _pwr;
+	int err;
+	u8 value;
+
+#ifdef CONFIG_LOCKDEP
+	/* WORKAROUND for lockdep forcing IRQF_DISABLED on us, which
+	 * we don't want and can't tolerate since this is a threaded
+	 * IRQ and can sleep due to the i2c reads it has to issue.
+	 * 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);
+	if (!err)  {
+		input_report_key(pwr, KEY_POWER, value & PWR_PWRON_IRQ);
+		input_sync(pwr);
+	} else {
+		dev_err(pwr->dev.parent, "twl4030: i2c error %d while reading"
+			" TWL4030 PM_MASTER STS_HW_CONDITIONS register\n", err);
+	}
+
+	return IRQ_HANDLED;
+}
+
+static int __devinit twl4030_pwrbutton_probe(struct platform_device *pdev)
+{
+	struct input_dev *pwr;
+	int irq = platform_get_irq(pdev, 0);
+	int err;
+
+	pwr = input_allocate_device();
+	if (!pwr) {
+		dev_dbg(&pdev->dev, "Can't allocate power button\n");
+		err = -ENOMEM;
+		goto out;
+	}
+
+	err = request_irq(irq, powerbutton_irq,
+			IRQF_TRIGGER_FALLING | IRQF_TRIGGER_RISING,
+			"twl4030_pwrbutton", pwr);
+	if (err < 0) {
+		dev_dbg(&pdev->dev, "Can't get IRQ for pwrbutton: %d\n", err);
+		goto free_input_dev;
+	}
+
+	pwr->evbit[0] = BIT_MASK(EV_KEY);
+	pwr->keybit[BIT_WORD(KEY_POWER)] = BIT_MASK(KEY_POWER);
+	pwr->name = "twl4030_pwrbutton";
+	pwr->phys = "twl4030_pwrbutton/input0";
+	pwr->dev.parent = &pdev->dev;
+	platform_set_drvdata(pdev, pwr);
+
+	err = input_register_device(pwr);
+	if (err) {
+		dev_dbg(&pdev->dev, "Can't register power button: %d\n", err);
+		goto free_irq_and_out;
+	}
+
+	return 0;
+
+free_irq_and_out:
+	free_irq(irq, NULL);
+free_input_dev:
+	input_free_device(pwr);
+out:
+	return err;
+}
+
+static int __devexit twl4030_pwrbutton_remove(struct platform_device *pdev)
+{
+	struct input_dev *pwr = platform_get_drvdata(pdev);
+	int irq = platform_get_irq(pdev, 0);
+
+	free_irq(irq, pwr);
+	input_unregister_device(pwr);
+
+	return 0;
+}
+
+struct platform_driver twl4030_pwrbutton_driver = {
+	.probe		= twl4030_pwrbutton_probe,
+	.remove		= __devexit_p(twl4030_pwrbutton_remove),
+	.driver		= {
+		.name	= "twl4030_pwrbutton",
+		.owner	= THIS_MODULE,
+	},
+};
+
+static int __init twl4030_pwrbutton_init(void)
+{
+	return platform_driver_register(&twl4030_pwrbutton_driver);
+}
+module_init(twl4030_pwrbutton_init);
+
+static void __exit twl4030_pwrbutton_exit(void)
+{
+	platform_driver_unregister(&twl4030_pwrbutton_driver);
+}
+module_exit(twl4030_pwrbutton_exit);
+
+MODULE_ALIAS("platform:twl4030_pwrbutton");
+MODULE_DESCRIPTION("Triton2 Power Button");
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Peter De Schrijver <peter.de-schrijver@nokia.com>");
+MODULE_AUTHOR("Felipe Balbi <felipe.balbi@nokia.com>");
+
-- 
1.6.2.rc0.61.g5cd12


-- 
balbi

Re: [PATCH 1/2] input: misc: add twl4030-pwrbutton driver

[Dmitry Torokhov]

On Sun, Mar 01, 2009 at 04:40:19PM +0200, Felipe Balbi wrote:
> On Sat, Feb 28, 2009 at 04:58:01PM -0800, Dmitry Torokhov wrote:
> > On Sun, Mar 01, 2009 at 02:30:18AM +0200, Felipe Balbi wrote:
> > > On Sat, Feb 28, 2009 at 02:23:03PM -0800, Dmitry Torokhov wrote:
> > > > Hi Felipe,
> > > > 
> > > > On Fri, Feb 27, 2009 at 09:28:02PM +0200, Felipe Balbi wrote:
> > > > > From: Felipe Balbi <felipe.balbi@nokia.com>
> > > > > 
> > > > > This is part of the twl4030 multifunction device driver.
> > > > > 
> > > > > With this driver we add support for reporting KEY_POWER
> > > > > events via the input layer.
> > > > 
> > > > ...
> > > 
> > > thanks for reviewing, how about the version below:
> > > 
> > 
> > Looks good, couple more items...
> 
> Fixed the extra comments and also got rid of the global input_dev
> structure. It could be fetched by passing it to request_irq() and
> platform_set/get_drvdata():
> 

Cool, I will keep it in my tree pending the threaded IRQ issue
resolution.

-- 
Dmitry

Re: [PATCH 1/2] input: misc: add twl4030-pwrbutton driver

[Felipe Balbi]

On Wed, Mar 04, 2009 at 10:00:27AM +0100, ext Dmitry Torokhov wrote:
> On Sun, Mar 01, 2009 at 04:40:19PM +0200, Felipe Balbi wrote:
> > On Sat, Feb 28, 2009 at 04:58:01PM -0800, Dmitry Torokhov wrote:
> > > On Sun, Mar 01, 2009 at 02:30:18AM +0200, Felipe Balbi wrote:
> > > > On Sat, Feb 28, 2009 at 02:23:03PM -0800, Dmitry Torokhov wrote:
> > > > > Hi Felipe,
> > > > > 
> > > > > On Fri, Feb 27, 2009 at 09:28:02PM +0200, Felipe Balbi wrote:
> > > > > > From: Felipe Balbi <felipe.balbi@nokia.com>
> > > > > > 
> > > > > > This is part of the twl4030 multifunction device driver.
> > > > > > 
> > > > > > With this driver we add support for reporting KEY_POWER
> > > > > > events via the input layer.
> > > > > 
> > > > > ...
> > > > 
> > > > thanks for reviewing, how about the version below:
> > > > 
> > > 
> > > Looks good, couple more items...
> > 
> > Fixed the extra comments and also got rid of the global input_dev
> > structure. It could be fetched by passing it to request_irq() and
> > platform_set/get_drvdata():
> > 
> 
> Cool, I will keep it in my tree pending the threaded IRQ issue
> resolution.

I guess the threaded IRQ issue will still take a while to be solved and
this driver is working as is, so there's no big problem in pushing it
and fixing the threaded irq on later patches. But I'd like to hear from
Andrew, Dave and the others about that.

-- 
balbi

Re: [PATCH 1/2] input: misc: add twl4030-pwrbutton driver

[David Brownell]

On Wednesday 04 March 2009, Felipe Balbi wrote:
> 
> > Cool, I will keep it in my tree pending the threaded IRQ issue
> > resolution.
> 
> I guess the threaded IRQ issue will still take a while to be solved and
> this driver is working as is, so there's no big problem in pushing it
> and fixing the threaded irq on later patches. But I'd like to hear from
> Andrew, Dave and the others about that.

I'd say just push this driver with the other 2.6.30 stuff.
There's no point in holding it back.

The yet-to-be-merged threaded IRQ support won't break the
in-tree stuff.  And it's not exactly fully cooked, since
it doesn't yet handle a highly-relevant use-case ... :)

So the plan of record remains unchanged:  once mainline
has adequately functional threaded IRQ support, update
twl4030-family drivers to use it.  Meanwhile, use the
current stuff, which despite some aesthetic warts still
works just fine.

The positive note seems to be that the IRQ threading
patches have gotten refreshed.

- Dave

Re: [PATCH 1/2] input: misc: add twl4030-pwrbutton driver

[David Brownell]

On Wednesday 04 March 2009, Dmitry Torokhov wrote:
> Cool, I will keep it in my tree pending the threaded IRQ issue
> resolution.

Speaking of which ... I'd like to confirm that you
already have the twl4030_keypad.c driver in your tree.

That was (re) submitted via linux-input on 6-Feb, after
addressing feedback from the previous version (and cc'd to
the OMAP list), but I've not heard anything ...

http://marc.info/?l=linux-omap&m=123391069411717&w=2

I don't know of a good linux-input archive or I'd
reference the post from there.

Re: [PATCH 1/2] input: misc: add twl4030-pwrbutton driver

[Dmitry Torokhov]

On Thu, Mar 05, 2009 at 03:54:40PM -0800, David Brownell wrote:
> On Wednesday 04 March 2009, Dmitry Torokhov wrote:
> > Cool, I will keep it in my tree pending the threaded IRQ issue
> > resolution.
> 
> Speaking of which ... I'd like to confirm that you
> already have the twl4030_keypad.c driver in your tree.
> 
> That was (re) submitted via linux-input on 6-Feb, after
> addressing feedback from the previous version (and cc'd to
> the OMAP list), but I've not heard anything ...
> 
> http://marc.info/?l=linux-omap&m=123391069411717&w=2
> 
> I don't know of a good linux-input archive or I'd
> reference the post from there.
> 
> 

Yep, I have it now. Thanks David.

-- 
Dmitry