[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <ebfaae8d-7186-454f-ba06-b86fea357d03@kernel.org>
Date: Mon, 7 Apr 2025 13:56:57 -0500
From: Mario Limonciello <superm1@...nel.org>
To: Ingo Molnar <mingo@...nel.org>, Borislav Petkov <bp@...en8.de>
Cc: 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
On 4/7/2025 1:46 PM, Ingo Molnar wrote:
>
> * 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.
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?
Powered by blists - more mailing lists