| Message ID | 1397016042-4451-1-git-send-email-gautam.vivek@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wednesday, April 09, 2014 1:01 PM, Vivek Gautam wrote: > > Patch 'b8efdaf USB: EHCI: add check for wakeup/suspend race' > adds a check for possible race between suspend and wakeup interrupt, > and thereby it returns -EBUSY as error code if there's a wakeup > interrupt. > So the platform host controller should not proceed further with > its suspend callback, rather should return immediately to avoid > powering down the essential things, like phy. > > Signed-off-by: Vivek Gautam <gautam.vivek@samsung.com> > Cc: Alan Stern <stern@rowland.harvard.edu> > Cc: Jingoo Han <jg1.han@samsung.com> Acked-by: Jingoo Han <jg1.han@samsung.com> Best regards, Jingoo Han > --- > > Based on 'usb-next' branch of Greg's usb tree. > > drivers/usb/host/ehci-exynos.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/drivers/usb/host/ehci-exynos.c b/drivers/usb/host/ehci-exynos.c > index d1d8c47..a4550eb 100644 > --- a/drivers/usb/host/ehci-exynos.c > +++ b/drivers/usb/host/ehci-exynos.c > @@ -212,6 +212,8 @@ static int exynos_ehci_suspend(struct device *dev) > int rc; > > rc = ehci_suspend(hcd, do_wakeup); > + if (rc) > + return rc; > > if (exynos_ehci->otg) > exynos_ehci->otg->set_host(exynos_ehci->otg, &hcd->self); > @@ -221,7 +223,7 @@ static int exynos_ehci_suspend(struct device *dev) > > clk_disable_unprepare(exynos_ehci->clk); > > - return rc; > + return 0; > } > > static int exynos_ehci_resume(struct device *dev) > -- > 1.7.10.4
On Wed, 9 Apr 2014, Vivek Gautam wrote: > Patch 'b8efdaf USB: EHCI: add check for wakeup/suspend race' > adds a check for possible race between suspend and wakeup interrupt, > and thereby it returns -EBUSY as error code if there's a wakeup > interrupt. > So the platform host controller should not proceed further with > its suspend callback, rather should return immediately to avoid > powering down the essential things, like phy. > > Signed-off-by: Vivek Gautam <gautam.vivek@samsung.com> > Cc: Alan Stern <stern@rowland.harvard.edu> > Cc: Jingoo Han <jg1.han@samsung.com> > --- > > Based on 'usb-next' branch of Greg's usb tree. > > drivers/usb/host/ehci-exynos.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/drivers/usb/host/ehci-exynos.c b/drivers/usb/host/ehci-exynos.c > index d1d8c47..a4550eb 100644 > --- a/drivers/usb/host/ehci-exynos.c > +++ b/drivers/usb/host/ehci-exynos.c > @@ -212,6 +212,8 @@ static int exynos_ehci_suspend(struct device *dev) > int rc; > > rc = ehci_suspend(hcd, do_wakeup); > + if (rc) > + return rc; > > if (exynos_ehci->otg) > exynos_ehci->otg->set_host(exynos_ehci->otg, &hcd->self); > @@ -221,7 +223,7 @@ static int exynos_ehci_suspend(struct device *dev) > > clk_disable_unprepare(exynos_ehci->clk); > > - return rc; > + return 0; > } > > static int exynos_ehci_resume(struct device *dev) The first hunk of this patch is correct, but the second hunk isn't needed. A similar remark is true for the ehci-platform patch. Alan Stern
On Thursday, April 10, 2014 3:32 AM, Alan Stern wrote: > On Wed, 9 Apr 2014, Vivek Gautam wrote: > > > Patch 'b8efdaf USB: EHCI: add check for wakeup/suspend race' > > adds a check for possible race between suspend and wakeup interrupt, > > and thereby it returns -EBUSY as error code if there's a wakeup > > interrupt. > > So the platform host controller should not proceed further with > > its suspend callback, rather should return immediately to avoid > > powering down the essential things, like phy. > > > > Signed-off-by: Vivek Gautam <gautam.vivek@samsung.com> > > Cc: Alan Stern <stern@rowland.harvard.edu> > > Cc: Jingoo Han <jg1.han@samsung.com> > > --- > > > > Based on 'usb-next' branch of Greg's usb tree. > > > > drivers/usb/host/ehci-exynos.c | 4 +++- > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/usb/host/ehci-exynos.c b/drivers/usb/host/ehci-exynos.c > > index d1d8c47..a4550eb 100644 > > --- a/drivers/usb/host/ehci-exynos.c > > +++ b/drivers/usb/host/ehci-exynos.c > > @@ -212,6 +212,8 @@ static int exynos_ehci_suspend(struct device *dev) > > int rc; > > > > rc = ehci_suspend(hcd, do_wakeup); > > + if (rc) > > + return rc; > > > > if (exynos_ehci->otg) > > exynos_ehci->otg->set_host(exynos_ehci->otg, &hcd->self); > > @@ -221,7 +223,7 @@ static int exynos_ehci_suspend(struct device *dev) > > > > clk_disable_unprepare(exynos_ehci->clk); > > > > - return rc; > > + return 0; > > } > > > > static int exynos_ehci_resume(struct device *dev) > > The first hunk of this patch is correct, but the second hunk isn't > needed. A similar remark is true for the ehci-platform patch. Hi Alan, Do you mean the following? 1st hunk + if (rc) + return rc; 2nd hunk - return rc; + return 0; Currently, the 'rc' will be always 'zero'; however, I don't Have any objection, because the code might be modified later. Best regards, Jingoo Han > > Alan Stern
On Thu, 10 Apr 2014, Jingoo Han wrote: > > > --- a/drivers/usb/host/ehci-exynos.c > > > +++ b/drivers/usb/host/ehci-exynos.c > > > @@ -212,6 +212,8 @@ static int exynos_ehci_suspend(struct device *dev) > > > int rc; > > > > > > rc = ehci_suspend(hcd, do_wakeup); > > > + if (rc) > > > + return rc; > > > > > > if (exynos_ehci->otg) > > > exynos_ehci->otg->set_host(exynos_ehci->otg, &hcd->self); > > > @@ -221,7 +223,7 @@ static int exynos_ehci_suspend(struct device *dev) > > > > > > clk_disable_unprepare(exynos_ehci->clk); > > > > > > - return rc; > > > + return 0; > > > } > > > > > > static int exynos_ehci_resume(struct device *dev) > > > > The first hunk of this patch is correct, but the second hunk isn't > > needed. A similar remark is true for the ehci-platform patch. > > Hi Alan, > > Do you mean the following? > > 1st hunk > + if (rc) > + return rc; > > 2nd hunk > - return rc; > + return 0; Yes, that's what I mean. > Currently, the 'rc' will be always 'zero'; however, I don't > Have any objection, because the code might be modified later. Exactly. We should add the new "if" statement but leave the "return rc" the way it is. Alan Stern
Hi Alan, On Thu, Apr 10, 2014 at 7:06 AM, Alan Stern <stern@rowland.harvard.edu> wrote: > On Thu, 10 Apr 2014, Jingoo Han wrote: > >> > > --- a/drivers/usb/host/ehci-exynos.c >> > > +++ b/drivers/usb/host/ehci-exynos.c >> > > @@ -212,6 +212,8 @@ static int exynos_ehci_suspend(struct device *dev) >> > > int rc; >> > > >> > > rc = ehci_suspend(hcd, do_wakeup); >> > > + if (rc) >> > > + return rc; >> > > >> > > if (exynos_ehci->otg) >> > > exynos_ehci->otg->set_host(exynos_ehci->otg, &hcd->self); >> > > @@ -221,7 +223,7 @@ static int exynos_ehci_suspend(struct device *dev) >> > > >> > > clk_disable_unprepare(exynos_ehci->clk); >> > > >> > > - return rc; >> > > + return 0; >> > > } >> > > >> > > static int exynos_ehci_resume(struct device *dev) >> > >> > The first hunk of this patch is correct, but the second hunk isn't >> > needed. A similar remark is true for the ehci-platform patch. >> >> Hi Alan, >> >> Do you mean the following? >> >> 1st hunk >> + if (rc) >> + return rc; >> >> 2nd hunk >> - return rc; >> + return 0; > > Yes, that's what I mean. > >> Currently, the 'rc' will be always 'zero'; however, I don't >> Have any objection, because the code might be modified later. > > Exactly. We should add the new "if" statement but leave the "return > rc" the way it is. Sure, i will update both the patches. > > Alan Stern > > -- > To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/usb/host/ehci-exynos.c b/drivers/usb/host/ehci-exynos.c index d1d8c47..a4550eb 100644 --- a/drivers/usb/host/ehci-exynos.c +++ b/drivers/usb/host/ehci-exynos.c @@ -212,6 +212,8 @@ static int exynos_ehci_suspend(struct device *dev) int rc; rc = ehci_suspend(hcd, do_wakeup); + if (rc) + return rc; if (exynos_ehci->otg) exynos_ehci->otg->set_host(exynos_ehci->otg, &hcd->self); @@ -221,7 +223,7 @@ static int exynos_ehci_suspend(struct device *dev) clk_disable_unprepare(exynos_ehci->clk); - return rc; + return 0; } static int exynos_ehci_resume(struct device *dev)
Patch 'b8efdaf USB: EHCI: add check for wakeup/suspend race' adds a check for possible race between suspend and wakeup interrupt, and thereby it returns -EBUSY as error code if there's a wakeup interrupt. So the platform host controller should not proceed further with its suspend callback, rather should return immediately to avoid powering down the essential things, like phy. Signed-off-by: Vivek Gautam <gautam.vivek@samsung.com> Cc: Alan Stern <stern@rowland.harvard.edu> Cc: Jingoo Han <jg1.han@samsung.com> --- Based on 'usb-next' branch of Greg's usb tree. drivers/usb/host/ehci-exynos.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)