diff mbox

[1/2] usb: musb: fix the possible panic while resuming

Message ID 4d34a0a70902250353i3c6e072bnd41af327bc77601b@mail.gmail.com (mailing list archive)
State Changes Requested, archived
Delegated to: Felipe Balbi
Headers show

Commit Message

kim kyuwon Feb. 25, 2009, 11:53 a.m. UTC
From: Kim Kyuwon <chammoru@gmail.com>

While waking up, musb can cause a kernel panic. This patch is fixing
it by enabling the clock in the resume_early method.

Signed-off-by: Kim Kyuwon <chammoru@gmail.com>
---
 drivers/usb/musb/musb_core.c |   21 +++++++--------------
 1 files changed, 7 insertions(+), 14 deletions(-)

 		 * the system up quickly enough to respond ...
@@ -2165,20 +2162,17 @@ static int musb_suspend(struct platform_device
*pdev, pm_message_t message)
 		musb->set_clock(musb->clock, 0);
 	else
 		clk_disable(musb->clock);
-	spin_unlock_irqrestore(&musb->lock, flags);
+
 	return 0;
 }

-static int musb_resume(struct platform_device *pdev)
+static int musb_resume_early(struct platform_device *pdev)
 {
-	unsigned long	flags;
-	struct musb	*musb = dev_to_musb(&pdev->dev);
+	struct musb *musb = dev_to_musb(&pdev->dev);

 	if (!musb->clock)
 		return 0;

-	spin_lock_irqsave(&musb->lock, flags);
-
 	if (musb->set_clock)
 		musb->set_clock(musb->clock, 1);
 	else
@@ -2188,7 +2182,6 @@ static int musb_resume(struct platform_device *pdev)
 	 * unless for some reason the whole soc powered down and we're
 	 * not treating that as a whole-system restart (e.g. swsusp)
 	 */
-	spin_unlock_irqrestore(&musb->lock, flags);
 	return 0;
 }

@@ -2205,8 +2198,8 @@ static struct platform_driver musb_driver = {
 	},
 	.remove		= __devexit_p(musb_remove),
 	.shutdown	= musb_shutdown,
-	.suspend	= musb_suspend,
-	.resume		= musb_resume,
+	.suspend_late	= musb_suspend_late,
+	.resume_early	= musb_resume_early,
 };

 /*-------------------------------------------------------------------------*/

Comments

Felipe Balbi Feb. 26, 2009, 12:13 a.m. UTC | #1
On Wed, Feb 25, 2009 at 08:53:00PM +0900, Kim Kyuwon wrote:
> From: Kim Kyuwon <chammoru@gmail.com>
> 
> While waking up, musb can cause a kernel panic. This patch is fixing
> it by enabling the clock in the resume_early method.
> 
> Signed-off-by: Kim Kyuwon <chammoru@gmail.com>
> ---
>  drivers/usb/musb/musb_core.c |   21 +++++++--------------
>  1 files changed, 7 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/usb/musb/musb_core.c b/drivers/usb/musb/musb_core.c
> index 2cc34fa..ae76ad7 100644
> --- a/drivers/usb/musb/musb_core.c
> +++ b/drivers/usb/musb/musb_core.c
> @@ -2141,16 +2141,13 @@ static int __devexit musb_remove(struct
> platform_device *pdev)
> 
>  #ifdef	CONFIG_PM
> 
> -static int musb_suspend(struct platform_device *pdev, pm_message_t message)
> +static int musb_suspend_late(struct platform_device *pdev, pm_message_t state)
>  {
> -	unsigned long	flags;
> -	struct musb	*musb = dev_to_musb(&pdev->dev);
> +	struct musb *musb = dev_to_musb(&pdev->dev);

this hunk is unecessary, please revert. I'll also take a closer look
tomorrow, it's already really late and need some nap (as suggested by
greg, hehe :-p)
kim kyuwon Feb. 26, 2009, 5:54 a.m. UTC | #2
On Thu, Feb 26, 2009 at 9:13 AM, Felipe Balbi <me@felipebalbi.com> wrote:
> On Wed, Feb 25, 2009 at 08:53:00PM +0900, Kim Kyuwon wrote:
>> From: Kim Kyuwon <chammoru@gmail.com>
>>
>> While waking up, musb can cause a kernel panic. This patch is fixing
>> it by enabling the clock in the resume_early method.
>>
>> Signed-off-by: Kim Kyuwon <chammoru@gmail.com>
>> ---
>>  drivers/usb/musb/musb_core.c |   21 +++++++--------------
>>  1 files changed, 7 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/usb/musb/musb_core.c b/drivers/usb/musb/musb_core.c
>> index 2cc34fa..ae76ad7 100644
>> --- a/drivers/usb/musb/musb_core.c
>> +++ b/drivers/usb/musb/musb_core.c
>> @@ -2141,16 +2141,13 @@ static int __devexit musb_remove(struct
>> platform_device *pdev)
>>
>>  #ifdef       CONFIG_PM
>>
>> -static int musb_suspend(struct platform_device *pdev, pm_message_t message)
>> +static int musb_suspend_late(struct platform_device *pdev, pm_message_t state)
>>  {
>> -     unsigned long   flags;
>> -     struct musb     *musb = dev_to_musb(&pdev->dev);
>> +     struct musb *musb = dev_to_musb(&pdev->dev);
>
> this hunk is unecessary, please revert. I'll also take a closer look
> tomorrow, it's already really late and need some nap (as suggested by
> greg, hehe :-p)
>

OK, but before I resend the patch, I have something to check again.

Now I know that, in addition to HSUSB_MC_NINT disabled by the previous
patch [ARM: OMAP: Disable USB interrupt in the musb_resume()
function], USBTLL_SWAKEUP and USBHOST_SWAKEUP can be also wake-up
source events to PRMC module. Sorry I didn't know that time. The
remote wake uses these two SWAKEUP. David, is my previous patch is
still NAK?

> --
> balbi
>
Anand Gadiyar Feb. 26, 2009, 6:37 a.m. UTC | #3
<snip>

> Now I know that, in addition to HSUSB_MC_NINT disabled by the previous
> patch [ARM: OMAP: Disable USB interrupt in the musb_resume()
> function], USBTLL_SWAKEUP and USBHOST_SWAKEUP can be also wake-up
> source events to PRMC module. Sorry I didn't know that time. The
> remote wake uses these two SWAKEUP. David, is my previous patch is
> still NAK?

USBTLL_SWAKEUP and  USBHOST_SWAKEUP are not for MUSB.


- Anand--
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
kim kyuwon Feb. 26, 2009, 6:57 a.m. UTC | #4
Hi,

On Thu, Feb 26, 2009 at 3:37 PM, Gadiyar, Anand <gadiyar@ti.com> wrote:
> <snip>
>
>> Now I know that, in addition to HSUSB_MC_NINT disabled by the previous
>> patch [ARM: OMAP: Disable USB interrupt in the musb_resume()
>> function], USBTLL_SWAKEUP and USBHOST_SWAKEUP can be also wake-up
>> source events to PRMC module. Sorry I didn't know that time. The
>> remote wake uses these two SWAKEUP. David, is my previous patch is
>> still NAK?
>
> USBTLL_SWAKEUP and  USBHOST_SWAKEUP are not for MUSB.
>

But they are used to wake up request. Can I ask what they are for?

>
> - Anand
Anand Gadiyar Feb. 26, 2009, 7:20 a.m. UTC | #5
> -----Original Message-----
> From: Kim Kyuwon [mailto:chammoru@gmail.com] 
> Sent: Thursday, February 26, 2009 12:28 PM
> To: Gadiyar, Anand
> Cc: me@felipebalbi.com; linux-usb@vger.kernel.org; OMAP; 
> David Brownell; q1.kim@samsung.com; Minkyu Kang
> Subject: Re: [PATCH 1/2] usb: musb: fix the possible panic 
> while resuming
> 
> Hi,
> 
> On Thu, Feb 26, 2009 at 3:37 PM, Gadiyar, Anand <gadiyar@ti.com> wrote:
> > <snip>
> >
> >> Now I know that, in addition to HSUSB_MC_NINT disabled by the previous
> >> patch [ARM: OMAP: Disable USB interrupt in the musb_resume()
> >> function], USBTLL_SWAKEUP and USBHOST_SWAKEUP can be also wake-up
> >> source events to PRMC module. Sorry I didn't know that time. The
> >> remote wake uses these two SWAKEUP. David, is my previous patch is
> >> still NAK?
> >
> > USBTLL_SWAKEUP and  USBHOST_SWAKEUP are not for MUSB.
> >
> 
> But they are used to wake up request. Can I ask what they are for?
> 

I'm not the PRCM expert, but USBHOST refers to EHCI/OHCI and USBTLL is
needed for EHCI (except in PHY mode) and for OHCI (all modes). These
wakeup sources are meant for wakeups initiated by these modules.

MUSB is a different module altogether.

--
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
diff mbox

Patch

diff --git a/drivers/usb/musb/musb_core.c b/drivers/usb/musb/musb_core.c
index 2cc34fa..ae76ad7 100644
--- a/drivers/usb/musb/musb_core.c
+++ b/drivers/usb/musb/musb_core.c
@@ -2141,16 +2141,13 @@  static int __devexit musb_remove(struct
platform_device *pdev)

 #ifdef	CONFIG_PM

-static int musb_suspend(struct platform_device *pdev, pm_message_t message)
+static int musb_suspend_late(struct platform_device *pdev, pm_message_t state)
 {
-	unsigned long	flags;
-	struct musb	*musb = dev_to_musb(&pdev->dev);
+	struct musb *musb = dev_to_musb(&pdev->dev);

 	if (!musb->clock)
 		return 0;

-	spin_lock_irqsave(&musb->lock, flags);
-
 	if (is_peripheral_active(musb)) {
 		/* FIXME force disconnect unless we know USB will wake