Re: [PATCH] Refine hpwdt message for UV platform

[Guenter Roeck]

On 3/16/26 14:45, Steve Wahl wrote:
> The watchdog hardware the hpwdt driver uses was added to the UV
> platform for UV5, but the logging mentioned by this driver was not
> added to the BIOS.  When the watchdog fires, the printed message had
> the administrators and developers looking for non-existent log files,
> and confused about whether a watchdog actually tripped.
>
> Change the message that prints on UV platforms so it doesn't send the
> user looking for non-existent logs.
>
> To aid in any future debugging, include all 8 bits of the NMISTAT
> register in the output, not just the two bits being used to determine
> this was "mynmi".  And provide names to the bits in NMISTAT so the
> code is easier to understand.
>
> Signed-off-by: Steve Wahl <steve.wahl@xxxxxxx>
> ---
>   drivers/watchdog/hpwdt.c | 27 ++++++++++++++++++++++-----
>   1 file changed, 22 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/watchdog/hpwdt.c b/drivers/watchdog/hpwdt.c
> index ae30e394d176..be5d8cbd0818 100644
> --- a/drivers/watchdog/hpwdt.c
> +++ b/drivers/watchdog/hpwdt.c
> @@ -24,6 +24,7 @@
>   #include <asm/nmi.h>
>   #endif
>   #include <linux/crash_dump.h>
> +#include <asm/uv/uv.h>
>   
>   #define HPWDT_VERSION			"2.0.4"
>   #define SECS_TO_TICKS(secs)		((secs) * 1000 / 128)
> @@ -158,9 +159,21 @@ static int hpwdt_set_pretimeout(struct watchdog_device *wdd, unsigned int req)
>   	return 0;
>   }
>   
> -static int hpwdt_my_nmi(void)
> +#define NMISTAT_EASR	BIT(0)
> +#define NMISTAT_EWDOG	BIT(1)
> +#define NMISTAT_RTRAP	BIT(2)
> +#define NMISTAT_DBELL	BIT(3)
> +#define NMISTAT_EMSWDG	BIT(4)
> +#define NMISTAT_GPI	BIT(5)
> +#define NMISTAT_NMIOUT	BIT(7)
> +
> +static bool hpwdt_my_nmi(u8 *nmistat)
>   {
> -	return ioread8(hpwdt_nmistat) & 0x6;
> +	u8 val = ioread8(hpwdt_nmistat);
> +
> +	if (nmistat)
> +		*nmistat = val;

What is the point of this NULL check ? The parameter is never NULL.

> +	return (val & (NMISTAT_EWDOG | NMISTAT_RTRAP)) != 0;

I am not sure if it adds any value to do this check here instead of just
returning nmistat and checking in the calling code. Seems to me this just
adds unnecessary complexity to the code.

Guenter

>   }
>   
>   /*
> @@ -168,14 +181,18 @@ static int hpwdt_my_nmi(void)
>    */
>   static int hpwdt_pretimeout(unsigned int ulReason, struct pt_regs *regs)
>   {
> -	unsigned int mynmi = hpwdt_my_nmi();
> -	static char panic_msg[] =
> +	u8 nmistat;
> +	bool mynmi = hpwdt_my_nmi(&nmistat);
> +	static char panic_msg_default[] =
>   		"00: An NMI occurred. Depending on your system the reason "
>   		"for the NMI is logged in any one of the following resources:\n"
>   		"1. Integrated Management Log (IML)\n"
>   		"2. OA Syslog\n"
>   		"3. OA Forward Progress Log\n"
>   		"4. iLO Event Log";
> +	static char panic_msg_uv[] =
> +		"00: A watchdog NMI occurred.";
> +	char *panic_msg = is_uv_system() ? panic_msg_uv : panic_msg_default;
>   
>   	if (ulReason == NMI_UNKNOWN && !mynmi)
>   		return NMI_DONE;
> @@ -189,7 +206,7 @@ static int hpwdt_pretimeout(unsigned int ulReason, struct pt_regs *regs)
>   		hpwdt_ping_ticks(SECS_TO_TICKS(val));
>   	}
>   
> -	hex_byte_pack(panic_msg, mynmi);
> +	hex_byte_pack(panic_msg, nmistat);
>   	nmi_panic(regs, panic_msg);
>   
>   	return NMI_HANDLED;