mail archive of the barebox mailing list
 help / color / mirror / Atom feed
From: Oleksij Rempel <fishor@gmx.net>
To: "Jan Lübbe" <jlu@pengutronix.de>,
	"Oleksij Rempel" <o.rempel@pengutronix.de>,
	barebox@lists.infradead.org
Subject: Re: [PATCH v1 6/6] watchdog: add watchdog poller
Date: Thu, 8 Mar 2018 15:16:19 +0100	[thread overview]
Message-ID: <cd46083c-123e-8ec9-015b-cd121c87434b@gmx.net> (raw)
In-Reply-To: <1520516969.31759.115.camel@pengutronix.de>


[-- Attachment #1.1.1: Type: text/plain, Size: 5819 bytes --]

Hi,

thank you for review.

Am 08.03.2018 um 14:49 schrieb Jan Lübbe:
> Hi Oleksij,
> 
> On Thu, 2018-03-08 at 12:05 +0100, Oleksij Rempel wrote:
>> In some cases it is practical to supervise as match as possible of
> s/match/much/
>> barebox execution with watchdog (or multiple watchdogs). This
>  ^ the                  ^ a
>> patch provide async poller for watchdog core framework which can
>               ^-s an
>> be enabled by user and store this configuration to nv.
>                ^the           ^-s
> 
> Also, does this patch make the poller support required for using
> watchdogs? It unconditionally calls the poller_* functions.
> 
> Also, it should be documented explicitly, that this will cause barebox
> to keep triggering the watchdog, even when it drops to the shell after
> a boot error. This makes it unsuitable for unattended use.

I would prefer to use controlled reboot over uncontrolled watchdog reset.
For example it would be better to have boot and fail strategy. In case
of network boot, it would be better to retry download in some time and
not cause watchdog reset. If retry count exceeded then some thing should
be done. It can be power off, reboot, fall back to CLI.

The reason for controlled reboot is the fact that the reset impact (or
Reset Sensitivity) is different for every product and source of reset.

This example is take from MiniRISC EZ4021-FC documentation:
				Soft				TAP Ctrl
Module			Reset	Reset	PrRst	ERst	TRST	Reset
CPU			yes	yes	yes	no	no	no
CP0			yes	yes	yes	no	no	no
ICCi			yes	yes	yes	no	no	no
DCC			yes	yes	yes	no	no	no
BIU			yes	yes	yes	no	no	no
MMU			yes	no	no	no	no	no
MDU			yes	yes	yes	no	no	no
EJTAG iface:
- DMA/CPU Acc		yes	yes	yes	yes	yes	yes
  logic	
- Protocol engine	yes	no	no	yes	yes	yes
- Breakpoint		yes	no	no	yes	no	no
- PC trace yes no no yes no no


Most Atheros/QCA WiSoCs will not reset complete SoC even with watchdog
triggered reset.


> I'd prefer to make this optional at compile time.
> 
>>
>> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
>> ---
>>  drivers/watchdog/wd_core.c | 54
>> ++++++++++++++++++++++++++++++++++++++++------
>>  include/watchdog.h         |  4 ++++
>>  2 files changed, 52 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/watchdog/wd_core.c b/drivers/watchdog/wd_core.c
>> index 3162dd59c..6f1cadaf8 100644
>> --- a/drivers/watchdog/wd_core.c
>> +++ b/drivers/watchdog/wd_core.c
>> @@ -31,6 +31,16 @@ static const char *watchdog_name(struct watchdog
>> *wd)
>>  	return "unknown";
>>  }
>>  
>> +static int _watchdog_set_timeout(struct watchdog *wd, unsigned
>> timeout)
>> +{
>> +	if (timeout > wd->timeout_max)
>> +		return -EINVAL;
>> +
>> +	pr_debug("setting timeout on %s to %ds\n",
>> watchdog_name(wd), timeout);
>> +
>> +	return wd->set_timeout(wd, timeout);
>> +}
>> +
>>  static int watchdog_set_cur(struct param_d *param, void *priv)
>>  {
>>  	struct watchdog *wd = priv;
>> @@ -41,6 +51,37 @@ static int watchdog_set_cur(struct param_d *param,
>> void *priv)
>>  	return 0;
>>  }
>>  
>> +static void watchdog_poller_cb(void *priv);
>> +
>> +static void watchdog_poller_start(struct watchdog *wd)
>> +{
>> +	_watchdog_set_timeout(wd, wd->timeout_cur);
>> +	poller_call_async(&wd->poller, 500 * MSECOND,
>> +			watchdog_poller_cb, wd);
>> +
>> +}
>> +
>> +static void watchdog_poller_cb(void *priv)
>> +{
>> +	struct watchdog *wd = priv;
>> +
>> +	if (wd->poller_enable)
>> +		watchdog_poller_start(wd);
>> +}
>> +
>> +static int watchdog_set_poller(struct param_d *param, void *priv)
>> +{
>> +	struct watchdog *wd = priv;
>> +
>> +
>> +	if (wd->poller_enable)
>> +		watchdog_poller_start(wd);
>> +	else
>> +		poller_async_cancel(&wd->poller);
>> +
>> +	return 0;
>> +}
>> +
>>  int watchdog_register(struct watchdog *wd)
>>  {
>>  
>> @@ -63,6 +104,10 @@ int watchdog_register(struct watchdog *wd)
>>  	dev_add_param_uint32(&wd->dev, "timeout_cur",
>> watchdog_set_cur, NULL,
>>  			&wd->timeout_cur, "%u", wd);
>>  
>> +	poller_async_register(&wd->poller);
>> +	dev_add_param_bool(&wd->dev, "poller_enable",
>> watchdog_set_poller, NULL,
>> +			&wd->poller_enable, wd);
> Isn't the usage of the poller an implementation detail? Perhaps calls
> this something like "auto_trigger"?
> 
>> +
>>  	list_add_tail(&wd->list, &watchdog_list);
>>  
>>  	pr_debug("registering watchdog %s with priority %d\n",
>> watchdog_name(wd),
>> @@ -74,6 +119,8 @@ EXPORT_SYMBOL(watchdog_register);
>>  
>>  int watchdog_deregister(struct watchdog *wd)
>>  {
>> +	poller_async_cancel(&wd->poller);
>> +	poller_async_unregister(&wd->poller);
>>  	unregister_device(&wd->dev);
>>  	list_del(&wd->list);
>>  
>> @@ -109,12 +156,7 @@ int watchdog_set_timeout(unsigned timeout)
>>  	if (!wd)
>>  		return -ENODEV;
>>  
>> -	if (timeout > wd->timeout_max)
>> -		return -EINVAL;
>> -
>> -	pr_debug("setting timeout on %s to %ds\n",
>> watchdog_name(wd), timeout);
>> -
>> -	return wd->set_timeout(wd, timeout);
>> +	return _watchdog_set_timeout(wd, timeout);
>>  }
>>  EXPORT_SYMBOL(watchdog_set_timeout);
>>  
>> diff --git a/include/watchdog.h b/include/watchdog.h
>> index 2f1874c19..0db4263a3 100644
>> --- a/include/watchdog.h
>> +++ b/include/watchdog.h
>> @@ -13,6 +13,8 @@
>>  #ifndef INCLUDE_WATCHDOG_H
>>  # define INCLUDE_WATCHDOG_H
>>  
>> +#include <poller.h>
>> +
>>  struct watchdog {
>>  	int (*set_timeout)(struct watchdog *, unsigned);
>>  	const char *name;
>> @@ -21,6 +23,8 @@ struct watchdog {
>>  	unsigned int priority;
>>  	unsigned int timeout_max;
>>  	unsigned int timeout_cur;
>> +	unsigned int poller_enable;
>> +	struct poller_async poller;
>>  	struct list_head list;
>>  };
>>  



[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

[-- Attachment #2: Type: text/plain, Size: 149 bytes --]

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

  reply	other threads:[~2018-03-08 14:16 UTC|newest]

Thread overview: 11+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-03-08 11:05 [PATCH v1 1/6] watchdog: rename dev to hwdev Oleksij Rempel
2018-03-08 11:05 ` [PATCH v1 2/6] watchdog: move max timeout test in to wd_core Oleksij Rempel
2018-03-08 11:05 ` [PATCH v1 3/6] watchdog: register watchdog virtual device with short name wdog Oleksij Rempel
2018-03-12 10:35   ` Sascha Hauer
2018-03-08 11:05 ` [PATCH v1 4/6] watchdog: set some reasonable timeout_max value if no other is available Oleksij Rempel
2018-03-12 10:37   ` Sascha Hauer
2018-03-08 11:05 ` [PATCH v1 5/6] watchdog: provide timeout_cur value Oleksij Rempel
2018-03-08 11:05 ` [PATCH v1 6/6] watchdog: add watchdog poller Oleksij Rempel
2018-03-08 13:49   ` Jan Lübbe
2018-03-08 14:16     ` Oleksij Rempel [this message]
2018-03-08 15:33       ` Jan Lübbe

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=cd46083c-123e-8ec9-015b-cd121c87434b@gmx.net \
    --to=fishor@gmx.net \
    --cc=barebox@lists.infradead.org \
    --cc=jlu@pengutronix.de \
    --cc=o.rempel@pengutronix.de \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox