tidspbridge issue with omap_dm_timer_free

tidspbridge issue with omap_dm_timer_free

[Felipe Contreras]

Hi,

I am trying to use a more recent version of the tidspbridge code in
the Nokia N9, but I'm stuck with this warning that is caused by using
the dm timer framework.

[   30.883636] BUG: sleeping function called from invalid context at
kernel/mutex.c:287
[   30.885925] in_atomic(): 1, irqs_disabled(): 0, pid: 305, name: mboxd/0
[   30.892517] 3 locks held by mboxd/0/305:
[   30.896392]  #0:  (mboxd){+.+...}, at: [<b0081778>] worker_thread+0x154/0x2bc
[   30.903533]  #1:  (&mq->work){+.+...}, at: [<b0081778>]
worker_thread+0x154/0x2bc
[   30.911041]  #2:  (pwr_lock){+.....}, at: [<af12f870>]
handle_hibernation_from_dsp+0x1c/0x158 [bridgedriver]
[   30.920959] [<b003f75c>] (unwind_backtrace+0x0/0xd4) from
[<b03a3ac4>] (mutex_lock_nested+0x30/0x32c)
[   30.930175] [<b03a3ac4>] (mutex_lock_nested+0x30/0x32c) from
[<b00568f8>] (clk_set_parent+0x34/0xf8)
[   30.939361] [<b00568f8>] (clk_set_parent+0x34/0xf8) from
[<b005e2e0>] (omap_dm_timer_set_source+0x34/0x58)
[   30.949035] [<b005e2e0>] (omap_dm_timer_set_source+0x34/0x58) from
[<b005e424>] (omap_dm_timer_reset+0x78/0xd0)
[   30.959136] [<b005e424>] (omap_dm_timer_reset+0x78/0xd0) from
[<b005e490>] (omap_dm_timer_free+0x14/0x48)
[   30.968780] [<b005e490>] (omap_dm_timer_free+0x14/0x48) from
[<af1309d4>] (dsp_clk_disable+0x98/0x15c [bridgedriver])
[   30.979492] [<af1309d4>] (dsp_clk_disable+0x98/0x15c
[bridgedriver]) from [<af130abc>] (dsp_clock_disable_all+0x24/0x34
[bridgedriver])
[   30.991668] [<af130abc>] (dsp_clock_disable_all+0x24/0x34
[bridgedriver]) from [<af12f914>]
(handle_hibernation_from_dsp+0xc0/0x158 [bridgedriver])
[   31.004913] [<af12f914>] (handle_hibernation_from_dsp+0xc0/0x158
[bridgedriver]) from [<af12c2bc>] (io_mbox_msg+0x8c/0x100
[bridgedriver])
[   31.017364] [<af12c2bc>] (io_mbox_msg+0x8c/0x100 [bridgedriver])
from [<af07043c>] (mbox_rx_work+0x3c/0xa0 [mailbox])
[   31.027954] [<af07043c>] (mbox_rx_work+0x3c/0xa0 [mailbox]) from
[<b00817e4>] (worker_thread+0x1c0/0x2bc)
[   31.037536] [<b00817e4>] (worker_thread+0x1c0/0x2bc) from
[<b008513c>] (kthread+0x7c/0x84)
[   31.045806] [<b008513c>] (kthread+0x7c/0x84) from [<b003b9d4>]
(kernel_thread_exit+0x0/0x8)
[   31.054290] ------------[ cut here ]------------
[   31.058837] WARNING: at kernel/mutex.c:214 mutex_lock_nested+0xb0/0x32c()
[   31.065643] Modules linked in: ramzswap dm_crypt omaplfb
bridgedriver(C) dm_mod omap_aes mtdswap g_file_storage cmt_speech
ssi_protocol hsi_char phonet pvrsrvkm omap_ssi mailbox_mach mailbox
lis3lv02d_i2c radio_wl1273 ak8974 lis3lv02d bcm4751_gps leds_lp5523
bhsfh rtc_twl4030 twl5031_aci twl4030_keypad twl4030_pwrbutton
hid_twl4030_vibra hci_h4p cmt
[   31.096649] [<b003f75c>] (unwind_backtrace+0x0/0xd4) from
[<b00709d4>] (warn_slowpath_common+0x48/0x60)
[   31.106079] [<b00709d4>] (warn_slowpath_common+0x48/0x60) from
[<b03a3b44>] (mutex_lock_nested+0xb0/0x32c)
[   31.115753] [<b03a3b44>] (mutex_lock_nested+0xb0/0x32c) from
[<b00568f8>] (clk_set_parent+0x34/0xf8)
[   31.124908] [<b00568f8>] (clk_set_parent+0x34/0xf8) from
[<b005e2e0>] (omap_dm_timer_set_source+0x34/0x58)
[   31.134613] [<b005e2e0>] (omap_dm_timer_set_source+0x34/0x58) from
[<b005e424>] (omap_dm_timer_reset+0x78/0xd0)
[   31.144714] [<b005e424>] (omap_dm_timer_reset+0x78/0xd0) from
[<b005e490>] (omap_dm_timer_free+0x14/0x48)
[   31.154357] [<b005e490>] (omap_dm_timer_free+0x14/0x48) from
[<af1309d4>] (dsp_clk_disable+0x98/0x15c [bridgedriver])
[   31.165039] [<af1309d4>] (dsp_clk_disable+0x98/0x15c
[bridgedriver]) from [<af130abc>] (dsp_clock_disable_all+0x24/0x34
[bridgedriver])
[   31.177215] [<af130abc>] (dsp_clock_disable_all+0x24/0x34
[bridgedriver]) from [<af12f914>]
(handle_hibernation_from_dsp+0xc0/0x158 [bridgedriver])
[   31.190490] [<af12f914>] (handle_hibernation_from_dsp+0xc0/0x158
[bridgedriver]) from [<af12c2bc>] (io_mbox_msg+0x8c/0x100
[bridgedriver])
[   31.202911] [<af12c2bc>] (io_mbox_msg+0x8c/0x100 [bridgedriver])
from [<af07043c>] (mbox_rx_work+0x3c/0xa0 [mailbox])
[   31.213531] [<af07043c>] (mbox_rx_work+0x3c/0xa0 [mailbox]) from
[<b00817e4>] (worker_thread+0x1c0/0x2bc)
[   31.223114] [<b00817e4>] (worker_thread+0x1c0/0x2bc) from
[<b008513c>] (kthread+0x7c/0x84)
[   31.231414] [<b008513c>] (kthread+0x7c/0x84) from [<b003b9d4>]
(kernel_thread_exit+0x0/0x8)
[   31.239746] ---[ end trace e14f2f8770932c43 ]---

>From what I can see this could be triggered in upstream by enabling PM
and debug mutex stuff right after loading the baseimage.

Any ideas?

-- 
Felipe Contreras

Re: tidspbridge issue with omap_dm_timer_free

[Felipe Contreras]

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

On Wed, Aug 10, 2011 at 9:42 PM, Felipe Contreras
<felipe.contreras@gmail.com> wrote:
> I am trying to use a more recent version of the tidspbridge code in
> the Nokia N9, but I'm stuck with this warning that is caused by using
> the dm timer framework.

Actually, the problem only happens on the 'dspbridge' branch, which
has some unmerged patches. These patches introduce some new mutexes
that trigger this.

My proposed solution is to request the dm timers at initialization
time, and just enable/disable them on wake/hibernate, which is a bit
similar to what was happening before, and probably more efficient and
proper.

I'm attaching the patch.

-- 
Felipe Contreras

[-- Attachment #2: 0001-staging-tidspbridge-split-gpt-requests.patch --]
[-- Type: text/x-patch, Size: 6168 bytes --]

From cd7f245ba42eb0d18bdfc3f29e2856f09227528e Mon Sep 17 00:00:00 2001
From: Felipe Contreras <felipe.contreras@nokia.com>
Date: Thu, 11 Aug 2011 01:28:08 +0300
Subject: [PATCH] staging: tidspbridge: split gpt requests

Otherwise I get this:

 BUG: sleeping function called from invalid context at kernel/mutex.c:287
 in_atomic(): 1, irqs_disabled(): 0, pid: 305, name: mboxd/0
 3 locks held by mboxd/0/305:
  #0:  (mboxd){+.+...}, at: [<b0081778>] worker_thread+0x154/0x2bc
  #1:  (&mq->work){+.+...}, at: [<b0081778>] worker_thread+0x154/0x2bc
  #2:  (pwr_lock){+.....}, at: [<af12f870>] handle_hibernation_from_dsp+0x1c/0x158 [bridgedriver]
 [<b003f75c>] (unwind_backtrace+0x0/0xd4) from [<b03a3ac4>] (mutex_lock_nested+0x30/0x32c)
 [<b03a3ac4>] (mutex_lock_nested+0x30/0x32c) from [<b00568f8>] (clk_set_parent+0x34/0xf8)
 [<b00568f8>] (clk_set_parent+0x34/0xf8) from [<b005e2e0>] (omap_dm_timer_set_source+0x34/0x58)
 [<b005e2e0>] (omap_dm_timer_set_source+0x34/0x58) from [<b005e424>] (omap_dm_timer_reset+0x78/0xd0)
 [<b005e424>] (omap_dm_timer_reset+0x78/0xd0) from [<b005e490>] (omap_dm_timer_free+0x14/0x48)
 [<b005e490>] (omap_dm_timer_free+0x14/0x48) from [<af1309d4>] (dsp_clk_disable+0x98/0x15c [bridgedriver])
 [<af1309d4>] (dsp_clk_disable+0x98/0x15c [bridgedriver]) from [<af130abc>] (dsp_clock_disable_all+0x24/0x34 [bridgedriver])
 [<af130abc>] (dsp_clock_disable_all+0x24/0x34 [bridgedriver]) from [<af12f914>] (handle_hibernation_from_dsp+0xc0/0x158 [bridgedriver])
 [<af12f914>] (handle_hibernation_from_dsp+0xc0/0x158 [bridgedriver]) from [<af12c2bc>] (io_mbox_msg+0x8c/0x100 [bridgedriver])
 [<af12c2bc>] (io_mbox_msg+0x8c/0x100 [bridgedriver]) from [<af07043c>] (mbox_rx_work+0x3c/0xa0 [mailbox])
 [<af07043c>] (mbox_rx_work+0x3c/0xa0 [mailbox]) from [<b00817e4>] (worker_thread+0x1c0/0x2bc)
 [<b00817e4>] (worker_thread+0x1c0/0x2bc) from [<b008513c>] (kthread+0x7c/0x84)
 [<b008513c>] (kthread+0x7c/0x84) from [<b003b9d4>] (kernel_thread_exit+0x0/0x8)
 ------------[ cut here ]------------
 WARNING: at kernel/mutex.c:214 mutex_lock_nested+0xb0/0x32c()

Signed-off-by: Felipe Contreras <felipe.contreras@nokia.com>
---
 drivers/staging/tidspbridge/core/dsp-clock.c       |   49 ++++++++++++++++++-
 drivers/staging/tidspbridge/core/tiomap3430.c      |    3 +
 .../staging/tidspbridge/include/dspbridge/clk.h    |    4 ++
 3 files changed, 53 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/tidspbridge/core/dsp-clock.c b/drivers/staging/tidspbridge/core/dsp-clock.c
index aac06c0..b0aa831 100644
--- a/drivers/staging/tidspbridge/core/dsp-clock.c
+++ b/drivers/staging/tidspbridge/core/dsp-clock.c
@@ -115,6 +115,7 @@ static s8 get_clk_type(u8 id)
 void dsp_clk_exit(void)
 {
 	dsp_clock_disable_all(dsp_clocks);
+	dsp_clock_release_all();
 
 	clk_put(iva2_clk);
 	clk_put(ssi.sst_fck);
@@ -146,6 +147,45 @@ void dsp_clk_init(void)
 					ssi.sst_fck, ssi.ssr_fck, ssi.ick);
 }
 
+int dsp_clk_request(enum dsp_clk_id clk_id)
+{
+	struct omap_dm_timer *t;
+
+	if (get_clk_type(clk_id) != GPT_CLK)
+		return -EPERM;
+
+	t = omap_dm_timer_request_specific(DMT_ID(clk_id));
+	if (!t)
+		return -EPERM;
+	timer[clk_id - 1] = t;
+
+	return 0;
+}
+
+int dsp_clk_release(enum dsp_clk_id clk_id)
+{
+	if (get_clk_type(clk_id) != GPT_CLK)
+		return -EPERM;
+
+	omap_dm_timer_free(timer[clk_id - 1]);
+	timer[clk_id - 1] = NULL;
+
+	return 0;
+}
+
+void dsp_clock_release_all(void)
+{
+	int i;
+
+	/* release dm timer clocks */
+	for (i = 0; i < ARRAY_SIZE(timer); i++) {
+		if (!timer[i])
+			continue;
+		omap_dm_timer_free(timer[i]);
+		timer[i] = NULL;
+	}
+}
+
 #ifdef CONFIG_OMAP_MCBSP
 static void mcbsp_clk_prepare(bool flag, u8 id)
 {
@@ -252,8 +292,9 @@ int dsp_clk_enable(enum dsp_clk_id clk_id)
 		clk_enable(iva2_clk);
 		break;
 	case GPT_CLK:
-		timer[clk_id - 1] =
-				omap_dm_timer_request_specific(DMT_ID(clk_id));
+		if (!timer[clk_id - 1])
+			return -EINVAL;
+		omap_dm_timer_enable(timer[clk_id - 1]);
 		break;
 #ifdef CONFIG_OMAP_MCBSP
 	case MCBSP_CLK:
@@ -330,7 +371,9 @@ int dsp_clk_disable(enum dsp_clk_id clk_id)
 		clk_disable(iva2_clk);
 		break;
 	case GPT_CLK:
-		omap_dm_timer_free(timer[clk_id - 1]);
+		if (!timer[clk_id - 1])
+			return -EINVAL;
+		omap_dm_timer_disable(timer[clk_id - 1]);
 		break;
 #ifdef CONFIG_OMAP_MCBSP
 	case MCBSP_CLK:
diff --git a/drivers/staging/tidspbridge/core/tiomap3430.c b/drivers/staging/tidspbridge/core/tiomap3430.c
index ecd865b..fbcea0b 100644
--- a/drivers/staging/tidspbridge/core/tiomap3430.c
+++ b/drivers/staging/tidspbridge/core/tiomap3430.c
@@ -512,6 +512,7 @@ static int bridge_brd_start(struct bridge_dev_context *dev_ctxt,
 		if (ul_load_monitor_timer != 0xFFFF) {
 			clk_cmd = (BPWR_ENABLE_CLOCK << MBX_PM_CLK_CMDSHIFT) |
 			    ul_load_monitor_timer;
+			dsp_clk_request(ul_load_monitor_timer - BPWR_GP_TIMER5 + DSP_CLK_GPT5);
 			dsp_peripheral_clk_ctrl(dev_context, &clk_cmd);
 		} else {
 			dev_dbg(bridge, "Not able to get the symbol for Load "
@@ -523,6 +524,7 @@ static int bridge_brd_start(struct bridge_dev_context *dev_ctxt,
 		if (ul_bios_gp_timer != 0xFFFF) {
 			clk_cmd = (BPWR_ENABLE_CLOCK << MBX_PM_CLK_CMDSHIFT) |
 			    ul_bios_gp_timer;
+			dsp_clk_request(ul_bios_gp_timer - BPWR_GP_TIMER5 + DSP_CLK_GPT5);
 			dsp_peripheral_clk_ctrl(dev_context, &clk_cmd);
 		} else {
 			dev_dbg(bridge,
@@ -691,6 +693,7 @@ static int bridge_brd_stop(struct bridge_dev_context *dev_ctxt)
 			OMAP3430_RST3_IVA2, OMAP3430_IVA2_MOD, RM_RSTCTRL);
 
 	dsp_clock_disable_all(dev_context->dsp_per_clks);
+	dsp_clock_release_all();
 	dsp_clk_disable(DSP_CLK_IVA2);
 
 	return status;
diff --git a/drivers/staging/tidspbridge/include/dspbridge/clk.h b/drivers/staging/tidspbridge/include/dspbridge/clk.h
index 685341c..14c5d7d 100644
--- a/drivers/staging/tidspbridge/include/dspbridge/clk.h
+++ b/drivers/staging/tidspbridge/include/dspbridge/clk.h
@@ -62,6 +62,10 @@ extern void dsp_clk_exit(void);
  */
 extern void dsp_clk_init(void);
 
+int dsp_clk_request(enum dsp_clk_id clk_id);
+int dsp_clk_release(enum dsp_clk_id clk_id);
+void dsp_clock_release_all(void);
+
 void dsp_gpt_wait_overflow(short int clk_id, unsigned int load);
 
 /*
-- 
1.7.6

Re: tidspbridge issue with omap_dm_timer_free

[Ramirez Luna, Omar]

On Wed, Aug 10, 2011 at 1:42 PM, Felipe Contreras
<felipe.contreras@gmail.com> wrote:
> Hi,
>
> I am trying to use a more recent version of the tidspbridge code in
> the Nokia N9, but I'm stuck with this warning that is caused by using
> the dm timer framework.
>
> [   30.883636] BUG: sleeping function called from invalid context at
> kernel/mutex.c:287
> [   30.885925] in_atomic(): 1, irqs_disabled(): 0, pid: 305, name: mboxd/0
> [   30.892517] 3 locks held by mboxd/0/305:
> [   30.896392]  #0:  (mboxd){+.+...}, at: [<b0081778>] worker_thread+0x154/0x2bc
> [   30.903533]  #1:  (&mq->work){+.+...}, at: [<b0081778>]
> worker_thread+0x154/0x2bc
> [   30.911041]  #2:  (pwr_lock){+.....}, at: [<af12f870>]
> handle_hibernation_from_dsp+0x1c/0x158 [bridgedriver]
> [   30.920959] [<b003f75c>] (unwind_backtrace+0x0/0xd4) from
> [<b03a3ac4>] (mutex_lock_nested+0x30/0x32c)
> [   30.930175] [<b03a3ac4>] (mutex_lock_nested+0x30/0x32c) from
> [<b00568f8>] (clk_set_parent+0x34/0xf8)
...
> From what I can see this could be triggered in upstream by enabling PM
> and debug mutex stuff right after loading the baseimage.
>
> Any ideas?

May I know which kernel version are you using? I can't seem to find
the mutex_lock inside clk_set_parent in arch/arm/plat-omap/clock.c.

Regards,

Omar
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: tidspbridge issue with omap_dm_timer_free

[Ramirez Luna, Omar]

On Wed, Aug 10, 2011 at 6:27 PM, Felipe Contreras
<felipe.contreras@gmail.com> wrote:
> On Wed, Aug 10, 2011 at 9:42 PM, Felipe Contreras
> <felipe.contreras@gmail.com> wrote:
>> I am trying to use a more recent version of the tidspbridge code in
>> the Nokia N9, but I'm stuck with this warning that is caused by using
>> the dm timer framework.
>
> Actually, the problem only happens on the 'dspbridge' branch, which
> has some unmerged patches. These patches introduce some new mutexes
> that trigger this.

The only lock introduced is a spin_lock_bh which makes sense to warn
if the underlying functions have a mutex, but I couldn't find the
clk_set_parent mutex which is causing this on the omap files.

> My proposed solution is to request the dm timers at initialization
> time, and just enable/disable them on wake/hibernate, which is a bit
> similar to what was happening before, and probably more efficient and
> proper.

When I made these changes I wanted to avoid keeping an array with the
clocks requested and nobody else being able to use them (even if that
meant a conflict between two drivers trying to use the same clock, but
other warnings could help to point to such cases). Also I wanted to
maintain uniformity, not just requesting some clock, freeing them and
then some others to be held and freed only on module removal.

That said, I have no problem on changing to your approach if needed.

Regards,

Omar
>
> I'm attaching the patch.
>
> --
> Felipe Contreras
>

Re: tidspbridge issue with omap_dm_timer_free

[Felipe Contreras]

On Sat, Aug 13, 2011 at 8:20 PM, Ramirez Luna, Omar <omar.ramirez@ti.com> wrote:
> On Wed, Aug 10, 2011 at 6:27 PM, Felipe Contreras
> <felipe.contreras@gmail.com> wrote:
>> On Wed, Aug 10, 2011 at 9:42 PM, Felipe Contreras
>> <felipe.contreras@gmail.com> wrote:
>>> I am trying to use a more recent version of the tidspbridge code in
>>> the Nokia N9, but I'm stuck with this warning that is caused by using
>>> the dm timer framework.
>>
>> Actually, the problem only happens on the 'dspbridge' branch, which
>> has some unmerged patches. These patches introduce some new mutexes
>> that trigger this.
>
> The only lock introduced is a spin_lock_bh which makes sense to warn
> if the underlying functions have a mutex, but I couldn't find the
> clk_set_parent mutex which is causing this on the omap files.

Hmmm, that's true. It seems I also have this series:
http://thread.gmane.org/gmane.linux.ports.arm.omap/18509/focus=18508

That for some reason never made it anywhere. Perhaps it will?

>> My proposed solution is to request the dm timers at initialization
>> time, and just enable/disable them on wake/hibernate, which is a bit
>> similar to what was happening before, and probably more efficient and
>> proper.
>
> When I made these changes I wanted to avoid keeping an array with the
> clocks requested and nobody else being able to use them (even if that
> meant a conflict between two drivers trying to use the same clock, but
> other warnings could help to point to such cases). Also I wanted to
> maintain uniformity, not just requesting some clock, freeing them and
> then some others to be held and freed only on module removal.
>
> That said, I have no problem on changing to your approach if needed.

Yeah, but with the current approach it would be possible that
everything works fine, and then the DSP goes to hibernation, a module
is loaded that uses one dm timer, and then when the DSP wakes up, that
timer would fail, there isn't even a check for that. If I follow the
code correctly, when the DSP goes back to hibernation, there would be
a crash, as a NULL timer would be freed. I think that's too error
prone.

-- 
Felipe Contreras

Re: tidspbridge issue with omap_dm_timer_free

[Ramirez Luna, Omar]

Hi,

On Sun, Aug 14, 2011 at 2:44 AM, Felipe Contreras
<felipe.contreras@gmail.com> wrote:
> Yeah, but with the current approach it would be possible that
> everything works fine, and then the DSP goes to hibernation, a module
> is loaded that uses one dm timer, and then when the DSP wakes up, that
> timer would fail, there isn't even a check for that. If I follow the
> code correctly, when the DSP goes back to hibernation, there would be
> a crash, as a NULL timer would be freed. I think that's too error
> prone.

Yes, this scenario could trigger such error condition. So, FWIW, I'm
fine with just a check or your approach. If possible to apply the same
mechanism for the other clocks, so in the enable/disable functions all
the clocks are then enabled/disabled avoiding mixed behavior of
enable/request, disable/free between gpt and mcbsp clocks.

Regards,

Omar