* [PATCH 1/2] input: misc: add twl4030-pwrbutton driver @ 2009-02-27 19:28 Felipe Balbi 2009-02-27 19:28 ` [PATCH 2/2] mfd: twl4030: add twl4030-pwrbutton as our child Felipe Balbi ` (3 more replies) 0 siblings, 4 replies; 106+ messages in thread From: Felipe Balbi @ 2009-02-27 19:28 UTC (permalink / raw) To: linux-kernel, linux-input Cc: Andrew Morton, Felipe Balbi, David Brownell, Dmitry Torokhov, Samuel Ortiz 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 ^ permalink raw reply related [flat|nested] 106+ messages in thread
* [PATCH 2/2] mfd: twl4030: add twl4030-pwrbutton as our child 2009-02-27 19:28 [PATCH 1/2] input: misc: add twl4030-pwrbutton driver Felipe Balbi @ 2009-02-27 19:28 ` Felipe Balbi 2009-02-27 20:36 ` Andrew Morton 2009-02-27 20:33 ` [PATCH 1/2] input: misc: add twl4030-pwrbutton driver Andrew Morton ` (2 subsequent siblings) 3 siblings, 1 reply; 106+ messages in thread From: Felipe Balbi @ 2009-02-27 19:28 UTC (permalink / raw) To: linux-kernel, linux-input Cc: Andrew Morton, Felipe Balbi, Samuel Ortiz, David Brownell 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 ^ permalink raw reply related [flat|nested] 106+ messages in thread
* Re: [PATCH 2/2] mfd: twl4030: add twl4030-pwrbutton as our child 2009-02-27 19:28 ` [PATCH 2/2] mfd: twl4030: add twl4030-pwrbutton as our child Felipe Balbi @ 2009-02-27 20:36 ` Andrew Morton 2009-02-27 21:58 ` David Brownell 0 siblings, 1 reply; 106+ messages in thread From: Andrew Morton @ 2009-02-27 20:36 UTC (permalink / raw) To: Felipe Balbi; +Cc: linux-kernel, linux-input, felipe.balbi, sameo, dbrownell 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? ^ permalink raw reply [flat|nested] 106+ messages in thread
* Re: [PATCH 2/2] mfd: twl4030: add twl4030-pwrbutton as our child 2009-02-27 20:36 ` Andrew Morton @ 2009-02-27 21:58 ` David Brownell 2009-02-27 22:09 ` Felipe Balbi 2009-02-27 22:12 ` Andrew Morton 0 siblings, 2 replies; 106+ messages in thread From: David Brownell @ 2009-02-27 21:58 UTC (permalink / raw) To: Andrew Morton Cc: Felipe Balbi, linux-kernel, linux-input, felipe.balbi, sameo 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 ^ permalink raw reply [flat|nested] 106+ messages in thread
* Re: [PATCH 2/2] mfd: twl4030: add twl4030-pwrbutton as our child 2009-02-27 21:58 ` David Brownell @ 2009-02-27 22:09 ` Felipe Balbi 2009-02-27 22:12 ` Andrew Morton 1 sibling, 0 replies; 106+ messages in thread From: Felipe Balbi @ 2009-02-27 22:09 UTC (permalink / raw) To: David Brownell Cc: Andrew Morton, Felipe Balbi, linux-kernel, linux-input, felipe.balbi, sameo 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 ^ permalink raw reply related [flat|nested] 106+ messages in thread
* Re: [PATCH 2/2] mfd: twl4030: add twl4030-pwrbutton as our child 2009-02-27 21:58 ` David Brownell 2009-02-27 22:09 ` Felipe Balbi @ 2009-02-27 22:12 ` Andrew Morton 2009-02-27 23:20 ` David Brownell 1 sibling, 1 reply; 106+ messages in thread From: Andrew Morton @ 2009-02-27 22:12 UTC (permalink / raw) To: David Brownell; +Cc: me, linux-kernel, linux-input, felipe.balbi, sameo 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 :( ^ permalink raw reply [flat|nested] 106+ messages in thread
* Re: [PATCH 2/2] mfd: twl4030: add twl4030-pwrbutton as our child 2009-02-27 22:12 ` Andrew Morton @ 2009-02-27 23:20 ` David Brownell 2009-02-27 23:42 ` Andrew Morton 0 siblings, 1 reply; 106+ messages in thread From: David Brownell @ 2009-02-27 23:20 UTC (permalink / raw) To: Andrew Morton; +Cc: me, linux-kernel, linux-input, felipe.balbi, sameo 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? ^ permalink raw reply [flat|nested] 106+ messages in thread
* Re: [PATCH 2/2] mfd: twl4030: add twl4030-pwrbutton as our child 2009-02-27 23:20 ` David Brownell @ 2009-02-27 23:42 ` Andrew Morton 2009-07-22 19:27 ` Trilok Soni 0 siblings, 1 reply; 106+ messages in thread From: Andrew Morton @ 2009-02-27 23:42 UTC (permalink / raw) To: David Brownell; +Cc: me, linux-kernel, linux-input, felipe.balbi, sameo 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. ^ permalink raw reply [flat|nested] 106+ messages in thread
* Re: [PATCH 2/2] mfd: twl4030: add twl4030-pwrbutton as our child 2009-02-27 23:42 ` Andrew Morton @ 2009-07-22 19:27 ` Trilok Soni 2009-07-22 22:25 ` David Brownell 0 siblings, 1 reply; 106+ messages in thread From: Trilok Soni @ 2009-07-22 19:27 UTC (permalink / raw) To: Andrew Morton Cc: David Brownell, me, linux-kernel, linux-input, felipe.balbi, Samuel Ortiz 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 ^ permalink raw reply [flat|nested] 106+ messages in thread
* Re: [PATCH 2/2] mfd: twl4030: add twl4030-pwrbutton as our child 2009-07-22 19:27 ` Trilok Soni @ 2009-07-22 22:25 ` David Brownell 0 siblings, 0 replies; 106+ messages in thread From: David Brownell @ 2009-07-22 22:25 UTC (permalink / raw) To: Trilok Soni Cc: Andrew Morton, me, linux-kernel, linux-input, felipe.balbi, Samuel Ortiz 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. > ^ permalink raw reply [flat|nested] 106+ messages in thread
* Re: [PATCH 1/2] input: misc: add twl4030-pwrbutton driver 2009-02-27 19:28 [PATCH 1/2] input: misc: add twl4030-pwrbutton driver Felipe Balbi 2009-02-27 19:28 ` [PATCH 2/2] mfd: twl4030: add twl4030-pwrbutton as our child Felipe Balbi @ 2009-02-27 20:33 ` Andrew Morton 2009-02-27 20:37 ` Felipe Balbi 2009-02-28 5:51 ` [PATCH 1/2] input: misc: add twl4030-pwrbutton driver Trilok Soni 2009-02-28 22:23 ` [PATCH 1/2] input: misc: add twl4030-pwrbutton driver Dmitry Torokhov 3 siblings, 1 reply; 106+ messages in thread From: Andrew Morton @ 2009-02-27 20:33 UTC (permalink / raw) To: Felipe Balbi Cc: linux-kernel, linux-input, felipe.balbi, dbrownell, dmitry.torokhov, sameo, Peter Zijlstra 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 ;) ^ permalink raw reply [flat|nested] 106+ messages in thread
* Re: [PATCH 1/2] input: misc: add twl4030-pwrbutton driver 2009-02-27 20:33 ` [PATCH 1/2] input: misc: add twl4030-pwrbutton driver Andrew Morton @ 2009-02-27 20:37 ` Felipe Balbi 2009-02-27 21:50 ` lockdep and threaded IRQs (was: [PATCH 1/2] input: misc: add twl4030-pwrbutton driver) David Brownell 0 siblings, 1 reply; 106+ messages in thread From: Felipe Balbi @ 2009-02-27 20:37 UTC (permalink / raw) To: Andrew Morton Cc: Felipe Balbi, linux-kernel, linux-input, felipe.balbi, dbrownell, dmitry.torokhov, sameo, Peter Zijlstra 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 ^ permalink raw reply [flat|nested] 106+ messages in thread
* Re: lockdep and threaded IRQs (was: [PATCH 1/2] input: misc: add twl4030-pwrbutton driver) 2009-02-27 20:37 ` Felipe Balbi @ 2009-02-27 21:50 ` David Brownell 2009-02-27 22:09 ` Andrew Morton 2009-02-28 11:20 ` lockdep and threaded IRQs Stefan Richter 0 siblings, 2 replies; 106+ messages in thread From: David Brownell @ 2009-02-27 21:50 UTC (permalink / raw) To: me Cc: Andrew Morton, linux-kernel, linux-input, felipe.balbi, dmitry.torokhov, sameo, Peter Zijlstra, Thomas Gleixner 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, ^ permalink raw reply [flat|nested] 106+ messages in thread
* Re: lockdep and threaded IRQs (was: [PATCH 1/2] input: misc: add twl4030-pwrbutton driver) 2009-02-27 21:50 ` lockdep and threaded IRQs (was: [PATCH 1/2] input: misc: add twl4030-pwrbutton driver) David Brownell @ 2009-02-27 22:09 ` Andrew Morton 2009-02-27 23:18 ` lockdep and threaded IRQs (was: ...) David Brownell 2009-02-28 11:20 ` lockdep and threaded IRQs Stefan Richter 1 sibling, 1 reply; 106+ messages in thread From: Andrew Morton @ 2009-02-27 22:09 UTC (permalink / raw) To: David Brownell Cc: me, linux-kernel, linux-input, felipe.balbi, dmitry.torokhov, sameo, a.p.zijlstra, tglx 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. ^ permalink raw reply [flat|nested] 106+ messages in thread
* Re: lockdep and threaded IRQs (was: ...) 2009-02-27 22:09 ` Andrew Morton @ 2009-02-27 23:18 ` David Brownell 2009-02-27 23:32 ` Andrew Morton ` (2 more replies) 0 siblings, 3 replies; 106+ messages in thread From: David Brownell @ 2009-02-27 23:18 UTC (permalink / raw) To: Andrew Morton Cc: me, linux-kernel, linux-input, felipe.balbi, dmitry.torokhov, sameo, a.p.zijlstra, tglx 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 ^ permalink raw reply [flat|nested] 106+ messages in thread
* Re: lockdep and threaded IRQs (was: ...) 2009-02-27 23:18 ` lockdep and threaded IRQs (was: ...) David Brownell @ 2009-02-27 23:32 ` Andrew Morton 2009-02-28 0:01 ` Andrew Morton 2009-03-02 13:16 ` lockdep and threaded IRQs (was: ...) Peter Zijlstra 2009-03-02 15:13 ` [PATCH] genirq: assert that irq handlers are indeed run in hardirq context Peter Zijlstra 2 siblings, 1 reply; 106+ messages in thread From: Andrew Morton @ 2009-02-27 23:32 UTC (permalink / raw) To: David Brownell Cc: me, linux-kernel, linux-input, felipe.balbi, dmitry.torokhov, sameo, a.p.zijlstra, tglx 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? ^ permalink raw reply [flat|nested] 106+ messages in thread
* Re: lockdep and threaded IRQs (was: ...) 2009-02-27 23:32 ` Andrew Morton @ 2009-02-28 0:01 ` Andrew Morton 2009-02-28 2:30 ` David Brownell 0 siblings, 1 reply; 106+ messages in thread From: Andrew Morton @ 2009-02-28 0:01 UTC (permalink / raw) To: david-b, me, linux-kernel, linux-input, felipe.balbi, dmitry.torokhov, sameo, a.p.zijlstra, tglx 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. ^ permalink raw reply [flat|nested] 106+ messages in thread
* Re: lockdep and threaded IRQs (was: ...) 2009-02-28 0:01 ` Andrew Morton @ 2009-02-28 2:30 ` David Brownell 2009-02-28 2:39 ` Andrew Morton 0 siblings, 1 reply; 106+ messages in thread From: David Brownell @ 2009-02-28 2:30 UTC (permalink / raw) To: Andrew Morton Cc: me, linux-kernel, linux-input, felipe.balbi, dmitry.torokhov, sameo, a.p.zijlstra, tglx 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. ^ permalink raw reply [flat|nested] 106+ messages in thread
* Re: lockdep and threaded IRQs (was: ...) 2009-02-28 2:30 ` David Brownell @ 2009-02-28 2:39 ` Andrew Morton 2009-02-28 4:46 ` David Brownell 0 siblings, 1 reply; 106+ messages in thread From: Andrew Morton @ 2009-02-28 2:39 UTC (permalink / raw) To: David Brownell Cc: me, linux-kernel, linux-input, felipe.balbi, dmitry.torokhov, sameo, a.p.zijlstra, tglx 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? ^ permalink raw reply [flat|nested] 106+ messages in thread
* Re: lockdep and threaded IRQs (was: ...) 2009-02-28 2:39 ` Andrew Morton @ 2009-02-28 4:46 ` David Brownell 2009-02-28 5:12 ` Andrew Morton ` (2 more replies) 0 siblings, 3 replies; 106+ messages in thread From: David Brownell @ 2009-02-28 4:46 UTC (permalink / raw) To: Andrew Morton Cc: me, linux-kernel, linux-input, felipe.balbi, dmitry.torokhov, sameo, a.p.zijlstra, tglx 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 ^ permalink raw reply [flat|nested] 106+ messages in thread
* Re: lockdep and threaded IRQs (was: ...) 2009-02-28 4:46 ` David Brownell @ 2009-02-28 5:12 ` Andrew Morton 2009-02-28 6:17 ` David Brownell 2009-02-28 11:13 ` Thomas Gleixner 2009-02-28 11:48 ` lockdep and threaded IRQs Stefan Richter 2 siblings, 1 reply; 106+ messages in thread From: Andrew Morton @ 2009-02-28 5:12 UTC (permalink / raw) To: David Brownell Cc: me, linux-kernel, linux-input, felipe.balbi, dmitry.torokhov, sameo, a.p.zijlstra, tglx 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, _ ^ permalink raw reply [flat|nested] 106+ messages in thread
* Re: lockdep and threaded IRQs (was: ...) 2009-02-28 5:12 ` Andrew Morton @ 2009-02-28 6:17 ` David Brownell 0 siblings, 0 replies; 106+ messages in thread From: David Brownell @ 2009-02-28 6:17 UTC (permalink / raw) To: Andrew Morton Cc: me, linux-kernel, linux-input, felipe.balbi, dmitry.torokhov, sameo, a.p.zijlstra, tglx 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, > ^ permalink raw reply [flat|nested] 106+ messages in thread
* Re: lockdep and threaded IRQs (was: ...) 2009-02-28 4:46 ` David Brownell 2009-02-28 5:12 ` Andrew Morton @ 2009-02-28 11:13 ` Thomas Gleixner 2009-02-28 12:16 ` David Brownell 2009-02-28 11:48 ` lockdep and threaded IRQs Stefan Richter 2 siblings, 1 reply; 106+ messages in thread From: Thomas Gleixner @ 2009-02-28 11:13 UTC (permalink / raw) To: David Brownell Cc: Andrew Morton, me, linux-kernel, linux-input, felipe.balbi, dmitry.torokhov, sameo, a.p.zijlstra 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 ^ permalink raw reply [flat|nested] 106+ messages in thread
* Re: lockdep and threaded IRQs (was: ...) 2009-02-28 11:13 ` Thomas Gleixner @ 2009-02-28 12:16 ` David Brownell 2009-02-28 16:42 ` Thomas Gleixner 0 siblings, 1 reply; 106+ messages in thread From: David Brownell @ 2009-02-28 12:16 UTC (permalink / raw) To: Thomas Gleixner Cc: Andrew Morton, me, linux-kernel, linux-input, felipe.balbi, dmitry.torokhov, sameo, a.p.zijlstra 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? ^ permalink raw reply [flat|nested] 106+ messages in thread
* Re: lockdep and threaded IRQs (was: ...) 2009-02-28 12:16 ` David Brownell @ 2009-02-28 16:42 ` Thomas Gleixner 2009-02-28 20:02 ` David Brownell 0 siblings, 1 reply; 106+ messages in thread From: Thomas Gleixner @ 2009-02-28 16:42 UTC (permalink / raw) To: David Brownell Cc: Andrew Morton, me, linux-kernel, linux-input, felipe.balbi, dmitry.torokhov, sameo, a.p.zijlstra 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 ^ permalink raw reply [flat|nested] 106+ messages in thread
* Re: lockdep and threaded IRQs (was: ...) 2009-02-28 16:42 ` Thomas Gleixner @ 2009-02-28 20:02 ` David Brownell 2009-02-28 20:55 ` Thomas Gleixner 0 siblings, 1 reply; 106+ messages in thread From: David Brownell @ 2009-02-28 20:02 UTC (permalink / raw) To: Thomas Gleixner Cc: Andrew Morton, me, linux-kernel, linux-input, felipe.balbi, dmitry.torokhov, sameo, a.p.zijlstra 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 ^ permalink raw reply [flat|nested] 106+ messages in thread
* Re: lockdep and threaded IRQs (was: ...) 2009-02-28 20:02 ` David Brownell @ 2009-02-28 20:55 ` Thomas Gleixner 2009-02-28 21:13 ` Thomas Gleixner ` (2 more replies) 0 siblings, 3 replies; 106+ messages in thread From: Thomas Gleixner @ 2009-02-28 20:55 UTC (permalink / raw) To: David Brownell Cc: Andrew Morton, me, linux-kernel, linux-input, felipe.balbi, dmitry.torokhov, sameo, a.p.zijlstra 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 ^ permalink raw reply [flat|nested] 106+ messages in thread
* Re: lockdep and threaded IRQs (was: ...) 2009-02-28 20:55 ` Thomas Gleixner @ 2009-02-28 21:13 ` Thomas Gleixner 2009-02-28 22:37 ` David Brownell 2009-02-28 22:05 ` David Brownell 2009-03-01 22:11 ` David Brownell 2 siblings, 1 reply; 106+ messages in thread From: Thomas Gleixner @ 2009-02-28 21:13 UTC (permalink / raw) To: David Brownell Cc: Andrew Morton, me, linux-kernel, linux-input, felipe.balbi, dmitry.torokhov, sameo, a.p.zijlstra 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 ^ permalink raw reply [flat|nested] 106+ messages in thread
* Re: lockdep and threaded IRQs (was: ...) 2009-02-28 21:13 ` Thomas Gleixner @ 2009-02-28 22:37 ` David Brownell 0 siblings, 0 replies; 106+ messages in thread From: David Brownell @ 2009-02-28 22:37 UTC (permalink / raw) To: Thomas Gleixner Cc: Andrew Morton, me, linux-kernel, linux-input, felipe.balbi, dmitry.torokhov, sameo, a.p.zijlstra 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 ^ permalink raw reply [flat|nested] 106+ messages in thread
* Re: lockdep and threaded IRQs (was: ...) 2009-02-28 20:55 ` Thomas Gleixner 2009-02-28 21:13 ` Thomas Gleixner @ 2009-02-28 22:05 ` David Brownell 2009-03-01 9:43 ` Thomas Gleixner 2009-03-01 22:11 ` David Brownell 2 siblings, 1 reply; 106+ messages in thread From: David Brownell @ 2009-02-28 22:05 UTC (permalink / raw) To: Thomas Gleixner Cc: Andrew Morton, me, linux-kernel, linux-input, felipe.balbi, dmitry.torokhov, sameo, a.p.zijlstra 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 ^ permalink raw reply [flat|nested] 106+ messages in thread
* Re: lockdep and threaded IRQs (was: ...) 2009-02-28 22:05 ` David Brownell @ 2009-03-01 9:43 ` Thomas Gleixner 2009-03-01 22:54 ` David Brownell 0 siblings, 1 reply; 106+ messages in thread From: Thomas Gleixner @ 2009-03-01 9:43 UTC (permalink / raw) To: David Brownell Cc: Andrew Morton, me, linux-kernel, linux-input, felipe.balbi, dmitry.torokhov, sameo, a.p.zijlstra 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 ^ permalink raw reply [flat|nested] 106+ messages in thread
* Re: lockdep and threaded IRQs (was: ...) 2009-03-01 9:43 ` Thomas Gleixner @ 2009-03-01 22:54 ` David Brownell 2009-03-02 13:16 ` Peter Zijlstra 0 siblings, 1 reply; 106+ messages in thread From: David Brownell @ 2009-03-01 22:54 UTC (permalink / raw) To: Thomas Gleixner Cc: Andrew Morton, me, linux-kernel, linux-input, felipe.balbi, dmitry.torokhov, sameo, a.p.zijlstra 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 ^ permalink raw reply [flat|nested] 106+ messages in thread
* Re: lockdep and threaded IRQs (was: ...) 2009-03-01 22:54 ` David Brownell @ 2009-03-02 13:16 ` Peter Zijlstra 2009-03-02 21:04 ` David Brownell 0 siblings, 1 reply; 106+ messages in thread From: Peter Zijlstra @ 2009-03-02 13:16 UTC (permalink / raw) To: David Brownell Cc: Thomas Gleixner, Andrew Morton, me, linux-kernel, linux-input, felipe.balbi, dmitry.torokhov, sameo 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. ^ permalink raw reply [flat|nested] 106+ messages in thread
* Re: lockdep and threaded IRQs (was: ...) 2009-03-02 13:16 ` Peter Zijlstra @ 2009-03-02 21:04 ` David Brownell 2009-03-02 21:16 ` Peter Zijlstra 0 siblings, 1 reply; 106+ messages in thread From: David Brownell @ 2009-03-02 21:04 UTC (permalink / raw) To: Peter Zijlstra Cc: Thomas Gleixner, Andrew Morton, me, linux-kernel, linux-input, felipe.balbi, dmitry.torokhov, sameo 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. ^ permalink raw reply [flat|nested] 106+ messages in thread
* Re: lockdep and threaded IRQs (was: ...) 2009-03-02 21:04 ` David Brownell @ 2009-03-02 21:16 ` Peter Zijlstra 2009-03-02 21:29 ` Andrew Morton ` (2 more replies) 0 siblings, 3 replies; 106+ messages in thread From: Peter Zijlstra @ 2009-03-02 21:16 UTC (permalink / raw) To: David Brownell Cc: Thomas Gleixner, Andrew Morton, me, linux-kernel, linux-input, felipe.balbi, dmitry.torokhov, sameo 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. ^ permalink raw reply [flat|nested] 106+ messages in thread
* Re: lockdep and threaded IRQs (was: ...) 2009-03-02 21:16 ` Peter Zijlstra @ 2009-03-02 21:29 ` Andrew Morton 2009-03-02 21:37 ` David Brownell 2009-03-02 23:35 ` lockdep and threaded IRQs (was: ...) Alan Cox 2 siblings, 0 replies; 106+ messages in thread From: Andrew Morton @ 2009-03-02 21:29 UTC (permalink / raw) To: Peter Zijlstra Cc: david-b, tglx, me, linux-kernel, linux-input, felipe.balbi, dmitry.torokhov, sameo 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? ^ permalink raw reply [flat|nested] 106+ messages in thread
* Re: lockdep and threaded IRQs (was: ...) 2009-03-02 21:16 ` Peter Zijlstra 2009-03-02 21:29 ` Andrew Morton @ 2009-03-02 21:37 ` David Brownell 2009-03-02 21:41 ` Peter Zijlstra 2009-03-02 23:35 ` lockdep and threaded IRQs (was: ...) Alan Cox 2 siblings, 1 reply; 106+ messages in thread From: David Brownell @ 2009-03-02 21:37 UTC (permalink / raw) To: Peter Zijlstra Cc: Thomas Gleixner, Andrew Morton, me, linux-kernel, linux-input, felipe.balbi, dmitry.torokhov, sameo 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. ^ permalink raw reply [flat|nested] 106+ messages in thread
* Re: lockdep and threaded IRQs (was: ...) 2009-03-02 21:37 ` David Brownell @ 2009-03-02 21:41 ` Peter Zijlstra 2009-03-02 22:09 ` David Brownell 0 siblings, 1 reply; 106+ messages in thread From: Peter Zijlstra @ 2009-03-02 21:41 UTC (permalink / raw) To: David Brownell Cc: Thomas Gleixner, Andrew Morton, me, linux-kernel, linux-input, felipe.balbi, dmitry.torokhov, sameo 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. ^ permalink raw reply [flat|nested] 106+ messages in thread
* Re: lockdep and threaded IRQs (was: ...) 2009-03-02 21:41 ` Peter Zijlstra @ 2009-03-02 22:09 ` David Brownell 2009-03-02 22:19 ` Peter Zijlstra 0 siblings, 1 reply; 106+ messages in thread From: David Brownell @ 2009-03-02 22:09 UTC (permalink / raw) To: Peter Zijlstra Cc: Thomas Gleixner, Andrew Morton, me, linux-kernel, linux-input, felipe.balbi, dmitry.torokhov, sameo 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 ^ permalink raw reply [flat|nested] 106+ messages in thread
* Re: lockdep and threaded IRQs (was: ...) 2009-03-02 22:09 ` David Brownell @ 2009-03-02 22:19 ` Peter Zijlstra 2009-03-02 22:40 ` David Brownell 2009-03-02 22:46 ` lockdep and threaded IRQs David Miller 0 siblings, 2 replies; 106+ messages in thread From: Peter Zijlstra @ 2009-03-02 22:19 UTC (permalink / raw) To: dbrownell Cc: Thomas Gleixner, Andrew Morton, me, linux-kernel, linux-input, felipe.balbi, dmitry.torokhov, sameo 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. ^ permalink raw reply [flat|nested] 106+ messages in thread
* Re: lockdep and threaded IRQs (was: ...) 2009-03-02 22:19 ` Peter Zijlstra @ 2009-03-02 22:40 ` David Brownell 2009-03-02 22:51 ` Peter Zijlstra 2009-03-02 22:46 ` lockdep and threaded IRQs David Miller 1 sibling, 1 reply; 106+ messages in thread From: David Brownell @ 2009-03-02 22:40 UTC (permalink / raw) To: Peter Zijlstra Cc: dbrownell, Thomas Gleixner, Andrew Morton, me, linux-kernel, linux-input, felipe.balbi, dmitry.torokhov, sameo 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 ^ permalink raw reply [flat|nested] 106+ messages in thread
* Re: lockdep and threaded IRQs (was: ...) 2009-03-02 22:40 ` David Brownell @ 2009-03-02 22:51 ` Peter Zijlstra 2009-03-02 23:29 ` David Brownell 0 siblings, 1 reply; 106+ messages in thread From: Peter Zijlstra @ 2009-03-02 22:51 UTC (permalink / raw) To: David Brownell Cc: dbrownell, Thomas Gleixner, Andrew Morton, me, linux-kernel, linux-input, felipe.balbi, dmitry.torokhov, sameo 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. ^ permalink raw reply [flat|nested] 106+ messages in thread
* Re: lockdep and threaded IRQs (was: ...) 2009-03-02 22:51 ` Peter Zijlstra @ 2009-03-02 23:29 ` David Brownell 2009-03-03 7:45 ` Peter Zijlstra 0 siblings, 1 reply; 106+ messages in thread From: David Brownell @ 2009-03-02 23:29 UTC (permalink / raw) To: Peter Zijlstra Cc: dbrownell, Thomas Gleixner, Andrew Morton, me, linux-kernel, linux-input, felipe.balbi, dmitry.torokhov, sameo 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 ^ permalink raw reply [flat|nested] 106+ messages in thread
* Re: lockdep and threaded IRQs (was: ...) 2009-03-02 23:29 ` David Brownell @ 2009-03-03 7:45 ` Peter Zijlstra 0 siblings, 0 replies; 106+ messages in thread From: Peter Zijlstra @ 2009-03-03 7:45 UTC (permalink / raw) To: David Brownell Cc: dbrownell, Thomas Gleixner, Andrew Morton, me, linux-kernel, linux-input, felipe.balbi, dmitry.torokhov, sameo 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 ^ permalink raw reply [flat|nested] 106+ messages in thread
* Re: lockdep and threaded IRQs 2009-03-02 22:19 ` Peter Zijlstra 2009-03-02 22:40 ` David Brownell @ 2009-03-02 22:46 ` David Miller 2009-03-02 22:57 ` Andrew Morton 2009-03-02 23:02 ` Peter Zijlstra 1 sibling, 2 replies; 106+ messages in thread From: David Miller @ 2009-03-02 22:46 UTC (permalink / raw) To: a.p.zijlstra Cc: dbrownell, tglx, akpm, me, linux-kernel, linux-input, felipe.balbi, dmitry.torokhov, sameo 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 :-) ^ permalink raw reply [flat|nested] 106+ messages in thread
* Re: lockdep and threaded IRQs 2009-03-02 22:46 ` lockdep and threaded IRQs David Miller @ 2009-03-02 22:57 ` Andrew Morton 2009-03-02 23:07 ` Peter Zijlstra 2009-03-02 23:02 ` Peter Zijlstra 1 sibling, 1 reply; 106+ messages in thread From: Andrew Morton @ 2009-03-02 22:57 UTC (permalink / raw) To: David Miller Cc: a.p.zijlstra, dbrownell, tglx, me, linux-kernel, linux-input, felipe.balbi, dmitry.torokhov, sameo 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. ^ permalink raw reply [flat|nested] 106+ messages in thread
* Re: lockdep and threaded IRQs 2009-03-02 22:57 ` Andrew Morton @ 2009-03-02 23:07 ` Peter Zijlstra 0 siblings, 0 replies; 106+ messages in thread From: Peter Zijlstra @ 2009-03-02 23:07 UTC (permalink / raw) To: Andrew Morton Cc: David Miller, dbrownell, tglx, me, linux-kernel, linux-input, felipe.balbi, dmitry.torokhov, sameo 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. ^ permalink raw reply [flat|nested] 106+ messages in thread
* Re: lockdep and threaded IRQs 2009-03-02 22:46 ` lockdep and threaded IRQs David Miller 2009-03-02 22:57 ` Andrew Morton @ 2009-03-02 23:02 ` Peter Zijlstra 1 sibling, 0 replies; 106+ messages in thread From: Peter Zijlstra @ 2009-03-02 23:02 UTC (permalink / raw) To: David Miller Cc: dbrownell, tglx, akpm, me, linux-kernel, linux-input, felipe.balbi, dmitry.torokhov, sameo 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. ^ permalink raw reply [flat|nested] 106+ messages in thread
* Re: lockdep and threaded IRQs (was: ...) 2009-03-02 21:16 ` Peter Zijlstra 2009-03-02 21:29 ` Andrew Morton 2009-03-02 21:37 ` David Brownell @ 2009-03-02 23:35 ` Alan Cox 2 siblings, 0 replies; 106+ messages in thread From: Alan Cox @ 2009-03-02 23:35 UTC (permalink / raw) To: Peter Zijlstra Cc: David Brownell, Thomas Gleixner, Andrew Morton, me, linux-kernel, linux-input, felipe.balbi, dmitry.torokhov, sameo > 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 ^ permalink raw reply [flat|nested] 106+ messages in thread
* Re: lockdep and threaded IRQs (was: ...) 2009-02-28 20:55 ` Thomas Gleixner 2009-02-28 21:13 ` Thomas Gleixner 2009-02-28 22:05 ` David Brownell @ 2009-03-01 22:11 ` David Brownell 2 siblings, 0 replies; 106+ messages in thread From: David Brownell @ 2009-03-01 22:11 UTC (permalink / raw) To: Thomas Gleixner Cc: Andrew Morton, me, linux-kernel, linux-input, felipe.balbi, dmitry.torokhov, sameo, a.p.zijlstra 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++) ^ permalink raw reply [flat|nested] 106+ messages in thread
* Re: lockdep and threaded IRQs 2009-02-28 4:46 ` David Brownell 2009-02-28 5:12 ` Andrew Morton 2009-02-28 11:13 ` Thomas Gleixner @ 2009-02-28 11:48 ` Stefan Richter 2009-02-28 20:19 ` David Brownell 2 siblings, 1 reply; 106+ messages in thread From: Stefan Richter @ 2009-02-28 11:48 UTC (permalink / raw) To: David Brownell Cc: Andrew Morton, me, linux-kernel, linux-input, felipe.balbi, dmitry.torokhov, sameo, a.p.zijlstra, tglx 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/ ^ permalink raw reply [flat|nested] 106+ messages in thread
* Re: lockdep and threaded IRQs 2009-02-28 11:48 ` lockdep and threaded IRQs Stefan Richter @ 2009-02-28 20:19 ` David Brownell 2009-02-28 21:10 ` Stefan Richter 0 siblings, 1 reply; 106+ messages in thread From: David Brownell @ 2009-02-28 20:19 UTC (permalink / raw) To: Stefan Richter Cc: Andrew Morton, me, linux-kernel, linux-input, felipe.balbi, dmitry.torokhov, sameo, a.p.zijlstra, tglx 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 ^ permalink raw reply [flat|nested] 106+ messages in thread
* Re: lockdep and threaded IRQs 2009-02-28 20:19 ` David Brownell @ 2009-02-28 21:10 ` Stefan Richter 0 siblings, 0 replies; 106+ messages in thread From: Stefan Richter @ 2009-02-28 21:10 UTC (permalink / raw) To: David Brownell Cc: Andrew Morton, me, linux-kernel, linux-input, felipe.balbi, dmitry.torokhov, sameo, a.p.zijlstra, tglx 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/ ^ permalink raw reply [flat|nested] 106+ messages in thread
* Re: lockdep and threaded IRQs (was: ...) 2009-02-27 23:18 ` lockdep and threaded IRQs (was: ...) David Brownell 2009-02-27 23:32 ` Andrew Morton @ 2009-03-02 13:16 ` Peter Zijlstra 2009-03-02 22:10 ` David Brownell 2009-03-02 15:13 ` [PATCH] genirq: assert that irq handlers are indeed run in hardirq context Peter Zijlstra 2 siblings, 1 reply; 106+ messages in thread From: Peter Zijlstra @ 2009-03-02 13:16 UTC (permalink / raw) To: David Brownell Cc: Andrew Morton, me, linux-kernel, linux-input, felipe.balbi, dmitry.torokhov, sameo, tglx 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. ^ permalink raw reply [flat|nested] 106+ messages in thread
* Re: lockdep and threaded IRQs (was: ...) 2009-03-02 13:16 ` lockdep and threaded IRQs (was: ...) Peter Zijlstra @ 2009-03-02 22:10 ` David Brownell 2009-03-02 22:25 ` Peter Zijlstra 0 siblings, 1 reply; 106+ messages in thread From: David Brownell @ 2009-03-02 22:10 UTC (permalink / raw) To: Peter Zijlstra Cc: Andrew Morton, me, linux-kernel, linux-input, felipe.balbi, dmitry.torokhov, sameo, tglx 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 ^ permalink raw reply [flat|nested] 106+ messages in thread
* Re: lockdep and threaded IRQs (was: ...) 2009-03-02 22:10 ` David Brownell @ 2009-03-02 22:25 ` Peter Zijlstra 2009-03-02 23:20 ` David Brownell 0 siblings, 1 reply; 106+ messages in thread From: Peter Zijlstra @ 2009-03-02 22:25 UTC (permalink / raw) To: David Brownell Cc: Andrew Morton, me, linux-kernel, linux-input, felipe.balbi, dmitry.torokhov, sameo, tglx 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. ^ permalink raw reply [flat|nested] 106+ messages in thread
* Re: lockdep and threaded IRQs (was: ...) 2009-03-02 22:25 ` Peter Zijlstra @ 2009-03-02 23:20 ` David Brownell 2009-03-02 23:26 ` Ingo Molnar 0 siblings, 1 reply; 106+ messages in thread From: David Brownell @ 2009-03-02 23:20 UTC (permalink / raw) To: Peter Zijlstra Cc: Andrew Morton, me, linux-kernel, linux-input, felipe.balbi, dmitry.torokhov, sameo, tglx 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 ^ permalink raw reply [flat|nested] 106+ messages in thread
* Re: lockdep and threaded IRQs (was: ...) 2009-03-02 23:20 ` David Brownell @ 2009-03-02 23:26 ` Ingo Molnar 2009-03-02 23:42 ` David Brownell 2009-03-02 23:48 ` David Brownell 0 siblings, 2 replies; 106+ messages in thread From: Ingo Molnar @ 2009-03-02 23:26 UTC (permalink / raw) To: David Brownell Cc: Peter Zijlstra, Andrew Morton, me, linux-kernel, linux-input, felipe.balbi, dmitry.torokhov, sameo, tglx * 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 ^ permalink raw reply [flat|nested] 106+ messages in thread
* Re: lockdep and threaded IRQs (was: ...) 2009-03-02 23:26 ` Ingo Molnar @ 2009-03-02 23:42 ` David Brownell 2009-03-02 23:53 ` Ingo Molnar 2009-03-03 11:53 ` lockdep and threaded IRQs (was: ...) Thomas Gleixner 2009-03-02 23:48 ` David Brownell 1 sibling, 2 replies; 106+ messages in thread From: David Brownell @ 2009-03-02 23:42 UTC (permalink / raw) To: Ingo Molnar Cc: Peter Zijlstra, Andrew Morton, me, linux-kernel, linux-input, felipe.balbi, dmitry.torokhov, sameo, tglx 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 ^ permalink raw reply [flat|nested] 106+ messages in thread
* Re: lockdep and threaded IRQs (was: ...) 2009-03-02 23:42 ` David Brownell @ 2009-03-02 23:53 ` Ingo Molnar 2009-03-03 0:33 ` David Brownell 2009-03-03 11:53 ` lockdep and threaded IRQs (was: ...) Thomas Gleixner 1 sibling, 1 reply; 106+ messages in thread From: Ingo Molnar @ 2009-03-02 23:53 UTC (permalink / raw) To: David Brownell Cc: Peter Zijlstra, Andrew Morton, me, linux-kernel, linux-input, felipe.balbi, dmitry.torokhov, sameo, tglx * 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 ^ permalink raw reply [flat|nested] 106+ messages in thread
* Re: lockdep and threaded IRQs (was: ...) 2009-03-02 23:53 ` Ingo Molnar @ 2009-03-03 0:33 ` David Brownell 2009-03-03 0:44 ` Ingo Molnar 0 siblings, 1 reply; 106+ messages in thread From: David Brownell @ 2009-03-03 0:33 UTC (permalink / raw) To: Ingo Molnar Cc: Peter Zijlstra, Andrew Morton, me, linux-kernel, linux-input, felipe.balbi, dmitry.torokhov, sameo, tglx 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. ^ permalink raw reply [flat|nested] 106+ messages in thread
* Re: lockdep and threaded IRQs (was: ...) 2009-03-03 0:33 ` David Brownell @ 2009-03-03 0:44 ` Ingo Molnar 2009-03-03 2:37 ` David Brownell ` (2 more replies) 0 siblings, 3 replies; 106+ messages in thread From: Ingo Molnar @ 2009-03-03 0:44 UTC (permalink / raw) To: David Brownell Cc: Peter Zijlstra, Andrew Morton, me, linux-kernel, linux-input, felipe.balbi, dmitry.torokhov, sameo, tglx * 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 ^ permalink raw reply [flat|nested] 106+ messages in thread
* Re: lockdep and threaded IRQs (was: ...) 2009-03-03 0:44 ` Ingo Molnar @ 2009-03-03 2:37 ` David Brownell 2009-03-03 9:27 ` Peter Zijlstra 2009-03-18 2:00 ` David Brownell 2009-03-18 2:14 ` [patch/rfc 0/2] handle_threaded_irq() David Brownell 2 siblings, 1 reply; 106+ messages in thread From: David Brownell @ 2009-03-03 2:37 UTC (permalink / raw) To: Ingo Molnar Cc: Peter Zijlstra, Andrew Morton, me, linux-kernel, linux-input, felipe.balbi, dmitry.torokhov, sameo, tglx 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 ^ permalink raw reply [flat|nested] 106+ messages in thread
* Re: lockdep and threaded IRQs (was: ...) 2009-03-03 2:37 ` David Brownell @ 2009-03-03 9:27 ` Peter Zijlstra 2009-03-03 9:45 ` Ingo Molnar 2009-03-03 9:47 ` Alan Cox 0 siblings, 2 replies; 106+ messages in thread From: Peter Zijlstra @ 2009-03-03 9:27 UTC (permalink / raw) To: David Brownell Cc: Ingo Molnar, Andrew Morton, me, linux-kernel, linux-input, felipe.balbi, dmitry.torokhov, sameo, tglx 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). ^ permalink raw reply [flat|nested] 106+ messages in thread
* Re: lockdep and threaded IRQs (was: ...) 2009-03-03 9:27 ` Peter Zijlstra @ 2009-03-03 9:45 ` Ingo Molnar 2009-03-03 9:47 ` Alan Cox 1 sibling, 0 replies; 106+ messages in thread From: Ingo Molnar @ 2009-03-03 9:45 UTC (permalink / raw) To: Peter Zijlstra Cc: David Brownell, Andrew Morton, me, linux-kernel, linux-input, felipe.balbi, dmitry.torokhov, sameo, tglx * 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 ^ permalink raw reply [flat|nested] 106+ messages in thread
* Re: lockdep and threaded IRQs (was: ...) 2009-03-03 9:27 ` Peter Zijlstra 2009-03-03 9:45 ` Ingo Molnar @ 2009-03-03 9:47 ` Alan Cox 2009-03-03 10:03 ` Ingo Molnar 1 sibling, 1 reply; 106+ messages in thread From: Alan Cox @ 2009-03-03 9:47 UTC (permalink / raw) To: Peter Zijlstra Cc: David Brownell, Ingo Molnar, Andrew Morton, me, linux-kernel, linux-input, felipe.balbi, dmitry.torokhov, sameo, tglx > 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" ^ permalink raw reply [flat|nested] 106+ messages in thread
* Re: lockdep and threaded IRQs (was: ...) 2009-03-03 9:47 ` Alan Cox @ 2009-03-03 10:03 ` Ingo Molnar 2009-03-03 10:30 ` Alan Cox 0 siblings, 1 reply; 106+ messages in thread From: Ingo Molnar @ 2009-03-03 10:03 UTC (permalink / raw) To: Alan Cox Cc: Peter Zijlstra, David Brownell, Andrew Morton, me, linux-kernel, linux-input, felipe.balbi, dmitry.torokhov, sameo, tglx * 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 ^ permalink raw reply [flat|nested] 106+ messages in thread
* Re: lockdep and threaded IRQs (was: ...) 2009-03-03 10:03 ` Ingo Molnar @ 2009-03-03 10:30 ` Alan Cox 2009-03-03 10:39 ` Peter Zijlstra 2009-03-03 10:48 ` Ingo Molnar 0 siblings, 2 replies; 106+ messages in thread From: Alan Cox @ 2009-03-03 10:30 UTC (permalink / raw) To: Ingo Molnar Cc: Peter Zijlstra, David Brownell, Andrew Morton, me, linux-kernel, linux-input, felipe.balbi, dmitry.torokhov, sameo, tglx > _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. ^ permalink raw reply [flat|nested] 106+ messages in thread
* Re: lockdep and threaded IRQs (was: ...) 2009-03-03 10:30 ` Alan Cox @ 2009-03-03 10:39 ` Peter Zijlstra 2009-03-03 10:48 ` Ingo Molnar 1 sibling, 0 replies; 106+ messages in thread From: Peter Zijlstra @ 2009-03-03 10:39 UTC (permalink / raw) To: Alan Cox Cc: Ingo Molnar, David Brownell, Andrew Morton, me, linux-kernel, linux-input, felipe.balbi, dmitry.torokhov, sameo, tglx 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. ^ permalink raw reply [flat|nested] 106+ messages in thread
* Re: lockdep and threaded IRQs (was: ...) 2009-03-03 10:30 ` Alan Cox 2009-03-03 10:39 ` Peter Zijlstra @ 2009-03-03 10:48 ` Ingo Molnar 2009-03-03 11:13 ` Alan Cox ` (2 more replies) 1 sibling, 3 replies; 106+ messages in thread From: Ingo Molnar @ 2009-03-03 10:48 UTC (permalink / raw) To: Alan Cox Cc: Peter Zijlstra, David Brownell, Andrew Morton, me, linux-kernel, linux-input, felipe.balbi, dmitry.torokhov, sameo, tglx * 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 ^ permalink raw reply [flat|nested] 106+ messages in thread
* Re: lockdep and threaded IRQs (was: ...) 2009-03-03 10:48 ` Ingo Molnar @ 2009-03-03 11:13 ` Alan Cox 2009-03-03 11:33 ` Ingo Molnar 2009-03-03 11:19 ` Ingo Molnar 2009-03-18 1:04 ` David Brownell 2 siblings, 1 reply; 106+ messages in thread From: Alan Cox @ 2009-03-03 11:13 UTC (permalink / raw) To: Ingo Molnar Cc: Peter Zijlstra, David Brownell, Andrew Morton, me, linux-kernel, linux-input, felipe.balbi, dmitry.torokhov, sameo, tglx > 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. ^ permalink raw reply [flat|nested] 106+ messages in thread
* Re: lockdep and threaded IRQs (was: ...) 2009-03-03 11:13 ` Alan Cox @ 2009-03-03 11:33 ` Ingo Molnar 0 siblings, 0 replies; 106+ messages in thread From: Ingo Molnar @ 2009-03-03 11:33 UTC (permalink / raw) To: Alan Cox Cc: Peter Zijlstra, David Brownell, Andrew Morton, me, linux-kernel, linux-input, felipe.balbi, dmitry.torokhov, sameo, tglx * 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 ^ permalink raw reply [flat|nested] 106+ messages in thread
* Re: lockdep and threaded IRQs (was: ...) 2009-03-03 10:48 ` Ingo Molnar 2009-03-03 11:13 ` Alan Cox @ 2009-03-03 11:19 ` Ingo Molnar 2009-03-18 1:04 ` David Brownell 2 siblings, 0 replies; 106+ messages in thread From: Ingo Molnar @ 2009-03-03 11:19 UTC (permalink / raw) To: Alan Cox Cc: Peter Zijlstra, David Brownell, Andrew Morton, me, linux-kernel, linux-input, felipe.balbi, dmitry.torokhov, sameo, tglx * 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 ^ permalink raw reply [flat|nested] 106+ messages in thread
* Re: lockdep and threaded IRQs (was: ...) 2009-03-03 10:48 ` Ingo Molnar 2009-03-03 11:13 ` Alan Cox 2009-03-03 11:19 ` Ingo Molnar @ 2009-03-18 1:04 ` David Brownell 2 siblings, 0 replies; 106+ messages in thread From: David Brownell @ 2009-03-18 1:04 UTC (permalink / raw) To: Ingo Molnar Cc: Alan Cox, Peter Zijlstra, Andrew Morton, me, linux-kernel, linux-input, felipe.balbi, dmitry.torokhov, sameo, tglx 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... ^ permalink raw reply [flat|nested] 106+ messages in thread
* Re: lockdep and threaded IRQs (was: ...) 2009-03-03 0:44 ` Ingo Molnar 2009-03-03 2:37 ` David Brownell @ 2009-03-18 2:00 ` David Brownell 2009-03-18 2:14 ` [patch/rfc 0/2] handle_threaded_irq() David Brownell 2 siblings, 0 replies; 106+ messages in thread From: David Brownell @ 2009-03-18 2:00 UTC (permalink / raw) To: Ingo Molnar Cc: Peter Zijlstra, Andrew Morton, me, linux-kernel, linux-input, felipe.balbi, dmitry.torokhov, sameo, tglx 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. ^ permalink raw reply [flat|nested] 106+ messages in thread
* [patch/rfc 0/2] handle_threaded_irq() 2009-03-03 0:44 ` Ingo Molnar 2009-03-03 2:37 ` David Brownell 2009-03-18 2:00 ` David Brownell @ 2009-03-18 2:14 ` David Brownell 2009-03-18 2:19 ` [patch/rfc 1/2] GENIRQ: add handle_threaded_irq() flow handler David Brownell 2009-03-18 2:22 ` [patch/rfc 2/2] twl4030: use new " David Brownell 2 siblings, 2 replies; 106+ messages in thread From: David Brownell @ 2009-03-18 2:14 UTC (permalink / raw) To: Ingo Molnar, linux-kernel, tglx Cc: Peter Zijlstra, me, dmitry.torokhov, sameo 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. ^ permalink raw reply [flat|nested] 106+ messages in thread
* [patch/rfc 1/2] GENIRQ: add handle_threaded_irq() flow handler 2009-03-18 2:14 ` [patch/rfc 0/2] handle_threaded_irq() David Brownell @ 2009-03-18 2:19 ` David Brownell 2009-03-18 12:00 ` Felipe Balbi 2009-03-18 2:22 ` [patch/rfc 2/2] twl4030: use new " David Brownell 1 sibling, 1 reply; 106+ messages in thread From: David Brownell @ 2009-03-18 2:19 UTC (permalink / raw) To: Ingo Molnar, linux-kernel, tglx Cc: Peter Zijlstra, me, dmitry.torokhov, sameo 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 ^ permalink raw reply [flat|nested] 106+ messages in thread
* Re: [patch/rfc 1/2] GENIRQ: add handle_threaded_irq() flow handler 2009-03-18 2:19 ` [patch/rfc 1/2] GENIRQ: add handle_threaded_irq() flow handler David Brownell @ 2009-03-18 12:00 ` Felipe Balbi 2009-03-18 18:31 ` David Brownell 0 siblings, 1 reply; 106+ messages in thread From: Felipe Balbi @ 2009-03-18 12:00 UTC (permalink / raw) To: ext David Brownell Cc: Ingo Molnar, linux-kernel, tglx, Peter Zijlstra, me, dmitry.torokhov, sameo 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 ^ permalink raw reply [flat|nested] 106+ messages in thread
* Re: [patch/rfc 1/2] GENIRQ: add handle_threaded_irq() flow handler 2009-03-18 12:00 ` Felipe Balbi @ 2009-03-18 18:31 ` David Brownell 2009-03-18 18:32 ` Felipe Balbi 0 siblings, 1 reply; 106+ messages in thread From: David Brownell @ 2009-03-18 18:31 UTC (permalink / raw) To: felipe.balbi Cc: Ingo Molnar, linux-kernel, tglx, Peter Zijlstra, me, dmitry.torokhov, sameo 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. ^ permalink raw reply [flat|nested] 106+ messages in thread
* Re: [patch/rfc 1/2] GENIRQ: add handle_threaded_irq() flow handler 2009-03-18 18:31 ` David Brownell @ 2009-03-18 18:32 ` Felipe Balbi 0 siblings, 0 replies; 106+ messages in thread From: Felipe Balbi @ 2009-03-18 18:32 UTC (permalink / raw) To: ext David Brownell Cc: Balbi Felipe (Nokia-D/Helsinki), Ingo Molnar, linux-kernel, tglx, Peter Zijlstra, me, dmitry.torokhov, sameo 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 ^ permalink raw reply [flat|nested] 106+ messages in thread
* [patch/rfc 2/2] twl4030: use new handle_threaded_irq() flow handler 2009-03-18 2:14 ` [patch/rfc 0/2] handle_threaded_irq() David Brownell 2009-03-18 2:19 ` [patch/rfc 1/2] GENIRQ: add handle_threaded_irq() flow handler David Brownell @ 2009-03-18 2:22 ` David Brownell 1 sibling, 0 replies; 106+ messages in thread From: David Brownell @ 2009-03-18 2:22 UTC (permalink / raw) To: Ingo Molnar, linux-kernel, tglx Cc: Peter Zijlstra, me, dmitry.torokhov, sameo 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) { ^ permalink raw reply [flat|nested] 106+ messages in thread
* Re: lockdep and threaded IRQs (was: ...) 2009-03-02 23:42 ` David Brownell 2009-03-02 23:53 ` Ingo Molnar @ 2009-03-03 11:53 ` Thomas Gleixner 2009-03-05 2:49 ` David Brownell 1 sibling, 1 reply; 106+ messages in thread From: Thomas Gleixner @ 2009-03-03 11:53 UTC (permalink / raw) To: David Brownell Cc: Ingo Molnar, Peter Zijlstra, Andrew Morton, me, linux-kernel, linux-input, felipe.balbi, dmitry.torokhov, sameo 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 ^ permalink raw reply [flat|nested] 106+ messages in thread
* Re: lockdep and threaded IRQs (was: ...) 2009-03-03 11:53 ` lockdep and threaded IRQs (was: ...) Thomas Gleixner @ 2009-03-05 2:49 ` David Brownell 2009-03-06 14:40 ` Thomas Gleixner 0 siblings, 1 reply; 106+ messages in thread From: David Brownell @ 2009-03-05 2:49 UTC (permalink / raw) To: Thomas Gleixner Cc: Ingo Molnar, Peter Zijlstra, Andrew Morton, me, linux-kernel, linux-input, felipe.balbi, dmitry.torokhov, sameo 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 ^ permalink raw reply [flat|nested] 106+ messages in thread
* Re: lockdep and threaded IRQs (was: ...) 2009-03-05 2:49 ` David Brownell @ 2009-03-06 14:40 ` Thomas Gleixner 2009-03-18 3:06 ` David Brownell 0 siblings, 1 reply; 106+ messages in thread From: Thomas Gleixner @ 2009-03-06 14:40 UTC (permalink / raw) To: David Brownell Cc: Ingo Molnar, Peter Zijlstra, Andrew Morton, me, linux-kernel, linux-input, felipe.balbi, dmitry.torokhov, sameo [-- 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 ^ permalink raw reply [flat|nested] 106+ messages in thread
* Re: lockdep and threaded IRQs (was: ...) 2009-03-06 14:40 ` Thomas Gleixner @ 2009-03-18 3:06 ` David Brownell 0 siblings, 0 replies; 106+ messages in thread From: David Brownell @ 2009-03-18 3:06 UTC (permalink / raw) To: Thomas Gleixner Cc: Ingo Molnar, Peter Zijlstra, Andrew Morton, me, linux-kernel, linux-input, felipe.balbi, dmitry.torokhov, sameo 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 ^ permalink raw reply [flat|nested] 106+ messages in thread
* Re: lockdep and threaded IRQs (was: ...) 2009-03-02 23:26 ` Ingo Molnar 2009-03-02 23:42 ` David Brownell @ 2009-03-02 23:48 ` David Brownell 2009-03-02 23:58 ` Ingo Molnar 1 sibling, 1 reply; 106+ messages in thread From: David Brownell @ 2009-03-02 23:48 UTC (permalink / raw) To: Ingo Molnar Cc: Peter Zijlstra, Andrew Morton, me, linux-kernel, linux-input, felipe.balbi, dmitry.torokhov, sameo, tglx 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. ^ permalink raw reply [flat|nested] 106+ messages in thread
* Re: lockdep and threaded IRQs (was: ...) 2009-03-02 23:48 ` David Brownell @ 2009-03-02 23:58 ` Ingo Molnar 0 siblings, 0 replies; 106+ messages in thread From: Ingo Molnar @ 2009-03-02 23:58 UTC (permalink / raw) To: David Brownell Cc: Peter Zijlstra, Andrew Morton, me, linux-kernel, linux-input, felipe.balbi, dmitry.torokhov, sameo, tglx * 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 ^ permalink raw reply [flat|nested] 106+ messages in thread
* [PATCH] genirq: assert that irq handlers are indeed run in hardirq context. 2009-02-27 23:18 ` lockdep and threaded IRQs (was: ...) David Brownell 2009-02-27 23:32 ` Andrew Morton 2009-03-02 13:16 ` lockdep and threaded IRQs (was: ...) Peter Zijlstra @ 2009-03-02 15:13 ` Peter Zijlstra 2009-03-02 19:48 ` David Brownell ` (2 more replies) 2 siblings, 3 replies; 106+ messages in thread From: Peter Zijlstra @ 2009-03-02 15:13 UTC (permalink / raw) To: David Brownell Cc: Andrew Morton, me, linux-kernel, linux-input, felipe.balbi, dmitry.torokhov, sameo, tglx, Ingo Molnar 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(); ^ permalink raw reply related [flat|nested] 106+ messages in thread
* Re: [PATCH] genirq: assert that irq handlers are indeed run in hardirq context. 2009-03-02 15:13 ` [PATCH] genirq: assert that irq handlers are indeed run in hardirq context Peter Zijlstra @ 2009-03-02 19:48 ` David Brownell 2009-03-02 22:01 ` [tip:irq/genirq] genirq: assert that irq handlers are indeed running " Peter Zijlstra 2009-03-02 23:15 ` Peter Zijlstra 2 siblings, 0 replies; 106+ messages in thread From: David Brownell @ 2009-03-02 19:48 UTC (permalink / raw) To: Peter Zijlstra Cc: Andrew Morton, me, linux-kernel, linux-input, felipe.balbi, dmitry.torokhov, sameo, tglx, Ingo Molnar 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 ^ permalink raw reply [flat|nested] 106+ messages in thread
* [tip:irq/genirq] genirq: assert that irq handlers are indeed running in hardirq context 2009-03-02 15:13 ` [PATCH] genirq: assert that irq handlers are indeed run in hardirq context Peter Zijlstra 2009-03-02 19:48 ` David Brownell @ 2009-03-02 22:01 ` Peter Zijlstra 2009-03-02 23:15 ` Peter Zijlstra 2 siblings, 0 replies; 106+ messages in thread From: Peter Zijlstra @ 2009-03-02 22:01 UTC (permalink / raw) To: linux-tip-commits Cc: linux-kernel, hpa, mingo, akpm, a.p.zijlstra, tglx, mingo 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(); ^ permalink raw reply related [flat|nested] 106+ messages in thread
* [tip:irq/genirq] genirq: assert that irq handlers are indeed running in hardirq context 2009-03-02 15:13 ` [PATCH] genirq: assert that irq handlers are indeed run in hardirq context Peter Zijlstra 2009-03-02 19:48 ` David Brownell 2009-03-02 22:01 ` [tip:irq/genirq] genirq: assert that irq handlers are indeed running " Peter Zijlstra @ 2009-03-02 23:15 ` Peter Zijlstra 2009-04-10 7:11 ` Eric Miao 2 siblings, 1 reply; 106+ messages in thread From: Peter Zijlstra @ 2009-03-02 23:15 UTC (permalink / raw) To: linux-tip-commits Cc: linux-kernel, hpa, mingo, akpm, a.p.zijlstra, tglx, mingo 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(); ^ permalink raw reply related [flat|nested] 106+ messages in thread
* Re: [tip:irq/genirq] genirq: assert that irq handlers are indeed running in hardirq context 2009-03-02 23:15 ` Peter Zijlstra @ 2009-04-10 7:11 ` Eric Miao 2009-04-10 9:57 ` Thomas Gleixner 0 siblings, 1 reply; 106+ messages in thread From: Eric Miao @ 2009-04-10 7:11 UTC (permalink / raw) To: mingo, hpa, linux-kernel, a.p.zijlstra, akpm, tglx, mingo Cc: linux-tip-commits 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 ^ permalink raw reply [flat|nested] 106+ messages in thread
* Re: [tip:irq/genirq] genirq: assert that irq handlers are indeed running in hardirq context 2009-04-10 7:11 ` Eric Miao @ 2009-04-10 9:57 ` Thomas Gleixner 0 siblings, 0 replies; 106+ messages in thread From: Thomas Gleixner @ 2009-04-10 9:57 UTC (permalink / raw) To: Eric Miao Cc: mingo, hpa, linux-kernel, a.p.zijlstra, akpm, mingo, linux-tip-commits [-- 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 ^ permalink raw reply [flat|nested] 106+ messages in thread
* Re: lockdep and threaded IRQs 2009-02-27 21:50 ` lockdep and threaded IRQs (was: [PATCH 1/2] input: misc: add twl4030-pwrbutton driver) David Brownell 2009-02-27 22:09 ` Andrew Morton @ 2009-02-28 11:20 ` Stefan Richter 2009-02-28 20:10 ` David Brownell 1 sibling, 1 reply; 106+ messages in thread From: Stefan Richter @ 2009-02-28 11:20 UTC (permalink / raw) To: David Brownell Cc: me, Andrew Morton, linux-kernel, linux-input, felipe.balbi, dmitry.torokhov, sameo, Peter Zijlstra, Thomas Gleixner 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/ ^ permalink raw reply [flat|nested] 106+ messages in thread
* Re: lockdep and threaded IRQs 2009-02-28 11:20 ` lockdep and threaded IRQs Stefan Richter @ 2009-02-28 20:10 ` David Brownell 0 siblings, 0 replies; 106+ messages in thread From: David Brownell @ 2009-02-28 20:10 UTC (permalink / raw) To: Stefan Richter Cc: me, Andrew Morton, linux-kernel, linux-input, felipe.balbi, dmitry.torokhov, sameo, Peter Zijlstra, Thomas Gleixner 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 ^ permalink raw reply [flat|nested] 106+ messages in thread
* Re: [PATCH 1/2] input: misc: add twl4030-pwrbutton driver 2009-02-27 19:28 [PATCH 1/2] input: misc: add twl4030-pwrbutton driver Felipe Balbi 2009-02-27 19:28 ` [PATCH 2/2] mfd: twl4030: add twl4030-pwrbutton as our child Felipe Balbi 2009-02-27 20:33 ` [PATCH 1/2] input: misc: add twl4030-pwrbutton driver Andrew Morton @ 2009-02-28 5:51 ` Trilok Soni 2009-02-28 12:05 ` [PATCH] mfd: twl4030: add twl4030-pwrbutton as our child Felipe Balbi 2009-02-28 22:23 ` [PATCH 1/2] input: misc: add twl4030-pwrbutton driver Dmitry Torokhov 3 siblings, 1 reply; 106+ messages in thread From: Trilok Soni @ 2009-02-28 5:51 UTC (permalink / raw) To: Felipe Balbi Cc: linux-kernel, linux-input, Andrew Morton, Felipe Balbi, David Brownell, Dmitry Torokhov, Samuel Ortiz 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 ^ permalink raw reply [flat|nested] 106+ messages in thread
* [PATCH] mfd: twl4030: add twl4030-pwrbutton as our child 2009-02-28 5:51 ` [PATCH 1/2] input: misc: add twl4030-pwrbutton driver Trilok Soni @ 2009-02-28 12:05 ` Felipe Balbi 0 siblings, 0 replies; 106+ messages in thread From: Felipe Balbi @ 2009-02-28 12:05 UTC (permalink / raw) To: linux-kernel, linux-input Cc: soni.trilok, Andrew Morton, Felipe Balbi, Samuel Ortiz, David Brownell 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 ^ permalink raw reply related [flat|nested] 106+ messages in thread
* Re: [PATCH 1/2] input: misc: add twl4030-pwrbutton driver 2009-02-27 19:28 [PATCH 1/2] input: misc: add twl4030-pwrbutton driver Felipe Balbi ` (2 preceding siblings ...) 2009-02-28 5:51 ` [PATCH 1/2] input: misc: add twl4030-pwrbutton driver Trilok Soni @ 2009-02-28 22:23 ` Dmitry Torokhov 2009-03-01 0:30 ` Felipe Balbi 3 siblings, 1 reply; 106+ messages in thread From: Dmitry Torokhov @ 2009-02-28 22:23 UTC (permalink / raw) To: Felipe Balbi Cc: linux-kernel, linux-input, Andrew Morton, Felipe Balbi, David Brownell, Samuel Ortiz 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 ^ permalink raw reply [flat|nested] 106+ messages in thread
* Re: [PATCH 1/2] input: misc: add twl4030-pwrbutton driver 2009-02-28 22:23 ` [PATCH 1/2] input: misc: add twl4030-pwrbutton driver Dmitry Torokhov @ 2009-03-01 0:30 ` Felipe Balbi 2009-03-01 0:58 ` Dmitry Torokhov 0 siblings, 1 reply; 106+ messages in thread From: Felipe Balbi @ 2009-03-01 0:30 UTC (permalink / raw) To: Dmitry Torokhov Cc: Felipe Balbi, linux-kernel, linux-input, Andrew Morton, Felipe Balbi, David Brownell, Samuel Ortiz 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 ^ permalink raw reply related [flat|nested] 106+ messages in thread
* Re: [PATCH 1/2] input: misc: add twl4030-pwrbutton driver 2009-03-01 0:30 ` Felipe Balbi @ 2009-03-01 0:58 ` Dmitry Torokhov 2009-03-01 14:40 ` Felipe Balbi 0 siblings, 1 reply; 106+ messages in thread From: Dmitry Torokhov @ 2009-03-01 0:58 UTC (permalink / raw) To: Felipe Balbi Cc: linux-kernel, linux-input, Andrew Morton, Felipe Balbi, David Brownell, Samuel Ortiz 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 ^ permalink raw reply [flat|nested] 106+ messages in thread
* Re: [PATCH 1/2] input: misc: add twl4030-pwrbutton driver 2009-03-01 0:58 ` Dmitry Torokhov @ 2009-03-01 14:40 ` Felipe Balbi 2009-03-04 9:00 ` Dmitry Torokhov 0 siblings, 1 reply; 106+ messages in thread From: Felipe Balbi @ 2009-03-01 14:40 UTC (permalink / raw) To: Dmitry Torokhov Cc: Felipe Balbi, linux-kernel, linux-input, Andrew Morton, Felipe Balbi, David Brownell, Samuel Ortiz 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 ^ permalink raw reply related [flat|nested] 106+ messages in thread
* Re: [PATCH 1/2] input: misc: add twl4030-pwrbutton driver 2009-03-01 14:40 ` Felipe Balbi @ 2009-03-04 9:00 ` Dmitry Torokhov 2009-03-04 10:12 ` Felipe Balbi 2009-03-05 23:54 ` David Brownell 0 siblings, 2 replies; 106+ messages in thread From: Dmitry Torokhov @ 2009-03-04 9:00 UTC (permalink / raw) To: Felipe Balbi Cc: linux-kernel, linux-input, Andrew Morton, Felipe Balbi, David Brownell, Samuel Ortiz 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 ^ permalink raw reply [flat|nested] 106+ messages in thread
* Re: [PATCH 1/2] input: misc: add twl4030-pwrbutton driver 2009-03-04 9:00 ` Dmitry Torokhov @ 2009-03-04 10:12 ` Felipe Balbi 2009-03-05 1:38 ` David Brownell 2009-03-05 23:54 ` David Brownell 1 sibling, 1 reply; 106+ messages in thread From: Felipe Balbi @ 2009-03-04 10:12 UTC (permalink / raw) To: ext Dmitry Torokhov Cc: Felipe Balbi, linux-kernel, linux-input, Andrew Morton, Balbi Felipe (Nokia-D/Helsinki), David Brownell, Samuel Ortiz 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 ^ permalink raw reply [flat|nested] 106+ messages in thread
* Re: [PATCH 1/2] input: misc: add twl4030-pwrbutton driver 2009-03-04 10:12 ` Felipe Balbi @ 2009-03-05 1:38 ` David Brownell 0 siblings, 0 replies; 106+ messages in thread From: David Brownell @ 2009-03-05 1:38 UTC (permalink / raw) To: felipe.balbi, ext Dmitry Torokhov Cc: Felipe Balbi, linux-kernel, linux-input, Andrew Morton, Samuel Ortiz 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 ^ permalink raw reply [flat|nested] 106+ messages in thread
* Re: [PATCH 1/2] input: misc: add twl4030-pwrbutton driver 2009-03-04 9:00 ` Dmitry Torokhov 2009-03-04 10:12 ` Felipe Balbi @ 2009-03-05 23:54 ` David Brownell 2009-03-06 9:43 ` Dmitry Torokhov 1 sibling, 1 reply; 106+ messages in thread From: David Brownell @ 2009-03-05 23:54 UTC (permalink / raw) To: Dmitry Torokhov Cc: Felipe Balbi, linux-kernel, linux-input, Andrew Morton, Felipe Balbi, Samuel Ortiz 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. ^ permalink raw reply [flat|nested] 106+ messages in thread
* Re: [PATCH 1/2] input: misc: add twl4030-pwrbutton driver 2009-03-05 23:54 ` David Brownell @ 2009-03-06 9:43 ` Dmitry Torokhov 0 siblings, 0 replies; 106+ messages in thread From: Dmitry Torokhov @ 2009-03-06 9:43 UTC (permalink / raw) To: David Brownell Cc: Felipe Balbi, linux-kernel, linux-input, Andrew Morton, Felipe Balbi, Samuel Ortiz 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 ^ permalink raw reply [flat|nested] 106+ messages in thread
end of thread, other threads:[~2009-07-22 22:25 UTC | newest] Thread overview: 106+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2009-02-27 19:28 [PATCH 1/2] input: misc: add twl4030-pwrbutton driver Felipe Balbi 2009-02-27 19:28 ` [PATCH 2/2] mfd: twl4030: add twl4030-pwrbutton as our child Felipe Balbi 2009-02-27 20:36 ` Andrew Morton 2009-02-27 21:58 ` David Brownell 2009-02-27 22:09 ` Felipe Balbi 2009-02-27 22:12 ` Andrew Morton 2009-02-27 23:20 ` David Brownell 2009-02-27 23:42 ` Andrew Morton 2009-07-22 19:27 ` Trilok Soni 2009-07-22 22:25 ` David Brownell 2009-02-27 20:33 ` [PATCH 1/2] input: misc: add twl4030-pwrbutton driver Andrew Morton 2009-02-27 20:37 ` Felipe Balbi 2009-02-27 21:50 ` lockdep and threaded IRQs (was: [PATCH 1/2] input: misc: add twl4030-pwrbutton driver) David Brownell 2009-02-27 22:09 ` Andrew Morton 2009-02-27 23:18 ` lockdep and threaded IRQs (was: ...) David Brownell 2009-02-27 23:32 ` Andrew Morton 2009-02-28 0:01 ` Andrew Morton 2009-02-28 2:30 ` David Brownell 2009-02-28 2:39 ` Andrew Morton 2009-02-28 4:46 ` David Brownell 2009-02-28 5:12 ` Andrew Morton 2009-02-28 6:17 ` David Brownell 2009-02-28 11:13 ` Thomas Gleixner 2009-02-28 12:16 ` David Brownell 2009-02-28 16:42 ` Thomas Gleixner 2009-02-28 20:02 ` David Brownell 2009-02-28 20:55 ` Thomas Gleixner 2009-02-28 21:13 ` Thomas Gleixner 2009-02-28 22:37 ` David Brownell 2009-02-28 22:05 ` David Brownell 2009-03-01 9:43 ` Thomas Gleixner 2009-03-01 22:54 ` David Brownell 2009-03-02 13:16 ` Peter Zijlstra 2009-03-02 21:04 ` David Brownell 2009-03-02 21:16 ` Peter Zijlstra 2009-03-02 21:29 ` Andrew Morton 2009-03-02 21:37 ` David Brownell 2009-03-02 21:41 ` Peter Zijlstra 2009-03-02 22:09 ` David Brownell 2009-03-02 22:19 ` Peter Zijlstra 2009-03-02 22:40 ` David Brownell 2009-03-02 22:51 ` Peter Zijlstra 2009-03-02 23:29 ` David Brownell 2009-03-03 7:45 ` Peter Zijlstra 2009-03-02 22:46 ` lockdep and threaded IRQs David Miller 2009-03-02 22:57 ` Andrew Morton 2009-03-02 23:07 ` Peter Zijlstra 2009-03-02 23:02 ` Peter Zijlstra 2009-03-02 23:35 ` lockdep and threaded IRQs (was: ...) Alan Cox 2009-03-01 22:11 ` David Brownell 2009-02-28 11:48 ` lockdep and threaded IRQs Stefan Richter 2009-02-28 20:19 ` David Brownell 2009-02-28 21:10 ` Stefan Richter 2009-03-02 13:16 ` lockdep and threaded IRQs (was: ...) Peter Zijlstra 2009-03-02 22:10 ` David Brownell 2009-03-02 22:25 ` Peter Zijlstra 2009-03-02 23:20 ` David Brownell 2009-03-02 23:26 ` Ingo Molnar 2009-03-02 23:42 ` David Brownell 2009-03-02 23:53 ` Ingo Molnar 2009-03-03 0:33 ` David Brownell 2009-03-03 0:44 ` Ingo Molnar 2009-03-03 2:37 ` David Brownell 2009-03-03 9:27 ` Peter Zijlstra 2009-03-03 9:45 ` Ingo Molnar 2009-03-03 9:47 ` Alan Cox 2009-03-03 10:03 ` Ingo Molnar 2009-03-03 10:30 ` Alan Cox 2009-03-03 10:39 ` Peter Zijlstra 2009-03-03 10:48 ` Ingo Molnar 2009-03-03 11:13 ` Alan Cox 2009-03-03 11:33 ` Ingo Molnar 2009-03-03 11:19 ` Ingo Molnar 2009-03-18 1:04 ` David Brownell 2009-03-18 2:00 ` David Brownell 2009-03-18 2:14 ` [patch/rfc 0/2] handle_threaded_irq() David Brownell 2009-03-18 2:19 ` [patch/rfc 1/2] GENIRQ: add handle_threaded_irq() flow handler David Brownell 2009-03-18 12:00 ` Felipe Balbi 2009-03-18 18:31 ` David Brownell 2009-03-18 18:32 ` Felipe Balbi 2009-03-18 2:22 ` [patch/rfc 2/2] twl4030: use new " David Brownell 2009-03-03 11:53 ` lockdep and threaded IRQs (was: ...) Thomas Gleixner 2009-03-05 2:49 ` David Brownell 2009-03-06 14:40 ` Thomas Gleixner 2009-03-18 3:06 ` David Brownell 2009-03-02 23:48 ` David Brownell 2009-03-02 23:58 ` Ingo Molnar 2009-03-02 15:13 ` [PATCH] genirq: assert that irq handlers are indeed run in hardirq context Peter Zijlstra 2009-03-02 19:48 ` David Brownell 2009-03-02 22:01 ` [tip:irq/genirq] genirq: assert that irq handlers are indeed running " Peter Zijlstra 2009-03-02 23:15 ` Peter Zijlstra 2009-04-10 7:11 ` Eric Miao 2009-04-10 9:57 ` Thomas Gleixner 2009-02-28 11:20 ` lockdep and threaded IRQs Stefan Richter 2009-02-28 20:10 ` David Brownell 2009-02-28 5:51 ` [PATCH 1/2] input: misc: add twl4030-pwrbutton driver Trilok Soni 2009-02-28 12:05 ` [PATCH] mfd: twl4030: add twl4030-pwrbutton as our child Felipe Balbi 2009-02-28 22:23 ` [PATCH 1/2] input: misc: add twl4030-pwrbutton driver Dmitry Torokhov 2009-03-01 0:30 ` Felipe Balbi 2009-03-01 0:58 ` Dmitry Torokhov 2009-03-01 14:40 ` Felipe Balbi 2009-03-04 9:00 ` Dmitry Torokhov 2009-03-04 10:12 ` Felipe Balbi 2009-03-05 1:38 ` David Brownell 2009-03-05 23:54 ` David Brownell 2009-03-06 9:43 ` Dmitry Torokhov
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).