pinctrl: rockchip: sleeping function called from atomic context

4.12 REGRESSION In review Brian Norris 6 participants 18 messages May 17, 2017 – Jun 29, 2017

Subsystems (7)

GPIO Arm Rockchip Driver core Pin control +3 more subsystems

lore

Share & tools

lore

Brian Norris

author

2017-05-17 22:56 UTC #0 lore raw

Hi,

Looks like we've added a mutex in the ->bus_lock() callback for Rockchip's pinctrl irqchip, which triggers a CONFIG_DEBUG_ATOMIC_SLEEP warning when entering system suspend:

[  151.406483] BUG: sleeping function called from invalid context at kernel/locking/mutex.c:238
[  151.420321] in_atomic(): 0, irqs_disabled(): 0, pid: 2447, name: cat
[  151.427457] CPU: 2 PID: 2447 Comm: cat Tainted: G        W     4.12.0-rc1+
[  151.435922] Hardware name: Google Kevin (DT)
[  151.440687] Call trace:
[  151.443440] [<ffffff900808b2fc>] dump_backtrace+0x0/0x2b8
[  151.449485] [<ffffff900808b5d4>] show_stack+0x20/0x28
[  151.455137] [<ffffff900848f738>] dump_stack+0xa4/0xcc
[  151.460797] [<ffffff90080e3118>] ___might_sleep+0x16c/0x188
[  151.467036] [<ffffff90080e31f4>] __might_sleep+0xc0/0xd4
[  151.472979] [<ffffff9008a70ca0>] mutex_lock+0x2c/0x68
[  151.478637] [<ffffff90084e806c>] rockchip_irq_bus_lock+0x48/0x54
[  151.485364] [<ffffff900811e3ac>] __irq_get_desc_lock+0x9c/0xc8
[  151.491895] [<ffffff900811fa3c>] irq_set_irq_wake+0x44/0x178
[  151.498235] [<ffffff900860d190>] dev_pm_arm_wake_irq+0x78/0x84
[  151.504769] [<ffffff90086137bc>] device_wakeup_arm_wake_irqs+0x48/0x70
[  151.512077] [<ffffff90086109c8>] dpm_suspend_noirq+0x180/0x51c
[  151.518608] [<ffffff90081171a4>] suspend_devices_and_enter+0x1dc/0xde4
[  151.525914] [<ffffff90081187a4>] pm_suspend+0x9f8/0xa38

[...]

The warning goes away if I revert commit 88bb94216f59 ("pinctrl:
rockchip: avoid hardirq-unsafe functions in irq_chip").

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.

Brian

Brian Norris

author

2017-05-27 02:19 UTC #1 lore raw

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.

Brian

Quoted 37 lines · Brian Norris

Hi,
Looks like we've added a mutex in the ->bus_lock() callback for Rockchip's pinctrl irqchip, which triggers a CONFIG_DEBUG_ATOMIC_SLEEP warning when entering system suspend:
[  151.406483] BUG: sleeping function called from invalid context at kernel/locking/mutex.c:238
[  151.420321] in_atomic(): 0, irqs_disabled(): 0, pid: 2447, name: cat
[  151.427457] CPU: 2 PID: 2447 Comm: cat Tainted: G        W     4.12.0-rc1+
[  151.435922] Hardware name: Google Kevin (DT)
[  151.440687] Call trace:
[  151.443440] [<ffffff900808b2fc>] dump_backtrace+0x0/0x2b8
[  151.449485] [<ffffff900808b5d4>] show_stack+0x20/0x28
[  151.455137] [<ffffff900848f738>] dump_stack+0xa4/0xcc
[  151.460797] [<ffffff90080e3118>] ___might_sleep+0x16c/0x188
[  151.467036] [<ffffff90080e31f4>] __might_sleep+0xc0/0xd4
[  151.472979] [<ffffff9008a70ca0>] mutex_lock+0x2c/0x68
[  151.478637] [<ffffff90084e806c>] rockchip_irq_bus_lock+0x48/0x54
[  151.485364] [<ffffff900811e3ac>] __irq_get_desc_lock+0x9c/0xc8
[  151.491895] [<ffffff900811fa3c>] irq_set_irq_wake+0x44/0x178
[  151.498235] [<ffffff900860d190>] dev_pm_arm_wake_irq+0x78/0x84
[  151.504769] [<ffffff90086137bc>] device_wakeup_arm_wake_irqs+0x48/0x70
[  151.512077] [<ffffff90086109c8>] dpm_suspend_noirq+0x180/0x51c
[  151.518608] [<ffffff90081171a4>] suspend_devices_and_enter+0x1dc/0xde4
[  151.525914] [<ffffff90081187a4>] pm_suspend+0x9f8/0xa38
[...]
The warning goes away if I revert commit 88bb94216f59 ("pinctrl:
rockchip: avoid hardirq-unsafe functions in irq_chip").
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.
Brian

Brian Norris

author

2017-06-23 20:59 UTC #2 lore raw

PATCH 1/1 Revert "pinctrl: rockchip: avoid hardirq-unsafe functions in irq_chip"

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 <[email protected]>

Cc Heiko Stuebner <[email protected]>

Cc Linus Walleij <[email protected]>

Signed-off-by Brian Norris <[email protected]>

---

+ Thomas, in case he has thoughts

Subject was "[4.12 REGRESSION] pinctrl: rockchip: sleeping function called from atomic context"

Quoted 10 lines · Brian Norris

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.
Quoted 5 lines · Brian Norris
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.

 drivers/pinctrl/pinctrl-rockchip.c | 44 ++++----------------------------------
 1 file changed, 4 insertions(+), 40 deletions(-)

drivers/pinctrl/pinctrl-rockchip.c +4 ~~-40~~

index f141aa0430b1..9dd981ddbb17 100644
@@ -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;

Signature · 1 line

2.13.1.611.g7e3b11ae1-goog

Heiko Stuebner

maintainer Reviewed-by on v1 1/1

2017-06-23 21:10 UTC #3 lore raw

Re: [PATCH for 4.12] Revert "pinctrl: rockchip: avoid hardirq-unsafe functions in irq_chip"

Am Freitag, 23. Juni 2017, 13:59:11 CEST schrieb Brian Norris:

Quoted 26 lines

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

...

Quoted 7 lines

It was reported on -rc1, and it's still not fixed in -rc6, so it should just be reverted.
Cc John Keeping <[email protected]>
Cc Heiko Stuebner <[email protected]>
Cc Linus Walleij <[email protected]>
Signed-off-by Brian Norris <[email protected]>

Reviewed-by Heiko Stuebner <[email protected]>

and should ideally be included for 4.12 still, as Brian's subject suggests.

a bit more below...

Quoted 13 lines

---
+ Thomas, in case he has thoughts
Subject was "[4.12 REGRESSION] pinctrl: rockchip: sleeping function called from atomic context"
Quoted 4 lines · Brian Norris
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:

Quoted 7 lines

Quoted 5 lines
Quoted 5 lines
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

Quoted 111 lines

 drivers/pinctrl/pinctrl-rockchip.c | 44 ++++----------------------------------
 1 file changed, 4 insertions(+), 40 deletions(-)

drivers/pinctrl/pinctrl-rockchip.c +4 ~~-40~~

index f141aa0430b1..9dd981ddbb17 100644
@@ -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;

Thomas Gleixner

2017-06-23 22:12 UTC #4 lore raw

Re: [PATCH for 4.12] Revert "pinctrl: rockchip: avoid hardirq-unsafe functions in irq_chip"

In reply to Brian Norris

Quoted 24 lines · 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

Quoted 6 lines

...
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

Quoted 14 lines

Subject was "[4.12 REGRESSION] pinctrl: rockchip: sleeping function called from atomic context"
Quoted 10 lines · Brian Norris
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.
Quoted 5 lines · Brian Norris
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 <[email protected]>
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

Paul E. McKenney

2017-06-23 22:40 UTC #5 lore raw

Re: [PATCH for 4.12] Revert "pinctrl: rockchip: avoid hardirq-unsafe functions in irq_chip"

Quoted 61 lines · Thomas Gleixner

Quoted 24 lines · 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
Quoted 6 lines
...
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
Quoted 14 lines
Subject was "[4.12 REGRESSION] pinctrl: rockchip: sleeping function called from atomic context"
Quoted 10 lines · Brian Norris
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.
Quoted 5 lines · Brian Norris
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 <[email protected]>
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.

Quoted 7 lines

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

Thomas Gleixner

2017-06-24 09:21 UTC #6 lore raw

Re: [PATCH for 4.12] Revert "pinctrl: rockchip: avoid hardirq-unsafe functions in irq_chip"

Quoted 8 lines · Paul E. McKenney

Quoted 7 lines · Thomas Gleixner
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.

Quoted 7 lines

Quoted 3 lines
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

Brian Norris

author

2017-06-27 00:06 UTC #7 lore raw

Re: [PATCH for 4.12] Revert "pinctrl: rockchip: avoid hardirq-unsafe functions in irq_chip"

In reply to Thomas Gleixner

Hi again Thomas,

Quoted 12 lines · Thomas Gleixner

Quoted 8 lines · Brian Norris
Quoted 7 lines · Brian Norris
Quoted 5 lines · Brian Norris
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:

Quoted 8 lines

commit 4990d4fe327b9d9a7a3be7103a82699406fdde69
Author: Tony Lindgren <[email protected]>
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 :)

Tony Lindgren

2017-06-27 06:24 UTC #8 lore raw

Re: [PATCH for 4.12] Revert "pinctrl: rockchip: avoid hardirq-unsafe functions in irq_chip"

Quoted 18 lines · Brian Norris

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

Quoted 8 lines

[   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

Brian Norris

author

2017-06-27 07:07 UTC #9 lore raw

Re: [PATCH for 4.12] Revert "pinctrl: rockchip: avoid hardirq-unsafe functions in irq_chip"

Quoted 2 lines · Tony Lindgren

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".)

Quoted 2 lines

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

Tony Lindgren

2017-06-27 07:32 UTC #10 lore raw

Re: [PATCH for 4.12] Revert "pinctrl: rockchip: avoid hardirq-unsafe functions in irq_chip"

Quoted 7 lines · Brian Norris

Quoted 2 lines · Tony Lindgren
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.

Yes several issues got fixed over past few years. If you find issues please let me know, otherwise I can't help.

Quoted 4 lines

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".)

Well typically the wakeirq needs to be disabled during runtime like we do for the generic wakeirqs. It might be connected to the RX line for example so it's just noise during runtime. If you actually need it during runtime then it's a separate story but I doubt that's the case here.

Talking of GPIO interrupt triggering, I wonder if something like below might help? It seems we're missing the setting of triggering, see below.

Quoted 5 lines

Quoted 2 lines
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.

Certainly all these issues need to be fixed if we intent to use it. Funny how I have not run into these with my test cases. Do you have a GPIO irqchip on I2C for handling the wake-up interrupts?

Regards,

Tony

8< -------

drivers/base/power/wakeirq.c +2 -1

@@ -198,7 +198,8 @@ int dev_pm_set_dedicated_wake_irq(struct device *dev, int irq)
 	 * so we use a threaded irq.
 	 */
 	err = request_threaded_irq(irq, NULL, handle_threaded_wake_irq,
-				   IRQF_ONESHOT, dev_name(dev), wirq);
+				   irq_get_trigger_type(irq) | IRQF_ONESHOT,
+				   dev_name(dev), wirq);
 	if (err)
 		goto err_free;

Thomas Gleixner

2017-06-27 13:01 UTC #11 lore raw

Re: [PATCH for 4.12] Revert "pinctrl: rockchip: avoid hardirq-unsafe functions in irq_chip"

In reply to Brian Norris

Quoted 36 lines · Brian Norris

[   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

Quoted 3 lines

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

Thomas Gleixner

2017-06-27 13:06 UTC #12 lore raw

Re: [PATCH for 4.12] Revert "pinctrl: rockchip: avoid hardirq-unsafe functions in irq_chip"

Quoted 14 lines · Thomas Gleixner

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:

Quoted 6 lines

        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

Brian Norris

author

2017-06-27 17:23 UTC #13 lore raw

Re: [PATCH for 4.12] Revert "pinctrl: rockchip: avoid hardirq-unsafe functions in irq_chip"

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:

Quoted 18 lines · Thomas Gleixner

Quoted 17 lines · Thomas Gleixner
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);

Quoted 7 lines

Quoted 3 lines
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

Thomas Gleixner

2017-06-27 18:07 UTC #14 lore raw

Re: [PATCH for 4.12] Revert "pinctrl: rockchip: avoid hardirq-unsafe functions in irq_chip"

Quoted 10 lines · Brian Norris

Quoted 3 lines · Thomas Gleixner
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

Heiko Stübner

maintainer

2017-06-27 13:26 UTC #15 lore raw

Re: [PATCH for 4.12] Revert "pinctrl: rockchip: avoid hardirq-unsafe functions in irq_chip"

In reply to Thomas Gleixner

Hi Thomas,

Am Dienstag, 27. Juni 2017, 15:01:32 CEST schrieb Thomas Gleixner:

Quoted 96 lines · Brian Norris

Quoted 38 lines
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
Quoted 3 lines
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:

Quoted 6 lines

        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.

Thomas Gleixner

2017-06-27 16:35 UTC #16 lore raw

Re: [PATCH for 4.12] Revert "pinctrl: rockchip: avoid hardirq-unsafe functions in irq_chip"

Quoted 13 lines · Heiko Stübner

Am Dienstag, 27. Juni 2017, 15:01:32 CEST schrieb Thomas Gleixner:
Quoted 11 lines
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?

Quoted 7 lines

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

Linus Walleij

2017-06-29 13:05 UTC #17 lore raw

Re: [PATCH for 4.12] Revert "pinctrl: rockchip: avoid hardirq-unsafe functions in irq_chip"

In reply to Brian Norris

Quoted 26 lines · 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

...

Quoted 7 lines

It was reported on -rc1, and it's still not fixed in -rc6, so it should just be reverted.
Cc John Keeping <[email protected]>
Cc Heiko Stuebner <[email protected]>
Cc Linus Walleij <[email protected]>
Signed-off-by Brian Norris <[email protected]>

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