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

[Ingo Molnar <mingo@kernel.org>]

* Mario Limonciello <superm1@kernel.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