* [PATCH v4] ARM: davinci: da8xx: Fix sleeping function called from invalid context @ 2016-12-02 14:53 Alexandre Bailon 2016-12-05 3:44 ` David Lechner 0 siblings, 1 reply; 4+ messages in thread From: Alexandre Bailon @ 2016-12-02 14:53 UTC (permalink / raw) To: linux-arm-kernel Everytime the usb20 phy is enabled, there is a "sleeping function called from invalid context" BUG. clk_enable() from arch/arm/mach-davinci/clock.c uses spin_lock_irqsave() before to invoke the callback usb20_phy_clk_enable(). usb20_phy_clk_enable() uses clk_get() and clk_enable_prepapre() which may sleep. Move clk_get() to da8xx_register_usb20_phy_clk() and replace clk_prepare_enable() by clk_enable(). Signed-off-by: Alexandre Bailon <abailon@baylibre.com> --- arch/arm/mach-davinci/usb-da8xx.c | 28 +++++++++++++++------------- 1 file changed, 15 insertions(+), 13 deletions(-) diff --git a/arch/arm/mach-davinci/usb-da8xx.c b/arch/arm/mach-davinci/usb-da8xx.c index b010e5f..704f506 100644 --- a/arch/arm/mach-davinci/usb-da8xx.c +++ b/arch/arm/mach-davinci/usb-da8xx.c @@ -22,6 +22,8 @@ #define DA8XX_USB0_BASE 0x01e00000 #define DA8XX_USB1_BASE 0x01e25000 +static struct clk *usb20_clk; + static struct platform_device da8xx_usb_phy = { .name = "da8xx-usb-phy", .id = -1, @@ -158,21 +160,14 @@ int __init da8xx_register_usb_refclkin(int rate) static void usb20_phy_clk_enable(struct clk *clk) { - struct clk *usb20_clk; int err; u32 val; u32 timeout = 500000; /* 500 msec */ val = readl(DA8XX_SYSCFG0_VIRT(DA8XX_CFGCHIP2_REG)); - usb20_clk = clk_get(&da8xx_usb20_dev.dev, "usb20"); - if (IS_ERR(usb20_clk)) { - pr_err("could not get usb20 clk: %ld\n", PTR_ERR(usb20_clk)); - return; - } - /* The USB 2.O PLL requires that the USB 2.O PSC is enabled as well. */ - err = clk_prepare_enable(usb20_clk); + err = clk_enable(usb20_clk); if (err) { pr_err("failed to enable usb20 clk: %d\n", err); clk_put(usb20_clk); @@ -197,8 +192,7 @@ static void usb20_phy_clk_enable(struct clk *clk) pr_err("Timeout waiting for USB 2.0 PHY clock good\n"); done: - clk_disable_unprepare(usb20_clk); - clk_put(usb20_clk); + clk_disable(usb20_clk); } static void usb20_phy_clk_disable(struct clk *clk) @@ -285,11 +279,19 @@ static struct clk_lookup usb20_phy_clk_lookup = int __init da8xx_register_usb20_phy_clk(bool use_usb_refclkin) { struct clk *parent; - int ret = 0; + int ret; + + usb20_clk = clk_get(&da8xx_usb20_dev.dev, "usb20"); + ret = PTR_ERR_OR_ZERO(usb20_clk); + if (ret) + return ret; parent = clk_get(NULL, use_usb_refclkin ? "usb_refclkin" : "pll0_aux"); - if (IS_ERR(parent)) - return PTR_ERR(parent); + ret = PTR_ERR_OR_ZERO(parent); + if (ret) { + clk_put(usb20_clk); + return ret; + } usb20_phy_clk.parent = parent; ret = clk_register(&usb20_phy_clk); -- 2.7.3 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* [PATCH v4] ARM: davinci: da8xx: Fix sleeping function called from invalid context 2016-12-02 14:53 [PATCH v4] ARM: davinci: da8xx: Fix sleeping function called from invalid context Alexandre Bailon @ 2016-12-05 3:44 ` David Lechner 2016-12-05 6:32 ` Sekhar Nori 2016-12-05 10:00 ` Alexandre Bailon 0 siblings, 2 replies; 4+ messages in thread From: David Lechner @ 2016-12-05 3:44 UTC (permalink / raw) To: linux-arm-kernel On 12/02/2016 08:53 AM, Alexandre Bailon wrote: > Everytime the usb20 phy is enabled, there is a > "sleeping function called from invalid context" BUG. > > clk_enable() from arch/arm/mach-davinci/clock.c uses spin_lock_irqsave() > before to invoke the callback usb20_phy_clk_enable(). > usb20_phy_clk_enable() uses clk_get() and clk_enable_prepapre() > which may sleep. > Move clk_get() to da8xx_register_usb20_phy_clk() and > replace clk_prepare_enable() by clk_enable(). > > Signed-off-by: Alexandre Bailon <abailon@baylibre.com> > --- > arch/arm/mach-davinci/usb-da8xx.c | 28 +++++++++++++++------------- > 1 file changed, 15 insertions(+), 13 deletions(-) > > diff --git a/arch/arm/mach-davinci/usb-da8xx.c b/arch/arm/mach-davinci/usb-da8xx.c > index b010e5f..704f506 100644 > --- a/arch/arm/mach-davinci/usb-da8xx.c > +++ b/arch/arm/mach-davinci/usb-da8xx.c > @@ -22,6 +22,8 @@ > #define DA8XX_USB0_BASE 0x01e00000 > #define DA8XX_USB1_BASE 0x01e25000 > > +static struct clk *usb20_clk; > + > static struct platform_device da8xx_usb_phy = { > .name = "da8xx-usb-phy", > .id = -1, > @@ -158,21 +160,14 @@ int __init da8xx_register_usb_refclkin(int rate) > > static void usb20_phy_clk_enable(struct clk *clk) > { > - struct clk *usb20_clk; > int err; > u32 val; > u32 timeout = 500000; /* 500 msec */ > > val = readl(DA8XX_SYSCFG0_VIRT(DA8XX_CFGCHIP2_REG)); > > - usb20_clk = clk_get(&da8xx_usb20_dev.dev, "usb20"); > - if (IS_ERR(usb20_clk)) { > - pr_err("could not get usb20 clk: %ld\n", PTR_ERR(usb20_clk)); > - return; > - } > - > /* The USB 2.O PLL requires that the USB 2.O PSC is enabled as well. */ > - err = clk_prepare_enable(usb20_clk); > + err = clk_enable(usb20_clk); > if (err) { > pr_err("failed to enable usb20 clk: %d\n", err); > clk_put(usb20_clk); Need to remove clk_put() here. > @@ -197,8 +192,7 @@ static void usb20_phy_clk_enable(struct clk *clk) > > pr_err("Timeout waiting for USB 2.0 PHY clock good\n"); > done: > - clk_disable_unprepare(usb20_clk); > - clk_put(usb20_clk); > + clk_disable(usb20_clk); > } > > static void usb20_phy_clk_disable(struct clk *clk) > @@ -285,11 +279,19 @@ static struct clk_lookup usb20_phy_clk_lookup = > int __init da8xx_register_usb20_phy_clk(bool use_usb_refclkin) > { > struct clk *parent; > - int ret = 0; > + int ret; > + > + usb20_clk = clk_get(&da8xx_usb20_dev.dev, "usb20"); > + ret = PTR_ERR_OR_ZERO(usb20_clk); > + if (ret) > + return ret; > > parent = clk_get(NULL, use_usb_refclkin ? "usb_refclkin" : "pll0_aux"); > - if (IS_ERR(parent)) > - return PTR_ERR(parent); > + ret = PTR_ERR_OR_ZERO(parent); > + if (ret) { > + clk_put(usb20_clk); > + return ret; > + } > > usb20_phy_clk.parent = parent; > ret = clk_register(&usb20_phy_clk); > I have just tried this patch with a bunch of kernel hacking options enabled. I am getting the message show at the end of this email. We still have the problem of nested spin locks due to nested calls to clk_enable(). So, really, we need to use __clk_enable() and __clk_disable() from arch/arm/mach-davinci/clock.c in usb20_phy_clk_enable() above. Applying the following patch on top of your patch fixed the recursive lock message. --- diff --git a/arch/arm/mach-davinci/clock.c b/arch/arm/mach-davinci/clock.c index df42c93..4fba579 100644 --- a/arch/arm/mach-davinci/clock.c +++ b/arch/arm/mach-davinci/clock.c @@ -31,7 +31,7 @@ static LIST_HEAD(clocks); static DEFINE_MUTEX(clocks_mutex); static DEFINE_SPINLOCK(clockfw_lock); -static void __clk_enable(struct clk *clk) +void __clk_enable(struct clk *clk) { if (clk->parent) __clk_enable(clk->parent); @@ -44,7 +44,7 @@ static void __clk_enable(struct clk *clk) } } -static void __clk_disable(struct clk *clk) +void __clk_disable(struct clk *clk) { if (WARN_ON(clk->usecount == 0)) return; diff --git a/arch/arm/mach-davinci/clock.h b/arch/arm/mach-davinci/clock.h index e2a5437..8493242 100644 --- a/arch/arm/mach-davinci/clock.h +++ b/arch/arm/mach-davinci/clock.h @@ -136,6 +136,9 @@ int davinci_clk_reset(struct clk *clk, bool reset); extern struct platform_device davinci_wdt_device; extern void davinci_watchdog_reset(struct platform_device *); +void __clk_enable(struct clk *clk); +void __clk_disable(struct clk *clk); + #endif #endif diff --git a/arch/arm/mach-davinci/usb-da8xx.c b/arch/arm/mach-davinci/usb-da8xx .c index 896d014..80ba0df 100644 --- a/arch/arm/mach-davinci/usb-da8xx.c +++ b/arch/arm/mach-davinci/usb-da8xx.c @@ -160,19 +160,13 @@ int __init da8xx_register_usb_refclkin(int rate) static void usb20_phy_clk_enable(struct clk *clk) { - int err; u32 val; u32 timeout = 500000; /* 500 msec */ val = readl(DA8XX_SYSCFG0_VIRT(DA8XX_CFGCHIP2_REG)); /* The USB 2.O PLL requires that the USB 2.O PSC is enabled as well. */ - err = clk_enable(usb20_clk); - if (err) { - pr_err("failed to enable usb20 clk: %d\n", err); - clk_put(usb20_clk); - return; - } + __clk_enable(usb20_clk); /* * Turn on the USB 2.0 PHY, but just the PLL, and not OTG. The USB 1.1 @@ -192,7 +186,7 @@ static void usb20_phy_clk_enable(struct clk *clk) pr_err("Timeout waiting for USB 2.0 PHY clock good\n"); done: - clk_disable(usb20_clk); + __clk_disable(usb20_clk); } static void usb20_phy_clk_disable(struct clk *clk) --- ============================================= [ INFO: possible recursive locking detected ] 4.9.0-rc8-dlech-ev3-lms2012+ #22 Not tainted --------------------------------------------- swapper/1 is trying to acquire lock: ( clockfw_lock ){......} , at: [<c0014884>] clk_enable+0x24/0x50 k: ( clockfw_lock ){......} , at: [<c0014884>] clk_enable+0x24/0x50 ebug this: Possible unsafe locking scenario: CPU0 ---- lock( clockfw_lock ); lock( clockfw_lock ); May be due to missing lock nesting notation 4 locks held by swapper/1: #0: ( &dev->mutex ){......} , at: [<c02bd60c>] __driver_attach+0x68/0xb4 #1: ( &dev->mutex ){......} , at: [<c02bd61c>] __driver_attach+0x78/0xb4 #2: ( &phy->mutex ){+.+...} , at: [<c025ee18>] phy_power_on+0x5c/0xe4 #3: ( clockfw_lock ){......} , at: [<c0014884>] clk_enable+0x24/0x50 CPU: 0 PID: 1 Comm: swapper Not tainted 4.9.0-rc8-dlech-ev3-lms2012+ #22 Hardware name: Generic DA850/OMAP-L138/AM18x Backtrace: [<c000d824>] (dump_backtrace) from [<c000dac4>] (show_stack+0x18/0x1c) r7:c0e998a4 r6:c0820750 r5:c0820750 r4:c3838000 [<c000daac>] (show_stack) from [<c02393dc>] (dump_stack+0x20/0x28) [<c02393bc>] (dump_stack) from [<c004bd7c>] (__lock_acquire+0x10f4/0x167c) [<c004ac88>] (__lock_acquire) from [<c004c6ac>] (lock_acquire+0x78/0x98) r10:00000000 r9:c06a5ed4 r8:00000000 r7:00000001 r6:60000093 r5:00000000 r4:ffffe000 [<c004c634>] (lock_acquire) from [<c04d8fac>] (_raw_spin_lock_irqsave+0x60/0x74) r7:0000003b r6:c0014884 r5:80000093 r4:c06b09a0 [<c04d8f4c>] (_raw_spin_lock_irqsave) from [<c0014884>] (clk_enable+0x24/0x50) r6:c06f69f4 r5:0001ef02 r4:c06b3398 [<c0014860>] (clk_enable) from [<c0015c74>] (usb20_phy_clk_enable+0x24/0xb8) r5:0001ef02 r4:c06f69f0 [<c0015c50>] (usb20_phy_clk_enable) from [<c0014858>] (__clk_enable+0x74/0x7c) r9:c06a5ed4 r8:00000000 r7:0000003b r6:c3a6ca00 r5:80000013 r4:c06b7ee8 [<c00147e4>] (__clk_enable) from [<c0014808>] (__clk_enable+0x24/0x7c) r4:c06b8648 [<c00147e4>] (__clk_enable) from [<c0014890>] (clk_enable+0x30/0x50) r4:c06b8648 [<c0014860>] (clk_enable) from [<c025f43c>] (da8xx_usb11_phy_power_on+0x30/0x80) r5:c3a54050 r4:c06b8648 [<c025f40c>] (da8xx_usb11_phy_power_on) from [<c025ee3c>] (phy_power_on+0x80/0xe4) r5:c3a6c800 r4:fffffdf4 [<c025edbc>] (phy_power_on) from [<c032243c>] (ohci_da8xx_enable+0x4c/0x80) r7:0000003b r6:c3af6000 r5:c3af6000 r4:00000000 [<c03223f0>] (ohci_da8xx_enable) from [<c03224f0>] (ohci_da8xx_reset+0x1c/0xd8) r5:00000000 r4:c3af6000 [<c03224d4>] (ohci_da8xx_reset) from [<c0309118>] (usb_add_hcd+0x314/0x834) r7:0000003b r6:ffffffed r5:00000000 r4:c3af6000 [<c0308e04>] (usb_add_hcd) from [<c032200c>] (ohci_da8xx_probe+0x14c/0x21c) r9:c06a5ed4 r8:00000000 r7:c38bd010 r6:c38bd000 r5:c3af6000 r4:c38af900 [<c0321ec0>] (ohci_da8xx_probe) from [<c02bec44>] (platform_drv_probe+0x40/0x8c) r7:c06e610c r6:c06e610c r5:c38bd010 r4:c0321ec0 [<c02bec04>] (platform_drv_probe) from [<c02bd438>] (driver_probe_device+0x138/0x2a4) r7:c06e610c r6:c0e9db10 r5:00000000 r4:c38bd010 [<c02bd300>] (driver_probe_device) from [<c02bd634>] (__driver_attach+0x90/0xb4) r9:c06a5ed4 r8:c06f66e0 r7:c06e1250 r6:c06e610c r5:c38bd044 r4:c38bd010 [<c02bd5a4>] (__driver_attach) from [<c02bba5c>] (bus_for_each_dev+0x74/0x98) r7:c06e1250 r6:c02bd5a4 r5:c06e610c r4:00000000 [<c02bb9e8>] (bus_for_each_dev) from [<c02bcf2c>] (driver_attach+0x20/0x28) r6:c39a2300 r5:00000000 r4:c06e610c [<c02bcf0c>] (driver_attach) from [<c02bca80>] (bus_add_driver+0xd4/0x1f0) [<c02bc9ac>] (bus_add_driver) from [<c02bdcec>] (driver_register+0xa4/0xe8) r7:c0698850 r6:00000000 r5:00000000 r4:c06e610c [<c02bdc48>] (driver_register) from [<c02bebac>] (__platform_driver_register+0x38/0x4c) r5:00000000 r4:c06acce4 [<c02beb74>] (__platform_driver_register) from [<c068da20>] (ohci_da8xx_init+0x64/0x90) [<c068d9bc>] (ohci_da8xx_init) from [<c00096c0>] (do_one_initcall+0xb0/0x168) r5:c068d9bc r4:ffffe000 [<c0009610>] (do_one_initcall) from [<c0676e14>] (kernel_init_freeable+0x110/0x1cc) r9:c06a5ed4 r8:c06f66e0 r7:c0698850 r6:000000c1 r5:c06f66e0 r4:00000007 [<c0676d04>] (kernel_init_freeable) from [<c04d37e4>] (kernel_init+0x10/0xf8) r9:00000000 r8:00000000 r7:00000000 r6:00000000 r5:c04d37d4 r4:00000000 [<c04d37d4>] (kernel_init) from [<c000a2ac>] (ret_from_fork+0x14/0x28) r5:c04d37d4 r4:00000000 BUG: spinlock lockup suspected on CPU#0, swapper/1 lock: clockfw_lock+0x0/0x20, .magic: dead4ead, .owner: swapper/1, .owner_cpu: 0 CPU: 0 PID: 1 Comm: swapper Not tainted 4.9.0-rc8-dlech-ev3-lms2012+ #22 Hardware name: Generic DA850/OMAP-L138/AM18x Backtrace: [<c000d824>] (dump_backtrace) from [<c000dac4>] (show_stack+0x18/0x1c) r7:00000000 r6:04605000 r5:c06b09a0 r4:c3838000 [<c000daac>] (show_stack) from [<c02393dc>] (dump_stack+0x20/0x28) [<c02393bc>] (dump_stack) from [<c004f140>] (spin_dump+0x80/0x94) [<c004f0c0>] (spin_dump) from [<c004f2c4>] (do_raw_spin_lock+0xdc/0x11c) r5:00000000 r4:c06b09a0 [<c004f1e8>] (do_raw_spin_lock) from [<c04d8fb4>] (_raw_spin_lock_irqsave+0x68/0x74) r10:00000000 r9:c06a5ed4 r8:00000000 r7:0000003b r6:c0014884 r5:80000093 r4:c06b09a0 r3:c3838000 [<c04d8f4c>] (_raw_spin_lock_irqsave) from [<c0014884>] (clk_enable+0x24/0x50) r6:c06f69f4 r5:0001ef02 r4:c06b3398 [<c0014860>] (clk_enable) from [<c0015c74>] (usb20_phy_clk_enable+0x24/0xb8) r5:0001ef02 r4:c06f69f0 [<c0015c50>] (usb20_phy_clk_enable) from [<c0014858>] (__clk_enable+0x74/0x7c) r9:c06a5ed4 r8:00000000 r7:0000003b r6:c3a6ca00 r5:80000013 r4:c06b7ee8 [<c00147e4>] (__clk_enable) from [<c0014808>] (__clk_enable+0x24/0x7c) r4:c06b8648 [<c00147e4>] (__clk_enable) from [<c0014890>] (clk_enable+0x30/0x50) r4:c06b8648 [<c0014860>] (clk_enable) from [<c025f43c>] (da8xx_usb11_phy_power_on+0x30/0x80) r5:c3a54050 r4:c06b8648 [<c025f40c>] (da8xx_usb11_phy_power_on) from [<c025ee3c>] (phy_power_on+0x80/0xe4) r5:c3a6c800 r4:fffffdf4 [<c025edbc>] (phy_power_on) from [<c032243c>] (ohci_da8xx_enable+0x4c/0x80) r7:0000003b r6:c3af6000 r5:c3af6000 r4:00000000 [<c03223f0>] (ohci_da8xx_enable) from [<c03224f0>] (ohci_da8xx_reset+0x1c/0xd8) r5:00000000 r4:c3af6000 [<c03224d4>] (ohci_da8xx_reset) from [<c0309118>] (usb_add_hcd+0x314/0x834) r7:0000003b r6:ffffffed r5:00000000 r4:c3af6000 [<c0308e04>] (usb_add_hcd) from [<c032200c>] (ohci_da8xx_probe+0x14c/0x21c) r9:c06a5ed4 r8:00000000 r7:c38bd010 r6:c38bd000 r5:c3af6000 r4:c38af900 [<c0321ec0>] (ohci_da8xx_probe) from [<c02bec44>] (platform_drv_probe+0x40/0x8c) r7:c06e610c r6:c06e610c r5:c38bd010 r4:c0321ec0 [<c02bec04>] (platform_drv_probe) from [<c02bd438>] (driver_probe_device+0x138/0x2a4) r7:c06e610c r6:c0e9db10 r5:00000000 r4:c38bd010 [<c02bd300>] (driver_probe_device) from [<c02bd634>] (__driver_attach+0x90/0xb4) r9:c06a5ed4 r8:c06f66e0 r7:c06e1250 r6:c06e610c r5:c38bd044 r4:c38bd010 [<c02bd5a4>] (__driver_attach) from [<c02bba5c>] (bus_for_each_dev+0x74/0x98) r7:c06e1250 r6:c02bd5a4 r5:c06e610c r4:00000000 [<c02bb9e8>] (bus_for_each_dev) from [<c02bcf2c>] (driver_attach+0x20/0x28) r6:c39a2300 r5:00000000 r4:c06e610c [<c02bcf0c>] (driver_attach) from [<c02bca80>] (bus_add_driver+0xd4/0x1f0) [<c02bc9ac>] (bus_add_driver) from [<c02bdcec>] (driver_register+0xa4/0xe8) r7:c0698850 r6:00000000 r5:00000000 r4:c06e610c [<c02bdc48>] (driver_register) from [<c02bebac>] (__platform_driver_register+0x38/0x4c) r5:00000000 r4:c06acce4 [<c02beb74>] (__platform_driver_register) from [<c068da20>] (ohci_da8xx_init+0x64/0x90) [<c068d9bc>] (ohci_da8xx_init) from [<c00096c0>] (do_one_initcall+0xb0/0x168) r5:c068d9bc r4:ffffe000 [<c0009610>] (do_one_initcall) from [<c0676e14>] (kernel_init_freeable+0x110/0x1cc) r9:c06a5ed4 r8:c06f66e0 r7:c0698850 r6:000000c1 r5:c06f66e0 r4:00000007 [<c0676d04>] (kernel_init_freeable) from [<c04d37e4>] (kernel_init+0x10/0xf8) r9:00000000 r8:00000000 r7:00000000 r6:00000000 r5:c04d37d4 r4:00000000 [<c04d37d4>] (kernel_init) from [<c000a2ac>] (ret_from_fork+0x14/0x28) r5:c04d37d4 r4:00000000 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* [PATCH v4] ARM: davinci: da8xx: Fix sleeping function called from invalid context 2016-12-05 3:44 ` David Lechner @ 2016-12-05 6:32 ` Sekhar Nori 2016-12-05 10:00 ` Alexandre Bailon 1 sibling, 0 replies; 4+ messages in thread From: Sekhar Nori @ 2016-12-05 6:32 UTC (permalink / raw) To: linux-arm-kernel Hi David, On Monday 05 December 2016 09:14 AM, David Lechner wrote: > I have just tried this patch with a bunch of kernel hacking options > enabled. I am getting the message show at the end of this email. We > still have the problem of nested spin locks due to nested calls to > clk_enable(). So, really, we need to use __clk_enable() and > __clk_disable() from arch/arm/mach-davinci/clock.c in > usb20_phy_clk_enable() above. Good catch. I noticed that common clock framework uses a more elaborate mechanism to allow nested clock API calls (see clk_prepare_lock()), but we don't (yet) have that issue in mach-davinci since the recursive calls are still in mach-davinci and dont need the exported API to be recursively callable. > Applying the following patch on top of your patch fixed the recursive > lock message. The patch looks okay to me, except couple of minor adjustments. > > --- > > diff --git a/arch/arm/mach-davinci/clock.c b/arch/arm/mach-davinci/clock.c > index df42c93..4fba579 100644 > --- a/arch/arm/mach-davinci/clock.c > +++ b/arch/arm/mach-davinci/clock.c > @@ -31,7 +31,7 @@ static LIST_HEAD(clocks); > static DEFINE_MUTEX(clocks_mutex); > static DEFINE_SPINLOCK(clockfw_lock); > > -static void __clk_enable(struct clk *clk) > +void __clk_enable(struct clk *clk) Now that this function is going to be used outside of this file, lets drop the __ naming convention and call this davinci_clk_enable() in line with how other davinci-local clock APIs are named. Same thing for the disable counterpart. Also, the making these function non-static should be a patch of its own. Thanks, Sekhar ^ permalink raw reply [flat|nested] 4+ messages in thread
* [PATCH v4] ARM: davinci: da8xx: Fix sleeping function called from invalid context 2016-12-05 3:44 ` David Lechner 2016-12-05 6:32 ` Sekhar Nori @ 2016-12-05 10:00 ` Alexandre Bailon 1 sibling, 0 replies; 4+ messages in thread From: Alexandre Bailon @ 2016-12-05 10:00 UTC (permalink / raw) To: linux-arm-kernel On 12/05/2016 04:44 AM, David Lechner wrote: > On 12/02/2016 08:53 AM, Alexandre Bailon wrote: >> Everytime the usb20 phy is enabled, there is a >> "sleeping function called from invalid context" BUG. >> >> clk_enable() from arch/arm/mach-davinci/clock.c uses spin_lock_irqsave() >> before to invoke the callback usb20_phy_clk_enable(). >> usb20_phy_clk_enable() uses clk_get() and clk_enable_prepapre() >> which may sleep. >> Move clk_get() to da8xx_register_usb20_phy_clk() and >> replace clk_prepare_enable() by clk_enable(). >> >> Signed-off-by: Alexandre Bailon <abailon@baylibre.com> >> --- >> arch/arm/mach-davinci/usb-da8xx.c | 28 +++++++++++++++------------- >> 1 file changed, 15 insertions(+), 13 deletions(-) >> >> diff --git a/arch/arm/mach-davinci/usb-da8xx.c >> b/arch/arm/mach-davinci/usb-da8xx.c >> index b010e5f..704f506 100644 >> --- a/arch/arm/mach-davinci/usb-da8xx.c >> +++ b/arch/arm/mach-davinci/usb-da8xx.c >> @@ -22,6 +22,8 @@ >> #define DA8XX_USB0_BASE 0x01e00000 >> #define DA8XX_USB1_BASE 0x01e25000 >> >> +static struct clk *usb20_clk; >> + >> static struct platform_device da8xx_usb_phy = { >> .name = "da8xx-usb-phy", >> .id = -1, >> @@ -158,21 +160,14 @@ int __init da8xx_register_usb_refclkin(int rate) >> >> static void usb20_phy_clk_enable(struct clk *clk) >> { >> - struct clk *usb20_clk; >> int err; >> u32 val; >> u32 timeout = 500000; /* 500 msec */ >> >> val = readl(DA8XX_SYSCFG0_VIRT(DA8XX_CFGCHIP2_REG)); >> >> - usb20_clk = clk_get(&da8xx_usb20_dev.dev, "usb20"); >> - if (IS_ERR(usb20_clk)) { >> - pr_err("could not get usb20 clk: %ld\n", PTR_ERR(usb20_clk)); >> - return; >> - } >> - >> /* The USB 2.O PLL requires that the USB 2.O PSC is enabled as >> well. */ >> - err = clk_prepare_enable(usb20_clk); >> + err = clk_enable(usb20_clk); >> if (err) { >> pr_err("failed to enable usb20 clk: %d\n", err); >> clk_put(usb20_clk); > > Need to remove clk_put() here. I will do. > >> @@ -197,8 +192,7 @@ static void usb20_phy_clk_enable(struct clk *clk) >> >> pr_err("Timeout waiting for USB 2.0 PHY clock good\n"); >> done: >> - clk_disable_unprepare(usb20_clk); >> - clk_put(usb20_clk); >> + clk_disable(usb20_clk); >> } >> >> static void usb20_phy_clk_disable(struct clk *clk) >> @@ -285,11 +279,19 @@ static struct clk_lookup usb20_phy_clk_lookup = >> int __init da8xx_register_usb20_phy_clk(bool use_usb_refclkin) >> { >> struct clk *parent; >> - int ret = 0; >> + int ret; >> + >> + usb20_clk = clk_get(&da8xx_usb20_dev.dev, "usb20"); >> + ret = PTR_ERR_OR_ZERO(usb20_clk); >> + if (ret) >> + return ret; >> >> parent = clk_get(NULL, use_usb_refclkin ? "usb_refclkin" : >> "pll0_aux"); >> - if (IS_ERR(parent)) >> - return PTR_ERR(parent); >> + ret = PTR_ERR_OR_ZERO(parent); >> + if (ret) { >> + clk_put(usb20_clk); >> + return ret; >> + } >> >> usb20_phy_clk.parent = parent; >> ret = clk_register(&usb20_phy_clk); >> > > > I have just tried this patch with a bunch of kernel hacking options > enabled. I am getting the message show at the end of this email. We > still have the problem of nested spin locks due to nested calls to > clk_enable(). So, really, we need to use __clk_enable() and > __clk_disable() from arch/arm/mach-davinci/clock.c in > usb20_phy_clk_enable() above. > > Applying the following patch on top of your patch fixed the recursive > lock message. Good catch. Thanks, Alexandre > > --- > > diff --git a/arch/arm/mach-davinci/clock.c b/arch/arm/mach-davinci/clock.c > index df42c93..4fba579 100644 > --- a/arch/arm/mach-davinci/clock.c > +++ b/arch/arm/mach-davinci/clock.c > @@ -31,7 +31,7 @@ static LIST_HEAD(clocks); > static DEFINE_MUTEX(clocks_mutex); > static DEFINE_SPINLOCK(clockfw_lock); > > -static void __clk_enable(struct clk *clk) > +void __clk_enable(struct clk *clk) > { > if (clk->parent) > __clk_enable(clk->parent); > @@ -44,7 +44,7 @@ static void __clk_enable(struct clk *clk) > } > } > > -static void __clk_disable(struct clk *clk) > +void __clk_disable(struct clk *clk) > { > if (WARN_ON(clk->usecount == 0)) > return; > diff --git a/arch/arm/mach-davinci/clock.h b/arch/arm/mach-davinci/clock.h > index e2a5437..8493242 100644 > --- a/arch/arm/mach-davinci/clock.h > +++ b/arch/arm/mach-davinci/clock.h > @@ -136,6 +136,9 @@ int davinci_clk_reset(struct clk *clk, bool reset); > extern struct platform_device davinci_wdt_device; > extern void davinci_watchdog_reset(struct platform_device *); > > +void __clk_enable(struct clk *clk); > +void __clk_disable(struct clk *clk); > + > #endif > > #endif > diff --git a/arch/arm/mach-davinci/usb-da8xx.c > b/arch/arm/mach-davinci/usb-da8xx > .c > index 896d014..80ba0df 100644 > --- a/arch/arm/mach-davinci/usb-da8xx.c > +++ b/arch/arm/mach-davinci/usb-da8xx.c > @@ -160,19 +160,13 @@ int __init da8xx_register_usb_refclkin(int rate) > > static void usb20_phy_clk_enable(struct clk *clk) > { > - int err; > u32 val; > u32 timeout = 500000; /* 500 msec */ > > val = readl(DA8XX_SYSCFG0_VIRT(DA8XX_CFGCHIP2_REG)); > > /* The USB 2.O PLL requires that the USB 2.O PSC is enabled as > well. */ > - err = clk_enable(usb20_clk); > - if (err) { > - pr_err("failed to enable usb20 clk: %d\n", err); > - clk_put(usb20_clk); > - return; > - } > + __clk_enable(usb20_clk); > > /* > * Turn on the USB 2.0 PHY, but just the PLL, and not OTG. The > USB 1.1 > @@ -192,7 +186,7 @@ static void usb20_phy_clk_enable(struct clk *clk) > > pr_err("Timeout waiting for USB 2.0 PHY clock good\n"); > done: > - clk_disable(usb20_clk); > + __clk_disable(usb20_clk); > } > > static void usb20_phy_clk_disable(struct clk *clk) > > > > --- > > > ============================================= > [ INFO: possible recursive locking detected ] > 4.9.0-rc8-dlech-ev3-lms2012+ #22 Not tainted > --------------------------------------------- > swapper/1 is trying to acquire lock: > ( > clockfw_lock > ){......} > , at: > [<c0014884>] clk_enable+0x24/0x50 > k: > ( > clockfw_lock > ){......} > , at: > [<c0014884>] clk_enable+0x24/0x50 > ebug this: > Possible unsafe locking scenario: > CPU0 > ---- > lock( > clockfw_lock > ); > lock( > clockfw_lock > ); > May be due to missing lock nesting notation > 4 locks held by swapper/1: > #0: > ( > &dev->mutex > ){......} > , at: > [<c02bd60c>] __driver_attach+0x68/0xb4 > #1: > ( > &dev->mutex > ){......} > , at: > [<c02bd61c>] __driver_attach+0x78/0xb4 > #2: > ( > &phy->mutex > ){+.+...} > , at: > [<c025ee18>] phy_power_on+0x5c/0xe4 > #3: > ( > clockfw_lock > ){......} > , at: > [<c0014884>] clk_enable+0x24/0x50 > CPU: 0 PID: 1 Comm: swapper Not tainted 4.9.0-rc8-dlech-ev3-lms2012+ #22 > Hardware name: Generic DA850/OMAP-L138/AM18x > Backtrace: > [<c000d824>] (dump_backtrace) from [<c000dac4>] (show_stack+0x18/0x1c) > r7:c0e998a4 r6:c0820750 r5:c0820750 r4:c3838000 > [<c000daac>] (show_stack) from [<c02393dc>] (dump_stack+0x20/0x28) > [<c02393bc>] (dump_stack) from [<c004bd7c>] (__lock_acquire+0x10f4/0x167c) > [<c004ac88>] (__lock_acquire) from [<c004c6ac>] (lock_acquire+0x78/0x98) > r10:00000000 r9:c06a5ed4 r8:00000000 r7:00000001 r6:60000093 r5:00000000 > r4:ffffe000 > [<c004c634>] (lock_acquire) from [<c04d8fac>] > (_raw_spin_lock_irqsave+0x60/0x74) > r7:0000003b r6:c0014884 r5:80000093 r4:c06b09a0 > [<c04d8f4c>] (_raw_spin_lock_irqsave) from [<c0014884>] > (clk_enable+0x24/0x50) > r6:c06f69f4 r5:0001ef02 r4:c06b3398 > [<c0014860>] (clk_enable) from [<c0015c74>] > (usb20_phy_clk_enable+0x24/0xb8) > r5:0001ef02 r4:c06f69f0 > [<c0015c50>] (usb20_phy_clk_enable) from [<c0014858>] > (__clk_enable+0x74/0x7c) > r9:c06a5ed4 r8:00000000 r7:0000003b r6:c3a6ca00 r5:80000013 r4:c06b7ee8 > [<c00147e4>] (__clk_enable) from [<c0014808>] (__clk_enable+0x24/0x7c) > r4:c06b8648 > [<c00147e4>] (__clk_enable) from [<c0014890>] (clk_enable+0x30/0x50) > r4:c06b8648 > [<c0014860>] (clk_enable) from [<c025f43c>] > (da8xx_usb11_phy_power_on+0x30/0x80) > r5:c3a54050 r4:c06b8648 > [<c025f40c>] (da8xx_usb11_phy_power_on) from [<c025ee3c>] > (phy_power_on+0x80/0xe4) > r5:c3a6c800 r4:fffffdf4 > [<c025edbc>] (phy_power_on) from [<c032243c>] (ohci_da8xx_enable+0x4c/0x80) > r7:0000003b r6:c3af6000 r5:c3af6000 r4:00000000 > [<c03223f0>] (ohci_da8xx_enable) from [<c03224f0>] > (ohci_da8xx_reset+0x1c/0xd8) > r5:00000000 r4:c3af6000 > [<c03224d4>] (ohci_da8xx_reset) from [<c0309118>] (usb_add_hcd+0x314/0x834) > r7:0000003b r6:ffffffed r5:00000000 r4:c3af6000 > [<c0308e04>] (usb_add_hcd) from [<c032200c>] (ohci_da8xx_probe+0x14c/0x21c) > r9:c06a5ed4 r8:00000000 r7:c38bd010 r6:c38bd000 r5:c3af6000 r4:c38af900 > [<c0321ec0>] (ohci_da8xx_probe) from [<c02bec44>] > (platform_drv_probe+0x40/0x8c) > r7:c06e610c r6:c06e610c r5:c38bd010 r4:c0321ec0 > [<c02bec04>] (platform_drv_probe) from [<c02bd438>] > (driver_probe_device+0x138/0x2a4) > r7:c06e610c r6:c0e9db10 r5:00000000 r4:c38bd010 > [<c02bd300>] (driver_probe_device) from [<c02bd634>] > (__driver_attach+0x90/0xb4) > r9:c06a5ed4 r8:c06f66e0 r7:c06e1250 r6:c06e610c r5:c38bd044 r4:c38bd010 > [<c02bd5a4>] (__driver_attach) from [<c02bba5c>] > (bus_for_each_dev+0x74/0x98) > r7:c06e1250 r6:c02bd5a4 r5:c06e610c r4:00000000 > [<c02bb9e8>] (bus_for_each_dev) from [<c02bcf2c>] (driver_attach+0x20/0x28) > r6:c39a2300 r5:00000000 r4:c06e610c > [<c02bcf0c>] (driver_attach) from [<c02bca80>] (bus_add_driver+0xd4/0x1f0) > [<c02bc9ac>] (bus_add_driver) from [<c02bdcec>] (driver_register+0xa4/0xe8) > r7:c0698850 r6:00000000 r5:00000000 r4:c06e610c > [<c02bdc48>] (driver_register) from [<c02bebac>] > (__platform_driver_register+0x38/0x4c) > r5:00000000 r4:c06acce4 > [<c02beb74>] (__platform_driver_register) from [<c068da20>] > (ohci_da8xx_init+0x64/0x90) > [<c068d9bc>] (ohci_da8xx_init) from [<c00096c0>] > (do_one_initcall+0xb0/0x168) > r5:c068d9bc r4:ffffe000 > [<c0009610>] (do_one_initcall) from [<c0676e14>] > (kernel_init_freeable+0x110/0x1cc) > r9:c06a5ed4 r8:c06f66e0 r7:c0698850 r6:000000c1 r5:c06f66e0 r4:00000007 > [<c0676d04>] (kernel_init_freeable) from [<c04d37e4>] > (kernel_init+0x10/0xf8) > r9:00000000 r8:00000000 r7:00000000 r6:00000000 r5:c04d37d4 r4:00000000 > [<c04d37d4>] (kernel_init) from [<c000a2ac>] (ret_from_fork+0x14/0x28) > r5:c04d37d4 r4:00000000 > BUG: spinlock lockup suspected on CPU#0, swapper/1 > lock: clockfw_lock+0x0/0x20, .magic: dead4ead, .owner: swapper/1, > .owner_cpu: 0 > CPU: 0 PID: 1 Comm: swapper Not tainted 4.9.0-rc8-dlech-ev3-lms2012+ #22 > Hardware name: Generic DA850/OMAP-L138/AM18x > Backtrace: > [<c000d824>] (dump_backtrace) from [<c000dac4>] (show_stack+0x18/0x1c) > r7:00000000 r6:04605000 r5:c06b09a0 r4:c3838000 > [<c000daac>] (show_stack) from [<c02393dc>] (dump_stack+0x20/0x28) > [<c02393bc>] (dump_stack) from [<c004f140>] (spin_dump+0x80/0x94) > [<c004f0c0>] (spin_dump) from [<c004f2c4>] (do_raw_spin_lock+0xdc/0x11c) > r5:00000000 r4:c06b09a0 > [<c004f1e8>] (do_raw_spin_lock) from [<c04d8fb4>] > (_raw_spin_lock_irqsave+0x68/0x74) > r10:00000000 r9:c06a5ed4 r8:00000000 r7:0000003b r6:c0014884 r5:80000093 > r4:c06b09a0 r3:c3838000 > [<c04d8f4c>] (_raw_spin_lock_irqsave) from [<c0014884>] > (clk_enable+0x24/0x50) > r6:c06f69f4 r5:0001ef02 r4:c06b3398 > [<c0014860>] (clk_enable) from [<c0015c74>] > (usb20_phy_clk_enable+0x24/0xb8) > r5:0001ef02 r4:c06f69f0 > [<c0015c50>] (usb20_phy_clk_enable) from [<c0014858>] > (__clk_enable+0x74/0x7c) > r9:c06a5ed4 r8:00000000 r7:0000003b r6:c3a6ca00 r5:80000013 r4:c06b7ee8 > [<c00147e4>] (__clk_enable) from [<c0014808>] (__clk_enable+0x24/0x7c) > r4:c06b8648 > [<c00147e4>] (__clk_enable) from [<c0014890>] (clk_enable+0x30/0x50) > r4:c06b8648 > [<c0014860>] (clk_enable) from [<c025f43c>] > (da8xx_usb11_phy_power_on+0x30/0x80) > r5:c3a54050 r4:c06b8648 > [<c025f40c>] (da8xx_usb11_phy_power_on) from [<c025ee3c>] > (phy_power_on+0x80/0xe4) > r5:c3a6c800 r4:fffffdf4 > [<c025edbc>] (phy_power_on) from [<c032243c>] (ohci_da8xx_enable+0x4c/0x80) > r7:0000003b r6:c3af6000 r5:c3af6000 r4:00000000 > [<c03223f0>] (ohci_da8xx_enable) from [<c03224f0>] > (ohci_da8xx_reset+0x1c/0xd8) > r5:00000000 r4:c3af6000 > [<c03224d4>] (ohci_da8xx_reset) from [<c0309118>] (usb_add_hcd+0x314/0x834) > r7:0000003b r6:ffffffed r5:00000000 r4:c3af6000 > [<c0308e04>] (usb_add_hcd) from [<c032200c>] (ohci_da8xx_probe+0x14c/0x21c) > r9:c06a5ed4 r8:00000000 r7:c38bd010 r6:c38bd000 r5:c3af6000 r4:c38af900 > [<c0321ec0>] (ohci_da8xx_probe) from [<c02bec44>] > (platform_drv_probe+0x40/0x8c) > r7:c06e610c r6:c06e610c r5:c38bd010 r4:c0321ec0 > [<c02bec04>] (platform_drv_probe) from [<c02bd438>] > (driver_probe_device+0x138/0x2a4) > r7:c06e610c r6:c0e9db10 r5:00000000 r4:c38bd010 > [<c02bd300>] (driver_probe_device) from [<c02bd634>] > (__driver_attach+0x90/0xb4) > r9:c06a5ed4 r8:c06f66e0 r7:c06e1250 r6:c06e610c r5:c38bd044 r4:c38bd010 > [<c02bd5a4>] (__driver_attach) from [<c02bba5c>] > (bus_for_each_dev+0x74/0x98) > r7:c06e1250 r6:c02bd5a4 r5:c06e610c r4:00000000 > [<c02bb9e8>] (bus_for_each_dev) from [<c02bcf2c>] (driver_attach+0x20/0x28) > r6:c39a2300 r5:00000000 r4:c06e610c > [<c02bcf0c>] (driver_attach) from [<c02bca80>] (bus_add_driver+0xd4/0x1f0) > [<c02bc9ac>] (bus_add_driver) from [<c02bdcec>] (driver_register+0xa4/0xe8) > r7:c0698850 r6:00000000 r5:00000000 r4:c06e610c > [<c02bdc48>] (driver_register) from [<c02bebac>] > (__platform_driver_register+0x38/0x4c) > r5:00000000 r4:c06acce4 > [<c02beb74>] (__platform_driver_register) from [<c068da20>] > (ohci_da8xx_init+0x64/0x90) > [<c068d9bc>] (ohci_da8xx_init) from [<c00096c0>] > (do_one_initcall+0xb0/0x168) > r5:c068d9bc r4:ffffe000 > [<c0009610>] (do_one_initcall) from [<c0676e14>] > (kernel_init_freeable+0x110/0x1cc) > r9:c06a5ed4 r8:c06f66e0 r7:c0698850 r6:000000c1 r5:c06f66e0 r4:00000007 > [<c0676d04>] (kernel_init_freeable) from [<c04d37e4>] > (kernel_init+0x10/0xf8) > r9:00000000 r8:00000000 r7:00000000 r6:00000000 r5:c04d37d4 r4:00000000 > [<c04d37d4>] (kernel_init) from [<c000a2ac>] (ret_from_fork+0x14/0x28) > r5:c04d37d4 r4:00000000 ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2016-12-05 10:00 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2016-12-02 14:53 [PATCH v4] ARM: davinci: da8xx: Fix sleeping function called from invalid context Alexandre Bailon 2016-12-05 3:44 ` David Lechner 2016-12-05 6:32 ` Sekhar Nori 2016-12-05 10:00 ` Alexandre Bailon
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox