[PATCH v1] watchdog: intel-mid_wdt: Keep watchdog running

[PATCH v1] watchdog: intel-mid_wdt: Keep watchdog running

[Andy Shevchenko]

Firmware followed by bootloader leaves watchdog running.

Keep it running in the driver.

User will not need any additional options to reboot in case of panic.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/watchdog/intel-mid_wdt.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/watchdog/intel-mid_wdt.c b/drivers/watchdog/intel-mid_wdt.c
index 45e4d02221b5..a128ccb085b0 100644
--- a/drivers/watchdog/intel-mid_wdt.c
+++ b/drivers/watchdog/intel-mid_wdt.c
@@ -147,8 +147,8 @@ static int mid_wdt_probe(struct platform_device *pdev)
 		return ret;
 	}
 
-	/* Make sure the watchdog is not running */
-	wdt_stop(wdt_dev);
+	/* Make sure the watchdog is serviced */
+	set_bit(WDOG_HW_RUNNING, &wdt_dev->status);
 
 	ret = devm_watchdog_register_device(&pdev->dev, wdt_dev);
 	if (ret) {
-- 
2.11.0

Re: [PATCH v1] watchdog: intel-mid_wdt: Keep watchdog running

[Guenter Roeck]

Hi Andy,

On 03/08/2017 03:10 AM, Andy Shevchenko wrote:
> Firmware followed by bootloader leaves watchdog running.
>

Is that always the case ? If not, does it hurt if the watchdog is pinged
while not active ?

Also, this assumes that its default timeout is in the default range
configured by the driver, ie not (much) lower than 30 seconds.

Not objecting, but it would help to have comments in the code explaining
the context in some more detail.

Thanks,
Guenter

> Keep it running in the driver.
>
> User will not need any additional options to reboot in case of panic.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/watchdog/intel-mid_wdt.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/watchdog/intel-mid_wdt.c b/drivers/watchdog/intel-mid_wdt.c
> index 45e4d02221b5..a128ccb085b0 100644
> --- a/drivers/watchdog/intel-mid_wdt.c
> +++ b/drivers/watchdog/intel-mid_wdt.c
> @@ -147,8 +147,8 @@ static int mid_wdt_probe(struct platform_device *pdev)
>  		return ret;
>  	}
>
> -	/* Make sure the watchdog is not running */
> -	wdt_stop(wdt_dev);
> +	/* Make sure the watchdog is serviced */
> +	set_bit(WDOG_HW_RUNNING, &wdt_dev->status);
>
>  	ret = devm_watchdog_register_device(&pdev->dev, wdt_dev);
>  	if (ret) {
>

Re: [PATCH v1] watchdog: intel-mid_wdt: Keep watchdog running

[Andy Shevchenko]

On Wed, 2017-03-08 at 06:21 -0800, Guenter Roeck wrote:
> Hi Andy,
> 
> On 03/08/2017 03:10 AM, Andy Shevchenko wrote:
> > Firmware followed by bootloader leaves watchdog running.
> > 
> 
> Is that always the case ?

Yes if no-one patched boot loader specifically to avoid this.

>  If not, does it hurt if the watchdog is pinged
> while not active ?
> 
> Also, this assumes that its default timeout is in the default range
> configured by the driver, ie not (much) lower than 30 seconds.

Correct. It is hard coded in SCU firmware (60 seconds IIRC). U-Boot for
now doesn't change it.

> Not objecting, but it would help to have comments in the code
> explaining
> the context in some more detail.

Last time you were not satisfied by the line which stops the running
watchdog [1]. I found a way how to keep it running gracefully, which I
proposed to do at that time (assuming boot loader doesn't stop it) [2].

I'm confused now what is preferable way to do the things?

P.S. I suppose previously we missed the chance to do this better and I
perhaps have to add Fixes: tag as well.

[1] https://www.spinics.net/lists/linux-watchdog/msg10386.html
[2] https://www.spinics.net/lists/linux-watchdog/msg10377.html

> 
> Thanks,
> Guenter
> 
> > Keep it running in the driver.
> > 
> > User will not need any additional options to reboot in case of
> > panic.
> > 
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > ---
> >  drivers/watchdog/intel-mid_wdt.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/watchdog/intel-mid_wdt.c
> > b/drivers/watchdog/intel-mid_wdt.c
> > index 45e4d02221b5..a128ccb085b0 100644
> > --- a/drivers/watchdog/intel-mid_wdt.c
> > +++ b/drivers/watchdog/intel-mid_wdt.c
> > @@ -147,8 +147,8 @@ static int mid_wdt_probe(struct platform_device
> > *pdev)
> >  		return ret;
> >  	}
> > 
> > -	/* Make sure the watchdog is not running */
> > -	wdt_stop(wdt_dev);
> > +	/* Make sure the watchdog is serviced */
> > +	set_bit(WDOG_HW_RUNNING, &wdt_dev->status);
> > 
> >  	ret = devm_watchdog_register_device(&pdev->dev, wdt_dev);
> >  	if (ret) {
> > 
> 
> 

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH v1] watchdog: intel-mid_wdt: Keep watchdog running

[Guenter Roeck]

On Wed, Mar 08, 2017 at 05:11:10PM +0200, Andy Shevchenko wrote:
> On Wed, 2017-03-08 at 06:21 -0800, Guenter Roeck wrote:
> > Hi Andy,
> > 
> > On 03/08/2017 03:10 AM, Andy Shevchenko wrote:
> > > Firmware followed by bootloader leaves watchdog running.
> > > 
> > 
> > Is that always the case ?
> 
> Yes if no-one patched boot loader specifically to avoid this.
> 
> >  If not, does it hurt if the watchdog is pinged
> > while not active ?
> > 
> > Also, this assumes that its default timeout is in the default range
> > configured by the driver, ie not (much) lower than 30 seconds.
> 
> Correct. It is hard coded in SCU firmware (60 seconds IIRC). U-Boot for
> now doesn't change it.
> 
> > Not objecting, but it would help to have comments in the code
> > explaining
> > the context in some more detail.
> 
> Last time you were not satisfied by the line which stops the running
> watchdog [1]. I found a way how to keep it running gracefully, which I
> proposed to do at that time (assuming boot loader doesn't stop it) [2].
> 
> I'm confused now what is preferable way to do the things?
> 

The suggested patch is ok. I was just asking for additional comments,
along the line of 1) the SCU firmware sets the timeout to 60s, the default
timeout of 30s is therefore safe, and 2) that the watchdog is by default
enabled in the SCU firmware (and, if appropriate, that it doesn't get a
hiccup if it is pinged if it should have been disabled for some unspecified
/ unknown reason).

Thanks,
Guenter

Re: [PATCH v1] watchdog: intel-mid_wdt: Keep watchdog running

[Andy Shevchenko]

On Wed, 2017-03-08 at 11:16 -0800, Guenter Roeck wrote:
> On Wed, Mar 08, 2017 at 05:11:10PM +0200, Andy Shevchenko wrote:
> > On Wed, 2017-03-08 at 06:21 -0800, Guenter Roeck wrote:
> > > On 03/08/2017 03:10 AM, Andy Shevchenko wrote:
> > > > Firmware followed by bootloader leaves watchdog running.

> > > Is that always the case ?
> > 
> > Yes if no-one patched boot loader specifically to avoid this.
> > 
> > >  If not, does it hurt if the watchdog is pinged
> > > while not active ?
> > > 
> > > Also, this assumes that its default timeout is in the default
> > > range
> > > configured by the driver, ie not (much) lower than 30 seconds.
> > 
> > Correct. It is hard coded in SCU firmware (60 seconds IIRC). U-Boot
> > for
> > now doesn't change it.
> > 
> > > Not objecting, but it would help to have comments in the code
> > > explaining
> > > the context in some more detail.
> > 
> > Last time you were not satisfied by the line which stops the running
> > watchdog [1]. I found a way how to keep it running gracefully, which
> > I
> > proposed to do at that time (assuming boot loader doesn't stop it)
> > [2].
> > 
> > I'm confused now what is preferable way to do the things?
> > 
> 
> The suggested patch is ok. I was just asking for additional comments,
> along the line of 1) the SCU firmware sets the timeout to 60s, the
> default
> timeout of 30s is therefore safe, and 2) that the watchdog is by
> default
> enabled in the SCU firmware (and, if appropriate, that it doesn't get
> a
> hiccup if it is pinged if it should have been disabled for some
> unspecified
> / unknown reason).

Okay, I will send v2 with additional comments.

Thanks for clarifying!

P.S. It looks like we have to add some lines in the driver for U-Boot to
make developer pay attention to the potential asynchronization.

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy