Re: [PATCH] gpiolib: irqchip: use different lockdep class for each gpio irqchip
On 08/14/2015 03:34 PM, Linus Walleij wrote:
> On Thu, Aug 13, 2015 at 4:58 PM, Grygorii Strashko
> wrote:
>
>> Since IRQ chip helpers were introduced drivers lose ability to
>> register separate lockdep classes for each registered GPIO IRQ
>> chip and the gpiolib now is using shared lockdep class for
>> all GPIO IRQ chips (gpiochip_irq_lock_class).
>> As result, lockdep will produce warning when there are min two
>> stacked GPIO chips and all of them are interrupt controllers.
>>
>> HW configuration which generates lockdep warning (TI dra7-evm):
> (...)
>>
>> Cc: Geert Uytterhoeven
>> Cc: Roger Quadros
>> Reported-by: Roger Quadros
>> Signed-off-by: Grygorii Strashko
>
> Ah, I see...
>
>
>> * implies that if the chip supports IRQs, these IRQs need to be
>> threaded
>> * as the chip access may sleep when e.g. reading out the IRQ status
>>* registers.
>> + * @exported: flags if the gpiochip is exported for use from sysfs. Private.
>>* @irq_not_threaded: flag must be set if @can_sleep is set but the
>>* IRQs don't need to be threaded
>>*
>> @@ -126,6 +128,7 @@ struct gpio_chip {
>> irq_flow_handler_t irq_handler;
>> unsigned intirq_default_type;
>> int irq_parent;
>> + struct lock_class_key *lock_key;
>
> There is something weird with the kerneldoc. It is documenting something
> else but not documenting the new member.
>
Sorry, for that.
There are no kerneldocs for any of GPIO IRQ chip's specific fields,
Do you prefer me to add doc for "lock_key" or all of them?
--
regards,
-grygorii
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] gpiolib: irqchip: use different lockdep class for each gpio irqchip
On Fri, Aug 14, 2015 at 2:40 PM, Lars-Peter Clausen wrote: > On 08/14/2015 02:34 PM, Linus Walleij wrote: > [...] >> Every chip will get their own lock class on the heap. >> >> But I think it is a bit kludgy. >> >> Is it not possible to have the lock key in struct gpio_chip >> be a real member instead of a pointer and get a per-chip >> lock that way? >> >> (...) >> struct lock_class_key lock_key; >> >> instead of: >> >> struct lock_class_key *lock_key; >> >> -> problem solved, without kludgy header defines? > > > Lock keys need to be in persistent memory since they have a unlimited life > time. Once registered it is expected to exist until the system is reset. Aha I see. OK if we fix the documentation comment I guess we can go with this, even if it makes the header file somewhat hard to read. I have a bit of problem with it, because lockdep instrumentation is supposed to make development easier, not harder, now it helps in one end with lock validation and screws up in another end by creating API headers that are hopeless to read :( Linus -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] gpiolib: irqchip: use different lockdep class for each gpio irqchip
On Fri, Aug 14, 2015 at 2:40 PM, Lars-Peter Clausen l...@metafoo.de wrote: On 08/14/2015 02:34 PM, Linus Walleij wrote: [...] Every chip will get their own lock class on the heap. But I think it is a bit kludgy. Is it not possible to have the lock key in struct gpio_chip be a real member instead of a pointer and get a per-chip lock that way? (...) struct lock_class_key lock_key; instead of: struct lock_class_key *lock_key; - problem solved, without kludgy header defines? Lock keys need to be in persistent memory since they have a unlimited life time. Once registered it is expected to exist until the system is reset. Aha I see. OK if we fix the documentation comment I guess we can go with this, even if it makes the header file somewhat hard to read. I have a bit of problem with it, because lockdep instrumentation is supposed to make development easier, not harder, now it helps in one end with lock validation and screws up in another end by creating API headers that are hopeless to read :( Linus -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] gpiolib: irqchip: use different lockdep class for each gpio irqchip
On 08/14/2015 03:34 PM, Linus Walleij wrote:
On Thu, Aug 13, 2015 at 4:58 PM, Grygorii Strashko
grygorii.stras...@ti.com wrote:
Since IRQ chip helpers were introduced drivers lose ability to
register separate lockdep classes for each registered GPIO IRQ
chip and the gpiolib now is using shared lockdep class for
all GPIO IRQ chips (gpiochip_irq_lock_class).
As result, lockdep will produce warning when there are min two
stacked GPIO chips and all of them are interrupt controllers.
HW configuration which generates lockdep warning (TI dra7-evm):
(...)
Cc: Geert Uytterhoeven ge...@linux-m68k.org
Cc: Roger Quadros rog...@ti.com
Reported-by: Roger Quadros rog...@ti.com
Signed-off-by: Grygorii Strashko grygorii.stras...@ti.com
Ah, I see...
* implies that if the chip supports IRQs, these IRQs need to be
threaded
* as the chip access may sleep when e.g. reading out the IRQ status
* registers.
+ * @exported: flags if the gpiochip is exported for use from sysfs. Private.
* @irq_not_threaded: flag must be set if @can_sleep is set but the
* IRQs don't need to be threaded
*
@@ -126,6 +128,7 @@ struct gpio_chip {
irq_flow_handler_t irq_handler;
unsigned intirq_default_type;
int irq_parent;
+ struct lock_class_key *lock_key;
There is something weird with the kerneldoc. It is documenting something
else but not documenting the new member.
Sorry, for that.
There are no kerneldocs for any of GPIO IRQ chip's specific fields,
Do you prefer me to add doc for lock_key or all of them?
--
regards,
-grygorii
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] gpiolib: irqchip: use different lockdep class for each gpio irqchip
On 08/14/2015 03:40 PM, Lars-Peter Clausen wrote: > On 08/14/2015 02:34 PM, Linus Walleij wrote: > [...] >> Every chip will get their own lock class on the heap. >> >> But I think it is a bit kludgy. >> >> Is it not possible to have the lock key in struct gpio_chip >> be a real member instead of a pointer and get a per-chip >> lock that way? >> >> (...) >> struct lock_class_key lock_key; >> >> instead of: >> >> struct lock_class_key *lock_key; >> >> -> problem solved, without kludgy header defines? > > > Lock keys need to be in persistent memory since they have a unlimited life > time. Once registered it is expected to exist until the system is reset. > > We recently fixed the same issue of nested locks in regmap. For reference > the discussion with had a look at different ways to solve this can be found > here[1] and the final patch series that went in here[2]. > > - Lars > > [1] https://lkml.org/lkml/2015/6/25/144 > [2] https://lkml.org/lkml/2015/7/8/43 I've actually followed [2], but without deep digging in Lock keys anatomy :) (as mentioned in commit message) -- regards, -grygorii -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] gpiolib: irqchip: use different lockdep class for each gpio irqchip
On 08/14/2015 02:34 PM, Linus Walleij wrote: [...] > Every chip will get their own lock class on the heap. > > But I think it is a bit kludgy. > > Is it not possible to have the lock key in struct gpio_chip > be a real member instead of a pointer and get a per-chip > lock that way? > > (...) > struct lock_class_key lock_key; > > instead of: > > struct lock_class_key *lock_key; > > -> problem solved, without kludgy header defines? Lock keys need to be in persistent memory since they have a unlimited life time. Once registered it is expected to exist until the system is reset. We recently fixed the same issue of nested locks in regmap. For reference the discussion with had a look at different ways to solve this can be found here[1] and the final patch series that went in here[2]. - Lars [1] https://lkml.org/lkml/2015/6/25/144 [2] https://lkml.org/lkml/2015/7/8/43 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] gpiolib: irqchip: use different lockdep class for each gpio irqchip
On Thu, Aug 13, 2015 at 4:58 PM, Grygorii Strashko
wrote:
> Since IRQ chip helpers were introduced drivers lose ability to
> register separate lockdep classes for each registered GPIO IRQ
> chip and the gpiolib now is using shared lockdep class for
> all GPIO IRQ chips (gpiochip_irq_lock_class).
> As result, lockdep will produce warning when there are min two
> stacked GPIO chips and all of them are interrupt controllers.
>
> HW configuration which generates lockdep warning (TI dra7-evm):
(...)
>
> Cc: Geert Uytterhoeven
> Cc: Roger Quadros
> Reported-by: Roger Quadros
> Signed-off-by: Grygorii Strashko
Ah, I see...
> * implies that if the chip supports IRQs, these IRQs need to be threaded
> * as the chip access may sleep when e.g. reading out the IRQ status
> * registers.
> + * @exported: flags if the gpiochip is exported for use from sysfs. Private.
> * @irq_not_threaded: flag must be set if @can_sleep is set but the
> * IRQs don't need to be threaded
> *
> @@ -126,6 +128,7 @@ struct gpio_chip {
> irq_flow_handler_t irq_handler;
> unsigned intirq_default_type;
> int irq_parent;
> + struct lock_class_key *lock_key;
There is something weird with the kerneldoc. It is documenting something
else but not documenting the new member.
Anyway, so here:
> +int _gpiochip_irqchip_add(struct gpio_chip *gpiochip,
> + struct irq_chip *irqchip,
> + unsigned int first_irq,
> + irq_flow_handler_t handler,
> + unsigned int type,
> + struct lock_class_key *lock_key);
> +
> +#ifdef CONFIG_LOCKDEP
> +#define gpiochip_irqchip_add(...) \
> +( \
> + ({ \
> + static struct lock_class_key _key; \
> + _gpiochip_irqchip_add(__VA_ARGS__, &_key); \
> + }) \
> +)
> +#else
> +#define gpiochip_irqchip_add(...) \
> + _gpiochip_irqchip_add(__VA_ARGS__, NULL)
> +#endif
Every chip will get their own lock class on the heap.
But I think it is a bit kludgy.
Is it not possible to have the lock key in struct gpio_chip
be a real member instead of a pointer and get a per-chip
lock that way?
(...)
struct lock_class_key lock_key;
instead of:
struct lock_class_key *lock_key;
-> problem solved, without kludgy header defines?
Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] gpiolib: irqchip: use different lockdep class for each gpio irqchip
On 13/08/15 17:58, Grygorii Strashko wrote:
> Since IRQ chip helpers were introduced drivers lose ability to
> register separate lockdep classes for each registered GPIO IRQ
> chip and the gpiolib now is using shared lockdep class for
> all GPIO IRQ chips (gpiochip_irq_lock_class).
> As result, lockdep will produce warning when there are min two
> stacked GPIO chips and all of them are interrupt controllers.
>
> HW configuration which generates lockdep warning (TI dra7-evm):
>
> [SOC GPIO bankA.gpioX]
> <- irq - [pcf875x.gpioY]
> <- irq - DevZ.enable_irq_wake(pcf_gpioY_irq);
> The issue was reported in [1] and discussed [2].
>
> =
> [ INFO: possible recursive locking detected ]
> 4.2.0-rc6-00013-g5d050ed-dirty #55 Not tainted
> -
> sh/63 is trying to acquire lock:
> (class){..}, at: [] __irq_get_desc_lock+0x50/0x94
>
> but task is already holding lock:
> (class){..}, at: [] __irq_get_desc_lock+0x50/0x94
>
> other info that might help us debug this:
> Possible unsafe locking scenario:
>
>CPU0
>
> lock(class);
> lock(class);
>
> *** DEADLOCK ***
>
> May be due to missing lock nesting notation
>
> 7 locks held by sh/63:
> #0: (sb_writers#4){.+.+.+}, at: [] vfs_write+0x13c/0x164
> #1: (>mutex){+.+.+.}, at: [] kernfs_fop_write+0x4c/0x1a0
> #2: (s_active#36){.+.+.+}, at: [] kernfs_fop_write+0x54/0x1a0
> #3: (pm_mutex){+.+.+.}, at: [] pm_suspend+0xec/0x4c4
> #4: (>mutex){..}, at: [] __device_suspend+0xd4/0x398
> #5: (>lock){+.+.+.}, at: [] __irq_get_desc_lock+0x74/0x94
> #6: (class){..}, at: [] __irq_get_desc_lock+0x50/0x94
>
> stack backtrace:
> CPU: 0 PID: 63 Comm: sh Not tainted 4.2.0-rc6-00013-g5d050ed-dirty #55
> Hardware name: Generic DRA74X (Flattened Device Tree)
> [] (unwind_backtrace) from [] (show_stack+0x10/0x14)
> [] (show_stack) from [] (dump_stack+0x84/0x9c)
> [] (dump_stack) from [] (__lock_acquire+0x19c0/0x1e20)
> [] (__lock_acquire) from [] (lock_acquire+0xa8/0x128)
> [] (lock_acquire) from []
> (_raw_spin_lock_irqsave+0x38/0x4c)
> [] (_raw_spin_lock_irqsave) from []
> (__irq_get_desc_lock+0x50/0x94)
> [] (__irq_get_desc_lock) from []
> (irq_set_irq_wake+0x20/0xfc)
> [] (irq_set_irq_wake) from []
> (pcf857x_irq_set_wake+0x24/0x54)
> [] (pcf857x_irq_set_wake) from []
> (irq_set_irq_wake+0x8c/0xfc)
> [] (irq_set_irq_wake) from []
> (gpio_keys_suspend+0x70/0xd4)
> [] (gpio_keys_suspend) from []
> (dpm_run_callback+0x50/0x124)
> [] (dpm_run_callback) from []
> (__device_suspend+0x10c/0x398)
> [] (__device_suspend) from [] (dpm_suspend+0x134/0x2f4)
> [] (dpm_suspend) from []
> (suspend_devices_and_enter+0xa8/0x728)
> [] (suspend_devices_and_enter) from []
> (pm_suspend+0x32c/0x4c4)
> [] (pm_suspend) from [] (state_store+0x64/0xb8)
> [] (state_store) from [] (kernfs_fop_write+0xbc/0x1a0)
> [] (kernfs_fop_write) from [] (__vfs_write+0x20/0xd8)
> [] (__vfs_write) from [] (vfs_write+0x90/0x164)
> [] (vfs_write) from [] (SyS_write+0x44/0x9c)
> [] (SyS_write) from [] (ret_fast_syscall+0x0/0x54)
>
> Lets fix it by using separate lockdep class for each registered GPIO
> IRQ Chip. This is done by wrapping gpiochip_irqchip_add call into macros.
>
> The implementation of this patch inspired by solution done by Nicolas
> Boichat for regmap [3]
>
> [1] http://www.spinics.net/lists/linux-gpio/msg05844.html
> [2] http://www.spinics.net/lists/linux-gpio/msg06021.html
> [3] http://www.spinics.net/lists/arm-kernel/msg429834.html
>
> Cc: Geert Uytterhoeven
> Cc: Roger Quadros
> Reported-by: Roger Quadros
> Signed-off-by: Grygorii Strashko
Nice !! :)
Tested-by: Roger Quadros
cheers,
-roger
> ---
> drivers/gpio/gpiolib.c | 27 ++-
> include/linux/gpio/driver.h | 27 ++-
> 2 files changed, 36 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index bf4bd1d..75dddc1 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -456,12 +456,6 @@ void gpiochip_set_chained_irqchip(struct gpio_chip
> *gpiochip,
> }
> EXPORT_SYMBOL_GPL(gpiochip_set_chained_irqchip);
>
> -/*
> - * This lock class tells lockdep that GPIO irqs are in a different
> - * category than their parents, so it won't report false recursion.
> - */
> -static struct lock_class_key gpiochip_irq_lock_class;
> -
> /**
> * gpiochip_irq_map() - maps an IRQ into a GPIO irqchip
> * @d: the irqdomain used by this irqchip
> @@ -478,7 +472,11 @@ static int gpiochip_irq_map(struct irq_domain *d,
> unsigned int irq,
> struct gpio_chip *chip = d->host_data;
>
> irq_set_chip_data(irq, chip);
> - irq_set_lockdep_class(irq, _irq_lock_class);
> + /*
> + * This lock class tells lockdep that GPIO irqs are in a different
> + * category than their parents, so it won't report false recursion.
> + */
> +
Re: [PATCH] gpiolib: irqchip: use different lockdep class for each gpio irqchip
On 13/08/15 17:58, Grygorii Strashko wrote:
Since IRQ chip helpers were introduced drivers lose ability to
register separate lockdep classes for each registered GPIO IRQ
chip and the gpiolib now is using shared lockdep class for
all GPIO IRQ chips (gpiochip_irq_lock_class).
As result, lockdep will produce warning when there are min two
stacked GPIO chips and all of them are interrupt controllers.
HW configuration which generates lockdep warning (TI dra7-evm):
[SOC GPIO bankA.gpioX]
- irq - [pcf875x.gpioY]
- irq - DevZ.enable_irq_wake(pcf_gpioY_irq);
The issue was reported in [1] and discussed [2].
=
[ INFO: possible recursive locking detected ]
4.2.0-rc6-00013-g5d050ed-dirty #55 Not tainted
-
sh/63 is trying to acquire lock:
(class){..}, at: [c009b91c] __irq_get_desc_lock+0x50/0x94
but task is already holding lock:
(class){..}, at: [c009b91c] __irq_get_desc_lock+0x50/0x94
other info that might help us debug this:
Possible unsafe locking scenario:
CPU0
lock(class);
lock(class);
*** DEADLOCK ***
May be due to missing lock nesting notation
7 locks held by sh/63:
#0: (sb_writers#4){.+.+.+}, at: [c016bbb8] vfs_write+0x13c/0x164
#1: (of-mutex){+.+.+.}, at: [c01debf4] kernfs_fop_write+0x4c/0x1a0
#2: (s_active#36){.+.+.+}, at: [c01debfc] kernfs_fop_write+0x54/0x1a0
#3: (pm_mutex){+.+.+.}, at: [c009758c] pm_suspend+0xec/0x4c4
#4: (dev-mutex){..}, at: [c03f77f8] __device_suspend+0xd4/0x398
#5: (gpio-lock){+.+.+.}, at: [c009b940] __irq_get_desc_lock+0x74/0x94
#6: (class){..}, at: [c009b91c] __irq_get_desc_lock+0x50/0x94
stack backtrace:
CPU: 0 PID: 63 Comm: sh Not tainted 4.2.0-rc6-00013-g5d050ed-dirty #55
Hardware name: Generic DRA74X (Flattened Device Tree)
[c0016e24] (unwind_backtrace) from [c0013338] (show_stack+0x10/0x14)
[c0013338] (show_stack) from [c05f6b24] (dump_stack+0x84/0x9c)
[c05f6b24] (dump_stack) from [c00903f4] (__lock_acquire+0x19c0/0x1e20)
[c00903f4] (__lock_acquire) from [c0091098] (lock_acquire+0xa8/0x128)
[c0091098] (lock_acquire) from [c05fd61c]
(_raw_spin_lock_irqsave+0x38/0x4c)
[c05fd61c] (_raw_spin_lock_irqsave) from [c009b91c]
(__irq_get_desc_lock+0x50/0x94)
[c009b91c] (__irq_get_desc_lock) from [c009c4f4]
(irq_set_irq_wake+0x20/0xfc)
[c009c4f4] (irq_set_irq_wake) from [c0393ac4]
(pcf857x_irq_set_wake+0x24/0x54)
[c0393ac4] (pcf857x_irq_set_wake) from [c009c560]
(irq_set_irq_wake+0x8c/0xfc)
[c009c560] (irq_set_irq_wake) from [c04a02ac]
(gpio_keys_suspend+0x70/0xd4)
[c04a02ac] (gpio_keys_suspend) from [c03f6a00]
(dpm_run_callback+0x50/0x124)
[c03f6a00] (dpm_run_callback) from [c03f7830]
(__device_suspend+0x10c/0x398)
[c03f7830] (__device_suspend) from [c03f90f0] (dpm_suspend+0x134/0x2f4)
[c03f90f0] (dpm_suspend) from [c0096e20]
(suspend_devices_and_enter+0xa8/0x728)
[c0096e20] (suspend_devices_and_enter) from [c00977cc]
(pm_suspend+0x32c/0x4c4)
[c00977cc] (pm_suspend) from [c0096060] (state_store+0x64/0xb8)
[c0096060] (state_store) from [c01dec64] (kernfs_fop_write+0xbc/0x1a0)
[c01dec64] (kernfs_fop_write) from [c016b280] (__vfs_write+0x20/0xd8)
[c016b280] (__vfs_write) from [c016bb0c] (vfs_write+0x90/0x164)
[c016bb0c] (vfs_write) from [c016c330] (SyS_write+0x44/0x9c)
[c016c330] (SyS_write) from [c000f500] (ret_fast_syscall+0x0/0x54)
Lets fix it by using separate lockdep class for each registered GPIO
IRQ Chip. This is done by wrapping gpiochip_irqchip_add call into macros.
The implementation of this patch inspired by solution done by Nicolas
Boichat for regmap [3]
[1] http://www.spinics.net/lists/linux-gpio/msg05844.html
[2] http://www.spinics.net/lists/linux-gpio/msg06021.html
[3] http://www.spinics.net/lists/arm-kernel/msg429834.html
Cc: Geert Uytterhoeven ge...@linux-m68k.org
Cc: Roger Quadros rog...@ti.com
Reported-by: Roger Quadros rog...@ti.com
Signed-off-by: Grygorii Strashko grygorii.stras...@ti.com
Nice !! :)
Tested-by: Roger Quadros rog...@ti.com
cheers,
-roger
---
drivers/gpio/gpiolib.c | 27 ++-
include/linux/gpio/driver.h | 27 ++-
2 files changed, 36 insertions(+), 18 deletions(-)
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index bf4bd1d..75dddc1 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -456,12 +456,6 @@ void gpiochip_set_chained_irqchip(struct gpio_chip
*gpiochip,
}
EXPORT_SYMBOL_GPL(gpiochip_set_chained_irqchip);
-/*
- * This lock class tells lockdep that GPIO irqs are in a different
- * category than their parents, so it won't report false recursion.
- */
-static struct lock_class_key gpiochip_irq_lock_class;
-
/**
* gpiochip_irq_map() - maps an IRQ into a GPIO irqchip
* @d: the irqdomain used by this irqchip
@@ -478,7 +472,11 @@ static int gpiochip_irq_map(struct
Re: [PATCH] gpiolib: irqchip: use different lockdep class for each gpio irqchip
On Thu, Aug 13, 2015 at 4:58 PM, Grygorii Strashko
grygorii.stras...@ti.com wrote:
Since IRQ chip helpers were introduced drivers lose ability to
register separate lockdep classes for each registered GPIO IRQ
chip and the gpiolib now is using shared lockdep class for
all GPIO IRQ chips (gpiochip_irq_lock_class).
As result, lockdep will produce warning when there are min two
stacked GPIO chips and all of them are interrupt controllers.
HW configuration which generates lockdep warning (TI dra7-evm):
(...)
Cc: Geert Uytterhoeven ge...@linux-m68k.org
Cc: Roger Quadros rog...@ti.com
Reported-by: Roger Quadros rog...@ti.com
Signed-off-by: Grygorii Strashko grygorii.stras...@ti.com
Ah, I see...
* implies that if the chip supports IRQs, these IRQs need to be threaded
* as the chip access may sleep when e.g. reading out the IRQ status
* registers.
+ * @exported: flags if the gpiochip is exported for use from sysfs. Private.
* @irq_not_threaded: flag must be set if @can_sleep is set but the
* IRQs don't need to be threaded
*
@@ -126,6 +128,7 @@ struct gpio_chip {
irq_flow_handler_t irq_handler;
unsigned intirq_default_type;
int irq_parent;
+ struct lock_class_key *lock_key;
There is something weird with the kerneldoc. It is documenting something
else but not documenting the new member.
Anyway, so here:
+int _gpiochip_irqchip_add(struct gpio_chip *gpiochip,
+ struct irq_chip *irqchip,
+ unsigned int first_irq,
+ irq_flow_handler_t handler,
+ unsigned int type,
+ struct lock_class_key *lock_key);
+
+#ifdef CONFIG_LOCKDEP
+#define gpiochip_irqchip_add(...) \
+( \
+ ({ \
+ static struct lock_class_key _key; \
+ _gpiochip_irqchip_add(__VA_ARGS__, _key); \
+ }) \
+)
+#else
+#define gpiochip_irqchip_add(...) \
+ _gpiochip_irqchip_add(__VA_ARGS__, NULL)
+#endif
Every chip will get their own lock class on the heap.
But I think it is a bit kludgy.
Is it not possible to have the lock key in struct gpio_chip
be a real member instead of a pointer and get a per-chip
lock that way?
(...)
struct lock_class_key lock_key;
instead of:
struct lock_class_key *lock_key;
- problem solved, without kludgy header defines?
Yours,
Linus Walleij
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] gpiolib: irqchip: use different lockdep class for each gpio irqchip
On 08/14/2015 02:34 PM, Linus Walleij wrote: [...] Every chip will get their own lock class on the heap. But I think it is a bit kludgy. Is it not possible to have the lock key in struct gpio_chip be a real member instead of a pointer and get a per-chip lock that way? (...) struct lock_class_key lock_key; instead of: struct lock_class_key *lock_key; - problem solved, without kludgy header defines? Lock keys need to be in persistent memory since they have a unlimited life time. Once registered it is expected to exist until the system is reset. We recently fixed the same issue of nested locks in regmap. For reference the discussion with had a look at different ways to solve this can be found here[1] and the final patch series that went in here[2]. - Lars [1] https://lkml.org/lkml/2015/6/25/144 [2] https://lkml.org/lkml/2015/7/8/43 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] gpiolib: irqchip: use different lockdep class for each gpio irqchip
On 08/14/2015 03:40 PM, Lars-Peter Clausen wrote: On 08/14/2015 02:34 PM, Linus Walleij wrote: [...] Every chip will get their own lock class on the heap. But I think it is a bit kludgy. Is it not possible to have the lock key in struct gpio_chip be a real member instead of a pointer and get a per-chip lock that way? (...) struct lock_class_key lock_key; instead of: struct lock_class_key *lock_key; - problem solved, without kludgy header defines? Lock keys need to be in persistent memory since they have a unlimited life time. Once registered it is expected to exist until the system is reset. We recently fixed the same issue of nested locks in regmap. For reference the discussion with had a look at different ways to solve this can be found here[1] and the final patch series that went in here[2]. - Lars [1] https://lkml.org/lkml/2015/6/25/144 [2] https://lkml.org/lkml/2015/7/8/43 I've actually followed [2], but without deep digging in Lock keys anatomy :) (as mentioned in commit message) -- regards, -grygorii -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH] gpiolib: irqchip: use different lockdep class for each gpio irqchip
Since IRQ chip helpers were introduced drivers lose ability to
register separate lockdep classes for each registered GPIO IRQ
chip and the gpiolib now is using shared lockdep class for
all GPIO IRQ chips (gpiochip_irq_lock_class).
As result, lockdep will produce warning when there are min two
stacked GPIO chips and all of them are interrupt controllers.
HW configuration which generates lockdep warning (TI dra7-evm):
[SOC GPIO bankA.gpioX]
<- irq - [pcf875x.gpioY]
<- irq - DevZ.enable_irq_wake(pcf_gpioY_irq);
The issue was reported in [1] and discussed [2].
=
[ INFO: possible recursive locking detected ]
4.2.0-rc6-00013-g5d050ed-dirty #55 Not tainted
-
sh/63 is trying to acquire lock:
(class){..}, at: [] __irq_get_desc_lock+0x50/0x94
but task is already holding lock:
(class){..}, at: [] __irq_get_desc_lock+0x50/0x94
other info that might help us debug this:
Possible unsafe locking scenario:
CPU0
lock(class);
lock(class);
*** DEADLOCK ***
May be due to missing lock nesting notation
7 locks held by sh/63:
#0: (sb_writers#4){.+.+.+}, at: [] vfs_write+0x13c/0x164
#1: (>mutex){+.+.+.}, at: [] kernfs_fop_write+0x4c/0x1a0
#2: (s_active#36){.+.+.+}, at: [] kernfs_fop_write+0x54/0x1a0
#3: (pm_mutex){+.+.+.}, at: [] pm_suspend+0xec/0x4c4
#4: (>mutex){..}, at: [] __device_suspend+0xd4/0x398
#5: (>lock){+.+.+.}, at: [] __irq_get_desc_lock+0x74/0x94
#6: (class){..}, at: [] __irq_get_desc_lock+0x50/0x94
stack backtrace:
CPU: 0 PID: 63 Comm: sh Not tainted 4.2.0-rc6-00013-g5d050ed-dirty #55
Hardware name: Generic DRA74X (Flattened Device Tree)
[] (unwind_backtrace) from [] (show_stack+0x10/0x14)
[] (show_stack) from [] (dump_stack+0x84/0x9c)
[] (dump_stack) from [] (__lock_acquire+0x19c0/0x1e20)
[] (__lock_acquire) from [] (lock_acquire+0xa8/0x128)
[] (lock_acquire) from [] (_raw_spin_lock_irqsave+0x38/0x4c)
[] (_raw_spin_lock_irqsave) from []
(__irq_get_desc_lock+0x50/0x94)
[] (__irq_get_desc_lock) from []
(irq_set_irq_wake+0x20/0xfc)
[] (irq_set_irq_wake) from []
(pcf857x_irq_set_wake+0x24/0x54)
[] (pcf857x_irq_set_wake) from []
(irq_set_irq_wake+0x8c/0xfc)
[] (irq_set_irq_wake) from [] (gpio_keys_suspend+0x70/0xd4)
[] (gpio_keys_suspend) from [] (dpm_run_callback+0x50/0x124)
[] (dpm_run_callback) from [] (__device_suspend+0x10c/0x398)
[] (__device_suspend) from [] (dpm_suspend+0x134/0x2f4)
[] (dpm_suspend) from []
(suspend_devices_and_enter+0xa8/0x728)
[] (suspend_devices_and_enter) from []
(pm_suspend+0x32c/0x4c4)
[] (pm_suspend) from [] (state_store+0x64/0xb8)
[] (state_store) from [] (kernfs_fop_write+0xbc/0x1a0)
[] (kernfs_fop_write) from [] (__vfs_write+0x20/0xd8)
[] (__vfs_write) from [] (vfs_write+0x90/0x164)
[] (vfs_write) from [] (SyS_write+0x44/0x9c)
[] (SyS_write) from [] (ret_fast_syscall+0x0/0x54)
Lets fix it by using separate lockdep class for each registered GPIO
IRQ Chip. This is done by wrapping gpiochip_irqchip_add call into macros.
The implementation of this patch inspired by solution done by Nicolas
Boichat for regmap [3]
[1] http://www.spinics.net/lists/linux-gpio/msg05844.html
[2] http://www.spinics.net/lists/linux-gpio/msg06021.html
[3] http://www.spinics.net/lists/arm-kernel/msg429834.html
Cc: Geert Uytterhoeven
Cc: Roger Quadros
Reported-by: Roger Quadros
Signed-off-by: Grygorii Strashko
---
drivers/gpio/gpiolib.c | 27 ++-
include/linux/gpio/driver.h | 27 ++-
2 files changed, 36 insertions(+), 18 deletions(-)
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index bf4bd1d..75dddc1 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -456,12 +456,6 @@ void gpiochip_set_chained_irqchip(struct gpio_chip
*gpiochip,
}
EXPORT_SYMBOL_GPL(gpiochip_set_chained_irqchip);
-/*
- * This lock class tells lockdep that GPIO irqs are in a different
- * category than their parents, so it won't report false recursion.
- */
-static struct lock_class_key gpiochip_irq_lock_class;
-
/**
* gpiochip_irq_map() - maps an IRQ into a GPIO irqchip
* @d: the irqdomain used by this irqchip
@@ -478,7 +472,11 @@ static int gpiochip_irq_map(struct irq_domain *d, unsigned
int irq,
struct gpio_chip *chip = d->host_data;
irq_set_chip_data(irq, chip);
- irq_set_lockdep_class(irq, _irq_lock_class);
+ /*
+* This lock class tells lockdep that GPIO irqs are in a different
+* category than their parents, so it won't report false recursion.
+*/
+ irq_set_lockdep_class(irq, chip->lock_key);
irq_set_chip_and_handler(irq, chip->irqchip, chip->irq_handler);
/* Chips that can sleep need nested thread handlers */
if (chip->can_sleep && !chip->irq_not_threaded)
@@ -584,6 +582,7 @@ static void gpiochip_irqchip_remove(struct gpio_chip
*gpiochip)
* @handler: the irq handler to use (often a
[PATCH] gpiolib: irqchip: use different lockdep class for each gpio irqchip
Since IRQ chip helpers were introduced drivers lose ability to
register separate lockdep classes for each registered GPIO IRQ
chip and the gpiolib now is using shared lockdep class for
all GPIO IRQ chips (gpiochip_irq_lock_class).
As result, lockdep will produce warning when there are min two
stacked GPIO chips and all of them are interrupt controllers.
HW configuration which generates lockdep warning (TI dra7-evm):
[SOC GPIO bankA.gpioX]
- irq - [pcf875x.gpioY]
- irq - DevZ.enable_irq_wake(pcf_gpioY_irq);
The issue was reported in [1] and discussed [2].
=
[ INFO: possible recursive locking detected ]
4.2.0-rc6-00013-g5d050ed-dirty #55 Not tainted
-
sh/63 is trying to acquire lock:
(class){..}, at: [c009b91c] __irq_get_desc_lock+0x50/0x94
but task is already holding lock:
(class){..}, at: [c009b91c] __irq_get_desc_lock+0x50/0x94
other info that might help us debug this:
Possible unsafe locking scenario:
CPU0
lock(class);
lock(class);
*** DEADLOCK ***
May be due to missing lock nesting notation
7 locks held by sh/63:
#0: (sb_writers#4){.+.+.+}, at: [c016bbb8] vfs_write+0x13c/0x164
#1: (of-mutex){+.+.+.}, at: [c01debf4] kernfs_fop_write+0x4c/0x1a0
#2: (s_active#36){.+.+.+}, at: [c01debfc] kernfs_fop_write+0x54/0x1a0
#3: (pm_mutex){+.+.+.}, at: [c009758c] pm_suspend+0xec/0x4c4
#4: (dev-mutex){..}, at: [c03f77f8] __device_suspend+0xd4/0x398
#5: (gpio-lock){+.+.+.}, at: [c009b940] __irq_get_desc_lock+0x74/0x94
#6: (class){..}, at: [c009b91c] __irq_get_desc_lock+0x50/0x94
stack backtrace:
CPU: 0 PID: 63 Comm: sh Not tainted 4.2.0-rc6-00013-g5d050ed-dirty #55
Hardware name: Generic DRA74X (Flattened Device Tree)
[c0016e24] (unwind_backtrace) from [c0013338] (show_stack+0x10/0x14)
[c0013338] (show_stack) from [c05f6b24] (dump_stack+0x84/0x9c)
[c05f6b24] (dump_stack) from [c00903f4] (__lock_acquire+0x19c0/0x1e20)
[c00903f4] (__lock_acquire) from [c0091098] (lock_acquire+0xa8/0x128)
[c0091098] (lock_acquire) from [c05fd61c] (_raw_spin_lock_irqsave+0x38/0x4c)
[c05fd61c] (_raw_spin_lock_irqsave) from [c009b91c]
(__irq_get_desc_lock+0x50/0x94)
[c009b91c] (__irq_get_desc_lock) from [c009c4f4]
(irq_set_irq_wake+0x20/0xfc)
[c009c4f4] (irq_set_irq_wake) from [c0393ac4]
(pcf857x_irq_set_wake+0x24/0x54)
[c0393ac4] (pcf857x_irq_set_wake) from [c009c560]
(irq_set_irq_wake+0x8c/0xfc)
[c009c560] (irq_set_irq_wake) from [c04a02ac] (gpio_keys_suspend+0x70/0xd4)
[c04a02ac] (gpio_keys_suspend) from [c03f6a00] (dpm_run_callback+0x50/0x124)
[c03f6a00] (dpm_run_callback) from [c03f7830] (__device_suspend+0x10c/0x398)
[c03f7830] (__device_suspend) from [c03f90f0] (dpm_suspend+0x134/0x2f4)
[c03f90f0] (dpm_suspend) from [c0096e20]
(suspend_devices_and_enter+0xa8/0x728)
[c0096e20] (suspend_devices_and_enter) from [c00977cc]
(pm_suspend+0x32c/0x4c4)
[c00977cc] (pm_suspend) from [c0096060] (state_store+0x64/0xb8)
[c0096060] (state_store) from [c01dec64] (kernfs_fop_write+0xbc/0x1a0)
[c01dec64] (kernfs_fop_write) from [c016b280] (__vfs_write+0x20/0xd8)
[c016b280] (__vfs_write) from [c016bb0c] (vfs_write+0x90/0x164)
[c016bb0c] (vfs_write) from [c016c330] (SyS_write+0x44/0x9c)
[c016c330] (SyS_write) from [c000f500] (ret_fast_syscall+0x0/0x54)
Lets fix it by using separate lockdep class for each registered GPIO
IRQ Chip. This is done by wrapping gpiochip_irqchip_add call into macros.
The implementation of this patch inspired by solution done by Nicolas
Boichat for regmap [3]
[1] http://www.spinics.net/lists/linux-gpio/msg05844.html
[2] http://www.spinics.net/lists/linux-gpio/msg06021.html
[3] http://www.spinics.net/lists/arm-kernel/msg429834.html
Cc: Geert Uytterhoeven ge...@linux-m68k.org
Cc: Roger Quadros rog...@ti.com
Reported-by: Roger Quadros rog...@ti.com
Signed-off-by: Grygorii Strashko grygorii.stras...@ti.com
---
drivers/gpio/gpiolib.c | 27 ++-
include/linux/gpio/driver.h | 27 ++-
2 files changed, 36 insertions(+), 18 deletions(-)
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index bf4bd1d..75dddc1 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -456,12 +456,6 @@ void gpiochip_set_chained_irqchip(struct gpio_chip
*gpiochip,
}
EXPORT_SYMBOL_GPL(gpiochip_set_chained_irqchip);
-/*
- * This lock class tells lockdep that GPIO irqs are in a different
- * category than their parents, so it won't report false recursion.
- */
-static struct lock_class_key gpiochip_irq_lock_class;
-
/**
* gpiochip_irq_map() - maps an IRQ into a GPIO irqchip
* @d: the irqdomain used by this irqchip
@@ -478,7 +472,11 @@ static int gpiochip_irq_map(struct irq_domain *d, unsigned
int irq,
struct gpio_chip *chip = d-host_data;
irq_set_chip_data(irq, chip);
- irq_set_lockdep_class(irq, gpiochip_irq_lock_class);
+ /*
+* This lock class tells lockdep that
