| Message ID | 1431353601-21295-1-git-send-email-rogerq@ti.com |
|---|---|
| State | New |
| Headers | show |
Hi Roger, On Mon, May 11, 2015 at 4:13 PM, Roger Quadros <rogerq@ti.com> 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@ti.com> > --- > 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@linux-m68k.org 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-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On 05/11/2015 08:36 PM, Geert Uytterhoeven wrote: > On Mon, May 11, 2015 at 4:13 PM, Roger Quadros <rogerq@ti.com> 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? As I remember, there was similar problem, so I found corresponding patch (just FYI) ab2b926 mfd: Fix twl6030 lockdep recursion warning on setting wake IRQs Not sure such kind of solution is the best choice (
On Mon, May 11, 2015 at 4:13 PM, Roger Quadros <rogerq@ti.com> 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') > 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. Geert do you have a better solution to this or should I just apply the revert? Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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); }
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') 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 [ 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@ti.com> --- drivers/gpio/gpio-pcf857x.c | 37 +++---------------------------------- 1 file changed, 3 insertions(+), 34 deletions(-)