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

[Mario Limonciello <superm1@kernel.org>]

On 4/7/2025 1:46 PM, Ingo Molnar wrote:
> 
> * 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.

The way that I "envisioned" this working was someone uses their machine 
and suddenly hits a problem.  When they do they look at 
amd/debugging.rst and then run:

"journalctl -k | grep "System reset was due"

and then map the odd duck(s) out to the table.

> 
> 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.

Right.

> 
> 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?

Boris, your thoughts please?