2020-10-26 08:46:27

by Chunyan Zhang

Subject: [PATCH 1/3] watchdog: sprd: should not disable watchdog in resume

From: Lingling Xu <[email protected]>

Don't disable watchdog in resume process, otherwise system would crash
once kick watchdog.

Fixes: 477603467009 ("watchdog: Add Spreadtrum watchdog driver")
Signed-off-by: Lingling Xu <[email protected]>
Signed-off-by: Chunyan Zhang <[email protected]>
---
drivers/watchdog/sprd_wdt.c | 9 ++-------
1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/drivers/watchdog/sprd_wdt.c b/drivers/watchdog/sprd_wdt.c
index 65cb55f3916f..f3c90b4afead 100644
--- a/drivers/watchdog/sprd_wdt.c
+++ b/drivers/watchdog/sprd_wdt.c
@@ -345,15 +345,10 @@ static int __maybe_unused sprd_wdt_pm_resume(struct device *dev)
if (ret)
return ret;

- if (watchdog_active(&wdt->wdd)) {
+ if (watchdog_active(&wdt->wdd))
ret = sprd_wdt_start(&wdt->wdd);
- if (ret) {
- sprd_wdt_disable(wdt);
- return ret;
- }
- }

- return 0;
+ return ret;
}

static const struct dev_pm_ops sprd_wdt_pm_ops = {
--
2.20.1

2020-10-26 14:30:20

by Guenter Roeck

Subject: Re: [PATCH 1/3] watchdog: sprd: should not disable watchdog in resume

On 10/26/20 1:09 AM, Chunyan Zhang wrote:
> From: Lingling Xu <[email protected]>
>
> Don't disable watchdog in resume process, otherwise system would crash
> once kick watchdog.
>

This is a bit misleading: It is only disabled if the attempt to start it
has failed. Was this observed in practice ? If so, it might make sense
to identify and fix the underlying problem instead of trying to work
around it (or is this addressed with the second patch of the series ?)

Anyway, the patch itself is fine.

Reviewed-by: Guenter Roeck <[email protected]>

Thanks,
Guenter

> Fixes: 477603467009 ("watchdog: Add Spreadtrum watchdog driver")
> Signed-off-by: Lingling Xu <[email protected]>
> Signed-off-by: Chunyan Zhang <[email protected]>
> ---
> drivers/watchdog/sprd_wdt.c | 9 ++-------
> 1 file changed, 2 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/watchdog/sprd_wdt.c b/drivers/watchdog/sprd_wdt.c
> index 65cb55f3916f..f3c90b4afead 100644
> --- a/drivers/watchdog/sprd_wdt.c
> +++ b/drivers/watchdog/sprd_wdt.c
> @@ -345,15 +345,10 @@ static int __maybe_unused sprd_wdt_pm_resume(struct device *dev)
> if (ret)
> return ret;
>
> - if (watchdog_active(&wdt->wdd)) {
> + if (watchdog_active(&wdt->wdd))
> ret = sprd_wdt_start(&wdt->wdd);
> - if (ret) {
> - sprd_wdt_disable(wdt);
> - return ret;
> - }
> - }
>
> - return 0;
> + return ret;
> }
>
> static const struct dev_pm_ops sprd_wdt_pm_ops = {
>

2020-10-28 06:19:25

by Chunyan Zhang

Subject: Re: [PATCH 1/3] watchdog: sprd: should not disable watchdog in resume

On Mon, 26 Oct 2020 at 22:27, Guenter Roeck <[email protected]> wrote:
>
> On 10/26/20 1:09 AM, Chunyan Zhang wrote:
> > From: Lingling Xu <[email protected]>
> >
> > Don't disable watchdog in resume process, otherwise system would crash
> > once kick watchdog.
> >
>
> This is a bit misleading: It is only disabled if the attempt to start it
> has failed. Was this observed in practice ? If so, it might make sense
> to identify and fix the underlying problem instead of trying to work
> around it (or is this addressed with the second patch of the series ?)

Yes, I think the root cause of this problem was like what I explained
in the 3rd patch in this series.
Lingling found there was something wrong in sprd_wdt_pm_resume() when
debugging that issue, then we had this patch.

>
> Anyway, the patch itself is fine.
>
> Reviewed-by: Guenter Roeck <[email protected]>

Thanks,
Chunyan

>
> Thanks,
> Guenter
>
> > Fixes: 477603467009 ("watchdog: Add Spreadtrum watchdog driver")
> > Signed-off-by: Lingling Xu <[email protected]>
> > Signed-off-by: Chunyan Zhang <[email protected]>
> > ---
> > drivers/watchdog/sprd_wdt.c | 9 ++-------
> > 1 file changed, 2 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/watchdog/sprd_wdt.c b/drivers/watchdog/sprd_wdt.c
> > index 65cb55f3916f..f3c90b4afead 100644
> > --- a/drivers/watchdog/sprd_wdt.c
> > +++ b/drivers/watchdog/sprd_wdt.c
> > @@ -345,15 +345,10 @@ static int __maybe_unused sprd_wdt_pm_resume(struct device *dev)
> > if (ret)
> > return ret;
> >
> > - if (watchdog_active(&wdt->wdd)) {
> > + if (watchdog_active(&wdt->wdd))
> > ret = sprd_wdt_start(&wdt->wdd);
> > - if (ret) {
> > - sprd_wdt_disable(wdt);
> > - return ret;
> > - }
> > - }
> >
> > - return 0;
> > + return ret;
> > }
> >
> > static const struct dev_pm_ops sprd_wdt_pm_ops = {
> >
>