linux-kernel - Re: [PATCH v2] watchdog: sp5100_tco: Add "action" module parameter

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20220922175110.GC960@perchik>
Date:   Thu, 22 Sep 2022 11:51:11 -0600
From:   Jerry Hoemann <jerry.hoemann@....com>
To:     Vladimir Panteleev <git@...dimir.panteleev.md>
Cc:     Wim Van Sebroeck <wim@...ux-watchdog.org>,
        Guenter Roeck <linux@...ck-us.net>,
        linux-watchdog@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] watchdog: sp5100_tco: Add "action" module parameter

On Tue, Sep 20, 2022 at 09:27:21AM +0000, Vladimir Panteleev wrote:
> Allow configuring the "action" bit, as documented in [1].
> 
> Previously, the only action supported by this module was to reset the
> system (0).  It can now be configured to power off (1) instead.
> 
> [1]: https://www.amd.com/system/files/TechDocs/44413.pdf
> 
> Signed-off-by: Vladimir Panteleev <git@...dimir.panteleev.md>
> ---
> 
> Changes since v1:
>  - Drop the rename of the SP5100_WDT_ACTION_RESET define
>  - Make the new parameter not visible in sysfs for consistency
> 
>  drivers/watchdog/sp5100_tco.c | 13 +++++++++++--
>  1 file changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/watchdog/sp5100_tco.c b/drivers/watchdog/sp5100_tco.c
> index ae54dd33e233..fb426b7d81da 100644
> --- a/drivers/watchdog/sp5100_tco.c
> +++ b/drivers/watchdog/sp5100_tco.c
> @@ -65,6 +65,12 @@ static struct pci_dev *sp5100_tco_pci;
>  
>  /* module parameters */
>  
> +#define WATCHDOG_ACTION 0
> +static bool action = WATCHDOG_ACTION;
> +module_param(action, bool, 0);
> +MODULE_PARM_DESC(action, "Action taken when watchdog expires, 0 to reset, 1 to poweroff (default="
> +		 __MODULE_STRING(WATCHDOG_ACTION) ")");
> +
>  #define WATCHDOG_HEARTBEAT 60	/* 60 sec default heartbeat. */
>  static int heartbeat = WATCHDOG_HEARTBEAT;  /* in seconds */
>  module_param(heartbeat, int, 0);
> @@ -297,8 +303,11 @@ static int sp5100_tco_timer_init(struct sp5100_tco *tco)
>  	if (val & SP5100_WDT_FIRED)
>  		wdd->bootstatus = WDIOF_CARDRESET;
>  
> -	/* Set watchdog action to reset the system */
> -	val &= ~SP5100_WDT_ACTION_RESET;
> +	/* Set watchdog action */
> +	if (action)
> +		val |= SP5100_WDT_ACTION_RESET;
> +	else
> +		val &= ~SP5100_WDT_ACTION_RESET;
>  	writel(val, SP5100_WDT_CONTROL(tco->tcobase));
>  
>  	/* Set a reasonable heartbeat before we stop the timer */
> -- 
> 2.37.3

Looking at other WDT,  I see at least three  that have an "action"
parameter.  These appear to have inconsistent semantics with each 
other and this patch.

Is there something we could to create more uniformity with module
level parameters like these?


Examples:

machzwd.c:
MODULE_PARM_DESC(action, "after watchdog resets, generate: "
                                "0 = RESET(*)  1 = SMI  2 = NMI  3 = SCI");

pseries-wdt.c
static const unsigned long pseries_wdt_action[] = {
        [0] = PSERIES_WDTF_ACTION_HARD_POWEROFF,
	[1] = PSERIES_WDTF_ACTION_HARD_RESTART,
	[2] = PSERIES_WDTF_ACTION_DUMP_RESTART,

sbsa_gwdt.c
MODULE_PARM_DESC(action, "after watchdog gets WS0 interrupt, do: "
                 "0 = skip(*)  1 = panic");


-- 

-----------------------------------------------------------------------------
Jerry Hoemann                  Software Engineer   Hewlett Packard Enterprise
-----------------------------------------------------------------------------