usb otg: use atomic notifier instead of blocking notifier

PATCH In review Yang Ruirui 6 participants 11 messages Jan 19, 2011 – Jan 21, 2011 USB

lore

Share & tools

lore

Yang Ruirui

author

2011-01-19 09:12 UTC #0 lore raw

PATCH 1/1 usb otg: use atomic notifier instead of blocking notifier

following bug happens with meego 2.6.35 kernel on nokia n900:

[   28.693756] BUG: sleeping function called from invalid context at kernel/rwsem.c:21
[   28.693786] in_atomic(): 1, irqs_disabled(): 128, pid: 706, name: udisks-part-id
[   28.693817] 1 lock held by udisks-part-id/706:
[   28.693817]  #0:  (&(&musb->lock)->rlock){-.-...}, at: [<c0278590>] musb_g_disconnect+0x90/0x148
[   28.693908] irq event stamp: 1169
[   28.693908] hardirqs last  enabled at (1168): [<c00c4b3c>] kmem_cache_alloc+0xd0/0x128
[   28.693969] hardirqs last disabled at (1169): [<c002da34>] __irq_svc+0x34/0xb4
[   28.694000] softirqs last  enabled at (0): [<c0054bcc>] copy_process+0x304/0xe18
[   28.694030] softirqs last disabled at (0): [<(null)>] (null)
[   28.694091] [<c00326a0>] (unwind_backtrace+0x0/0xec) from [<c037e6b0>] (down_read+0x20/0x5c)
[   28.694152] [<c037e6b0>] (down_read+0x20/0x5c) from [<c0070770>] (__blocking_notifier_call_chain+0x2c/0x5c)
[   28.694183] [<c0070770>] (__blocking_notifier_call_chain+0x2c/0x5c) from [<c00707b4>] (blocking_notifier_call_chain+0x14/0x18)
[   28.694213] [<c00707b4>] (blocking_notifier_call_chain+0x14/0x18) from [<c02784d8>] (musb_gadget_vbus_draw+0x38/0x60)
[   28.694274] [<c02784d8>] (musb_gadget_vbus_draw+0x38/0x60) from [<c0276d10>] (musb_interrupt+0xb08/0xcb0)
[   28.694305] [<c0276d10>] (musb_interrupt+0xb08/0xcb0) from [<c0276f08>] (generic_interrupt+0x50/0x68)
[   28.694335] [<c0276f08>] (generic_interrupt+0x50/0x68) from [<c008fcf4>] (handle_IRQ_event+0x24/0xe8)
[   28.694396] [<c008fcf4>] (handle_IRQ_event+0x24/0xe8) from [<c0091924>] (handle_level_irq+0xac/0x128)
[   28.694427] [<c0091924>] (handle_level_irq+0xac/0x128) from [<c002d070>] (asm_do_IRQ+0x70/0x90)
[   28.694458] [<c002d070>] (asm_do_IRQ+0x70/0x90) from [<c002da4c>] (__irq_svc+0x4c/0xb4)
[   28.694488] Exception stack(0xcd835ed0 to 0xcd835f18)
[   28.694519] 5ec0:                                     cfc0a240 c8282000 0000006b 0000006b
[   28.694549] 5ee0: cfc0a240 0000041a 00001000 00000000 c8282000 cd834000 c8282000 be80c464
[   28.694580] 5f00: 00000c1e cd835f18 c00c333c c00c297c 80000013 ffffffff
[   28.694610] [<c002da4c>] (__irq_svc+0x4c/0xb4) from [<c00c297c>] (check_poison_obj+0x24/0x194)
[   28.694641] [<c00c297c>] (check_poison_obj+0x24/0x194) from [<c00c333c>] (cache_alloc_debugcheck_after+0x28/0x188)
[   28.694671] [<c00c333c>] (cache_alloc_debugcheck_after+0x28/0x188) from [<c00c4b54>] (kmem_cache_alloc+0xe8/0x128)
[   28.694732] [<c00c4b54>] (kmem_cache_alloc+0xe8/0x128) from [<c00d79dc>] (getname+0x18/0xcc)
[   28.694763] [<c00d79dc>] (getname+0x18/0xcc) from [<c00cbca8>] (do_sys_open+0x18/0x10c)
[   28.694793] [<c00cbca8>] (do_sys_open+0x18/0x10c) from [<c002df40>] (ret_fast_syscall+0x0/0x3c)
[   28.694854] 
[   28.694854] =================================
[   28.709014] [ INFO: inconsistent lock state ]
[   28.717254] 2.6.35.96.5-adaptation-n900 #1
[   28.725128] ---------------------------------
[   28.733123] inconsistent {HARDIRQ-ON-W} -> {IN-HARDIRQ-R} usage.
[   28.742980] udisks-part-id/706 [HC1[1]:SC0[0]:HE0:SE1] takes:
[   28.752441]  (&(&twl->otg.notifier)->rwsem){+-+...}, at: [<c0070770>] __blocking_notifier_call_chain+0x2c/0x5c
[   28.769836] {HARDIRQ-ON-W} state was registered at:
[   28.778320]   [<c007c1dc>] __lock_acquire+0x618/0x1730
[   28.787109]   [<c007d354>] lock_acquire+0x60/0x74
[   28.795227]   [<c037e67c>] down_write+0x48/0x5c
[   28.803131]   [<c00708a8>] blocking_notifier_chain_register+0x30/0x54
[   28.812896]   [<bf0da9f8>] isp1704_charger_probe+0x268/0x39c [isp1704_charger]
[   28.823394]   [<c0239410>] platform_drv_probe+0x18/0x1c
[   28.831787]   [<c02385c8>] driver_probe_device+0xa8/0x158
[   28.840332]   [<c02386e0>] __driver_attach+0x68/0x8c
[   28.848510]   [<c0237e68>] bus_for_each_dev+0x44/0x74
[   28.856689]   [<c02377c8>] bus_add_driver+0x9c/0x20c
[   28.864746]   [<c02389b0>] driver_register+0xa8/0x138
[   28.872955]   [<c002d340>] do_one_initcall+0x58/0x1ac
[   28.881134]   [<c0085cdc>] sys_init_module+0x90/0x1b0
[   28.889343]   [<c002df40>] ret_fast_syscall+0x0/0x3c
[   28.897491] irq event stamp: 1169
[   28.904083] hardirqs last  enabled at (1168): [<c00c4b3c>] kmem_cache_alloc+0xd0/0x128
[   28.915557] hardirqs last disabled at (1169): [<c002da34>] __irq_svc+0x34/0xb4
[   28.926239] softirqs last  enabled at (0): [<c0054bcc>] copy_process+0x304/0xe18
[   28.937194] softirqs last disabled at (0): [<(null)>] (null)
[   28.946258] 
[   28.946258] other info that might help us debug this:
[   28.959381] 1 lock held by udisks-part-id/706:
[   28.967163]  #0:  (&(&musb->lock)->rlock){-.-...}, at: [<c0278590>] musb_g_disconnect+0x90/0x148
[   28.979705] 
[   28.979705] stack backtrace:
[   28.990997] [<c00326a0>] (unwind_backtrace+0x0/0xec) from [<c007a39c>] (print_usage_bug+0x170/0x1b4)
[   29.007293] [<c007a39c>] (print_usage_bug+0x170/0x1b4) from [<c007a738>] (mark_lock+0x358/0x628)
[   29.019836] [<c007a738>] (mark_lock+0x358/0x628) from [<c007c118>] (__lock_acquire+0x554/0x1730)
[   29.032409] [<c007c118>] (__lock_acquire+0x554/0x1730) from [<c007d354>] (lock_acquire+0x60/0x74)
[   29.045166] [<c007d354>] (lock_acquire+0x60/0x74) from [<c037e6d8>] (down_read+0x48/0x5c)
[   29.057250] [<c037e6d8>] (down_read+0x48/0x5c) from [<c0070770>] (__blocking_notifier_call_chain+0x2c/0x5c)
[   29.074920] [<c0070770>] (__blocking_notifier_call_chain+0x2c/0x5c) from [<c00707b4>] (blocking_notifier_call_chain+0x14/0x18)
[   29.094573] [<c00707b4>] (blocking_notifier_call_chain+0x14/0x18) from [<c02784d8>] (musb_gadget_vbus_draw+0x38/0x60)
[   29.113891] [<c02784d8>] (musb_gadget_vbus_draw+0x38/0x60) from [<c0276d10>] (musb_interrupt+0xb08/0xcb0)
[   29.132629] [<c0276d10>] (musb_interrupt+0xb08/0xcb0) from [<c0276f08>] (generic_interrupt+0x50/0x68)
[   29.151580] [<c0276f08>] (generic_interrupt+0x50/0x68) from [<c008fcf4>] (handle_IRQ_event+0x24/0xe8)
[   29.171081] [<c008fcf4>] (handle_IRQ_event+0x24/0xe8) from [<c0091924>] (handle_level_irq+0xac/0x128)
[   29.190948] [<c0091924>] (handle_level_irq+0xac/0x128) from [<c002d070>] (asm_do_IRQ+0x70/0x90)
[   29.205291] [<c002d070>] (asm_do_IRQ+0x70/0x90) from [<c002da4c>] (__irq_svc+0x4c/0xb4)
[   29.218963] Exception stack(0xcd835ed0 to 0xcd835f18)
[   29.229644] 5ec0:                                     cfc0a240 c8282000 0000006b 0000006b
[   29.243591] 5ee0: cfc0a240 0000041a 00001000 00000000 c8282000 cd834000 c8282000 be80c464
[   29.257629] 5f00: 00000c1e cd835f18 c00c333c c00c297c 80000013 ffffffff
[   29.270111] [<c002da4c>] (__irq_svc+0x4c/0xb4) from [<c00c297c>] (check_poison_obj+0x24/0x194)
[   29.284637] [<c00c297c>] (check_poison_obj+0x24/0x194) from [<c00c333c>] (cache_alloc_debugcheck_after+0x28/0x188)
[   29.306732] [<c00c333c>] (cache_alloc_debugcheck_after+0x28/0x188) from [<c00c4b54>] (kmem_cache_alloc+0xe8/0x128)
[   29.329071] [<c00c4b54>] (kmem_cache_alloc+0xe8/0x128) from [<c00d79dc>] (getname+0x18/0xcc)
[   29.343597] [<c00d79dc>] (getname+0x18/0xcc) from [<c00cbca8>] (do_sys_open+0x18/0x10c)
[   29.357696] [<c00cbca8>] (do_sys_open+0x18/0x10c) from [<c002df40>] (ret_fast_syscall+0x0/0x3c)

Actually the blocking notifier chain runs in process context, so not fit for use here.

For mainline kernel there's such issue as well. Here fix this problem by changing to use atomic_notifier.

Signed-off-by Yang Ruirui <[email protected]>

---
 drivers/usb/otg/ab8500-usb.c  |    6 +++---
 drivers/usb/otg/twl4030-usb.c |    6 +++---
 drivers/usb/otg/twl6030-usb.c |    6 +++---
 include/linux/usb/otg.h       |    6 +++---
 4 files changed, 12 insertions(+), 12 deletions(-)

include/linux/usb/otg.h +3 -3

@@ -74,7 +74,7 @@ struct otg_transceiver {
 	void __iomem			*io_priv;
 
 	/* for notification of usb_xceiv_events */
-	struct blocking_notifier_head	notifier;
+	struct atomic_notifier_head	notifier;
 
 	/* to pass extra port status to the root hub */
 	u16			port_status;

@@ -234,13 +234,13 @@ otg_start_srp(struct otg_transceiver *ot
 static inline int
 otg_register_notifier(struct otg_transceiver *otg, struct notifier_block *nb)
 {
-	return blocking_notifier_chain_register(&otg->notifier, nb);
+	return atomic_notifier_chain_register(&otg->notifier, nb);
 }
 
 static inline void
 otg_unregister_notifier(struct otg_transceiver *otg, struct notifier_block *nb)
 {
-	blocking_notifier_chain_unregister(&otg->notifier, nb);
+	atomic_notifier_chain_unregister(&otg->notifier, nb);
 }
 
 /* for OTG controller drivers (and maybe other stuff) */

drivers/usb/otg/ab8500-usb.c +3 -3

@@ -212,7 +212,7 @@ static int ab8500_usb_link_status_update
 		break;
 	}
 
-	blocking_notifier_call_chain(&ab->otg.notifier, event, v);
+	atomic_notifier_call_chain(&ab->otg.notifier, event, v);
 
 	return 0;
 }

@@ -281,7 +281,7 @@ static int ab8500_usb_set_power(struct o
 	ab->vbus_draw = mA;
 
 	if (mA)
-		blocking_notifier_call_chain(&ab->otg.notifier,
+		atomic_notifier_call_chain(&ab->otg.notifier,
 				USB_EVENT_ENUMERATED, ab->otg.gadget);
 	return 0;
 }

@@ -500,7 +500,7 @@ static int __devinit ab8500_usb_probe(st
 
 	platform_set_drvdata(pdev, ab);
 
-	BLOCKING_INIT_NOTIFIER_HEAD(&ab->otg.notifier);
+	ATOMIC_INIT_NOTIFIER_HEAD(&ab->otg.notifier);
 
 	/* v1: Wait for link status to become stable.
 	 * all: Updates form set_host and set_peripheral as they are atomic.

drivers/usb/otg/twl4030-usb.c +3 -3

@@ -512,7 +512,7 @@ static irqreturn_t twl4030_usb_irq(int i
 		else
 			twl4030_phy_resume(twl);
 
-		blocking_notifier_call_chain(&twl->otg.notifier, status,
+		atomic_notifier_call_chain(&twl->otg.notifier, status,
 				twl->otg.gadget);
 	}
 	sysfs_notify(&twl->dev->kobj, NULL, "vbus");

@@ -534,7 +534,7 @@ static void twl4030_usb_phy_init(struct 
 			twl->asleep = 0;
 		}
 
-		blocking_notifier_call_chain(&twl->otg.notifier, status,
+		atomic_notifier_call_chain(&twl->otg.notifier, status,
 				twl->otg.gadget);
 	}
 	sysfs_notify(&twl->dev->kobj, NULL, "vbus");

@@ -623,7 +623,7 @@ static int __devinit twl4030_usb_probe(s
 	if (device_create_file(&pdev->dev, &dev_attr_vbus))
 		dev_warn(&pdev->dev, "could not create sysfs file\n");
 
-	BLOCKING_INIT_NOTIFIER_HEAD(&twl->otg.notifier);
+	ATOMIC_INIT_NOTIFIER_HEAD(&twl->otg.notifier);
 
 	/* Our job is to use irqs and status from the power module
 	 * to keep the transceiver disabled when nothing's connected.

drivers/usb/otg/twl6030-usb.c +3 -3

@@ -269,7 +269,7 @@ static irqreturn_t twl6030_usb_irq(int i
 		}
 		if (status >= 0) {
 			twl->linkstat = status;
-			blocking_notifier_call_chain(&twl->otg.notifier,
+			atomic_notifier_call_chain(&twl->otg.notifier,
 						status, twl->otg.gadget);
 		}
 	}

@@ -294,7 +294,7 @@ static irqreturn_t twl6030_usbotg_irq(in
 		status = USB_EVENT_ID;
 		twl->otg.default_a = true;
 		twl->otg.state = OTG_STATE_A_IDLE;
-		blocking_notifier_call_chain(&twl->otg.notifier, status,
+		atomic_notifier_call_chain(&twl->otg.notifier, status,
 							twl->otg.gadget);
 	} else  {
 		twl6030_writeb(twl, TWL_MODULE_USB, USB_ID_INT_EN_HI_CLR,

@@ -411,7 +411,7 @@ static int __devinit twl6030_usb_probe(s
 	if (device_create_file(&pdev->dev, &dev_attr_vbus))
 		dev_warn(&pdev->dev, "could not create sysfs file\n");
 
-	BLOCKING_INIT_NOTIFIER_HEAD(&twl->otg.notifier);
+	ATOMIC_INIT_NOTIFIER_HEAD(&twl->otg.notifier);
 
 	twl->irq_enabled = true;
 	status = request_threaded_irq(twl->irq1, NULL, twl6030_usbotg_irq,

Heikki Krogerus

2011-01-19 10:34 UTC #1 lore raw

Quoted 17 lines · ext Yang Ruirui

include/linux/usb/otg.h 2011-01-19 16:38:06.649546989 +0800 renamed from include/linux/usb/otg.h 2011-01-17 09:39:11.000000000 +0800 +2 -2

@@ -74,7 +74,7 @@ struct otg_transceiver {
        void __iomem                    *io_priv;
 
        /* for notification of usb_xceiv_events */
-       struct blocking_notifier_head   notifier;
+       struct atomic_notifier_head     notifier;
 
        /* to pass extra port status to the root hub */
        u16                     port_status;

@@ -234,13 +234,13 @@ otg_start_srp(struct otg_transceiver *ot
 static inline int
 otg_register_notifier(struct otg_transceiver *otg, struct notifier_block *nb)
 {
-       return blocking_notifier_chain_register(&otg->notifier, nb);
+       return atomic_notifier_chain_register(&otg->notifier, nb);

include/linux/usb/otg.h 2011-01-19 16:38:06.649546989 +0800 otg_start_srp(struct otg_transceiver *ot · line 246

Quoted 10 lines

include/linux/usb/otg.h 2011-01-19 16:38:06.649546989 +0800 renamed from include/linux/usb/otg.h 2011-01-17 09:39:11.000000000 +0800 +1 -1

 }
 
 static inline void
 otg_unregister_notifier(struct otg_transceiver *otg, struct notifier_block *nb)
 {
-       blocking_notifier_chain_unregister(&otg->notifier, nb);
+       atomic_notifier_chain_unregister(&otg->notifier, nb);
 }
 
 /* for OTG controller drivers (and maybe other stuff) */

Is drivers/power/twl4030_charger.c ready for atomic notifiers?

Signature · 1 line

heikki

Grazvydas Ignotas

2011-01-21 15:34 UTC #2 lore raw

Quoted 27 lines · Heikki Krogerus

Quoted 26 lines · ext Yang Ruirui
include/linux/usb/otg.h 2011-01-19 16:38:06.649546989 +0800 renamed from include/linux/usb/otg.h 2011-01-17 09:39:11.000000000 +0800 +1 -1
@@ -74,7 +74,7 @@ struct otg_transceiver {
         void __iomem                    *io_priv;
 
         /* for notification of usb_xceiv_events */
-       struct blocking_notifier_head   notifier;
+       struct atomic_notifier_head     notifier;
 
         /* to pass extra port status to the root hub */
         u16                     port_status;
@@ -234,13 +234,13 @@ otg_start_srp(struct otg_transceiver *ot
static inline int
otg_register_notifier(struct otg_transceiver *otg, struct notifier_block *nb) {
- return blocking_notifier_chain_register(&otg->notifier, nb);
+ return atomic_notifier_chain_register(&otg->notifier, nb); }
static inline void
otg_unregister_notifier(struct otg_transceiver *otg, struct notifier_block *nb) {
- blocking_notifier_chain_unregister(&otg->notifier, nb);
+ atomic_notifier_chain_unregister(&otg->notifier, nb);
}

Quoted 3 lines

Quoted 1 line
/* for OTG controller drivers (and maybe other stuff) */
Is drivers/power/twl4030_charger.c ready for atomic notifiers?

No it is not, it's doing some i2c stuff. With twl4030 it's called from threaded_irq in twl4030-usb instead of musb core and is working fine..

Felipe Balbi

2011-01-19 10:47 UTC #3 lore raw

In reply to Yang Ruirui

hi,

Quoted 2 lines · Yang Ruirui

following bug happens with meego 2.6.35 kernel on nokia n900:

<snip>

Quoted 10 lines

Actually the blocking notifier chain runs in process context, so not fit for use here.
For mainline kernel there's such issue as well. Here fix this problem by changing to use atomic_notifier.
Signed-off-by Yang Ruirui <[email protected]>
---
 drivers/usb/otg/ab8500-usb.c  |    6 +++---
 drivers/usb/otg/twl4030-usb.c |    6 +++---
 drivers/usb/otg/twl6030-usb.c |    6 +++---

you're missing the other transceivers:
drivers/usb/otg/langwell_otg.c
drivers/usb/otg/msm72k_otg.c
drivers/usb/otg/gpio_vbus.c
drivers/usb/otg/nop-usb-xceiv.c
drivers/usb/otg/ulpi.c
drivers/usb/otg/isp1301_omap.c

Signature · 1 line

balbi

Yang Ruirui

author

2011-01-20 01:58 UTC #4 lore raw

Quoted 19 lines · Felipe Balbi

hi,
Quoted 2 lines · Yang Ruirui
following bug happens with meego 2.6.35 kernel on nokia n900:
<snip>
Quoted 10 lines
Actually the blocking notifier chain runs in process context, so not fit for use here.
For mainline kernel there's such issue as well. Here fix this problem by changing to use atomic_notifier.
Signed-off-by Yang Ruirui <[email protected]>
---
 drivers/usb/otg/ab8500-usb.c  |    6 +++---
 drivers/usb/otg/twl4030-usb.c |    6 +++---
 drivers/usb/otg/twl6030-usb.c |    6 +++---

Quoted 7 lines

you're missing the other transceivers:
drivers/usb/otg/langwell_otg.c
drivers/usb/otg/msm72k_otg.c
drivers/usb/otg/gpio_vbus.c
drivers/usb/otg/nop-usb-xceiv.c
drivers/usb/otg/ulpi.c
drivers/usb/otg/isp1301_omap.c

Hi, thanks

Actually I does not found where notifiers are used in these files except langwell_otg

For langwell_otg.c there's a wrapper struct named "intel mid_otg_xceiv" in include/linux/usb/intel_mid_otg.h, it use an atomic notifier for interrupt context.

So I think we'd better write another patch to make it use general otg_transceiver for notifier or just keep current code. What's your opinion?

Quoted 3 lines

Signature · 2 lines
-- 
balbi

Felipe Balbi

2011-01-20 04:01 UTC #5 lore raw

Quoted 9 lines · Yang Ruirui

Actually I does not found where notifiers are used in these files except langwell_otg
For langwell_otg.c there's a wrapper struct named "intel mid_otg_xceiv" in include/linux/usb/intel_mid_otg.h, it use an atomic notifier for interrupt context.
So I think we'd better write another patch to make it use general otg_transceiver for notifier or just keep current code. What's your opinion?

I'm not the maintainer but I would rather see it using the notifier on the otg_transceiver structure, and later add the support for the other transceivers too.

Signature · 1 line

balbi

Ming Lei

2011-01-19 15:19 UTC #6 lore raw

In reply to Yang Ruirui

Hi Yang,

2011/1/19 Yang Ruirui <[email protected]>:

Quoted 8 lines

[ 28.990997] [<c00326a0>] (unwind_backtrace+0x0/0xec) from [<c007a39c>] (print_usage_bug+0x170/0x1b4)
[ 29.007293] [<c007a39c>] (print_usage_bug+0x170/0x1b4) from [<c007a738>] (mark_lock+0x358/0x628)
[ 29.019836] [<c007a738>] (mark_lock+0x358/0x628) from [<c007c118>] (__lock_acquire+0x554/0x1730)
[ 29.032409] [<c007c118>] (__lock_acquire+0x554/0x1730) from [<c007d354>] (lock_acquire+0x60/0x74)
[ 29.045166] [<c007d354>] (lock_acquire+0x60/0x74) from [<c037e6d8>] (down_read+0x48/0x5c)
[ 29.057250] [<c037e6d8>] (down_read+0x48/0x5c) from [<c0070770>] (__blocking_notifier_call_chain+0x2c/0x5c) [ 29.074920] [<c0070770>] (__blocking_notifier_call_chain+0x2c/0x5c) from [<c00707b4>] (blocking_notifier_call_chain+0x14/0x18) [ 29.094573] [<c00707b4>] (blocking_notifier_call_chain+0x14/0x18) from [<c02784d8>] (musb_gadget_vbus_draw+0x38/0x60)

I am a little confused, why is blocking_notifier_call_chain called from musb_gadget_vbus_draw? See source code of musb_gadget_vbus_draw:

1587 static int musb_gadget_vbus_draw(struct usb_gadget *gadget, unsigned mA) 1588 {
1589 struct musb *musb = gadget_to_musb(gadget);
1590
1591 if (!musb->xceiv->set_power)
1592 return -EOPNOTSUPP;
1593 return otg_set_power(musb->xceiv, mA);
1594 }

Quoted 5 lines

Actually the blocking notifier chain runs in process context, so not fit for use here.
For mainline kernel there's such issue as well. Here fix this problem by changing to use atomic_notifier.

Are you sure the issue can be reproduced in 2.6.37 or -next tree?

In mainline code(2.6.37), blocking_notifier_call_chain is only called in threaded irq handler of otg driver wrt. musb driver, so seems enough.

thanks,

Signature · 1 line

Lei Ming

Felipe Balbi

2011-01-19 15:31 UTC #7 lore raw

Hi,

Quoted 13 lines · Ming Lei

2011/1/19 Yang Ruirui <[email protected]>:
Quoted 8 lines
[ 28.990997] [<c00326a0>] (unwind_backtrace+0x0/0xec) from [<c007a39c>] (print_usage_bug+0x170/0x1b4)
[ 29.007293] [<c007a39c>] (print_usage_bug+0x170/0x1b4) from [<c007a738>] (mark_lock+0x358/0x628)
[ 29.019836] [<c007a738>] (mark_lock+0x358/0x628) from [<c007c118>] (__lock_acquire+0x554/0x1730)
[ 29.032409] [<c007c118>] (__lock_acquire+0x554/0x1730) from [<c007d354>] (lock_acquire+0x60/0x74)
[ 29.045166] [<c007d354>] (lock_acquire+0x60/0x74) from [<c037e6d8>] (down_read+0x48/0x5c)
[ 29.057250] [<c037e6d8>] (down_read+0x48/0x5c) from [<c0070770>] (__blocking_notifier_call_chain+0x2c/0x5c) [ 29.074920] [<c0070770>] (__blocking_notifier_call_chain+0x2c/0x5c) from [<c00707b4>] (blocking_notifier_call_chain+0x14/0x18) [ 29.094573] [<c00707b4>] (blocking_notifier_call_chain+0x14/0x18) from [<c02784d8>] (musb_gadget_vbus_draw+0x38/0x60)
I am a little confused, why is blocking_notifier_call_chain called from musb_gadget_vbus_draw? See source code of musb_gadget_vbus_draw:

Quoted 8 lines

1587 static int musb_gadget_vbus_draw(struct usb_gadget *gadget, unsigned mA) 1588 {
1589 struct musb *musb = gadget_to_musb(gadget);
1590
1591 if (!musb->xceiv->set_power)
1592 return -EOPNOTSUPP;
1593 return otg_set_power(musb->xceiv, mA);
1594 }

that's easy to guess since commit log says it's a problem on Meego kernel.

Quoted 4 lines

Are you sure the issue can be reproduced in 2.6.37 or -next tree?
In mainline code(2.6.37), blocking_notifier_call_chain is only called in threaded irq handler of otg driver wrt. musb driver, so seems enough.

true, but seems like Nokia is using to notify gadget states, which makes a lot of sense when you start thinking about things such as battery charging.

Signature · 1 line

balbi

Ming Lei

2011-01-19 15:39 UTC #8 lore raw

Hi,

2011/1/19 Felipe Balbi <[email protected]>:

Quoted 2 lines

that's easy to guess since commit log says it's a problem on Meego kernel.

OK, it is a specific issue on Meego kernel, so it is better to add "Meego " in the patch title.

Quoted 9 lines

Quoted 5 lines
Are you sure the issue can be reproduced in 2.6.37 or -next tree?
In mainline code(2.6.37), blocking_notifier_call_chain is only called in threaded irq handler of otg driver wrt. musb driver, so seems enough.
true, but seems like Nokia is using to notify gadget states, which makes a lot of sense when you start thinking about things such as battery charging.

Please go ahead to submit and merge it into mainline first if it makes sense, :-)

thanks,

Signature · 1 line

Lei Ming

Felipe Balbi

2011-01-19 16:01 UTC #9 lore raw

Quoted 16 lines · Ming Lei

Hi,
2011/1/19 Felipe Balbi <[email protected]>:
Quoted 2 lines
that's easy to guess since commit log says it's a problem on Meego kernel.
OK, it is a specific issue on Meego kernel, so it is better to add "Meego " in the patch title.
Quoted 6 lines
Quoted 5 lines
Are you sure the issue can be reproduced in 2.6.37 or -next tree?
In mainline code(2.6.37), blocking_notifier_call_chain is only called in threaded irq handler of otg driver wrt. musb driver, so seems enough.

Quoted 6 lines

Quoted 3 lines
true, but seems like Nokia is using to notify gadget states, which makes a lot of sense when you start thinking about things such as battery charging.
Please go ahead to submit and merge it into mainline first if it makes sense, :-)

isn't it what his patch is already intending to do ? Or do you want them to send the initial buggy version and only afterwards send the bugfix ? A bit weird decision wouldn't that be ?

Signature · 1 line

balbi

Mian Yousaf Kaukab

Acked-by on v1 1/1

2011-01-20 15:52 UTC #10 lore raw

In reply to Yang Ruirui

Quoted 90 lines · Yang Ruirui

following bug happens with meego 2.6.35 kernel on nokia n900:

[   28.693756] BUG: sleeping function called from invalid context at kernel/rwsem.c:21
[   28.693786] in_atomic(): 1, irqs_disabled(): 128, pid: 706, name: udisks-part-id
[   28.693817] 1 lock held by udisks-part-id/706:
[   28.693817]  #0:  (&(&musb->lock)->rlock){-.-...}, at: [<c0278590>] musb_g_disconnect+0x90/0x148
[   28.693908] irq event stamp: 1169
[   28.693908] hardirqs last  enabled at (1168): [<c00c4b3c>] kmem_cache_alloc+0xd0/0x128
[   28.693969] hardirqs last disabled at (1169): [<c002da34>] __irq_svc+0x34/0xb4
[   28.694000] softirqs last  enabled at (0): [<c0054bcc>] copy_process+0x304/0xe18
[   28.694030] softirqs last disabled at (0): [<(null)>] (null)
[   28.694091] [<c00326a0>] (unwind_backtrace+0x0/0xec) from [<c037e6b0>] (down_read+0x20/0x5c)
[   28.694152] [<c037e6b0>] (down_read+0x20/0x5c) from [<c0070770>] (__blocking_notifier_call_chain+0x2c/0x5c)
[   28.694183] [<c0070770>] (__blocking_notifier_call_chain+0x2c/0x5c) from [<c00707b4>] (blocking_notifier_call_chain+0x14/0x18)
[   28.694213] [<c00707b4>] (blocking_notifier_call_chain+0x14/0x18) from [<c02784d8>] (musb_gadget_vbus_draw+0x38/0x60)
[   28.694274] [<c02784d8>] (musb_gadget_vbus_draw+0x38/0x60) from [<c0276d10>] (musb_interrupt+0xb08/0xcb0)
[   28.694305] [<c0276d10>] (musb_interrupt+0xb08/0xcb0) from [<c0276f08>] (generic_interrupt+0x50/0x68)
[   28.694335] [<c0276f08>] (generic_interrupt+0x50/0x68) from [<c008fcf4>] (handle_IRQ_event+0x24/0xe8)
[   28.694396] [<c008fcf4>] (handle_IRQ_event+0x24/0xe8) from [<c0091924>] (handle_level_irq+0xac/0x128)
[   28.694427] [<c0091924>] (handle_level_irq+0xac/0x128) from [<c002d070>] (asm_do_IRQ+0x70/0x90)
[   28.694458] [<c002d070>] (asm_do_IRQ+0x70/0x90) from [<c002da4c>] (__irq_svc+0x4c/0xb4)
[   28.694488] Exception stack(0xcd835ed0 to 0xcd835f18)
[   28.694519] 5ec0:                                     cfc0a240 c8282000 0000006b 0000006b
[   28.694549] 5ee0: cfc0a240 0000041a 00001000 00000000 c8282000 cd834000 c8282000 be80c464
[   28.694580] 5f00: 00000c1e cd835f18 c00c333c c00c297c 80000013 ffffffff
[   28.694610] [<c002da4c>] (__irq_svc+0x4c/0xb4) from [<c00c297c>] (check_poison_obj+0x24/0x194)
[   28.694641] [<c00c297c>] (check_poison_obj+0x24/0x194) from [<c00c333c>] (cache_alloc_debugcheck_after+0x28/0x188)
[   28.694671] [<c00c333c>] (cache_alloc_debugcheck_after+0x28/0x188) from [<c00c4b54>] (kmem_cache_alloc+0xe8/0x128)
[   28.694732] [<c00c4b54>] (kmem_cache_alloc+0xe8/0x128) from [<c00d79dc>] (getname+0x18/0xcc)
[   28.694763] [<c00d79dc>] (getname+0x18/0xcc) from [<c00cbca8>] (do_sys_open+0x18/0x10c)
[   28.694793] [<c00cbca8>] (do_sys_open+0x18/0x10c) from [<c002df40>] (ret_fast_syscall+0x0/0x3c)
[   28.694854]
[   28.694854] =================================
[   28.709014] [ INFO: inconsistent lock state ]
[   28.717254] 2.6.35.96.5-adaptation-n900 #1
[   28.725128] ---------------------------------
[   28.733123] inconsistent {HARDIRQ-ON-W} ->  {IN-HARDIRQ-R} usage.
[   28.742980] udisks-part-id/706 [HC1[1]:SC0[0]:HE0:SE1] takes:
[   28.752441]  (&(&twl->otg.notifier)->rwsem){+-+...}, at: [<c0070770>] __blocking_notifier_call_chain+0x2c/0x5c
[   28.769836] {HARDIRQ-ON-W} state was registered at:
[   28.778320]   [<c007c1dc>] __lock_acquire+0x618/0x1730
[   28.787109]   [<c007d354>] lock_acquire+0x60/0x74
[   28.795227]   [<c037e67c>] down_write+0x48/0x5c
[   28.803131]   [<c00708a8>] blocking_notifier_chain_register+0x30/0x54
[   28.812896]   [<bf0da9f8>] isp1704_charger_probe+0x268/0x39c [isp1704_charger]
[   28.823394]   [<c0239410>] platform_drv_probe+0x18/0x1c
[   28.831787]   [<c02385c8>] driver_probe_device+0xa8/0x158
[   28.840332]   [<c02386e0>] __driver_attach+0x68/0x8c
[   28.848510]   [<c0237e68>] bus_for_each_dev+0x44/0x74
[   28.856689]   [<c02377c8>] bus_add_driver+0x9c/0x20c
[   28.864746]   [<c02389b0>] driver_register+0xa8/0x138
[   28.872955]   [<c002d340>] do_one_initcall+0x58/0x1ac
[   28.881134]   [<c0085cdc>] sys_init_module+0x90/0x1b0
[   28.889343]   [<c002df40>] ret_fast_syscall+0x0/0x3c
[   28.897491] irq event stamp: 1169
[   28.904083] hardirqs last  enabled at (1168): [<c00c4b3c>] kmem_cache_alloc+0xd0/0x128
[   28.915557] hardirqs last disabled at (1169): [<c002da34>] __irq_svc+0x34/0xb4
[   28.926239] softirqs last  enabled at (0): [<c0054bcc>] copy_process+0x304/0xe18
[   28.937194] softirqs last disabled at (0): [<(null)>] (null)
[   28.946258]
[   28.946258] other info that might help us debug this:
[   28.959381] 1 lock held by udisks-part-id/706:
[   28.967163]  #0:  (&(&musb->lock)->rlock){-.-...}, at: [<c0278590>] musb_g_disconnect+0x90/0x148
[   28.979705]
[   28.979705] stack backtrace:
[   28.990997] [<c00326a0>] (unwind_backtrace+0x0/0xec) from [<c007a39c>] (print_usage_bug+0x170/0x1b4)
[   29.007293] [<c007a39c>] (print_usage_bug+0x170/0x1b4) from [<c007a738>] (mark_lock+0x358/0x628)
[   29.019836] [<c007a738>] (mark_lock+0x358/0x628) from [<c007c118>] (__lock_acquire+0x554/0x1730)
[   29.032409] [<c007c118>] (__lock_acquire+0x554/0x1730) from [<c007d354>] (lock_acquire+0x60/0x74)
[   29.045166] [<c007d354>] (lock_acquire+0x60/0x74) from [<c037e6d8>] (down_read+0x48/0x5c)
[   29.057250] [<c037e6d8>] (down_read+0x48/0x5c) from [<c0070770>] (__blocking_notifier_call_chain+0x2c/0x5c)
[   29.074920] [<c0070770>] (__blocking_notifier_call_chain+0x2c/0x5c) from [<c00707b4>] (blocking_notifier_call_chain+0x14/0x18)
[   29.094573] [<c00707b4>] (blocking_notifier_call_chain+0x14/0x18) from [<c02784d8>] (musb_gadget_vbus_draw+0x38/0x60)
[   29.113891] [<c02784d8>] (musb_gadget_vbus_draw+0x38/0x60) from [<c0276d10>] (musb_interrupt+0xb08/0xcb0)
[   29.132629] [<c0276d10>] (musb_interrupt+0xb08/0xcb0) from [<c0276f08>] (generic_interrupt+0x50/0x68)
[   29.151580] [<c0276f08>] (generic_interrupt+0x50/0x68) from [<c008fcf4>] (handle_IRQ_event+0x24/0xe8)
[   29.171081] [<c008fcf4>] (handle_IRQ_event+0x24/0xe8) from [<c0091924>] (handle_level_irq+0xac/0x128)
[   29.190948] [<c0091924>] (handle_level_irq+0xac/0x128) from [<c002d070>] (asm_do_IRQ+0x70/0x90)
[   29.205291] [<c002d070>] (asm_do_IRQ+0x70/0x90) from [<c002da4c>] (__irq_svc+0x4c/0xb4)
[   29.218963] Exception stack(0xcd835ed0 to 0xcd835f18)
[   29.229644] 5ec0:                                     cfc0a240 c8282000 0000006b 0000006b
[   29.243591] 5ee0: cfc0a240 0000041a 00001000 00000000 c8282000 cd834000 c8282000 be80c464
[   29.257629] 5f00: 00000c1e cd835f18 c00c333c c00c297c 80000013 ffffffff
[   29.270111] [<c002da4c>] (__irq_svc+0x4c/0xb4) from [<c00c297c>] (check_poison_obj+0x24/0x194)
[   29.284637] [<c00c297c>] (check_poison_obj+0x24/0x194) from [<c00c333c>] (cache_alloc_debugcheck_after+0x28/0x188)
[   29.306732] [<c00c333c>] (cache_alloc_debugcheck_after+0x28/0x188) from [<c00c4b54>] (kmem_cache_alloc+0xe8/0x128)
[   29.329071] [<c00c4b54>] (kmem_cache_alloc+0xe8/0x128) from [<c00d79dc>] (getname+0x18/0xcc)
[   29.343597] [<c00d79dc>] (getname+0x18/0xcc) from [<c00cbca8>] (do_sys_open+0x18/0x10c)
[   29.357696] [<c00cbca8>] (do_sys_open+0x18/0x10c) from [<c002df40>] (ret_fast_syscall+0x0/0x3c)

Quoted 7 lines

Actually the blocking notifier chain runs in process context, so not fit for use here.
For mainline kernel there's such issue as well. Here fix this problem by changing to use atomic_notifier.
Signed-off-by Yang Ruirui<[email protected]>

for ab8500-usb.c parts:

Acked-by Mian Yousaf Kaukab<[email protected]>

--
Mian Yousaf Kaukab