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

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

[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

Re: [4.12 REGRESSION] pinctrl: rockchip: sleeping function called from atomic context

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

Brian

On Wed, May 17, 2017 at 03:56:34PM -0700, Brian Norris wrote:
> 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

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

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

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

 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;
-- 
2.13.1.611.g7e3b11ae1-goog

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

[Heiko Stuebner]

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

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

[Thomas Gleixner]

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

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

[Paul E. McKenney]

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

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

[Thomas Gleixner]

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

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

[Brian Norris]

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 :)

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

[Tony Lindgren]

* 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

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

[Brian Norris]

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

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

[Tony Lindgren]

* Brian Norris <briannorris@chromium.org> [170627 00:07]:
> 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.

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

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

> > 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< -------
diff --git a/drivers/base/power/wakeirq.c b/drivers/base/power/wakeirq.c
--- a/drivers/base/power/wakeirq.c
+++ b/drivers/base/power/wakeirq.c
@@ -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;
 

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

[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?

Thanks,

	tglx

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

[Thomas Gleixner]

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

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

[Heiko Stübner]

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.

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

[Thomas Gleixner]

[-- Attachment #1: Type: text/plain, Size: 1453 bytes --]

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

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

[Brian Norris]

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

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

[Brian Norris]

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

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

[Thomas Gleixner]

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

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

[Linus Walleij]

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