Linux-Watchdog Archive on lore.kernel.org
 help / color / Atom feed
* What to set in struct watchdog_device::bootstatus?
@ 2019-02-08 10:52 Uwe Kleine-König
  2019-02-08 14:05 ` Guenter Roeck
  0 siblings, 1 reply; 5+ messages in thread
From: Uwe Kleine-König @ 2019-02-08 10:52 UTC (permalink / raw)
  To: Wim Van Sebroeck, Guenter Roeck; +Cc: linux-watchdog

Hello,

it's unclear to me, which bits I am supposed to set in the bootstatus
member of struct watchdog_device at probe time.

The i.MX watchdog differentiates the following reset causes:

 - Power On
 - external reset
 - watchdog timeout
 - software reset using a bit in the watchdog register set

(Not all i.MX variants implement all bits according to the respective
reference manuals.)

Should "Power On" result in setting WDIOF_POWERUNDER?

Should "software reset [...]" result in WDIOF_CARDRESET?

Should "external reset" result in WDIOF_EXTERN1? (I guess that no)

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: What to set in struct watchdog_device::bootstatus?
  2019-02-08 10:52 What to set in struct watchdog_device::bootstatus? Uwe Kleine-König
@ 2019-02-08 14:05 ` Guenter Roeck
  2019-02-20 20:14   ` Uwe Kleine-König
  0 siblings, 1 reply; 5+ messages in thread
From: Guenter Roeck @ 2019-02-08 14:05 UTC (permalink / raw)
  To: Uwe Kleine-König, Wim Van Sebroeck; +Cc: linux-watchdog

On 2/8/19 2:52 AM, Uwe Kleine-König wrote:
> Hello,
> 
> it's unclear to me, which bits I am supposed to set in the bootstatus
> member of struct watchdog_device at probe time.
> 
> The i.MX watchdog differentiates the following reset causes:
> 
>   - Power On
None.

>   - external reset

None.

>   - watchdog timeout

WDIOF_CARDRESET

>   - software reset using a bit in the watchdog register set
> 
> (Not all i.MX variants implement all bits according to the respective
> reference manuals.)
> 
> Should "Power On" result in setting WDIOF_POWERUNDER?
> 
No.

> Should "software reset [...]" result in WDIOF_CARDRESET?
> 
That would be the best fit if you want a bit to be set, but it would be
misleading since it would suggest that the watchdog fired.

> Should "external reset" result in WDIOF_EXTERN1? (I guess that no)
> 
No.

Overall, the bits are only to be set if the reset was triggered by
the watchdog controller.

Guenter

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: What to set in struct watchdog_device::bootstatus?
  2019-02-08 14:05 ` Guenter Roeck
@ 2019-02-20 20:14   ` Uwe Kleine-König
  2019-05-09 19:25     ` Uwe Kleine-König
  0 siblings, 1 reply; 5+ messages in thread
From: Uwe Kleine-König @ 2019-02-20 20:14 UTC (permalink / raw)
  To: Guenter Roeck; +Cc: Wim Van Sebroeck, linux-watchdog

Hello Guenter,

On Fri, Feb 08, 2019 at 06:05:43AM -0800, Guenter Roeck wrote:
> On 2/8/19 2:52 AM, Uwe Kleine-König wrote:
> > Hello,
> > 
> > it's unclear to me, which bits I am supposed to set in the bootstatus
> > member of struct watchdog_device at probe time.
> > 
> > The i.MX watchdog differentiates the following reset causes:
> > 
> >   - Power On
> None.
> 
> >   - external reset
> 
> None.
> 
> >   - watchdog timeout
> 
> WDIOF_CARDRESET
> 
> >   - software reset using a bit in the watchdog register set
> > 
> > (Not all i.MX variants implement all bits according to the respective
> > reference manuals.)
> > 
> > Should "Power On" result in setting WDIOF_POWERUNDER?
> > 
> No.
> 
> > Should "software reset [...]" result in WDIOF_CARDRESET?
> > 
> That would be the best fit if you want a bit to be set, but it would be
> misleading since it would suggest that the watchdog fired.
> 
> > Should "external reset" result in WDIOF_EXTERN1? (I guess that no)
>
> No.

In a custom kernel patch stack I found a patch that uses (apart from
WDIOF_CARDRESET also) WDIOF_POWERUNDER and WDIOF_EXTERN1 to
differentiate the different reset causes. Now that you told using this
is wrong, I wonder how these are supposed to be used instead; and there
are a few more that according to
Documentation/watchdog/watchdog-kernel-api.txt might be used to set
bootstatus. Are these a relic? What do these signal?

IMHO there is a patch opportunity waiting to improve the documenation
:-) Or maybe even change watchdog_get_status() to ensure that only
WDIOF_CARDRESET, WDIOF_MAGICCLOSE and WDIOF_KEEPALIVEPING can be set?

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: What to set in struct watchdog_device::bootstatus?
  2019-02-20 20:14   ` Uwe Kleine-König
@ 2019-05-09 19:25     ` Uwe Kleine-König
  2019-05-10  3:53       ` Guenter Roeck
  0 siblings, 1 reply; 5+ messages in thread
From: Uwe Kleine-König @ 2019-05-09 19:25 UTC (permalink / raw)
  To: Guenter Roeck; +Cc: Wim Van Sebroeck, linux-watchdog, kernel

Hello Guenter,

On Wed, Feb 20, 2019 at 09:14:08PM +0100, Uwe Kleine-König wrote:
> In a custom kernel patch stack I found a patch that uses (apart from
> WDIOF_CARDRESET also) WDIOF_POWERUNDER and WDIOF_EXTERN1 to
> differentiate the different reset causes. Now that you told using this
> is wrong, I wonder how these are supposed to be used instead; and there
> are a few more that according to
> Documentation/watchdog/watchdog-kernel-api.txt might be used to set
> bootstatus. Are these a relic? What do these signal?

I'm still interested in an answer here. While it is currently not
possible to "fix" the custom kernel as some other software that is
already shipping depends on this. Still I'd like to know the details
here to maybe suggest an alternative for the longterm future.

> IMHO there is a patch opportunity waiting to improve the documenation
> :-) Or maybe even change watchdog_get_status() to ensure that only
> WDIOF_CARDRESET, WDIOF_MAGICCLOSE and WDIOF_KEEPALIVEPING can be set?

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: What to set in struct watchdog_device::bootstatus?
  2019-05-09 19:25     ` Uwe Kleine-König
@ 2019-05-10  3:53       ` Guenter Roeck
  0 siblings, 0 replies; 5+ messages in thread
From: Guenter Roeck @ 2019-05-10  3:53 UTC (permalink / raw)
  To: Uwe Kleine-König; +Cc: Wim Van Sebroeck, linux-watchdog, kernel

On 5/9/19 12:25 PM, Uwe Kleine-König wrote:
> Hello Guenter,
> 
> On Wed, Feb 20, 2019 at 09:14:08PM +0100, Uwe Kleine-König wrote:
>> In a custom kernel patch stack I found a patch that uses (apart from
>> WDIOF_CARDRESET also) WDIOF_POWERUNDER and WDIOF_EXTERN1 to
>> differentiate the different reset causes. Now that you told using this
>> is wrong, I wonder how these are supposed to be used instead; and there
>> are a few more that according to
>> Documentation/watchdog/watchdog-kernel-api.txt might be used to set
>> bootstatus. Are these a relic? What do these signal?
> 
> I'm still interested in an answer here. While it is currently not
> possible to "fix" the custom kernel as some other software that is
> already shipping depends on this. Still I'd like to know the details
> here to maybe suggest an alternative for the longterm future.
> 
>> IMHO there is a patch opportunity waiting to improve the documenation
>> :-) Or maybe even change watchdog_get_status() to ensure that only
>> WDIOF_CARDRESET, WDIOF_MAGICCLOSE and WDIOF_KEEPALIVEPING can be set?
> 

The basic problem, as I see it, is that the bits were defined a long time
ago as standard API, even tough they really only apply to a small subset
of watchdog cards. Personally I tend to believe that any set of bits
would be insufficient. Maybe it should have been a string. Either case,
I don't believe in forcing more or less random reboot reasons into a
given set of bits. On the other side I also don't see the point in
arbitrarily limiting the bit values returned by WDIOC_GETSTATUS.
With that, my general approach would be some kind of report-what-makes-sense
approach. But using WDIOF_POWERUNDER for "power cycle" just goes a bit too
far.

Thanks,
Guenter

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, back to index

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-02-08 10:52 What to set in struct watchdog_device::bootstatus? Uwe Kleine-König
2019-02-08 14:05 ` Guenter Roeck
2019-02-20 20:14   ` Uwe Kleine-König
2019-05-09 19:25     ` Uwe Kleine-König
2019-05-10  3:53       ` Guenter Roeck

Linux-Watchdog Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-watchdog/0 linux-watchdog/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-watchdog linux-watchdog/ https://lore.kernel.org/linux-watchdog \
		linux-watchdog@vger.kernel.org linux-watchdog@archiver.kernel.org
	public-inbox-index linux-watchdog


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-watchdog


AGPL code for this site: git clone https://public-inbox.org/ public-inbox