linux-kernel - Re: [PATCH 2/2] x86/CPU/AMD: Print the reason for the last reset

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 for Android: free password hash cracker in your pocket

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

Message-ID: <Z_Qdn_WYAalNAHOi@gmail.com>
Date: Mon, 7 Apr 2025 20:46:55 +0200
From: Ingo Molnar <mingo@...nel.org>
To: Mario Limonciello <superm1@...nel.org>
Cc: Borislav Petkov <bp@...en8.de>, Jonathan Corbet <corbet@....net>,
	Thomas Gleixner <tglx@...utronix.de>,
	Ingo Molnar <mingo@...hat.com>,
	Dave Hansen <dave.hansen@...ux.intel.com>,
	"maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT)" <x86@...nel.org>,
	"H . Peter Anvin" <hpa@...or.com>,
	"open list:DOCUMENTATION" <linux-doc@...r.kernel.org>,
	open list <linux-kernel@...r.kernel.org>,
	Yazen Ghannam <yazen.ghannam@....com>,
	Mario Limonciello <mario.limonciello@....com>
Subject: Re: [PATCH 2/2] x86/CPU/AMD: Print the reason for the last reset


* Mario Limonciello <superm1@...nel.org> wrote:

> +++ b/Documentation/admin-guide/amd/amd-reboot-reason.csv
> @@ -0,0 +1,21 @@
> +Bit, Type, Reason
> +0, Pin, Thermal trip (BP_THERMTRIP_L)
> +1, Pin, Power button
> +2, Pin, SHUTDOWN# pin
> +4, Remote, Remote ASF power off command
> +9, Internal, Thermal trip (internal)
> +16, Pin, User reset (BP_SYS_RST_L)
> +17, Software, PCI reset (PMIO 0xC4)
> +18, Software, CF9 reset (0x04)
> +19, Software, CF9 reset (0x06)
> +20, Software, CF9 reset (0x0E)
> +21, Sleep, Power state or ACPI state transition
> +22, Pin, Keyboard reset (KB_RST_L)
> +23, Internal, Internal CPU shutdown
> +24, Hardware, Failed boot timer
> +25, Hardware, Watchdog timer
> +26, Remote, Remote ASF reset command
> +27, Internal, Data fabric sync flood event due to uncorrected error
> +29, Internal, MP1 Watchdog timer timeout
> +30, Internal, Parity error
> +31, Internal, SW sync flood event

So I'd much prefer if each bit was iterated, and the above reasons were 
printed out clearly, instead of some arbitrary meta grouping that 
removes useful diagnostic information:

> +#define PIN_RESET	(BIT(0) | BIT(1) | BIT(2) | BIT(16) | BIT(22))
> +#define REMOTE_RESET	(BIT(4) | BIT(26))
> +#define INTERNAL_RESET	(BIT(9) | BIT(23) | BIT(27) | BIT(29) | BIT(30) | BIT(31))
> +#define SW_RESET	(BIT(17) | BIT(18) | BIT(19) | BIT(20))
> +#define HW_RESET	(BIT(24) | BIT(25))
> +#define SLEEP_RESET	(BIT(21))

> +	pr_info("System reset was due to %s (0x%08x)\n",
> +		get_s5_reset_reason(value), value);

I realize that the entire numeric value gets printed as well, but it's 
the symbolic decoding that is most useful to humans.

Also, by printing unknown but set bits as 'unknown' we'd have a way to 
clearly signal to users that there's some new diagnostic flag the 
kernel doesn't understand yet.

Just a couple of examples:

 - Printing "Internal, Data fabric sync flood event due to uncorrected error"
   or "Internal, Parity error" would indicate potential RAM module troubles,
   while "Internal, Thermal trip (internal)" would indicate cooling system
   troubles. But with your patch both get printed as some sort of 'internal CPU'
   problem that is unnecessarily unhelpful...

 - I don't think representing bit 24 ('Hardware, Failed boot timer') as 
   'Hardware induced' reboot is really helpful either, to me it appears to be
   a failed bootup time treshold that is more of a firmware thing that may or 
   may not indicate hardware troubles.

 - etc. etc.

Basically, the finegrained list of reasons looks perfectly usable to 
me, let's not dumb it down for users unnecessarily, okay?

Thanks,

	Ingo