Messages in this thread |  | | Date | Fri, 11 Apr 2025 07:12:24 -0500 | | Subject | Re: [PATCH v3 4/4] x86/CPU/AMD: Print the reason for the last reset | | From | Mario Limonciello <> |
| |
On 4/11/25 07:06, Borislav Petkov wrote: > On Thu, Apr 10, 2025 at 03:02:02PM -0500, Mario Limonciello wrote: >> +static __init int print_s5_reset_status_mmio(void) >> +{ >> + void __iomem *addr; >> + unsigned long value; >> + int bit = -1; >> + >> + if (!cpu_feature_enabled(X86_FEATURE_ZEN)) >> + return 0; >> + >> + addr = ioremap(FCH_PM_BASE + FCH_PM_S5_RESET_STATUS, sizeof(value)); >> + if (!addr) >> + return 0; > > newline. > >> + value = ioread32(addr); >> + iounmap(addr); >> + >> + do { >> + bit = find_next_bit(&value, BITS_PER_LONG, bit + 1); >> + } while (!s5_reset_reason_txt[bit]); > > What's the idea here? The highest bit is the most fitting one? > > So why don't you do fls() or so?
The idea was to walk all the bits and pick the first one that has a string associated with it. I was finding that sometimes the reserved bits are set which would get you a NULL pointer deref.
> >> + pr_info("x86/amd: Previous system reset reason [0x%08lx]: %s\n", >> + value, s5_reset_reason_txt[bit]); > > What's guaranteeing that s5_reset_reason_txt[bit] is still set here? > > I'd suggest you check it again and never trust the hw because we'll be fixing > a null ptr here at some point otherwise... >
Right; I was worried about that too but find_next_bit() will return the size argument when it doesn't find anything.
So that should be s5_reset_reason_txt[32] which has the "Unknown" string.
|  |