Re: [RFC PATCH] extcon: Fix sleeping in atomic context after connecting USB cable

Re: [RFC PATCH] extcon: Fix sleeping in atomic context after connecting USB cable

[함명주]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=euc-kr, Size: 3563 bytes --]

>   
>  Kernel built with extcon and charger-manager.
> 
> After connecting the USB cable sleeping function was called from atomic
> context:
> [   63.328648] BUG: sleeping function called from invalid context at kernel/locking/mutex.c:586
[]
> [   63.388743] Workqueue: events max14577_muic_irq_work
> [   63.393707] [<c00167a4>] (unwind_backtrace) from [<c0012c08>] (show_stack+0x10/0x14)
> [   63.401422] [<c0012c08>] (show_stack) from [<c06232ec>] (dump_stack+0x70/0xbc)
> [   63.408625] [<c06232ec>] (dump_stack) from [<c0629d0c>] (mutex_lock_nested+0x24/0x410)
> [   63.416525] [<c0629d0c>] (mutex_lock_nested) from [<c0354474>] (regmap_read+0x30/0x60)
> [   63.424424] [<c0354474>] (regmap_read) from [<c0406b84>] (max14577_charger_get_property+0x1e4/0x250)
> [   63.433535] [<c0406b84>] (max14577_charger_get_property) from [<c0403834>] (is_ext_pwr_online+0x30/0x6c)
> [   63.442994] [<c0403834>] (is_ext_pwr_online) from [<c0404a54>] (charger_extcon_notifier+0x40/0x70)
> [   63.451934] [<c0404a54>] (charger_extcon_notifier) from [<c044cbe4>] (_call_per_cable+0x40/0x4c)
> [   63.460704] [<c044cbe4>] (_call_per_cable) from [<c0051560>] (notifier_call_chain+0x64/0x128)
> [   63.469209] [<c0051560>] (notifier_call_chain) from [<c0051640>] (raw_notifier_call_chain+0x18/0x20)
> [   63.478321] [<c0051640>] (raw_notifier_call_chain) from [<c044d0fc>] (extcon_update_state+0xa8/0x204)
> [   63.487522] [<c044d0fc>] (extcon_update_state) from [<c044e5f8>] (max14577_muic_chg_handler+0xdc/0x180)
> [   63.496897] [<c044e5f8>] (max14577_muic_chg_handler) from [<c044e924>] (max14577_muic_irq_work+0x7c/0xd8)
> [   63.506445] [<c044e924>] (max14577_muic_irq_work) from [<c004480c>] (process_one_work+0x198/0x66c)
> [   63.515385] [<c004480c>] (process_one_work) from [<c0044d4c>] (worker_thread+0x38/0x564)
> [   63.523455] [<c0044d4c>] (worker_thread) from [<c004c4ec>] (kthread+0xcc/0xe8)
> [   63.530661] [<c004c4ec>] (kthread) from [<c000f1f8>] (ret_from_fork+0x14/0x3c)
> [   63.543926] charger_manager: Set current limit of CHARGER : 450000uA ~ 450000uA
> [   63.548870] extcon-port jack: jack event CHGDET=usb
> [   63.550592] extcon-port jack: jack event CHGDET=charger
> [   64.188607] charger-manager charger-manager@0: Failed to get battery temperature
> [   64.200684] charger-manager charger-manager@0: CHARGING
> 
> The first sleeping function is is_ext_pwr_online()
> (drivers/power/charger-manager.c). The atomic context initiating the
> flow is set up in extcon_update_state() (drivers/extcon/extcon-class.c).
> 
> The extcon_update_state() uses spin locks which are not necessary
> because the function is not called from interrupt service routines.
> Instead, the extcon_update_state() is called from:
> 1. Threaded interrupt handlers.
> 2. Work queues.
> 
> Replace the spin lock with mutex and update the documentation of this
> function.

No. You've done it in the opposite way.

update_state is often called in a interrupt handler that cannot sleep.

For the context you've mentioned, we'd need workqueue invoked by
_call_per_cable or notifier callback.


Cheers,
MyungJoo

> 
> Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
> ---
>  drivers/extcon/extcon-class.c | 15 ++++++++-------
>  include/linux/extcon.h        |  3 ++-
>  2 files changed, 10 insertions(+), 8 deletions(-)
> 
[]
> -	spin_lock_irqsave(&edev->lock, flags);
> +	mutex_lock(&edev->lock);
[]
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [RFC PATCH] extcon: Fix sleeping in atomic context after connecting USB cable

[Krzysztof Kozlowski]

On pon, 2014-09-29 at 04:25 +0000, 함명주 wrote:
> >   
> >  Kernel built with extcon and charger-manager.
> > 
> > After connecting the USB cable sleeping function was called from atomic
> > context:
> > [   63.328648] BUG: sleeping function called from invalid context at kernel/locking/mutex.c:586
> []
> > [   63.388743] Workqueue: events max14577_muic_irq_work
> > [   63.393707] [<c00167a4>] (unwind_backtrace) from [<c0012c08>] (show_stack+0x10/0x14)
> > [   63.401422] [<c0012c08>] (show_stack) from [<c06232ec>] (dump_stack+0x70/0xbc)
> > [   63.408625] [<c06232ec>] (dump_stack) from [<c0629d0c>] (mutex_lock_nested+0x24/0x410)
> > [   63.416525] [<c0629d0c>] (mutex_lock_nested) from [<c0354474>] (regmap_read+0x30/0x60)
> > [   63.424424] [<c0354474>] (regmap_read) from [<c0406b84>] (max14577_charger_get_property+0x1e4/0x250)
> > [   63.433535] [<c0406b84>] (max14577_charger_get_property) from [<c0403834>] (is_ext_pwr_online+0x30/0x6c)
> > [   63.442994] [<c0403834>] (is_ext_pwr_online) from [<c0404a54>] (charger_extcon_notifier+0x40/0x70)
> > [   63.451934] [<c0404a54>] (charger_extcon_notifier) from [<c044cbe4>] (_call_per_cable+0x40/0x4c)
> > [   63.460704] [<c044cbe4>] (_call_per_cable) from [<c0051560>] (notifier_call_chain+0x64/0x128)
> > [   63.469209] [<c0051560>] (notifier_call_chain) from [<c0051640>] (raw_notifier_call_chain+0x18/0x20)
> > [   63.478321] [<c0051640>] (raw_notifier_call_chain) from [<c044d0fc>] (extcon_update_state+0xa8/0x204)
> > [   63.487522] [<c044d0fc>] (extcon_update_state) from [<c044e5f8>] (max14577_muic_chg_handler+0xdc/0x180)
> > [   63.496897] [<c044e5f8>] (max14577_muic_chg_handler) from [<c044e924>] (max14577_muic_irq_work+0x7c/0xd8)
> > [   63.506445] [<c044e924>] (max14577_muic_irq_work) from [<c004480c>] (process_one_work+0x198/0x66c)
> > [   63.515385] [<c004480c>] (process_one_work) from [<c0044d4c>] (worker_thread+0x38/0x564)
> > [   63.523455] [<c0044d4c>] (worker_thread) from [<c004c4ec>] (kthread+0xcc/0xe8)
> > [   63.530661] [<c004c4ec>] (kthread) from [<c000f1f8>] (ret_from_fork+0x14/0x3c)
> > [   63.543926] charger_manager: Set current limit of CHARGER : 450000uA ~ 450000uA
> > [   63.548870] extcon-port jack: jack event CHGDET=usb
> > [   63.550592] extcon-port jack: jack event CHGDET=charger
> > [   64.188607] charger-manager charger-manager@0: Failed to get battery temperature
> > [   64.200684] charger-manager charger-manager@0: CHARGING
> > 
> > The first sleeping function is is_ext_pwr_online()
> > (drivers/power/charger-manager.c). The atomic context initiating the
> > flow is set up in extcon_update_state() (drivers/extcon/extcon-class.c).
> > 
> > The extcon_update_state() uses spin locks which are not necessary
> > because the function is not called from interrupt service routines.
> > Instead, the extcon_update_state() is called from:
> > 1. Threaded interrupt handlers.
> > 2. Work queues.
> > 
> > Replace the spin lock with mutex and update the documentation of this
> > function.
> 
> No. You've done it in the opposite way.
> 
> update_state is often called in a interrupt handler that cannot sleep.
> 
> For the context you've mentioned, we'd need workqueue invoked by
> _call_per_cable or notifier callback.

I'm still not convinced that calling raw_notifier_chain under spin lock
in extcon_update_state() is safe. I think it is not safe because current
design implies silently that all notifiers registered with
extcon_register_notifier() must not sleep.

_call_per_cable() is just one of ways to trigger this issue.

Best regards,
Krzysztof

[RFC PATCH] extcon: Fix sleeping in atomic context

[Krzysztof Kozlowski]

Hi,

When kernel was built with charger manager and extcon, I encountered
a sleep in atomic context after connecting USB cable to the device.

I found this on internal kernel 3.16 on Exynos3250 development board.
Unfortunately I could not reproduce it on mainline kernel because
none of my boards were able to bring up full charging stack
(fuel gauge, charger, charger-manager, extcon).

However, after looking at source code, this should happen also on
mainline kernel.

The fix for bug is simple - use mutexes instead of spin locks. It was
tested on 3.16 but *should* work on mainline.

My questions/request for comments are:
1. Could someone verify this on mainline (test it)?
2. Should I mark it cc-stable? I think yes *but* the patch changes
   few extcon functions from non-sleeping to might_sleep.
   On mainline these affected functions are not used in atomic
   contexts but I am not sure about older stable kernels.
   Applying this to older kernel could lead to another sleep-in-atomic
   issue...


Any comments?

Best regards,
Krzysztof

Krzysztof Kozlowski (1):
  extcon: Fix sleeping in atomic context after connecting USB cable

 drivers/extcon/extcon-class.c | 15 ++++++++-------
 include/linux/extcon.h        |  3 ++-
 2 files changed, 10 insertions(+), 8 deletions(-)

-- 
1.9.1

[RFC PATCH] extcon: Fix sleeping in atomic context after connecting USB cable

[Krzysztof Kozlowski]

Kernel built with extcon and charger-manager.

After connecting the USB cable sleeping function was called from atomic
context:
[   63.328648] BUG: sleeping function called from invalid context at kernel/locking/mutex.c:586
[   63.331449] in_atomic(): 1, irqs_disabled(): 128, pid: 906, name: kworker/1:2
[   63.338562] INFO: lockdep is turned off.
[   63.342468] irq event stamp: 0
[   63.345507] hardirqs last  enabled at (0): [<  (null)>]   (null)
[   63.351495] hardirqs last disabled at (0): [<c00246c8>] copy_process.part.46+0x4a0/0x1684
[   63.359654] softirqs last  enabled at (0): [<c00246c8>] copy_process.part.46+0x4a0/0x1684
[   63.367813] softirqs last disabled at (0): [<  (null)>]   (null)
[   63.373802] Preemption disabled at:[<  (null)>]   (null)
[   63.379097]
[   63.380581] CPU: 1 PID: 906 Comm: kworker/1:2 Not tainted 3.16.2-00822-g691a2ac4e574 #954
[   63.388743] Workqueue: events max14577_muic_irq_work
[   63.393707] [<c00167a4>] (unwind_backtrace) from [<c0012c08>] (show_stack+0x10/0x14)
[   63.401422] [<c0012c08>] (show_stack) from [<c06232ec>] (dump_stack+0x70/0xbc)
[   63.408625] [<c06232ec>] (dump_stack) from [<c0629d0c>] (mutex_lock_nested+0x24/0x410)
[   63.416525] [<c0629d0c>] (mutex_lock_nested) from [<c0354474>] (regmap_read+0x30/0x60)
[   63.424424] [<c0354474>] (regmap_read) from [<c0406b84>] (max14577_charger_get_property+0x1e4/0x250)
[   63.433535] [<c0406b84>] (max14577_charger_get_property) from [<c0403834>] (is_ext_pwr_online+0x30/0x6c)
[   63.442994] [<c0403834>] (is_ext_pwr_online) from [<c0404a54>] (charger_extcon_notifier+0x40/0x70)
[   63.451934] [<c0404a54>] (charger_extcon_notifier) from [<c044cbe4>] (_call_per_cable+0x40/0x4c)
[   63.460704] [<c044cbe4>] (_call_per_cable) from [<c0051560>] (notifier_call_chain+0x64/0x128)
[   63.469209] [<c0051560>] (notifier_call_chain) from [<c0051640>] (raw_notifier_call_chain+0x18/0x20)
[   63.478321] [<c0051640>] (raw_notifier_call_chain) from [<c044d0fc>] (extcon_update_state+0xa8/0x204)
[   63.487522] [<c044d0fc>] (extcon_update_state) from [<c044e5f8>] (max14577_muic_chg_handler+0xdc/0x180)
[   63.496897] [<c044e5f8>] (max14577_muic_chg_handler) from [<c044e924>] (max14577_muic_irq_work+0x7c/0xd8)
[   63.506445] [<c044e924>] (max14577_muic_irq_work) from [<c004480c>] (process_one_work+0x198/0x66c)
[   63.515385] [<c004480c>] (process_one_work) from [<c0044d4c>] (worker_thread+0x38/0x564)
[   63.523455] [<c0044d4c>] (worker_thread) from [<c004c4ec>] (kthread+0xcc/0xe8)
[   63.530661] [<c004c4ec>] (kthread) from [<c000f1f8>] (ret_from_fork+0x14/0x3c)
[   63.543926] charger_manager: Set current limit of CHARGER : 450000uA ~ 450000uA
[   63.548870] extcon-port jack: jack event CHGDET=usb
[   63.550592] extcon-port jack: jack event CHGDET=charger
[   64.188607] charger-manager charger-manager@0: Failed to get battery temperature
[   64.200684] charger-manager charger-manager@0: CHARGING

The first sleeping function is is_ext_pwr_online()
(drivers/power/charger-manager.c). The atomic context initiating the
flow is set up in extcon_update_state() (drivers/extcon/extcon-class.c).

The extcon_update_state() uses spin locks which are not necessary
because the function is not called from interrupt service routines.
Instead, the extcon_update_state() is called from:
1. Threaded interrupt handlers.
2. Work queues.

Replace the spin lock with mutex and update the documentation of this
function.

Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
---
 drivers/extcon/extcon-class.c | 15 ++++++++-------
 include/linux/extcon.h        |  3 ++-
 2 files changed, 10 insertions(+), 8 deletions(-)

diff --git a/drivers/extcon/extcon-class.c b/drivers/extcon/extcon-class.c
index 4c2f2c543bb7..7485600baeff 100644
--- a/drivers/extcon/extcon-class.c
+++ b/drivers/extcon/extcon-class.c
@@ -201,6 +201,8 @@ static ssize_t cable_state_show(struct device *dev,
  *
  * Note that the notifier provides which bits are changed in the state
  * variable with the val parameter (second) to the callback.
+ *
+ * The function may sleep thus may not be called from atomic context.
  */
 int extcon_update_state(struct extcon_dev *edev, u32 mask, u32 state)
 {
@@ -210,16 +212,15 @@ int extcon_update_state(struct extcon_dev *edev, u32 mask, u32 state)
 	char *envp[3];
 	int env_offset = 0;
 	int length;
-	unsigned long flags;
 
-	spin_lock_irqsave(&edev->lock, flags);
+	mutex_lock(&edev->lock);
 
 	if (edev->state != ((edev->state & ~mask) | (state & mask))) {
 		u32 old_state = edev->state;
 
 		if (check_mutually_exclusive(edev, (edev->state & ~mask) |
 						   (state & mask))) {
-			spin_unlock_irqrestore(&edev->lock, flags);
+			mutex_unlock(&edev->lock);
 			return -EPERM;
 		}
 
@@ -248,20 +249,20 @@ int extcon_update_state(struct extcon_dev *edev, u32 mask, u32 state)
 			}
 			envp[env_offset] = NULL;
 			/* Unlock early before uevent */
-			spin_unlock_irqrestore(&edev->lock, flags);
+			mutex_unlock(&edev->lock);
 
 			kobject_uevent_env(&edev->dev.kobj, KOBJ_CHANGE, envp);
 			free_page((unsigned long)prop_buf);
 		} else {
 			/* Unlock early before uevent */
-			spin_unlock_irqrestore(&edev->lock, flags);
+			mutex_unlock(&edev->lock);
 
 			dev_err(&edev->dev, "out of memory in extcon_set_state\n");
 			kobject_uevent(&edev->dev.kobj, KOBJ_CHANGE);
 		}
 	} else {
 		/* No changes */
-		spin_unlock_irqrestore(&edev->lock, flags);
+		mutex_unlock(&edev->lock);
 	}
 
 	return 0;
@@ -834,7 +835,7 @@ int extcon_dev_register(struct extcon_dev *edev)
 		ret = class_compat_create_link(switch_class, &edev->dev, NULL);
 #endif /* CONFIG_ANDROID */
 
-	spin_lock_init(&edev->lock);
+	mutex_init(&edev->lock);
 
 	RAW_INIT_NOTIFIER_HEAD(&edev->nh);
 
diff --git a/include/linux/extcon.h b/include/linux/extcon.h
index 36f49c405dfb..166f42cecd5a 100644
--- a/include/linux/extcon.h
+++ b/include/linux/extcon.h
@@ -26,6 +26,7 @@
 #include <linux/device.h>
 #include <linux/notifier.h>
 #include <linux/sysfs.h>
+#include <linux/mutex.h>
 
 #define SUPPORTED_CABLE_MAX	32
 #define CABLE_NAME_MAX		30
@@ -125,7 +126,7 @@ struct extcon_dev {
 	struct raw_notifier_head nh;
 	struct list_head entry;
 	int max_supported;
-	spinlock_t lock;	/* could be called by irq handler */
+	struct mutex lock;
 	u32 state;
 
 	/* /sys/class/extcon/.../cable.n/... */
-- 
1.9.1