| Message ID | 20250305101025.2279951-1-george.cherian@marvell.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Add stop_on_panic support for watchdog | expand |
On Wed, Mar 05, 2025 at 10:10:23AM +0000, George Cherian wrote: > This series adds a new kernel command line option to watchdog core to > stop the watchdog on panic. This is useul in certain systems which prevents > successful loading of kdump kernel due to watchdog reset. > > Some of the watchdog drivers stop function could sleep. For such > drivers the stop_on_panic is not valid as the notifier callback happens > in atomic context. Introduce WDIOF_STOP_MAYSLEEP flag to watchdog_info > options to indicate whether the stop function would sleep. Should you only enable this if the kdump is enabled in the kernel configuration?
Hi George, On 05.03.25 11:10, George Cherian wrote: > This series adds a new kernel command line option to watchdog core to > stop the watchdog on panic. This is useul in certain systems which prevents > successful loading of kdump kernel due to watchdog reset. > > Some of the watchdog drivers stop function could sleep. For such > drivers the stop_on_panic is not valid as the notifier callback happens > in atomic context. Introduce WDIOF_STOP_MAYSLEEP flag to watchdog_info > options to indicate whether the stop function would sleep. Did you consider having a reset_on_panic instead, which sets a user-specified timeout on panic? This would make the mechanism useful also for watchdogs that can't be disabled and would protect against system lock up: Consider a memory-corruption bug (perhaps externally via DMA), which partially overwrites both main and kdump kernel. With a disabled watchdog, the system may not be able to recover on its own. If you did consider it, what made you decide against it? Thanks, Ahmad > > > Changelog: > v1 -> v2 > - Remove the per driver flag setting option > - Take the parameter via kernel command-line parameter to watchdog_core. > > v2 -> v3 > - Remove the helper function watchdog_stop_on_panic() from watchdog.h. > - There are no users for this. > > v3 -> v4 > - Since the panic notifier is in atomic context, watchdog functions > which sleep can't be called. > - Add an options flag WDIOF_STOP_MAYSLEEP to indicate whether stop > function sleeps. > - Simplify the stop_on_panic kernel command line parsing. > - Enable the panic notiffier only if the watchdog stop function doesn't > sleep > > George Cherian (2): > watchdog: Add a new flag WDIOF_STOP_MAYSLEEP > drivers: watchdog: Add support for panic notifier callback > > drivers/watchdog/advantech_ec_wdt.c | 3 ++- > drivers/watchdog/arm_smc_wdt.c | 3 ++- > drivers/watchdog/armada_37xx_wdt.c | 2 +- > drivers/watchdog/asm9260_wdt.c | 2 +- > drivers/watchdog/bcm47xx_wdt.c | 3 ++- > drivers/watchdog/bd9576_wdt.c | 2 +- > drivers/watchdog/bd96801_wdt.c | 2 +- > drivers/watchdog/cgbc_wdt.c | 2 +- > drivers/watchdog/cros_ec_wdt.c | 5 ++++- > drivers/watchdog/da9052_wdt.c | 3 ++- > drivers/watchdog/da9055_wdt.c | 4 +++- > drivers/watchdog/da9062_wdt.c | 4 +++- > drivers/watchdog/da9063_wdt.c | 4 +++- > drivers/watchdog/db8500_wdt.c | 5 ++++- > drivers/watchdog/dw_wdt.c | 5 +++-- > drivers/watchdog/f71808e_wdt.c | 3 ++- > drivers/watchdog/gpio_wdt.c | 2 +- > drivers/watchdog/i6300esb.c | 5 ++++- > drivers/watchdog/imx_sc_wdt.c | 2 +- > drivers/watchdog/intel-mid_wdt.c | 5 ++++- > drivers/watchdog/it87_wdt.c | 5 ++++- > drivers/watchdog/jz4740_wdt.c | 5 ++++- > drivers/watchdog/kempld_wdt.c | 3 ++- > drivers/watchdog/lenovo_se10_wdt.c | 5 ++++- > drivers/watchdog/max77620_wdt.c | 5 ++++- > drivers/watchdog/mei_wdt.c | 3 ++- > drivers/watchdog/menf21bmc_wdt.c | 4 +++- > drivers/watchdog/mlx_wdt.c | 2 +- > drivers/watchdog/msc313e_wdt.c | 5 ++++- > drivers/watchdog/npcm_wdt.c | 3 ++- > drivers/watchdog/omap_wdt.c | 5 ++++- > drivers/watchdog/pm8916_wdt.c | 5 +++-- > drivers/watchdog/pseries-wdt.c | 2 +- > drivers/watchdog/rave-sp-wdt.c | 5 ++++- > drivers/watchdog/renesas_wdt.c | 7 ++++-- > drivers/watchdog/retu_wdt.c | 5 ++++- > drivers/watchdog/rn5t618_wdt.c | 6 +++-- > drivers/watchdog/rzg2l_wdt.c | 5 ++++- > drivers/watchdog/rzv2h_wdt.c | 5 ++++- > drivers/watchdog/shwdt.c | 6 +++-- > drivers/watchdog/sl28cpld_wdt.c | 5 ++++- > drivers/watchdog/softdog.c | 5 ++++- > drivers/watchdog/sp805_wdt.c | 5 ++++- > drivers/watchdog/starfive-wdt.c | 3 ++- > drivers/watchdog/stpmic1_wdt.c | 5 ++++- > drivers/watchdog/ts4800_wdt.c | 5 ++++- > drivers/watchdog/twl4030_wdt.c | 5 ++++- > drivers/watchdog/uniphier_wdt.c | 3 ++- > drivers/watchdog/w83627hf_wdt.c | 5 ++++- > drivers/watchdog/watchdog_core.c | 35 +++++++++++++++++++++++++++++ > drivers/watchdog/wm831x_wdt.c | 5 ++++- > drivers/watchdog/wm8350_wdt.c | 5 ++++- > drivers/watchdog/xen_wdt.c | 5 ++++- > drivers/watchdog/ziirave_wdt.c | 5 ++++- > include/linux/watchdog.h | 2 ++ > include/uapi/linux/watchdog.h | 1 + > 56 files changed, 198 insertions(+), 58 deletions(-) >
Hi Ahmad, >Hi George, > On 05.03.25 11:10, George Cherian wrote: >> This series adds a new kernel command line option to watchdog core to >> stop the watchdog on panic. This is useul in certain systems which prevents >> successful loading of kdump kernel due to watchdog reset. >> >> Some of the watchdog drivers stop function could sleep. For such >> drivers the stop_on_panic is not valid as the notifier callback happens >> in atomic context. Introduce WDIOF_STOP_MAYSLEEP flag to watchdog_info >> options to indicate whether the stop function would sleep. > >Did you consider having a reset_on_panic instead, which sets a user-specified >timeout on panic? This would make the mechanism useful also for watchdogs /proc/sys/kernel/panic already provides that support. You may echo a non-zero value and the system tries for a soft reboot after those many seconds. But this doesn't happen in case of a kdump kernel load after panic. >that can't be disabled and would protect against system lock up: >Consider a memory-corruption bug (perhaps externally via DMA), which partially >overwrites both main and kdump kernel. With a disabled watchdog, the system >may not be able to recover on its own. Yes, that is the reason why the kernel command-line is optional and by default it is set to zero. So that in cases if you have a corrupted kdump kernel then watchdog kicks in. > >If you did consider it, what made you decide against it? watchdog.stop_on_panic=1 is specifically for systems which can't boot a kdump kernel due to the fact that the kdump kernel gets a watchdog reset while booting, may be due to a shorter watchdog time. For eg: a 32-bit watchdog down counter running at 1GHz. reset_on_panic can guarantee only the largest watchdog timeout supported by HW, since there is no one to ping the watchdog. > >Thanks, >Ahmad > >> >> > Changelog: > v1 -> v2 > - Remove the per driver flag setting option > - Take the parameter via kernel command-line parameter to watchdog_core. > > v2 -> v3 > - Remove the helper function watchdog_stop_on_panic() from watchdog.h. > - There are no users for this. > > v3 -> v4 > - Since the panic notifier is in atomic context, watchdog functions > which sleep can't be called. > - Add an options flag WDIOF_STOP_MAYSLEEP to indicate whether stop > function sleeps. > - Simplify the stop_on_panic kernel command line parsing. > - Enable the panic notiffier only if the watchdog stop function doesn't > sleep > > George Cherian (2): > watchdog: Add a new flag WDIOF_STOP_MAYSLEEP > drivers: watchdog: Add support for panic notifier callback - George
Hi George, On 05.03.25 12:28, George Cherian wrote: > Hi Ahmad, >> Hi George, >> On 05.03.25 11:10, George Cherian wrote: >>> This series adds a new kernel command line option to watchdog core to >>> stop the watchdog on panic. This is useul in certain systems which prevents >>> successful loading of kdump kernel due to watchdog reset. >>> >>> Some of the watchdog drivers stop function could sleep. For such >>> drivers the stop_on_panic is not valid as the notifier callback happens >>> in atomic context. Introduce WDIOF_STOP_MAYSLEEP flag to watchdog_info >>> options to indicate whether the stop function would sleep. >> >> Did you consider having a reset_on_panic instead, which sets a user-specified >> timeout on panic? This would make the mechanism useful also for watchdogs > > /proc/sys/kernel/panic already provides that support. You may echo a non-zero value > and the system tries for a soft reboot after those many seconds. But this doesn't happen > in case of a kdump kernel load after panic. The timeout specified to the Watchdog reset_on_panic option would be programmed into the active watchdogs and not be used to trigger a software-induced reboot. >> that can't be disabled and would protect against system lock up: >> Consider a memory-corruption bug (perhaps externally via DMA), which partially >> overwrites both main and kdump kernel. With a disabled watchdog, the system >> may not be able to recover on its own. > > Yes, that is the reason why the kernel command-line is optional and by default it is set to zero. > So that in cases if you have a corrupted kdump kernel then watchdog kicks in. The existing option isn't enough for the kdump kernel use case. If we (i.e. you) are going to do something about it, wouldn't it be better to have a solution that's applicable to a wider number of watchdog devices? >> If you did consider it, what made you decide against it? > watchdog.stop_on_panic=1 is specifically for systems which can't boot a kdump kernel due to the fact > that the kdump kernel gets a watchdog reset while booting, may be due to a shorter watchdog time. > For eg: a 32-bit watchdog down counter running at 1GHz. > reset_on_panic can guarantee only the largest watchdog timeout supported by HW, > since there is no one to ping the watchdog. If you are serious with the watchdog use, you'll want to use the watchdog to monitor kernel startup as well. If the bootloader can set a watchdog timeout just before starting the kernel and it doesn't expire before the kernel watchdog driver takes over, why can't we do the same just before starting the dumpkernel? Thanks, Ahmad >> >> Thanks, >> Ahmad >> >>> >>> >> Changelog: >> v1 -> v2 >> - Remove the per driver flag setting option >> - Take the parameter via kernel command-line parameter to watchdog_core. >> >> v2 -> v3 >> - Remove the helper function watchdog_stop_on_panic() from watchdog.h. >> - There are no users for this. >> >> v3 -> v4 >> - Since the panic notifier is in atomic context, watchdog functions >> which sleep can't be called. >> - Add an options flag WDIOF_STOP_MAYSLEEP to indicate whether stop >> function sleeps. >> - Simplify the stop_on_panic kernel command line parsing. >> - Enable the panic notiffier only if the watchdog stop function doesn't >> sleep >> >> George Cherian (2): >> watchdog: Add a new flag WDIOF_STOP_MAYSLEEP >> drivers: watchdog: Add support for panic notifier callback > > - George
Hi Ahmad, >Hi George, > >On 05.03.25 12:28, George Cherian wrote: > Hi Ahmad, >>> Hi George, >>> On 05.03.25 11:10, George Cherian wrote: >>>> This series adds a new kernel command line option to watchdog core to >>>> stop the watchdog on panic. This is useul in certain systems which prevents >>>> successful loading of kdump kernel due to watchdog reset. >>>> >>>> Some of the watchdog drivers stop function could sleep. For such >>>> drivers the stop_on_panic is not valid as the notifier callback happens >>>> in atomic context. Introduce WDIOF_STOP_MAYSLEEP flag to watchdog_info >>>> options to indicate whether the stop function would sleep. >>> >>> Did you consider having a reset_on_panic instead, which sets a user-specified >>> timeout on panic? This would make the mechanism useful also for watchdogs >> >> /proc/sys/kernel/panic already provides that support. You may echo a non-zero value >> and the system tries for a soft reboot after those many seconds. But this doesn't happen >> in case of a kdump kernel load after panic. > >The timeout specified to the Watchdog reset_on_panic option would be programmed into >the active watchdogs and not be used to trigger a software-induced reboot. Yes. > >>> that can't be disabled and would protect against system lock up: >>> Consider a memory-corruption bug (perhaps externally via DMA), which partially >>> overwrites both main and kdump kernel. With a disabled watchdog, the system >>> may not be able to recover on its own. >> >> Yes, that is the reason why the kernel command-line is optional and by default it is set to zero. >> So that in cases if you have a corrupted kdump kernel then watchdog kicks in. > >The existing option isn't enough for the kdump kernel use case. >If we (i.e. you) are going to do something about it, wouldn't it be >better to have a solution that's applicable to a wider number of >watchdog devices? I need a slight clarification here. 1. reset_on_panic takes the number of seconds to be reloaded to watchdog HW, so that it initiates a watchdog reset after the specified timeout, if kdump kernel fails to boot or hung while booting. 2. in case reset_on_panic = 0 then it behaves like stop_on_panic=1. Is this what you meant? I would let Guenter comment on this approach. >>> If you did consider it, what made you decide against it? >> watchdog.stop_on_panic=1 is specifically for systems which can't boot a kdump kernel due to the fact >> that the kdump kernel gets a watchdog reset while booting, may be due to a shorter watchdog time. >> For eg: a 32-bit watchdog down counter running at 1GHz. >> reset_on_panic can guarantee only the largest watchdog timeout supported by HW, >> since there is no one to ping the watchdog. >If you are serious with the watchdog use, you'll want to use the watchdog to >monitor kernel startup as well. If the bootloader can set a watchdog timeout >just before starting the kernel and it doesn't expire before the kernel watchdog >driver takes over, why can't we do the same just before starting the dumpkernel? Yes, in an ideal world with ideal HW. But there are HW with issues which cannot have large enough Watchdog time. Such HW would boot from FW to kernel without watchdog enabled. And stop_on_panic does the similar for kdump kernel too. -George > >Thanks, >Ahmad > > >> >> Thanks, >> Ahmad >> >>> >>> >> Changelog: >> v1 -> v2 >> - Remove the per driver flag setting option >> - Take the parameter via kernel command-line parameter to watchdog_core. >> >> v2 -> v3 >> - Remove the helper function watchdog_stop_on_panic() from watchdog.h. >> - There are no users for this. >> >> v3 -> v4 >> - Since the panic notifier is in atomic context, watchdog functions >> which sleep can't be called. >> - Add an options flag WDIOF_STOP_MAYSLEEP to indicate whether stop >> function sleeps. >> - Simplify the stop_on_panic kernel command line parsing. >> - Enable the panic notiffier only if the watchdog stop function doesn't >> sleep >> >> George Cherian (2): >> watchdog: Add a new flag WDIOF_STOP_MAYSLEEP >> drivers: watchdog: Add support for panic notifier callback > > - George
Hello George, On 05.03.25 13:15, George Cherian wrote: >> On 05.03.25 12:28, George Cherian wrote: >>>> that can't be disabled and would protect against system lock up: >>>> Consider a memory-corruption bug (perhaps externally via DMA), which partially >>>> overwrites both main and kdump kernel. With a disabled watchdog, the system >>>> may not be able to recover on its own. >>> >>> Yes, that is the reason why the kernel command-line is optional and by default it is set to zero. >>> So that in cases if you have a corrupted kdump kernel then watchdog kicks in. >> >> The existing option isn't enough for the kdump kernel use case. >> If we (i.e. you) are going to do something about it, wouldn't it be >> better to have a solution that's applicable to a wider number of >> watchdog devices? > > I need a slight clarification here. > 1. reset_on_panic takes the number of seconds to be reloaded to watchdog HW, so that it initiates a > watchdog reset after the specified timeout, if kdump kernel fails to boot or hung while booting. Yes. > 2. in case reset_on_panic = 0 then it behaves like stop_on_panic=1. > Is this what you meant? Alternatively, reset_on_panic = 0 could also mean stopping the watchdog as you do now. I haven't thought though yet what would make the most sense. > I would let Guenter comment on this approach. +1. >> If you are serious with the watchdog use, you'll want to use the watchdog to >> monitor kernel startup as well. If the bootloader can set a watchdog timeout >> just before starting the kernel and it doesn't expire before the kernel watchdog >> driver takes over, why can't we do the same just before starting the dumpkernel? > > Yes, in an ideal world with ideal HW. But there are HW with issues which cannot have large > enough Watchdog time. Such HW would boot from FW to kernel without watchdog enabled. > And stop_on_panic does the similar for kdump kernel too. Yes, but there is likely more kinds of watchdog devices that can not be disabled, so it makes sense to have a solution that is more broadly applicable from the get-go. Cheers, Ahmad > > -George >> >> Thanks, >> Ahmad >> >> >>> >>> Thanks, >>> Ahmad >>> >>>> >>>> >>> Changelog: >>> v1 -> v2 >>> - Remove the per driver flag setting option >>> - Take the parameter via kernel command-line parameter to watchdog_core. >>> >>> v2 -> v3 >>> - Remove the helper function watchdog_stop_on_panic() from watchdog.h. >>> - There are no users for this. >>> >>> v3 -> v4 >>> - Since the panic notifier is in atomic context, watchdog functions >>> which sleep can't be called. >>> - Add an options flag WDIOF_STOP_MAYSLEEP to indicate whether stop >>> function sleeps. >>> - Simplify the stop_on_panic kernel command line parsing. >>> - Enable the panic notiffier only if the watchdog stop function doesn't >>> sleep >>> >>> George Cherian (2): >>> watchdog: Add a new flag WDIOF_STOP_MAYSLEEP >>> drivers: watchdog: Add support for panic notifier callback >> >> - George > >