What to set in struct watchdog_device::bootstatus?

What to set in struct watchdog_device::bootstatus?

[Uwe Kleine-König]

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/  |

Re: What to set in struct watchdog_device::bootstatus?

[Guenter Roeck]

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

Re: What to set in struct watchdog_device::bootstatus?

[Uwe Kleine-König]

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/  |

Re: What to set in struct watchdog_device::bootstatus?

[Uwe Kleine-König]

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/  |

Re: What to set in struct watchdog_device::bootstatus?

[Guenter Roeck]

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