[PATCH 06/11] USB: OHCI: Properly handle ohci-exynos suspend
From: Manjunath Goudar 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 Signed-off-by: Manjunath Goudar Acked-by: Alan Stern Cc: Arnd Bergmann Cc: Greg KH Cc: linux-usb@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 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 06/11] USB: OHCI: Properly handle ohci-exynos suspend
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 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 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 06/11] USB: OHCI: Properly handle ohci-exynos suspend
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 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 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 06/11] USB: OHCI: Properly handle ohci-exynos suspend
On Thu, 3 Oct 2013, manju goudar wrote: > On Wed, Oct 2, 2013 at 9:22 PM, Alan Stern 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 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 06/11] USB: OHCI: Properly handle ohci-exynos suspend
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 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 06/11] USB: OHCI: Properly handle ohci-exynos suspend
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 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 06/11] USB: OHCI: Properly handle ohci-exynos suspend
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 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 06/11] USB: OHCI: Properly handle ohci-exynos suspend
Hi, On Wednesday, October 02, 2013 03:45:42 PM Majunath Goudar wrote: > From: Manjunath Goudar > > 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 > Signed-off-by: Manjunath Goudar > Acked-by: Alan Stern > Cc: Arnd Bergmann > Cc: Greg KH > Cc: linux-usb@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 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 06/11] USB: OHCI: Properly handle ohci-exynos suspend
From: Manjunath Goudar 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 Signed-off-by: Manjunath Goudar Acked-by: Alan Stern Cc: Arnd Bergmann Cc: Greg KH Cc: linux-usb@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 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 06/11] USB: OHCI: Properly handle ohci-exynos suspend
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 Acked-by: Alan Stern Cc: Arnd Bergmann Cc: Greg KH Cc: linux-usb@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 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html