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

[Mario Limonciello]

On 4/7/2025 1:46 PM, Ingo Molnar wrote:
> * Mario Limonciello <superm1@xxxxxxxxxx> 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?