| Message ID | 20170623205911.GA143883@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Am Freitag, 23. Juni 2017, 13:59:11 CEST schrieb Brian Norris: > This reverts commit 88bb94216f59e10802aaf78c858a4146085faf18. > > It introduced a new CONFIG_DEBUG_ATOMIC_SLEEP warning in v4.12-rc1: > > [ 7226.716713] BUG: sleeping function called from invalid context at kernel/locking/mutex.c:238 > [ 7226.716716] in_atomic(): 0, irqs_disabled(): 0, pid: 1708, name: bash > [ 7226.716722] CPU: 1 PID: 1708 Comm: bash Not tainted 4.12.0-rc6+ #1213 > [ 7226.716724] Hardware name: Google Kevin (DT) > [ 7226.716726] Call trace: > [ 7226.716738] [<ffffff8008089928>] dump_backtrace+0x0/0x24c > [ 7226.716743] [<ffffff8008089b94>] show_stack+0x20/0x28 > [ 7226.716749] [<ffffff8008371370>] dump_stack+0x90/0xb0 > [ 7226.716755] [<ffffff80080cd2a0>] ___might_sleep+0x10c/0x124 > [ 7226.716760] [<ffffff80080cd330>] __might_sleep+0x78/0x88 > [ 7226.716765] [<ffffff800879e210>] mutex_lock+0x2c/0x64 > [ 7226.716771] [<ffffff80083ad678>] rockchip_irq_bus_lock+0x30/0x3c > [ 7226.716777] [<ffffff80080f6d40>] __irq_get_desc_lock+0x78/0x98 > [ 7226.716782] [<ffffff80080f7e6c>] irq_set_irq_wake+0x44/0x12c > [ 7226.716787] [<ffffff8008486e18>] dev_pm_arm_wake_irq+0x4c/0x58 > [ 7226.716792] [<ffffff800848b80c>] device_wakeup_arm_wake_irqs+0x3c/0x58 > [ 7226.716796] [<ffffff80084896fc>] dpm_suspend_noirq+0xf8/0x3a0 > [ 7226.716800] [<ffffff80080f1384>] suspend_devices_and_enter+0x1a4/0x9a8 > [ 7226.716803] [<ffffff80080f21ec>] pm_suspend+0x664/0x6a4 > [ 7226.716807] [<ffffff80080f04d8>] state_store+0xd4/0xf8 > ... > > It was reported on -rc1, and it's still not fixed in -rc6, so it should > just be reverted. > > Cc: John Keeping <john@metanate.com> > Cc: Heiko Stuebner <heiko@sntech.de> > Cc: Linus Walleij <linus.walleij@linaro.org> > Signed-off-by: Brian Norris <briannorris@chromium.org> Reviewed-by: Heiko Stuebner <heiko@sntech.de> and should ideally be included for 4.12 still, as Brian's subject suggests. a bit more below... > --- > > + Thomas, in case he has thoughts > > Subject was "[4.12 REGRESSION] pinctrl: rockchip: sleeping function > called from atomic context" > > On Fri, May 26, 2017 at 07:19:00PM -0700, Brian Norris wrote: > > Any thoughts? Revert the offending patch? I can spend a little more time > > next week trying to debug what's actually going on if needed. > > > > On Wed, May 17, 2017 at 03:56:34PM -0700, Brian Norris wrote: > > > > The thing is, the documentation (and apparent design) suggest that > > > calling sleeping functions from ->irq_bus_lock() is perfectly valid. I'm > > > not 100% following the ___might_sleep() logic, but is this complaining > > > because of the RCU read locking in device_wakeup_arm_wake_irqs()? I have > > > CONFIG_PREEMPT_RCU and CONFIG_PREEMPT enabled, FWIW. > > I've seen no reply that indicates anyone wants to fix the patch. There were some mails exchanged with John, as David Wu did find the same issue from a different calling context - namely disable_irq_nosync from irq-context. I just realized they never included mailing lists though. David's stack trace looked like BUG: sleeping function called from invalid context at kernel/locking/mutex.c:620 in_atomic(): 1, irqs_disabled(): 128, pid: 141, name: irq/95-fusb302 1 lock held by irq/95-fusb302/141: #0: (&(&chip->irq_lock)->rlock){......}, at: [<ffffff800859e3a0>] fusb_irq_disable+0x20/0x68 irq event stamp: 52 hardirqs last enabled at (51): [<ffffff80080bcc30>] queue_work_on+0x68/0x80 hardirqs last disabled at (52): [<ffffff8008c6f41c>] _raw_spin_lock_irqsave+0x20/0x60 softirqs last enabled at (0): [<ffffff800809e9ec>] copy_process.isra.54+0x390/0x1728 softirqs last disabled at (0): [< (null)>] (null) Preemption disabled at:[<ffffff800859e3a0>] fusb_irq_disable+0x20/0x68 CPU: 5 PID: 141 Comm: irq/95-fusb302 Not tainted 4.4.70 #30 Hardware name: Rockchip RK3399 Evaluation Board v3 (Android) (DT) Call trace: [<ffffff800808a82c>] dump_backtrace+0x0/0x1c4 [<ffffff800808aa04>] show_stack+0x14/0x1c [<ffffff80083c3b90>] dump_stack+0xa8/0xe0 [<ffffff80080cf560>] ___might_sleep+0x214/0x224 [<ffffff80080cf5e4>] __might_sleep+0x74/0x84 [<ffffff8008c6c1ac>] mutex_lock_nested+0x48/0x3cc [<ffffff80083fe2b0>] rockchip_irq_bus_lock+0x28/0x34 [<ffffff800810b680>] __irq_get_desc_lock+0x68/0x88 [<ffffff800810d558>] __disable_irq_nosync+0x28/0x70 [<ffffff800810d5ac>] disable_irq_nosync+0xc/0x14 [<ffffff800859e3b4>] fusb_irq_disable+0x34/0x68 [<ffffff800859e410>] cc_interrupt_handler+0x28/0x38 [<ffffff800810cd48>] irq_thread_fn+0x28/0x68 [<ffffff800810cf80>] irq_thread+0x130/0x234 [<ffffff80080c58e8>] kthread+0x104/0x10c [<ffffff8008083080>] ret_from_fork+0x10/0x50 So it should really be reverted until we get this sorted. Heiko > drivers/pinctrl/pinctrl-rockchip.c | 44 ++++---------------------------------- > 1 file changed, 4 insertions(+), 40 deletions(-) > > diff --git a/drivers/pinctrl/pinctrl-rockchip.c b/drivers/pinctrl/pinctrl-rockchip.c > index f141aa0430b1..9dd981ddbb17 100644 > --- a/drivers/pinctrl/pinctrl-rockchip.c > +++ b/drivers/pinctrl/pinctrl-rockchip.c > @@ -143,9 +143,6 @@ struct rockchip_drv { > * @gpio_chip: gpiolib chip > * @grange: gpio range > * @slock: spinlock for the gpio bank > - * @irq_lock: bus lock for irq chip > - * @new_irqs: newly configured irqs which must be muxed as GPIOs in > - * irq_bus_sync_unlock() > */ > struct rockchip_pin_bank { > void __iomem *reg_base; > @@ -168,8 +165,6 @@ struct rockchip_pin_bank { > struct pinctrl_gpio_range grange; > raw_spinlock_t slock; > u32 toggle_edge_mode; > - struct mutex irq_lock; > - u32 new_irqs; > }; > > #define PIN_BANK(id, pins, label) \ > @@ -2134,12 +2129,11 @@ static int rockchip_irq_set_type(struct irq_data *d, unsigned int type) > int ret; > > /* make sure the pin is configured as gpio input */ > - ret = rockchip_verify_mux(bank, d->hwirq, RK_FUNC_GPIO); > + ret = rockchip_set_mux(bank, d->hwirq, RK_FUNC_GPIO); > if (ret < 0) > return ret; > > - bank->new_irqs |= mask; > - > + clk_enable(bank->clk); > raw_spin_lock_irqsave(&bank->slock, flags); > > data = readl_relaxed(bank->reg_base + GPIO_SWPORT_DDR); > @@ -2197,6 +2191,7 @@ static int rockchip_irq_set_type(struct irq_data *d, unsigned int type) > default: > irq_gc_unlock(gc); > raw_spin_unlock_irqrestore(&bank->slock, flags); > + clk_disable(bank->clk); > return -EINVAL; > } > > @@ -2205,6 +2200,7 @@ static int rockchip_irq_set_type(struct irq_data *d, unsigned int type) > > irq_gc_unlock(gc); > raw_spin_unlock_irqrestore(&bank->slock, flags); > + clk_disable(bank->clk); > > return 0; > } > @@ -2248,34 +2244,6 @@ static void rockchip_irq_disable(struct irq_data *d) > clk_disable(bank->clk); > } > > -static void rockchip_irq_bus_lock(struct irq_data *d) > -{ > - struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d); > - struct rockchip_pin_bank *bank = gc->private; > - > - clk_enable(bank->clk); > - mutex_lock(&bank->irq_lock); > -} > - > -static void rockchip_irq_bus_sync_unlock(struct irq_data *d) > -{ > - struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d); > - struct rockchip_pin_bank *bank = gc->private; > - > - while (bank->new_irqs) { > - unsigned int irq = __ffs(bank->new_irqs); > - int ret; > - > - ret = rockchip_set_mux(bank, irq, RK_FUNC_GPIO); > - WARN_ON(ret < 0); > - > - bank->new_irqs &= ~BIT(irq); > - } > - > - mutex_unlock(&bank->irq_lock); > - clk_disable(bank->clk); > -} > - > static int rockchip_interrupts_register(struct platform_device *pdev, > struct rockchip_pinctrl *info) > { > @@ -2342,9 +2310,6 @@ static int rockchip_interrupts_register(struct platform_device *pdev, > gc->chip_types[0].chip.irq_suspend = rockchip_irq_suspend; > gc->chip_types[0].chip.irq_resume = rockchip_irq_resume; > gc->chip_types[0].chip.irq_set_type = rockchip_irq_set_type; > - gc->chip_types[0].chip.irq_bus_lock = rockchip_irq_bus_lock; > - gc->chip_types[0].chip.irq_bus_sync_unlock = > - rockchip_irq_bus_sync_unlock; > gc->wake_enabled = IRQ_MSK(bank->nr_pins); > > irq_set_chained_handler_and_data(bank->irq, > @@ -2518,7 +2483,6 @@ static struct rockchip_pin_ctrl *rockchip_pinctrl_get_soc_data( > int bank_pins = 0; > > raw_spin_lock_init(&bank->slock); > - mutex_init(&bank->irq_lock); > bank->drvdata = d; > bank->pin_base = ctrl->nr_pins; > ctrl->nr_pins += bank->nr_pins; >
On Fri, 23 Jun 2017, Brian Norris wrote: > This reverts commit 88bb94216f59e10802aaf78c858a4146085faf18. > > It introduced a new CONFIG_DEBUG_ATOMIC_SLEEP warning in v4.12-rc1: > > [ 7226.716713] BUG: sleeping function called from invalid context at kernel/locking/mutex.c:238 > [ 7226.716716] in_atomic(): 0, irqs_disabled(): 0, pid: 1708, name: bash > [ 7226.716722] CPU: 1 PID: 1708 Comm: bash Not tainted 4.12.0-rc6+ #1213 > [ 7226.716724] Hardware name: Google Kevin (DT) > [ 7226.716726] Call trace: > [ 7226.716738] [<ffffff8008089928>] dump_backtrace+0x0/0x24c > [ 7226.716743] [<ffffff8008089b94>] show_stack+0x20/0x28 > [ 7226.716749] [<ffffff8008371370>] dump_stack+0x90/0xb0 > [ 7226.716755] [<ffffff80080cd2a0>] ___might_sleep+0x10c/0x124 > [ 7226.716760] [<ffffff80080cd330>] __might_sleep+0x78/0x88 > [ 7226.716765] [<ffffff800879e210>] mutex_lock+0x2c/0x64 > [ 7226.716771] [<ffffff80083ad678>] rockchip_irq_bus_lock+0x30/0x3c > [ 7226.716777] [<ffffff80080f6d40>] __irq_get_desc_lock+0x78/0x98 > [ 7226.716782] [<ffffff80080f7e6c>] irq_set_irq_wake+0x44/0x12c > [ 7226.716787] [<ffffff8008486e18>] dev_pm_arm_wake_irq+0x4c/0x58 > [ 7226.716792] [<ffffff800848b80c>] device_wakeup_arm_wake_irqs+0x3c/0x58 > [ 7226.716796] [<ffffff80084896fc>] dpm_suspend_noirq+0xf8/0x3a0 > [ 7226.716800] [<ffffff80080f1384>] suspend_devices_and_enter+0x1a4/0x9a8 > [ 7226.716803] [<ffffff80080f21ec>] pm_suspend+0x664/0x6a4 > [ 7226.716807] [<ffffff80080f04d8>] state_store+0xd4/0xf8 > ... > > It was reported on -rc1, and it's still not fixed in -rc6, so it should > just be reverted. > > + Thomas, in case he has thoughts + Peter and Paul, Tony > Subject was "[4.12 REGRESSION] pinctrl: rockchip: sleeping function > called from atomic context" > > On Fri, May 26, 2017 at 07:19:00PM -0700, Brian Norris wrote: > > Any thoughts? Revert the offending patch? I can spend a little more time > > next week trying to debug what's actually going on if needed. > > > > On Wed, May 17, 2017 at 03:56:34PM -0700, Brian Norris wrote: > > > > The thing is, the documentation (and apparent design) suggest that > > > calling sleeping functions from ->irq_bus_lock() is perfectly valid. I'm > > > not 100% following the ___might_sleep() logic, but is this complaining > > > because of the RCU read locking in device_wakeup_arm_wake_irqs()? I have > > > CONFIG_PREEMPT_RCU and CONFIG_PREEMPT enabled, FWIW. Sigh, The real wreckage happened in commit: commit 4990d4fe327b9d9a7a3be7103a82699406fdde69 Author: Tony Lindgren <tony@atomide.com> Date: Mon May 18 15:40:29 2015 -0700 PM / Wakeirq: Add automated device wake IRQ handling which added that RCU locking stuff and thereby broke the long existing bus_lock() facility of the interrupt core. irq_bus_lock/unlock was explicitely made to allow sleeping locks for interrupt chips which hang behind slow busses like i2c or spi. It took us quite some effort to get this done and that patch broke it permanently. I'm not sure what to do here. This is an ever recurring issue simply because RT requires that sleeping locks can be taken inside rcu locked regions. So sooner than later we need a resoilution for that problem. Thanks, tglx
On Sat, Jun 24, 2017 at 12:12:49AM +0200, Thomas Gleixner wrote: > On Fri, 23 Jun 2017, Brian Norris wrote: > > > This reverts commit 88bb94216f59e10802aaf78c858a4146085faf18. > > > > It introduced a new CONFIG_DEBUG_ATOMIC_SLEEP warning in v4.12-rc1: > > > > [ 7226.716713] BUG: sleeping function called from invalid context at kernel/locking/mutex.c:238 > > [ 7226.716716] in_atomic(): 0, irqs_disabled(): 0, pid: 1708, name: bash > > [ 7226.716722] CPU: 1 PID: 1708 Comm: bash Not tainted 4.12.0-rc6+ #1213 > > [ 7226.716724] Hardware name: Google Kevin (DT) > > [ 7226.716726] Call trace: > > [ 7226.716738] [<ffffff8008089928>] dump_backtrace+0x0/0x24c > > [ 7226.716743] [<ffffff8008089b94>] show_stack+0x20/0x28 > > [ 7226.716749] [<ffffff8008371370>] dump_stack+0x90/0xb0 > > [ 7226.716755] [<ffffff80080cd2a0>] ___might_sleep+0x10c/0x124 > > [ 7226.716760] [<ffffff80080cd330>] __might_sleep+0x78/0x88 > > [ 7226.716765] [<ffffff800879e210>] mutex_lock+0x2c/0x64 > > [ 7226.716771] [<ffffff80083ad678>] rockchip_irq_bus_lock+0x30/0x3c > > [ 7226.716777] [<ffffff80080f6d40>] __irq_get_desc_lock+0x78/0x98 > > [ 7226.716782] [<ffffff80080f7e6c>] irq_set_irq_wake+0x44/0x12c > > [ 7226.716787] [<ffffff8008486e18>] dev_pm_arm_wake_irq+0x4c/0x58 > > [ 7226.716792] [<ffffff800848b80c>] device_wakeup_arm_wake_irqs+0x3c/0x58 > > [ 7226.716796] [<ffffff80084896fc>] dpm_suspend_noirq+0xf8/0x3a0 > > [ 7226.716800] [<ffffff80080f1384>] suspend_devices_and_enter+0x1a4/0x9a8 > > [ 7226.716803] [<ffffff80080f21ec>] pm_suspend+0x664/0x6a4 > > [ 7226.716807] [<ffffff80080f04d8>] state_store+0xd4/0xf8 > > ... > > > > It was reported on -rc1, and it's still not fixed in -rc6, so it should > > just be reverted. > > > > + Thomas, in case he has thoughts > > + Peter and Paul, Tony > > > Subject was "[4.12 REGRESSION] pinctrl: rockchip: sleeping function > > called from atomic context" > > > > On Fri, May 26, 2017 at 07:19:00PM -0700, Brian Norris wrote: > > > Any thoughts? Revert the offending patch? I can spend a little more time > > > next week trying to debug what's actually going on if needed. > > > > > > On Wed, May 17, 2017 at 03:56:34PM -0700, Brian Norris wrote: > > > > > > The thing is, the documentation (and apparent design) suggest that > > > > calling sleeping functions from ->irq_bus_lock() is perfectly valid. I'm > > > > not 100% following the ___might_sleep() logic, but is this complaining > > > > because of the RCU read locking in device_wakeup_arm_wake_irqs()? I have > > > > CONFIG_PREEMPT_RCU and CONFIG_PREEMPT enabled, FWIW. > > Sigh, The real wreckage happened in commit: > > commit 4990d4fe327b9d9a7a3be7103a82699406fdde69 > Author: Tony Lindgren <tony@atomide.com> > Date: Mon May 18 15:40:29 2015 -0700 > > PM / Wakeirq: Add automated device wake IRQ handling > > which added that RCU locking stuff and thereby broke the long existing > bus_lock() facility of the interrupt core. > > irq_bus_lock/unlock was explicitely made to allow sleeping locks for > interrupt chips which hang behind slow busses like i2c or spi. It took us > quite some effort to get this done and that patch broke it permanently. > > I'm not sure what to do here. This is an ever recurring issue simply > because RT requires that sleeping locks can be taken inside rcu locked > regions. So sooner than later we need a resoilution for that problem. The usual advice would be for 4990d4fe327b ("PM / Wakeirq: Add automated device wake IRQ handling") to use SRCU rather than RCU. Is there some reason that won't work? And yes, my commit 5b72f9643b52a ("rcu: Complain if blocking in preemptible RCU read-side critical section") that is now in -tip needs adjustment for -rt. I can easily add "&& IS_ENABLED(CONFIG_PREEMPT_RT)" or whatever the current incantation is. Just let me know! Thanx, Paul
On Fri, 23 Jun 2017, Paul E. McKenney wrote: > On Sat, Jun 24, 2017 at 12:12:49AM +0200, Thomas Gleixner wrote: > > which added that RCU locking stuff and thereby broke the long existing > > bus_lock() facility of the interrupt core. > > > > irq_bus_lock/unlock was explicitely made to allow sleeping locks for > > interrupt chips which hang behind slow busses like i2c or spi. It took us > > quite some effort to get this done and that patch broke it permanently. > > > > I'm not sure what to do here. This is an ever recurring issue simply > > because RT requires that sleeping locks can be taken inside rcu locked > > regions. So sooner than later we need a resoilution for that problem. > > The usual advice would be for 4990d4fe327b ("PM / Wakeirq: Add automated > device wake IRQ handling") to use SRCU rather than RCU. Is there some > reason that won't work? I can't see one. So yes, we should rather convert that stuff to SRCU instead of playing ping pong forever. Thanks, tglx
Hi again Thomas, On Sat, Jun 24, 2017 at 12:12:49AM +0200, Thomas Gleixner wrote: > On Fri, 23 Jun 2017, Brian Norris wrote: > > On Fri, May 26, 2017 at 07:19:00PM -0700, Brian Norris wrote: > > > On Wed, May 17, 2017 at 03:56:34PM -0700, Brian Norris wrote: > > > > > > The thing is, the documentation (and apparent design) suggest that > > > > calling sleeping functions from ->irq_bus_lock() is perfectly valid. I'm > > > > not 100% following the ___might_sleep() logic, but is this complaining > > > > because of the RCU read locking in device_wakeup_arm_wake_irqs()? I have > > > > CONFIG_PREEMPT_RCU and CONFIG_PREEMPT enabled, FWIW. > > Sigh, The real wreckage happened in commit: > > commit 4990d4fe327b9d9a7a3be7103a82699406fdde69 > Author: Tony Lindgren <tony@atomide.com> > Date: Mon May 18 15:40:29 2015 -0700 > > PM / Wakeirq: Add automated device wake IRQ handling > > which added that RCU locking stuff and thereby broke the long existing > bus_lock() facility of the interrupt core. So I agree that the above commit was problematic, and that you have fixed that in your patch ("PM / wakeirq: Convert to SRCU"). But I noticed there were other threads where people have complained about the $subject patch also causing problems with drivers that call disable_irq_nosync() from within an IRQ context. So I poked around with one such driver that calls disable_irq_nosync() from its ISR [1], and saw this: [ 14.524945] Bluetooth: : OOB Wake-on-BT configured at IRQ 56 [ 14.531657] usbcore: registered new interface driver btusb [ 18.973886] BUG: sleeping function called from invalid context at kernel/locking/mutex.c:238 [ 18.987695] in_atomic(): 1, irqs_disabled(): 128, pid: 0, name: swapper/0 [ 18.995282] CPU: 0 PID: 0 Comm: swapper/0 Not tainted 4.12.0-rc6+ #1233 [ 19.002669] Hardware name: Google Kevin (DT) [ 19.007435] Call trace: [ 19.010171] [<ffffff8008089928>] dump_backtrace+0x0/0x24c [ 19.016202] [<ffffff8008089b94>] show_stack+0x20/0x28 [ 19.021846] [<ffffff8008371270>] dump_stack+0x90/0xb0 [ 19.027488] [<ffffff80080cd2a0>] ___might_sleep+0x10c/0x124 [ 19.033713] [<ffffff80080cd330>] __might_sleep+0x78/0x88 [ 19.039647] [<ffffff800879e248>] mutex_lock+0x2c/0x64 [ 19.045291] [<ffffff80083ad578>] rockchip_irq_bus_lock+0x30/0x3c [ 19.052003] [<ffffff80080f6c68>] __irq_get_desc_lock+0x78/0x98 [ 19.058519] [<ffffff80080f8e90>] __disable_irq_nosync+0x38/0x80 [ 19.065132] [<ffffff80080f8ef8>] disable_irq_nosync+0x20/0x2c [ 19.071555] [<ffffff8000a99f58>] btusb_oob_wake_handler+0x4c/0x68 [btusb] [ 19.079140] [<ffffff80080f7428>] __handle_irq_event_percpu+0xf0/0x254 [ 19.086336] [<ffffff80080f75c4>] handle_irq_event_percpu+0x38/0x88 [ 19.093239] [<ffffff80080f7660>] handle_irq_event+0x4c/0x7c [ 19.099464] [<ffffff80080fb5dc>] handle_level_irq+0xd0/0x108 [ 19.105785] [<ffffff80080f64e0>] generic_handle_irq+0x30/0x44 [ 19.112204] [<ffffff80083ad308>] rockchip_irq_demux+0xe8/0x190 [ 19.118720] [<ffffff80080f64e0>] generic_handle_irq+0x30/0x44 [ 19.125138] [<ffffff80080f6b88>] __handle_domain_irq+0x90/0xbc [ 19.131652] [<ffffff8008080e98>] gic_handle_irq+0xe8/0x1b0 The documentation is fairly suggestive that ->irq_bus_lock() can sleep, but then it also suggests that disable_irq_nosync() is safe in IRQ context. So which is the "more true" one? Brian [1] Seem familiar? You were complaining about this driver previously. At least I didn't point you at an out-of-tree driver, where some of the other reports came from :)
* Brian Norris <briannorris@chromium.org> [170626 17:06]: > So I agree that the above commit was problematic, and that you have > fixed that in your patch ("PM / wakeirq: Convert to SRCU"). But I > noticed there were other threads where people have complained about the > $subject patch also causing problems with drivers that call > disable_irq_nosync() from within an IRQ context. So I poked around with > one such driver that calls disable_irq_nosync() from its ISR [1], and > saw this: > > [ 14.524945] Bluetooth: : OOB Wake-on-BT configured at IRQ 56 > [ 14.531657] usbcore: registered new interface driver btusb > [ 18.973886] BUG: sleeping function called from invalid context at kernel/locking/mutex.c:238 > [ 18.987695] in_atomic(): 1, irqs_disabled(): 128, pid: 0, name: swapper/0 > [ 18.995282] CPU: 0 PID: 0 Comm: swapper/0 Not tainted 4.12.0-rc6+ #1233 > [ 19.002669] Hardware name: Google Kevin (DT) > [ 19.007435] Call trace: > [ 19.010171] [<ffffff8008089928>] dump_backtrace+0x0/0x24c > [ 19.016202] [<ffffff8008089b94>] show_stack+0x20/0x28 > [ 19.021846] [<ffffff8008371270>] dump_stack+0x90/0xb0 > [ 19.027488] [<ffffff80080cd2a0>] ___might_sleep+0x10c/0x124 > [ 19.033713] [<ffffff80080cd330>] __might_sleep+0x78/0x88 > [ 19.039647] [<ffffff800879e248>] mutex_lock+0x2c/0x64 > [ 19.045291] [<ffffff80083ad578>] rockchip_irq_bus_lock+0x30/0x3c > [ 19.052003] [<ffffff80080f6c68>] __irq_get_desc_lock+0x78/0x98 > [ 19.058519] [<ffffff80080f8e90>] __disable_irq_nosync+0x38/0x80 > [ 19.065132] [<ffffff80080f8ef8>] disable_irq_nosync+0x20/0x2c > [ 19.071555] [<ffffff8000a99f58>] btusb_oob_wake_handler+0x4c/0x68 [btusb] Hmm so how come drivers/bluetooth/btusb.c can't use the generic dev_pm_set_dedicated_wake_irq()? Can you please take a look? If there are issues remaining let's rather fix them so we can get rid of the custom tinkering of wake-up events in the drivers. Regards, Tony
On Mon, Jun 26, 2017 at 11:24:09PM -0700, Tony Lindgren wrote: > Hmm so how come drivers/bluetooth/btusb.c can't use the generic > dev_pm_set_dedicated_wake_irq()? Can you please take a look? I took a look previously, and last time I did, there were too many bugs for it to be useful. You may have fixed the ones I reported w.r.t. assumptions about runtime PM. I also recall there being some difficulty with supporting level-triggered interrupts that way. (This signal has no device-level mask, and it triggers for all sorts of BT activity. There may not be a relevant "edge".) > If there are issues remaining let's rather fix them so we can get rid > of the custom tinkering of wake-up events in the drivers. That's nice, but that doesn't answer my questions. Perhaps that's a side project. The point is that we're clearly violating the documented APIs. Brian
On Mon, 26 Jun 2017, Brian Norris wrote: > So I agree that the above commit was problematic, and that you have > fixed that in your patch ("PM / wakeirq: Convert to SRCU"). But I > noticed there were other threads where people have complained about the > $subject patch also causing problems with drivers that call > disable_irq_nosync() from within an IRQ context. So I poked around with > one such driver that calls disable_irq_nosync() from its ISR [1], and > saw this: > > [ 14.524945] Bluetooth: : OOB Wake-on-BT configured at IRQ 56 > [ 14.531657] usbcore: registered new interface driver btusb > [ 18.973886] BUG: sleeping function called from invalid context at kernel/locking/mutex.c:238 > [ 18.987695] in_atomic(): 1, irqs_disabled(): 128, pid: 0, name: swapper/0 > [ 18.995282] CPU: 0 PID: 0 Comm: swapper/0 Not tainted 4.12.0-rc6+ #1233 > [ 19.002669] Hardware name: Google Kevin (DT) > [ 19.007435] Call trace: > [ 19.010171] [<ffffff8008089928>] dump_backtrace+0x0/0x24c > [ 19.016202] [<ffffff8008089b94>] show_stack+0x20/0x28 > [ 19.021846] [<ffffff8008371270>] dump_stack+0x90/0xb0 > [ 19.027488] [<ffffff80080cd2a0>] ___might_sleep+0x10c/0x124 > [ 19.033713] [<ffffff80080cd330>] __might_sleep+0x78/0x88 > [ 19.039647] [<ffffff800879e248>] mutex_lock+0x2c/0x64 > [ 19.045291] [<ffffff80083ad578>] rockchip_irq_bus_lock+0x30/0x3c > [ 19.052003] [<ffffff80080f6c68>] __irq_get_desc_lock+0x78/0x98 > [ 19.058519] [<ffffff80080f8e90>] __disable_irq_nosync+0x38/0x80 > [ 19.065132] [<ffffff80080f8ef8>] disable_irq_nosync+0x20/0x2c > [ 19.071555] [<ffffff8000a99f58>] btusb_oob_wake_handler+0x4c/0x68 [btusb] > [ 19.079140] [<ffffff80080f7428>] __handle_irq_event_percpu+0xf0/0x254 > [ 19.086336] [<ffffff80080f75c4>] handle_irq_event_percpu+0x38/0x88 > [ 19.093239] [<ffffff80080f7660>] handle_irq_event+0x4c/0x7c > [ 19.099464] [<ffffff80080fb5dc>] handle_level_irq+0xd0/0x108 > [ 19.105785] [<ffffff80080f64e0>] generic_handle_irq+0x30/0x44 > [ 19.112204] [<ffffff80083ad308>] rockchip_irq_demux+0xe8/0x190 > [ 19.118720] [<ffffff80080f64e0>] generic_handle_irq+0x30/0x44 > [ 19.125138] [<ffffff80080f6b88>] __handle_domain_irq+0x90/0xbc > [ 19.131652] [<ffffff8008080e98>] gic_handle_irq+0xe8/0x1b0 > > The documentation is fairly suggestive that ->irq_bus_lock() can sleep, > but then it also suggests that disable_irq_nosync() is safe in IRQ > context. So which is the "more true" one? The function kerneldoc comment says: * This function may be called from IRQ context. 'May be called' is definitely different from 'is safe'. So yes, there are issues with the interrupt controllers behind slow busses, but OTOH, if you look at the complete picture: |-----------| [GPOI] - | | [GPOI] - | | [GPOI] - | I2C GPIO |-----------------[ CPU IRQ ] [GPOI] - | | [GPOI] - | |-----------------[ I2C Controller ] |-----------| Then it's pretty obvious that you cannot access the I2C controller from the hard interrupt context of the CPU IRQ. The wakeup machinery here needs to mark the GPIO pin as wakeup irq and the underlying parent CPU irq as well. So the CPU IRQ is what triggers the wakeup and that needs to be disabled until the system comes back and the real stuff gets called when the CPU interrupt is replayed. Now the problem is that the CPU IRQ might be implemented as chained interrupt. And chained interrupts are not playing well with all of this because they evade all the normal interrupt handling mechanisms completely. So in the wakeup case the CPU irq cannot be disabled by the generic mechanisms, instead the chained handler is invoked, demuxes stuff and you end up with a call into the slow irq chip. As a side note: I recently converted the AMD pinctrl driver to use a regular interrupt for demultiplexing because BIOS wreckaged machines drowned in spurious interrupts and locked up hard because chained interrupt handlers have no safety net whatsoever. That aside, looking at the commit which caused this discussion: 88bb94216f59e pinctrl: rockchip: avoid hardirq-unsafe functions in irq_chip I assume (the changelog lacks details) that the patch want's to avoid a might sleep splat from the irq callbacks caused by the regmap spinlock, which gets converted into a sleeping lock on RT. It does this by abusing the irq_bus_lock() mechanism, which is wrong to begin with. The only irq chip function which uses the regmap magic is the irq_set_type() callback. Now, I have a hard time to understand (though I'm no regmap/pinctrl expert) why that regmap stuff needs to be called in the first place. The level and the polarity are programmed via: writel_relaxed(level, gc->reg_base + GPIO_INTTYPE_LEVEL); writel_relaxed(polarity, gc->reg_base + GPIO_INT_POLARITY); Why needs the regmap machinery to be invoked there? The GPIO is already muxed and configured as interrupt, otherwise none of the irq functions could be invoked. Hmm? Thanks, tglx
On Tue, 27 Jun 2017, Thomas Gleixner wrote: > That aside, looking at the commit which caused this discussion: > > 88bb94216f59e pinctrl: rockchip: avoid hardirq-unsafe functions in irq_chip > > I assume (the changelog lacks details) that the patch want's to avoid a > might sleep splat from the irq callbacks caused by the regmap spinlock, > which gets converted into a sleeping lock on RT. It does this by abusing > the irq_bus_lock() mechanism, which is wrong to begin with. > > The only irq chip function which uses the regmap magic is the > irq_set_type() callback. Now, I have a hard time to understand (though I'm > no regmap/pinctrl expert) why that regmap stuff needs to be called in the > first place. The level and the polarity are programmed via: > > writel_relaxed(level, gc->reg_base + GPIO_INTTYPE_LEVEL); > writel_relaxed(polarity, gc->reg_base + GPIO_INT_POLARITY); > > Why needs the regmap machinery to be invoked there? The GPIO is already > muxed and configured as interrupt, otherwise none of the irq functions > could be invoked. Hmm? That said, the commit should be reverted and the issue needs to analyzed proper. We still need the RCU -> SCRU conversion, but that's a different problem. Thanks, tglx
Hi Thomas, Am Dienstag, 27. Juni 2017, 15:01:32 CEST schrieb Thomas Gleixner: > On Mon, 26 Jun 2017, Brian Norris wrote: > > So I agree that the above commit was problematic, and that you have > > fixed that in your patch ("PM / wakeirq: Convert to SRCU"). But I > > noticed there were other threads where people have complained about the > > $subject patch also causing problems with drivers that call > > disable_irq_nosync() from within an IRQ context. So I poked around with > > one such driver that calls disable_irq_nosync() from its ISR [1], and > > saw this: > > > > [ 14.524945] Bluetooth: : OOB Wake-on-BT configured at IRQ 56 > > [ 14.531657] usbcore: registered new interface driver btusb > > [ 18.973886] BUG: sleeping function called from invalid context at > > kernel/locking/mutex.c:238 [ 18.987695] in_atomic(): 1, > > irqs_disabled(): 128, pid: 0, name: swapper/0 [ 18.995282] CPU: 0 PID: > > 0 Comm: swapper/0 Not tainted 4.12.0-rc6+ #1233 [ 19.002669] Hardware > > name: Google Kevin (DT) > > [ 19.007435] Call trace: > > [ 19.010171] [<ffffff8008089928>] dump_backtrace+0x0/0x24c > > [ 19.016202] [<ffffff8008089b94>] show_stack+0x20/0x28 > > [ 19.021846] [<ffffff8008371270>] dump_stack+0x90/0xb0 > > [ 19.027488] [<ffffff80080cd2a0>] ___might_sleep+0x10c/0x124 > > [ 19.033713] [<ffffff80080cd330>] __might_sleep+0x78/0x88 > > [ 19.039647] [<ffffff800879e248>] mutex_lock+0x2c/0x64 > > [ 19.045291] [<ffffff80083ad578>] rockchip_irq_bus_lock+0x30/0x3c > > [ 19.052003] [<ffffff80080f6c68>] __irq_get_desc_lock+0x78/0x98 > > [ 19.058519] [<ffffff80080f8e90>] __disable_irq_nosync+0x38/0x80 > > [ 19.065132] [<ffffff80080f8ef8>] disable_irq_nosync+0x20/0x2c > > [ 19.071555] [<ffffff8000a99f58>] btusb_oob_wake_handler+0x4c/0x68 > > [btusb] [ 19.079140] [<ffffff80080f7428>] > > __handle_irq_event_percpu+0xf0/0x254 [ 19.086336] [<ffffff80080f75c4>] > > handle_irq_event_percpu+0x38/0x88 [ 19.093239] [<ffffff80080f7660>] > > handle_irq_event+0x4c/0x7c > > [ 19.099464] [<ffffff80080fb5dc>] handle_level_irq+0xd0/0x108 > > [ 19.105785] [<ffffff80080f64e0>] generic_handle_irq+0x30/0x44 > > [ 19.112204] [<ffffff80083ad308>] rockchip_irq_demux+0xe8/0x190 > > [ 19.118720] [<ffffff80080f64e0>] generic_handle_irq+0x30/0x44 > > [ 19.125138] [<ffffff80080f6b88>] __handle_domain_irq+0x90/0xbc > > [ 19.131652] [<ffffff8008080e98>] gic_handle_irq+0xe8/0x1b0 > > > > The documentation is fairly suggestive that ->irq_bus_lock() can sleep, > > but then it also suggests that disable_irq_nosync() is safe in IRQ > > context. So which is the "more true" one? > > The function kerneldoc comment says: > > * This function may be called from IRQ context. > > 'May be called' is definitely different from 'is safe'. > > So yes, there are issues with the interrupt controllers behind slow busses, > > but OTOH, if you look at the complete picture: > |-----------| > > [GPOI] - | | > [GPOI] - | | > [GPOI] - | I2C GPIO |-----------------[ CPU IRQ ] > [GPOI] - | | > [GPOI] - | |-----------------[ I2C Controller ] > > |-----------| > > Then it's pretty obvious that you cannot access the I2C controller from the > hard interrupt context of the CPU IRQ. The wakeup machinery here needs to > mark the GPIO pin as wakeup irq and the underlying parent CPU irq as well. > > So the CPU IRQ is what triggers the wakeup and that needs to be disabled > until the system comes back and the real stuff gets called when the > CPU interrupt is replayed. > > Now the problem is that the CPU IRQ might be implemented as chained > interrupt. And chained interrupts are not playing well with all of this > because they evade all the normal interrupt handling mechanisms > completely. So in the wakeup case the CPU irq cannot be disabled by the > generic mechanisms, instead the chained handler is invoked, demuxes stuff > and you end up with a call into the slow irq chip. > > As a side note: I recently converted the AMD pinctrl driver to use a > regular interrupt for demultiplexing because BIOS wreckaged machines > drowned in spurious interrupts and locked up hard because chained interrupt > handlers have no safety net whatsoever. > > That aside, looking at the commit which caused this discussion: > > 88bb94216f59e pinctrl: rockchip: avoid hardirq-unsafe functions in irq_chip > > I assume (the changelog lacks details) that the patch want's to avoid a > might sleep splat from the irq callbacks caused by the regmap spinlock, > which gets converted into a sleeping lock on RT. It does this by abusing > the irq_bus_lock() mechanism, which is wrong to begin with. > > The only irq chip function which uses the regmap magic is the > irq_set_type() callback. Now, I have a hard time to understand (though I'm > no regmap/pinctrl expert) why that regmap stuff needs to be called in the > first place. The level and the polarity are programmed via: > > writel_relaxed(level, gc->reg_base + GPIO_INTTYPE_LEVEL); > writel_relaxed(polarity, gc->reg_base + GPIO_INT_POLARITY); > > Why needs the regmap machinery to be invoked there? The GPIO is already > muxed and configured as interrupt, otherwise none of the irq functions > could be invoked. Hmm? That is a safeguard against the pinmux not being set as "gpio" but some other function, if the irq is requested directly without going through the gpio API. But looking at struct irq_chip and also other pinctrl drivers again, it seems the new [0] irq_request_resources might be the way saner place for this. Especially as it also prevents the mux-setting from being called more than once. Heiko [0] The pinctrl driver is from 2013, while the irq resources feature is from 2014.
On Tue, 27 Jun 2017, Heiko Stübner wrote: > Am Dienstag, 27. Juni 2017, 15:01:32 CEST schrieb Thomas Gleixner: > > The only irq chip function which uses the regmap magic is the > > irq_set_type() callback. Now, I have a hard time to understand (though I'm > > no regmap/pinctrl expert) why that regmap stuff needs to be called in the > > first place. The level and the polarity are programmed via: > > > > writel_relaxed(level, gc->reg_base + GPIO_INTTYPE_LEVEL); > > writel_relaxed(polarity, gc->reg_base + GPIO_INT_POLARITY); > > > > Why needs the regmap machinery to be invoked there? The GPIO is already > > muxed and configured as interrupt, otherwise none of the irq functions > > could be invoked. Hmm? > > That is a safeguard against the pinmux not being set as "gpio" but some other > function, if the irq is requested directly without going through the gpio API. > > But looking at struct irq_chip and also other pinctrl drivers again, it seems > the new [0] irq_request_resources might be the way saner place for this. > Especially as it also prevents the mux-setting from being called more than > once. That'll fail on RT as well because irq_request_resources() is called with irq_desc->lock held and interrupts disabled. irq_request_resources() can probably be called without holding desc->lock, though we need some form of protection against concurrent irq requests/free. I'll have look into that. Thanks, tglx
Hi Linus, I'm not sure I follow all of Thomas's suggestions on what should be done in the future yet, but I agree that can be done in parallel: On Tue, Jun 27, 2017 at 03:06:26PM +0200, Thomas Gleixner wrote: > On Tue, 27 Jun 2017, Thomas Gleixner wrote: > > That aside, looking at the commit which caused this discussion: > > > > 88bb94216f59e pinctrl: rockchip: avoid hardirq-unsafe functions in irq_chip > > > > I assume (the changelog lacks details) that the patch want's to avoid a > > might sleep splat from the irq callbacks caused by the regmap spinlock, > > which gets converted into a sleeping lock on RT. It does this by abusing > > the irq_bus_lock() mechanism, which is wrong to begin with. > > > > The only irq chip function which uses the regmap magic is the > > irq_set_type() callback. Now, I have a hard time to understand (though I'm > > no regmap/pinctrl expert) why that regmap stuff needs to be called in the > > first place. The level and the polarity are programmed via: > > > > writel_relaxed(level, gc->reg_base + GPIO_INTTYPE_LEVEL); > > writel_relaxed(polarity, gc->reg_base + GPIO_INT_POLARITY); > > > > Why needs the regmap machinery to be invoked there? The GPIO is already > > muxed and configured as interrupt, otherwise none of the irq functions > > could be invoked. Hmm? > > That said, the commit should be reverted and the issue needs to analyzed > proper. We still need the RCU -> SCRU conversion, but that's a different > problem. Can we consider this an "ack" for the $subject then? Heiko also gave his approval. How can this get merged? It's running a bit late for 4.12, though it really shouldn't be risky (at least for non-RT stuff that was working warning-free already in 4.11), but a 4.13-rc1 with -stable tag could work as well. I suppose I didn't put a proper 'Fixes' tag (though it should be obvious from the commit message), so here goes: Fixes: 88bb94216f59 ("pinctrl: rockchip: avoid hardirq-unsafe functions in irq_chip") Regards, Brian
On Tue, 27 Jun 2017, Brian Norris wrote: > On Tue, Jun 27, 2017 at 03:06:26PM +0200, Thomas Gleixner wrote: > > That said, the commit should be reverted and the issue needs to analyzed > > proper. We still need the RCU -> SCRU conversion, but that's a different > > problem. > > Can we consider this an "ack" for the $subject then? Heiko also gave his > approval. How can this get merged? It's running a bit late for 4.12, > though it really shouldn't be risky (at least for non-RT stuff that was > working warning-free already in 4.11), but a 4.13-rc1 with -stable tag > could work as well. Yes. That revert can go into 4.12 from my POV, but I leave that to Linus. Thanks, tglx
On Fri, Jun 23, 2017 at 10:59 PM, Brian Norris <briannorris@chromium.org> wrote: > This reverts commit 88bb94216f59e10802aaf78c858a4146085faf18. > > It introduced a new CONFIG_DEBUG_ATOMIC_SLEEP warning in v4.12-rc1: > > [ 7226.716713] BUG: sleeping function called from invalid context at kernel/locking/mutex.c:238 > [ 7226.716716] in_atomic(): 0, irqs_disabled(): 0, pid: 1708, name: bash > [ 7226.716722] CPU: 1 PID: 1708 Comm: bash Not tainted 4.12.0-rc6+ #1213 > [ 7226.716724] Hardware name: Google Kevin (DT) > [ 7226.716726] Call trace: > [ 7226.716738] [<ffffff8008089928>] dump_backtrace+0x0/0x24c > [ 7226.716743] [<ffffff8008089b94>] show_stack+0x20/0x28 > [ 7226.716749] [<ffffff8008371370>] dump_stack+0x90/0xb0 > [ 7226.716755] [<ffffff80080cd2a0>] ___might_sleep+0x10c/0x124 > [ 7226.716760] [<ffffff80080cd330>] __might_sleep+0x78/0x88 > [ 7226.716765] [<ffffff800879e210>] mutex_lock+0x2c/0x64 > [ 7226.716771] [<ffffff80083ad678>] rockchip_irq_bus_lock+0x30/0x3c > [ 7226.716777] [<ffffff80080f6d40>] __irq_get_desc_lock+0x78/0x98 > [ 7226.716782] [<ffffff80080f7e6c>] irq_set_irq_wake+0x44/0x12c > [ 7226.716787] [<ffffff8008486e18>] dev_pm_arm_wake_irq+0x4c/0x58 > [ 7226.716792] [<ffffff800848b80c>] device_wakeup_arm_wake_irqs+0x3c/0x58 > [ 7226.716796] [<ffffff80084896fc>] dpm_suspend_noirq+0xf8/0x3a0 > [ 7226.716800] [<ffffff80080f1384>] suspend_devices_and_enter+0x1a4/0x9a8 > [ 7226.716803] [<ffffff80080f21ec>] pm_suspend+0x664/0x6a4 > [ 7226.716807] [<ffffff80080f04d8>] state_store+0xd4/0xf8 > ... > > It was reported on -rc1, and it's still not fixed in -rc6, so it should > just be reverted. > > Cc: John Keeping <john@metanate.com> > Cc: Heiko Stuebner <heiko@sntech.de> > Cc: Linus Walleij <linus.walleij@linaro.org> > Signed-off-by: Brian Norris <briannorris@chromium.org> Revert applied for fixes with Heiko's review tag after reading up on the discussion. What a mess, sigh. Thanks to everyone who is helping to fix this up. Yours, Linus Walleij
diff --git a/drivers/pinctrl/pinctrl-rockchip.c b/drivers/pinctrl/pinctrl-rockchip.c index f141aa0430b1..9dd981ddbb17 100644 --- a/drivers/pinctrl/pinctrl-rockchip.c +++ b/drivers/pinctrl/pinctrl-rockchip.c @@ -143,9 +143,6 @@ struct rockchip_drv { * @gpio_chip: gpiolib chip * @grange: gpio range * @slock: spinlock for the gpio bank - * @irq_lock: bus lock for irq chip - * @new_irqs: newly configured irqs which must be muxed as GPIOs in - * irq_bus_sync_unlock() */ struct rockchip_pin_bank { void __iomem *reg_base; @@ -168,8 +165,6 @@ struct rockchip_pin_bank { struct pinctrl_gpio_range grange; raw_spinlock_t slock; u32 toggle_edge_mode; - struct mutex irq_lock; - u32 new_irqs; }; #define PIN_BANK(id, pins, label) \ @@ -2134,12 +2129,11 @@ static int rockchip_irq_set_type(struct irq_data *d, unsigned int type) int ret; /* make sure the pin is configured as gpio input */ - ret = rockchip_verify_mux(bank, d->hwirq, RK_FUNC_GPIO); + ret = rockchip_set_mux(bank, d->hwirq, RK_FUNC_GPIO); if (ret < 0) return ret; - bank->new_irqs |= mask; - + clk_enable(bank->clk); raw_spin_lock_irqsave(&bank->slock, flags); data = readl_relaxed(bank->reg_base + GPIO_SWPORT_DDR); @@ -2197,6 +2191,7 @@ static int rockchip_irq_set_type(struct irq_data *d, unsigned int type) default: irq_gc_unlock(gc); raw_spin_unlock_irqrestore(&bank->slock, flags); + clk_disable(bank->clk); return -EINVAL; } @@ -2205,6 +2200,7 @@ static int rockchip_irq_set_type(struct irq_data *d, unsigned int type) irq_gc_unlock(gc); raw_spin_unlock_irqrestore(&bank->slock, flags); + clk_disable(bank->clk); return 0; } @@ -2248,34 +2244,6 @@ static void rockchip_irq_disable(struct irq_data *d) clk_disable(bank->clk); } -static void rockchip_irq_bus_lock(struct irq_data *d) -{ - struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d); - struct rockchip_pin_bank *bank = gc->private; - - clk_enable(bank->clk); - mutex_lock(&bank->irq_lock); -} - -static void rockchip_irq_bus_sync_unlock(struct irq_data *d) -{ - struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d); - struct rockchip_pin_bank *bank = gc->private; - - while (bank->new_irqs) { - unsigned int irq = __ffs(bank->new_irqs); - int ret; - - ret = rockchip_set_mux(bank, irq, RK_FUNC_GPIO); - WARN_ON(ret < 0); - - bank->new_irqs &= ~BIT(irq); - } - - mutex_unlock(&bank->irq_lock); - clk_disable(bank->clk); -} - static int rockchip_interrupts_register(struct platform_device *pdev, struct rockchip_pinctrl *info) { @@ -2342,9 +2310,6 @@ static int rockchip_interrupts_register(struct platform_device *pdev, gc->chip_types[0].chip.irq_suspend = rockchip_irq_suspend; gc->chip_types[0].chip.irq_resume = rockchip_irq_resume; gc->chip_types[0].chip.irq_set_type = rockchip_irq_set_type; - gc->chip_types[0].chip.irq_bus_lock = rockchip_irq_bus_lock; - gc->chip_types[0].chip.irq_bus_sync_unlock = - rockchip_irq_bus_sync_unlock; gc->wake_enabled = IRQ_MSK(bank->nr_pins); irq_set_chained_handler_and_data(bank->irq, @@ -2518,7 +2483,6 @@ static struct rockchip_pin_ctrl *rockchip_pinctrl_get_soc_data( int bank_pins = 0; raw_spin_lock_init(&bank->slock); - mutex_init(&bank->irq_lock); bank->drvdata = d; bank->pin_base = ctrl->nr_pins; ctrl->nr_pins += bank->nr_pins;