Re: [PATCH] gpiolib: irqchip: use different lockdep class for each gpio irqchip

From: Roger Quadros
Date: Fri Aug 14 2015 - 05:01:13 EST

Next message: Wanpeng Li: "Re: [PATCH] mm/hwpoison: fix race between soft_offline_page and unpoison_memory"
Previous message: Michal Kubecek: "Re: GCOV_PROFILE_ALL breaks BUILD_BUG_ON(!is_power_of_2(8))"
In reply to: Grygorii Strashko: "[PATCH] gpiolib: irqchip: use different lockdep class for each gpio irqchip"
Next in thread: Linus Walleij: "Re: [PATCH] gpiolib: irqchip: use different lockdep class for each gpio irqchip"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

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 <geert@xxxxxxxxxxxxxx>
> Cc: Roger Quadros <rogerq@xxxxxx>
> Reported-by: Roger Quadros <rogerq@xxxxxx>
> Signed-off-by: Grygorii Strashko <grygorii.strashko@xxxxxx>

Nice !! :)

Tested-by: Roger Quadros <rogerq@xxxxxx>

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, &gpiochip_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 predefined irq core function)
> * @type: the default type for IRQs on this irqchip, pass IRQ_TYPE_NONE
> * to have the core avoid setting up any default type in the hardware.
> + * @lock_key: lockdep class
> *
> * This function closely associates a certain irqchip with a certain
> * gpiochip, providing an irq domain to translate the local IRQs to
> @@ -599,11 +598,12 @@ static void gpiochip_irqchip_remove(struct gpio_chip *gpiochip)
> * the pins on the gpiochip can generate a unique IRQ. Everything else
> * need to be open coded.
> */
> -int gpiochip_irqchip_add(struct gpio_chip *gpiochip,
> - struct irq_chip *irqchip,
> - unsigned int first_irq,
> - irq_flow_handler_t handler,
> - unsigned int type)
> +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)
> {
> struct device_node *of_node;
> unsigned int offset;
> @@ -629,6 +629,7 @@ int gpiochip_irqchip_add(struct gpio_chip *gpiochip,
> gpiochip->irq_handler = handler;
> gpiochip->irq_default_type = type;
> gpiochip->to_irq = gpiochip_to_irq;
> + gpiochip->lock_key = lock_key;
> gpiochip->irqdomain = irq_domain_add_simple(of_node,
> gpiochip->ngpio, first_irq,
> &gpiochip_domain_ops, gpiochip);
> @@ -658,7 +659,7 @@ int gpiochip_irqchip_add(struct gpio_chip *gpiochip,
>
> return 0;
> }
> -EXPORT_SYMBOL_GPL(gpiochip_irqchip_add);
> +EXPORT_SYMBOL_GPL(_gpiochip_irqchip_add);
>
> #else /* CONFIG_GPIOLIB_IRQCHIP */
>
> diff --git a/include/linux/gpio/driver.h b/include/linux/gpio/driver.h
> index c8393cd..dd1dfbb 100644
> --- a/include/linux/gpio/driver.h
> +++ b/include/linux/gpio/driver.h
> @@ -6,6 +6,7 @@
> #include <linux/irq.h>
> #include <linux/irqchip/chained_irq.h>
> #include <linux/irqdomain.h>
> +#include <linux/lockdep.h>
> #include <linux/pinctrl/pinctrl.h>
>
> struct device;
> @@ -62,6 +63,7 @@ struct seq_file;
> * 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 int irq_default_type;
> int irq_parent;
> + struct lock_class_key *lock_key;
> #endif
>
> #if defined(CONFIG_OF_GPIO)
> @@ -171,11 +174,25 @@ void gpiochip_set_chained_irqchip(struct gpio_chip *gpiochip,
> int parent_irq,
> irq_flow_handler_t parent_handler);
>
> -int gpiochip_irqchip_add(struct gpio_chip *gpiochip,
> - struct irq_chip *irqchip,
> - unsigned int first_irq,
> - irq_flow_handler_t handler,
> - unsigned int type);
> +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
>
> #endif /* CONFIG_GPIOLIB_IRQCHIP */
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Next message: Wanpeng Li: "Re: [PATCH] mm/hwpoison: fix race between soft_offline_page and unpoison_memory"
Previous message: Michal Kubecek: "Re: GCOV_PROFILE_ALL breaks BUILD_BUG_ON(!is_power_of_2(8))"
In reply to: Grygorii Strashko: "[PATCH] gpiolib: irqchip: use different lockdep class for each gpio irqchip"
Next in thread: Linus Walleij: "Re: [PATCH] gpiolib: irqchip: use different lockdep class for each gpio irqchip"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]