[PATCH 06/11] USB: OHCI: Properly handle ohci-exynos suspend

[PATCH 06/11] USB: OHCI: Properly handle ohci-exynos suspend

[Manjunath Goudar]

Suspend scenario in case of ohci-exynos glue was not
properly handled as it was not suspending generic part
of ohci controller. Calling explicitly the ohci_suspend()
routine in exynos_ohci_suspend() will ensure proper
handling of suspend scenario.

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

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

V3:
 -rid of unwanted code from ohci_hcd_s3c2410_drv_suspend() which already
  ohci_suspend() does it.
 -Aligned variable "do_wakeup" and "ret".

V4:
 -The do_wakeup and rc variable alignment is removed.
---
 drivers/usb/host/ohci-exynos.c |   20 +++++---------------
 1 file changed, 5 insertions(+), 15 deletions(-)

diff --git a/drivers/usb/host/ohci-exynos.c b/drivers/usb/host/ohci-exynos.c
index ae6068d..17de3dd 100644
--- a/drivers/usb/host/ohci-exynos.c
+++ b/drivers/usb/host/ohci-exynos.c
@@ -203,24 +203,15 @@ static int exynos_ohci_suspend(struct device *dev)
 	struct exynos_ohci_hcd *exynos_ohci = to_exynos_ohci(hcd);
 	struct ohci_hcd *ohci = hcd_to_ohci(hcd);
 	struct platform_device *pdev = to_platform_device(dev);
+	bool do_wakeup = device_may_wakeup(dev);
 	unsigned long flags;
 	int rc = 0;
 
-	/*
-	 * Root hub was already suspended. Disable irq emission and
-	 * mark HW unaccessible, bail out if RH has been resumed. Use
-	 * the spinlock to properly synchronize with possible pending
-	 * RH suspend or resume activity.
-	 */
-	spin_lock_irqsave(&ohci->lock, flags);
-	if (ohci->rh_state != OHCI_RH_SUSPENDED &&
-			ohci->rh_state != OHCI_RH_HALTED) {
-		rc = -EINVAL;
-		goto fail;
-	}
-
-	clear_bit(HCD_FLAG_HW_ACCESSIBLE, &hcd->flags);
+	rc = ohci_suspend(hcd, do_wakeup);
+	if (rc)
+		return rc;
 
+	spin_lock_irqsave(&ohci->lock, flags);
 	if (exynos_ohci->otg)
 		exynos_ohci->otg->set_host(exynos_ohci->otg, &hcd->self);
 
@@ -228,7 +219,6 @@ static int exynos_ohci_suspend(struct device *dev)
 
 	clk_disable_unprepare(exynos_ohci->clk);
 
-fail:
 	spin_unlock_irqrestore(&ohci->lock, flags);
 
 	return rc;
-- 
1.7.9.5

[PATCH 06/11] USB: OHCI: Properly handle ohci-exynos suspend

[Majunath Goudar]

From: Manjunath Goudar <manjunath.goudar@linaro.org>

Suspend scenario in case of ohci-exynos glue was not
properly handled as it was not suspending generic part
of ohci controller. Alan Stern suggested, properly handle
ohci-exynos suspend scenario.

Calling explicitly the ohci_suspend() routine in
exynos_ohci_suspend() will ensure proper handling of suspend
scenario.

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

diff --git a/drivers/usb/host/ohci-exynos.c b/drivers/usb/host/ohci-exynos.c
index 3e4bc74..f5f372e 100644
--- a/drivers/usb/host/ohci-exynos.c
+++ b/drivers/usb/host/ohci-exynos.c
@@ -203,24 +203,15 @@ static int exynos_ohci_suspend(struct device *dev)
 	struct exynos_ohci_hcd *exynos_ohci = to_exynos_ohci(hcd);
 	struct ohci_hcd *ohci = hcd_to_ohci(hcd);
 	struct platform_device *pdev = to_platform_device(dev);
+	bool do_wakeup = device_may_wakeup(dev);
 	unsigned long flags;
 	int rc = 0;
 
-	/*
-	 * Root hub was already suspended. Disable irq emission and
-	 * mark HW unaccessible, bail out if RH has been resumed. Use
-	 * the spinlock to properly synchronize with possible pending
-	 * RH suspend or resume activity.
-	 */
-	spin_lock_irqsave(&ohci->lock, flags);
-	if (ohci->rh_state != OHCI_RH_SUSPENDED &&
-			ohci->rh_state != OHCI_RH_HALTED) {
-		rc = -EINVAL;
-		goto fail;
-	}
-
-	clear_bit(HCD_FLAG_HW_ACCESSIBLE, &hcd->flags);
+	rc = ohci_suspend(hcd, do_wakeup);
+	if (rc)
+		return rc;
 
+	spin_lock_irqsave(&ohci->lock, flags);
 	if (exynos_ohci->otg)
 		exynos_ohci->otg->set_host(exynos_ohci->otg, &hcd->self);
 
@@ -228,7 +219,6 @@ static int exynos_ohci_suspend(struct device *dev)
 
 	clk_disable_unprepare(exynos_ohci->clk);
 
-fail:
 	spin_unlock_irqrestore(&ohci->lock, flags);
 
 	return rc;
-- 
1.7.9.5

[PATCH 06/11] USB: OHCI: Properly handle ohci-exynos suspend

[Bartlomiej Zolnierkiewicz]

Hi,

On Wednesday, October 02, 2013 03:45:42 PM Majunath Goudar wrote:
> From: Manjunath Goudar <manjunath.goudar@linaro.org>
> 
> Suspend scenario in case of ohci-exynos glue was not
> properly handled as it was not suspending generic part
> of ohci controller. Alan Stern suggested, properly handle
> ohci-exynos suspend scenario.
> 
> Calling explicitly the ohci_suspend() routine in
> exynos_ohci_suspend() will ensure proper handling of suspend
> scenario.
> 
> Signed-off-by: Manjunath Goudar <manjunath.goudar@linaro.org>
> Signed-off-by: Manjunath Goudar <csmanjuvijay@gmail.com>
> Acked-by: Alan Stern <stern@rowland.harvard.edu>
> Cc: Arnd Bergmann <arnd@arndb.de>
> Cc: Greg KH <greg@kroah.com>
> Cc: linux-usb at vger.kernel.org
> ---
>  drivers/usb/host/ohci-exynos.c |   20 +++++---------------
>  1 file changed, 5 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/usb/host/ohci-exynos.c b/drivers/usb/host/ohci-exynos.c
> index 3e4bc74..f5f372e 100644
> --- a/drivers/usb/host/ohci-exynos.c
> +++ b/drivers/usb/host/ohci-exynos.c
> @@ -203,24 +203,15 @@ static int exynos_ohci_suspend(struct device *dev)
>  	struct exynos_ohci_hcd *exynos_ohci = to_exynos_ohci(hcd);
>  	struct ohci_hcd *ohci = hcd_to_ohci(hcd);
>  	struct platform_device *pdev = to_platform_device(dev);
> +	bool do_wakeup = device_may_wakeup(dev);
>  	unsigned long flags;
>  	int rc = 0;
>  
> -	/*
> -	 * Root hub was already suspended. Disable irq emission and
> -	 * mark HW unaccessible, bail out if RH has been resumed. Use
> -	 * the spinlock to properly synchronize with possible pending
> -	 * RH suspend or resume activity.
> -	 */
> -	spin_lock_irqsave(&ohci->lock, flags);
> -	if (ohci->rh_state != OHCI_RH_SUSPENDED &&
> -			ohci->rh_state != OHCI_RH_HALTED) {
> -		rc = -EINVAL;
> -		goto fail;
> -	}
> -
> -	clear_bit(HCD_FLAG_HW_ACCESSIBLE, &hcd->flags);
> +	rc = ohci_suspend(hcd, do_wakeup);

Maybe it would make sense to cleanup ohci_suspend() first (before adding
new ohci_suspend() users) and remove unused do_wakeup parameter?

> +	if (rc)
> +		return rc;
>  
> +	spin_lock_irqsave(&ohci->lock, flags);
>  	if (exynos_ohci->otg)
>  		exynos_ohci->otg->set_host(exynos_ohci->otg, &hcd->self);
>  
> @@ -228,7 +219,6 @@ static int exynos_ohci_suspend(struct device *dev)
>  
>  	clk_disable_unprepare(exynos_ohci->clk);
>  
> -fail:
>  	spin_unlock_irqrestore(&ohci->lock, flags);
>  
>  	return rc;

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics

[PATCH 06/11] USB: OHCI: Properly handle ohci-exynos suspend

[Alan Stern]

On Wed, 2 Oct 2013, Bartlomiej Zolnierkiewicz wrote:

> Maybe it would make sense to cleanup ohci_suspend() first (before adding
> new ohci_suspend() users) and remove unused do_wakeup parameter?

Not possible.  The do_wakeup parameter is part of a function prototype
shared by other callback routines (such as ehci_suspend()) that _do_
use the parameter.

Alan Stern

[PATCH 06/11] USB: OHCI: Properly handle ohci-exynos suspend

[Bartlomiej Zolnierkiewicz]

Hi,

On Wednesday, October 02, 2013 10:38:58 AM Alan Stern wrote:
> On Wed, 2 Oct 2013, Bartlomiej Zolnierkiewicz wrote:
> 
> > Maybe it would make sense to cleanup ohci_suspend() first (before adding
> > new ohci_suspend() users) and remove unused do_wakeup parameter?
> 
> Not possible.  The do_wakeup parameter is part of a function prototype
> shared by other callback routines (such as ehci_suspend()) that _do_
> use the parameter.

If you mean ohci-pci.c usage (which is currently the only usage of
ohci_suspend() looking at the latest -next kernel) than it is enough
to add a simple wrapper for it in ohci-pci.c:

...
static int ohci_pci_suspend(struct usb_hcd *hcd, bool do_wakeup)
{
	ohci_suspend(hcd);
}
...
    ohci_pci_hc_driver.pci_suspend = ohci_pci_suspend;
...


Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics

[PATCH 06/11] USB: OHCI: Properly handle ohci-exynos suspend

[Alan Stern]

On Wed, 2 Oct 2013, Bartlomiej Zolnierkiewicz wrote:

> 
> Hi,
> 
> On Wednesday, October 02, 2013 10:38:58 AM Alan Stern wrote:
> > On Wed, 2 Oct 2013, Bartlomiej Zolnierkiewicz wrote:
> > 
> > > Maybe it would make sense to cleanup ohci_suspend() first (before adding
> > > new ohci_suspend() users) and remove unused do_wakeup parameter?
> > 
> > Not possible.  The do_wakeup parameter is part of a function prototype
> > shared by other callback routines (such as ehci_suspend()) that _do_
> > use the parameter.
> 
> If you mean ohci-pci.c usage (which is currently the only usage of
> ohci_suspend() looking at the latest -next kernel) than it is enough
> to add a simple wrapper for it in ohci-pci.c:
> 
> ...
> static int ohci_pci_suspend(struct usb_hcd *hcd, bool do_wakeup)
> {
> 	ohci_suspend(hcd);
> }
> ...
>     ohci_pci_hc_driver.pci_suspend = ohci_pci_suspend;
> ...

Ah, now I see your point.  Yes, it's true; that parameter could be 
eliminated.

Manjunath, would you like to update your patch series to get rid of the 
do_wakeup argument to ohci_suspend()?

Alan Stern

[PATCH 06/11] USB: OHCI: Properly handle ohci-exynos suspend

[Alan Stern]

On Thu, 3 Oct 2013, manju goudar wrote:

> On Wed, Oct 2, 2013 at 9:22 PM, Alan Stern <stern@rowland.harvard.edu>wrote:
> 
> > On Wed, 2 Oct 2013, Bartlomiej Zolnierkiewicz wrote:
> >
> > >
> > > Hi,
> > >
> > > On Wednesday, October 02, 2013 10:38:58 AM Alan Stern wrote:
> > > > On Wed, 2 Oct 2013, Bartlomiej Zolnierkiewicz wrote:
> > > >
> > > > > Maybe it would make sense to cleanup ohci_suspend() first (before
> > adding
> > > > > new ohci_suspend() users) and remove unused do_wakeup parameter?
> > > >
> > > > Not possible.  The do_wakeup parameter is part of a function prototype
> > > > shared by other callback routines (such as ehci_suspend()) that _do_
> > > > use the parameter.
> > >
> > > If you mean ohci-pci.c usage (which is currently the only usage of
> > > ohci_suspend() looking at the latest -next kernel) than it is enough
> > > to add a simple wrapper for it in ohci-pci.c:
> > >
> > > ...
> > > static int ohci_pci_suspend(struct usb_hcd *hcd, bool do_wakeup)
> > > {
> > >       ohci_suspend(hcd);
> > > }
> > > ...
> > >     ohci_pci_hc_driver.pci_suspend = ohci_pci_suspend;
> > > ...
> >
> > Ah, now I see your point.  Yes, it's true; that parameter could be
> > eliminated.
> >
> > Manjunath, would you like to update your patch series to get rid of the
> > do_wakeup argument to ohci_suspend()?
> >
> > Yes I will do. I think we can also rid of ehci_suspend() do_wakeup
> argument.

Arrgh!  Manjunath, I was wrong.  I'm sorry to make you do all this
extra work -- your original patch series was correct.

Bartlomiej, we both failed to notice that the 1/11 patch in the 
original series adds a usage of do_wakeup.  Therefore that argument 
cannot be removed.

Greg, please ignore Manjunath's V2 series (sent today) and merge the 
original 11-patch series posted on October 2.

Alan Stern

[PATCH 06/11] USB: OHCI: Properly handle ohci-exynos suspend

[Bartlomiej Zolnierkiewicz]

On Thursday, October 03, 2013 10:31:53 AM Alan Stern wrote:
> On Thu, 3 Oct 2013, manju goudar wrote:
> 
> > On Wed, Oct 2, 2013 at 9:22 PM, Alan Stern <stern@rowland.harvard.edu>wrote:
> > 
> > > On Wed, 2 Oct 2013, Bartlomiej Zolnierkiewicz wrote:
> > >
> > > >
> > > > Hi,
> > > >
> > > > On Wednesday, October 02, 2013 10:38:58 AM Alan Stern wrote:
> > > > > On Wed, 2 Oct 2013, Bartlomiej Zolnierkiewicz wrote:
> > > > >
> > > > > > Maybe it would make sense to cleanup ohci_suspend() first (before
> > > adding
> > > > > > new ohci_suspend() users) and remove unused do_wakeup parameter?
> > > > >
> > > > > Not possible.  The do_wakeup parameter is part of a function prototype
> > > > > shared by other callback routines (such as ehci_suspend()) that _do_
> > > > > use the parameter.
> > > >
> > > > If you mean ohci-pci.c usage (which is currently the only usage of
> > > > ohci_suspend() looking at the latest -next kernel) than it is enough
> > > > to add a simple wrapper for it in ohci-pci.c:
> > > >
> > > > ...
> > > > static int ohci_pci_suspend(struct usb_hcd *hcd, bool do_wakeup)
> > > > {
> > > >       ohci_suspend(hcd);
> > > > }
> > > > ...
> > > >     ohci_pci_hc_driver.pci_suspend = ohci_pci_suspend;
> > > > ...
> > >
> > > Ah, now I see your point.  Yes, it's true; that parameter could be
> > > eliminated.
> > >
> > > Manjunath, would you like to update your patch series to get rid of the
> > > do_wakeup argument to ohci_suspend()?
> > >
> > > Yes I will do. I think we can also rid of ehci_suspend() do_wakeup
> > argument.
> 
> Arrgh!  Manjunath, I was wrong.  I'm sorry to make you do all this
> extra work -- your original patch series was correct.
> 
> Bartlomiej, we both failed to notice that the 1/11 patch in the 
> original series adds a usage of do_wakeup.  Therefore that argument 
> cannot be removed.

Uhh, indeed. Sorry about that.

> Greg, please ignore Manjunath's V2 series (sent today) and merge the 
> original 11-patch series posted on October 2.

Yep.

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics

[PATCH 06/11] USB: OHCI: Properly handle ohci-exynos suspend

[Greg KH]

On Thu, Oct 03, 2013 at 10:31:53AM -0400, Alan Stern wrote:
> On Thu, 3 Oct 2013, manju goudar wrote:
> 
> > On Wed, Oct 2, 2013 at 9:22 PM, Alan Stern <stern@rowland.harvard.edu>wrote:
> > 
> > > On Wed, 2 Oct 2013, Bartlomiej Zolnierkiewicz wrote:
> > >
> > > >
> > > > Hi,
> > > >
> > > > On Wednesday, October 02, 2013 10:38:58 AM Alan Stern wrote:
> > > > > On Wed, 2 Oct 2013, Bartlomiej Zolnierkiewicz wrote:
> > > > >
> > > > > > Maybe it would make sense to cleanup ohci_suspend() first (before
> > > adding
> > > > > > new ohci_suspend() users) and remove unused do_wakeup parameter?
> > > > >
> > > > > Not possible.  The do_wakeup parameter is part of a function prototype
> > > > > shared by other callback routines (such as ehci_suspend()) that _do_
> > > > > use the parameter.
> > > >
> > > > If you mean ohci-pci.c usage (which is currently the only usage of
> > > > ohci_suspend() looking at the latest -next kernel) than it is enough
> > > > to add a simple wrapper for it in ohci-pci.c:
> > > >
> > > > ...
> > > > static int ohci_pci_suspend(struct usb_hcd *hcd, bool do_wakeup)
> > > > {
> > > >       ohci_suspend(hcd);
> > > > }
> > > > ...
> > > >     ohci_pci_hc_driver.pci_suspend = ohci_pci_suspend;
> > > > ...
> > >
> > > Ah, now I see your point.  Yes, it's true; that parameter could be
> > > eliminated.
> > >
> > > Manjunath, would you like to update your patch series to get rid of the
> > > do_wakeup argument to ohci_suspend()?
> > >
> > > Yes I will do. I think we can also rid of ehci_suspend() do_wakeup
> > argument.
> 
> Arrgh!  Manjunath, I was wrong.  I'm sorry to make you do all this
> extra work -- your original patch series was correct.
> 
> Bartlomiej, we both failed to notice that the 1/11 patch in the 
> original series adds a usage of do_wakeup.  Therefore that argument 
> cannot be removed.
> 
> Greg, please ignore Manjunath's V2 series (sent today) and merge the 
> original 11-patch series posted on October 2.

I no longer have these.  Manjunath, can you please resend the "correct"
series that I should apply so that it is obvious which is your latest
version?

thanks,

greg k-h