[10/10] USB: OHCI: Properly handle ohci-spear suspend

Message ID 1371114661-2207-11-git-send-email-manjunath.goudar@linaro.org
State New
Headers show

Commit Message

manjunath.goudar@linaro.org June 13, 2013, 9:11 a.m.
Suspend scenario in case of ohci-spear glue was not
properly handled as it was not suspending generic part
of ohci controller.Calling explicitly the ohci_suspend()
routine in spear_ohci_hcd_drv_suspend() will ensure proper
handling of suspend scenario.

V2:
 -Incase ohci_suspend() fails, return right away without
  executing further.

Signed-off-by: Manjunath Goudar <manjunath.goudar@linaro.org>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Alan Stern <stern@rowland.harvard.edu>
Cc: Greg KH <greg@kroah.com>
Cc: linux-usb@vger.kernel.org
---
 drivers/usb/host/ohci-spear.c |   16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

Comments

Viresh Kumar June 13, 2013, 9:19 a.m. | #1
On 13 June 2013 14:41, Manjunath Goudar <manjunath.goudar@linaro.org> wrote:
> Suspend scenario in case of ohci-spear glue was not
> properly handled as it was not suspending generic part
> of ohci controller.Calling explicitly the ohci_suspend()

Add space after full stop.

> routine in spear_ohci_hcd_drv_suspend() will ensure proper
> handling of suspend scenario.
>
> V2:
>  -Incase ohci_suspend() fails, return right away without
>   executing further.

You added changelog at wrong place. This will get commited once
somebody applies this patch.

> Signed-off-by: Manjunath Goudar <manjunath.goudar@linaro.org>
> Cc: Arnd Bergmann <arnd@arndb.de>
> Cc: Alan Stern <stern@rowland.harvard.edu>
> Cc: Greg KH <greg@kroah.com>
> Cc: linux-usb@vger.kernel.org
> ---

Add it here.

>  drivers/usb/host/ohci-spear.c |   16 +++++++++++++---
>  1 file changed, 13 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/usb/host/ohci-spear.c b/drivers/usb/host/ohci-spear.c
> index 31ff3fc..2ff867b 100644
> --- a/drivers/usb/host/ohci-spear.c
> +++ b/drivers/usb/host/ohci-spear.c
> @@ -130,12 +130,22 @@ static int spear_ohci_hcd_drv_remove(struct platform_device *pdev)
>  }
>
>  #if defined(CONFIG_PM)
> -static int spear_ohci_hcd_drv_suspend(struct platform_device *dev,
> +static int spear_ohci_hcd_drv_suspend(struct platform_device *pdev,
>                 pm_message_t message)
>  {
> -       struct usb_hcd *hcd = platform_get_drvdata(dev);
> +       struct usb_hcd *hcd = platform_get_drvdata(pdev);
>         struct ohci_hcd *ohci = hcd_to_ohci(hcd);
>         struct spear_ohci *sohci_p = to_spear_ohci(hcd);
> +       bool do_wakeup = device_may_wakeup(&pdev->dev);
> +       int ret;
> +
> +       ret = ohci_suspend(hcd, do_wakeup);
> +       if (ret == 0 && do_wakeup && HCD_WAKEUP_PENDING(hcd)) {

maybe s/ret == 0/!ret

I thought ret == 0 is the success case and not error case.

> +               ohci_resume(hcd, false);
> +               ret = -EBUSY;
> +       }
> +       if (ret)
> +               return ret;
>
>         if (time_before(jiffies, ohci->next_statechange))
>                 msleep(5);
> @@ -143,7 +153,7 @@ static int spear_ohci_hcd_drv_suspend(struct platform_device *dev,
>
>         clk_disable_unprepare(sohci_p->clk);
>
> -       return 0;
> +       return ret;
>  }
>
>  static int spear_ohci_hcd_drv_resume(struct platform_device *dev)
> --
> 1.7.9.5
>
>
> _______________________________________________
> linaro-dev mailing list
> linaro-dev@lists.linaro.org
> http://lists.linaro.org/mailman/listinfo/linaro-dev
manjunath.goudar@linaro.org June 13, 2013, 10:27 a.m. | #2
On 13 June 2013 14:49, Viresh Kumar <viresh.kumar@linaro.org> wrote:

> On 13 June 2013 14:41, Manjunath Goudar <manjunath.goudar@linaro.org>
> wrote:
> > Suspend scenario in case of ohci-spear glue was not
> > properly handled as it was not suspending generic part
> > of ohci controller.Calling explicitly the ohci_suspend()
>
> Add space after full stop.
>
> > routine in spear_ohci_hcd_drv_suspend() will ensure proper
> > handling of suspend scenario.
> >
> > V2:
> >  -Incase ohci_suspend() fails, return right away without
> >   executing further.
>
> You added changelog at wrong place. This will get commited once
> somebody applies this patch.
>
> > Signed-off-by: Manjunath Goudar <manjunath.goudar@linaro.org>
> > Cc: Arnd Bergmann <arnd@arndb.de>
> > Cc: Alan Stern <stern@rowland.harvard.edu>
> > Cc: Greg KH <greg@kroah.com>
> > Cc: linux-usb@vger.kernel.org
> > ---
>
> Add it here.
>
> >  drivers/usb/host/ohci-spear.c |   16 +++++++++++++---
> >  1 file changed, 13 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/usb/host/ohci-spear.c
> b/drivers/usb/host/ohci-spear.c
> > index 31ff3fc..2ff867b 100644
> > --- a/drivers/usb/host/ohci-spear.c
> > +++ b/drivers/usb/host/ohci-spear.c
> > @@ -130,12 +130,22 @@ static int spear_ohci_hcd_drv_remove(struct
> platform_device *pdev)
> >  }
> >
> >  #if defined(CONFIG_PM)
> > -static int spear_ohci_hcd_drv_suspend(struct platform_device *dev,
> > +static int spear_ohci_hcd_drv_suspend(struct platform_device *pdev,
> >                 pm_message_t message)
> >  {
> > -       struct usb_hcd *hcd = platform_get_drvdata(dev);
> > +       struct usb_hcd *hcd = platform_get_drvdata(pdev);
> >         struct ohci_hcd *ohci = hcd_to_ohci(hcd);
> >         struct spear_ohci *sohci_p = to_spear_ohci(hcd);
> > +       bool do_wakeup = device_may_wakeup(&pdev->dev);
> > +       int ret;
> > +
> > +       ret = ohci_suspend(hcd, do_wakeup);
> > +       if (ret == 0 && do_wakeup && HCD_WAKEUP_PENDING(hcd)) {
>
> maybe s/ret == 0/!ret
>
> I thought ret == 0 is the success case and not error case



yes ret == 0 is success when do_wakeup=false and HCD_WAKEUP_PENDING()
return 0  or any one return 0 otherwise it is fail only.


> > +               ohci_resume(hcd, false);
> > +               ret = -EBUSY;
> > +       }
> > +       if (ret)
> > +               return ret;
> >
> >         if (time_before(jiffies, ohci->next_statechange))
> >                 msleep(5);
> > @@ -143,7 +153,7 @@ static int spear_ohci_hcd_drv_suspend(struct
> platform_device *dev,
> >
> >         clk_disable_unprepare(sohci_p->clk);
> >
> > -       return 0;
> > +       return ret;
> >  }
> >
> >  static int spear_ohci_hcd_drv_resume(struct platform_device *dev)
> > --
> > 1.7.9.5
> >
> >
> > _______________________________________________
> > linaro-dev mailing list
> > linaro-dev@lists.linaro.org
> > http://lists.linaro.org/mailman/listinfo/linaro-dev
>
Alan Stern June 13, 2013, 2:33 p.m. | #3
On Thu, 13 Jun 2013, Viresh Kumar wrote:

> > +       bool do_wakeup = device_may_wakeup(&pdev->dev);
> > +       int ret;
> > +
> > +       ret = ohci_suspend(hcd, do_wakeup);
> > +       if (ret == 0 && do_wakeup && HCD_WAKEUP_PENDING(hcd)) {
> 
> maybe s/ret == 0/!ret

I tend to prefer using ! for boolean values and == 0 for numeric 
values.  Just a matter of personal taste.

> I thought ret == 0 is the success case and not error case.
> 
> > +               ohci_resume(hcd, false);
> > +               ret = -EBUSY;
> > +       }

Yes, 0 is the success case.  You must not have seen the explanation for
this code; it is needed to handle a race.  If the suspend succeeds but
a wakeup request has already arrived, we need to undo the suspend.  
Obviously, if the suspend failed then there's no need to undo anything.

Alan Stern

Patch

diff --git a/drivers/usb/host/ohci-spear.c b/drivers/usb/host/ohci-spear.c
index 31ff3fc..2ff867b 100644
--- a/drivers/usb/host/ohci-spear.c
+++ b/drivers/usb/host/ohci-spear.c
@@ -130,12 +130,22 @@  static int spear_ohci_hcd_drv_remove(struct platform_device *pdev)
 }
 
 #if defined(CONFIG_PM)
-static int spear_ohci_hcd_drv_suspend(struct platform_device *dev,
+static int spear_ohci_hcd_drv_suspend(struct platform_device *pdev,
 		pm_message_t message)
 {
-	struct usb_hcd *hcd = platform_get_drvdata(dev);
+	struct usb_hcd *hcd = platform_get_drvdata(pdev);
 	struct ohci_hcd	*ohci = hcd_to_ohci(hcd);
 	struct spear_ohci *sohci_p = to_spear_ohci(hcd);
+	bool do_wakeup = device_may_wakeup(&pdev->dev);
+	int ret;
+
+	ret = ohci_suspend(hcd, do_wakeup);
+	if (ret == 0 && do_wakeup && HCD_WAKEUP_PENDING(hcd)) {
+		ohci_resume(hcd, false);
+		ret = -EBUSY;
+	}
+	if (ret)
+		return ret;
 
 	if (time_before(jiffies, ohci->next_statechange))
 		msleep(5);
@@ -143,7 +153,7 @@  static int spear_ohci_hcd_drv_suspend(struct platform_device *dev,
 
 	clk_disable_unprepare(sohci_p->clk);
 
-	return 0;
+	return ret;
 }
 
 static int spear_ohci_hcd_drv_resume(struct platform_device *dev)