Re: [PATCH] gpio: pcf875x: Revert "gpio: pcf857x: Propagate wake-up setting to parent irq controller"

From: Geert Uytterhoeven
Date: Mon May 11 2015 - 13:36:31 EST

Next message: Darren Hart: "Re: [PATCH] toshiba_bluetooth: Fix build warning"
Previous message: Frederic Weisbecker: "Re: [PATCH 0/6] support "dataplane" mode for nohz_full"
Next in thread: Grygorii.Strashko@xxxxxxxxxx: "Re: [PATCH] gpio: pcf875x: Revert "gpio: pcf857x: Propagate wake-up setting to parent irq controller""
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Hi Roger,

On Mon, May 11, 2015 at 4:13 PM, Roger Quadros <rogerq@xxxxxx> wrote:
> commit b80eef95beb0 ('gpio: pcf857x: Propagate wake-up setting to parent irq controller')
> introduces the following recursive locking warning while suspending dra7-evm.
>
> The issue addressed by that commit has been already resolved by
> commit 10a50f1ab5f0 ('genirq: Set IRQCHIP_SKIP_SET_WAKE flag for dummy_irq_chip')

That's not 100% correct: commit b80eef95beb0 ('gpio: pcf857x: Propagate wake-up
setting to parent irq controller') fixes _two_ things:
1. warning due to missing irq_set_wake / IRQCHIP_SKIP_SET_WAKE,
2. propagating set_wake, so the parent interrupt controller stays awake, as
it's needed for wake-up,

Only the first issue is addressed by commit 10a50f1ab5f0 ('genirq: Set
IRQCHIP_SKIP_SET_WAKE flag for dummy_irq_chip').

> and so let's revert commit b80eef95beb0 ('gpio: pcf857x: Propagate wake-up setting to parent irq controller')
>
> At least the recursive locking message no longer appears after the revert.
>
> [ 30.591905] PM: Syncing filesystems ... done.
> [ 30.623060] Freezing user space processes ... (elapsed 0.003 seconds) done.
> [ 30.634470] Freezing remaining freezable tasks ... (elapsed 0.002 seconds) done.
> [ 30.658288] sd 0:0:0:0: [sda] Synchronizing SCSI cache
> [ 30.663678]
> [ 30.663681] =============================================
> [ 30.663683] [ INFO: possible recursive locking detected ]
> [ 30.663688] 4.1.0-rc3 #1115 Not tainted
> [ 30.663693] ---------------------------------------------
> [ 30.663697] suspend.sh/2319 is trying to acquire lock:
> [ 30.663719] (class){......}, at: [<c0096ebc>] __irq_get_desc_lock+0x48/0x88
> [ 30.663722]
> [ 30.663722] but task is already holding lock:
> [ 30.663734] (class){......}, at: [<c0096ebc>] __irq_get_desc_lock+0x48/0x88

Does this mean .set_irq_wake() cannot call irq_set_irq_wake()?
Many GPIO drivers do that, as they need to propagate wake-up state to the
parent interrupt controller?

> [ 30.663736]
> [ 30.663736] other info that might help us debug this:
> [ 30.663739] Possible unsafe locking scenario:
> [ 30.663739]
> [ 30.663740] CPU0
> [ 30.663743] ----
> [ 30.663747] lock(class);
> [ 30.663752] lock(class);
> [ 30.663755]
> [ 30.663755] *** DEADLOCK ***
> [ 30.663755]
> [ 30.663757] May be due to missing lock nesting notation
> [ 30.663757]
> [ 30.663761] 6 locks held by suspend.sh/2319:
> [ 30.663778] #0: (sb_writers#7){.+.+.+}, at: [<c015fb1c>] vfs_write+0x130/0x14c
> [ 30.663793] #1: (&of->mutex){+.+.+.}, at: [<c01cdbb0>] kernfs_fop_write+0x4c/0x198
> [ 30.663807] #2: (s_active#37){.+.+.+}, at: [<c01cdbb8>] kernfs_fop_write+0x54/0x198
> [ 30.663820] #3: (pm_mutex){+.+.+.}, at: [<c0092ef8>] pm_suspend+0x240/0x4b8
> [ 30.663834] #4: (&dev->mutex){......}, at: [<c03dc918>] __device_suspend+0xd0/0x378
> [ 30.663847] #5: (class){......}, at: [<c0096ebc>] __irq_get_desc_lock+0x48/0x88
> [ 30.663849]
> [ 30.663849] stack backtrace:
> [ 30.663856] CPU: 0 PID: 2319 Comm: suspend.sh Not tainted 4.1.0-rc3 #1115
> [ 30.663859] Hardware name: Generic DRA74X (Flattened Device Tree)
> [ 30.663873] [<c00163bc>] (unwind_backtrace) from [<c0012930>] (show_stack+0x10/0x14)
> [ 30.663884] [<c0012930>] (show_stack) from [<c05f9ee8>] (dump_stack+0x80/0x9c)
> [ 30.663896] [<c05f9ee8>] (dump_stack) from [<c008bba4>] (__lock_acquire+0x13f0/0x1c14)
> [ 30.663905] [<c008bba4>] (__lock_acquire) from [<c008c940>] (lock_acquire+0x9c/0x114)
> [ 30.663914] [<c008c940>] (lock_acquire) from [<c0600214>] (_raw_spin_lock_irqsave+0x38/0x4c)
> [ 30.663922] [<c0600214>] (_raw_spin_lock_irqsave) from [<c0096ebc>] (__irq_get_desc_lock+0x48/0x88)
> [ 30.663932] [<c0096ebc>] (__irq_get_desc_lock) from [<c00977cc>] (irq_set_irq_wake+0x20/0xf0)
> [ 30.663946] [<c00977cc>] (irq_set_irq_wake) from [<bf0000c8>] (pcf857x_irq_set_wake+0x14/0x1c [gpio_pcf857x])
> [ 30.663962] [<bf0000c8>] (pcf857x_irq_set_wake [gpio_pcf857x]) from [<c009764c>] (set_irq_wake_real+0x30/0x44)
> [ 30.663970] [<c009764c>] (set_irq_wake_real) from [<c0097838>] (irq_set_irq_wake+0x8c/0xf0)
> [ 30.663979] [<c0097838>] (irq_set_irq_wake) from [<bf059074>] (usb_extcon_suspend+0x2c/0x48 [extcon_usb_gpio])
> [ 30.663992] [<bf059074>] (usb_extcon_suspend [extcon_usb_gpio]) from [<c03d38e8>] (platform_pm_suspend+0x2c/0x54)
> [ 30.664003] [<c03d38e8>] (platform_pm_suspend) from [<c03dbe10>] (dpm_run_callback+0x4c/0x110)
> [ 30.664012] [<c03dbe10>] (dpm_run_callback) from [<c03dc974>] (__device_suspend+0x12c/0x378)
> [ 30.664019] [<c03dc974>] (__device_suspend) from [<c03de4ac>] (dpm_suspend+0x128/0x2d8)
> [ 30.664025] [<c03de4ac>] (dpm_suspend) from [<c00926a0>] (suspend_devices_and_enter+0x84/0x69c)
> [ 30.664032] [<c00926a0>] (suspend_devices_and_enter) from [<c00930f4>] (pm_suspend+0x43c/0x4b8)
> [ 30.664039] [<c00930f4>] (pm_suspend) from [<c0091654>] (state_store+0x68/0xb8)
> [ 30.664049] [<c0091654>] (state_store) from [<c03421b8>] (kobj_attr_store+0x14/0x20)
> [ 30.664058] [<c03421b8>] (kobj_attr_store) from [<c01ce84c>] (sysfs_kf_write+0x4c/0x50)
> [ 30.664065] [<c01ce84c>] (sysfs_kf_write) from [<c01cdc20>] (kernfs_fop_write+0xbc/0x198)
> [ 30.664074] [<c01cdc20>] (kernfs_fop_write) from [<c015eab0>] (__vfs_write+0x2c/0xd8)
> [ 30.664082] [<c015eab0>] (__vfs_write) from [<c015fa7c>] (vfs_write+0x90/0x14c)
> [ 30.664088] [<c015fa7c>] (vfs_write) from [<c015fd04>] (SyS_write+0x40/0x8c)
> [ 30.664097] [<c015fd04>] (SyS_write) from [<c000f540>] (ret_fast_syscall+0x0/0x4c)
> [ 31.038856] sd 0:0:0:0: [sda] Stopping disk
> [ 31.644707] PM: suspend of devices complete after 996.653 msecs
> [ 31.654312] PM: late suspend of devices complete after 3.329 msecs
> [ 31.663835] PM: noirq suspend of devices complete after 3.029 msecs
> [ 31.670422] Disabling non-boot CPUs ...
> [ 31.675294] CPU1: shutdown
>
> Signed-off-by: Roger Quadros <rogerq@xxxxxx>
> ---
> drivers/gpio/gpio-pcf857x.c | 37 +++----------------------------------
> 1 file changed, 3 insertions(+), 34 deletions(-)
>
> diff --git a/drivers/gpio/gpio-pcf857x.c b/drivers/gpio/gpio-pcf857x.c
> index 945f0cd..126c937 100644
> --- a/drivers/gpio/gpio-pcf857x.c
> +++ b/drivers/gpio/gpio-pcf857x.c
> @@ -204,36 +204,6 @@ static irqreturn_t pcf857x_irq(int irq, void *data)
> return IRQ_HANDLED;
> }
>
> -/*
> - * NOP functions
> - */
> -static void noop(struct irq_data *data) { }
> -
> -static unsigned int noop_ret(struct irq_data *data)
> -{
> - return 0;
> -}
> -
> -static int pcf857x_irq_set_wake(struct irq_data *data, unsigned int on)
> -{
> - struct pcf857x *gpio = irq_data_get_irq_chip_data(data);
> -
> - irq_set_irq_wake(gpio->client->irq, on);
> - return 0;
> -}
> -
> -static struct irq_chip pcf857x_irq_chip = {
> - .name = "pcf857x",
> - .irq_startup = noop_ret,
> - .irq_shutdown = noop,
> - .irq_enable = noop,
> - .irq_disable = noop,
> - .irq_ack = noop,
> - .irq_mask = noop,
> - .irq_unmask = noop,
> - .irq_set_wake = pcf857x_irq_set_wake,
> -};
> -
> /*-------------------------------------------------------------------------*/
>
> static int pcf857x_probe(struct i2c_client *client,
> @@ -347,9 +317,8 @@ static int pcf857x_probe(struct i2c_client *client,
>
> /* Enable irqchip if we have an interrupt */
> if (client->irq) {
> - status = gpiochip_irqchip_add(&gpio->chip, &pcf857x_irq_chip,
> - 0, handle_level_irq,
> - IRQ_TYPE_NONE);
> + status = gpiochip_irqchip_add(&gpio->chip, &dummy_irq_chip, 0,
> + handle_level_irq, IRQ_TYPE_NONE);
> if (status) {
> dev_err(&client->dev, "cannot add irqchip\n");
> goto fail_irq;
> @@ -362,7 +331,7 @@ static int pcf857x_probe(struct i2c_client *client,
> if (status)
> goto fail_irq;
>
> - gpiochip_set_chained_irqchip(&gpio->chip, &pcf857x_irq_chip,
> + gpiochip_set_chained_irqchip(&gpio->chip, &dummy_irq_chip,
> client->irq, NULL);
> }
>
> --
> 2.1.4

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
--
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: Darren Hart: "Re: [PATCH] toshiba_bluetooth: Fix build warning"
Previous message: Frederic Weisbecker: "Re: [PATCH 0/6] support "dataplane" mode for nohz_full"
Next in thread: Grygorii.Strashko@xxxxxxxxxx: "Re: [PATCH] gpio: pcf875x: Revert "gpio: pcf857x: Propagate wake-up setting to parent irq controller""
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]