Re: [PATCH v9 3/7] power: reset: Introduce PSCR Recording Framework for Non-Volatile Storage

[Oleksij Rempel]

Hi Sebastian,

Thanks for the feedback.

On Fri, May 02, 2025 at 01:20:50AM +0200, Sebastian Reichel wrote:
> Hi,
> 
> On Tue, Apr 22, 2025 at 10:57:13AM +0200, Oleksij Rempel wrote:
> > This commit introduces the Power State Change Reasons Recording (PSCRR)
> > framework. It provides a generic mechanism to store shutdown or reboot
> > reasons, such as under-voltage, thermal events, or software-triggered
> > actions, into non-volatile storage.
> > 
> > PSCRR is primarily intended for systems where software is able to detect
> > a power event in time and store the reason—typically when backup power
> > (e.g., capacitors) allows a short window before shutdown. This enables
> > reliable postmortem diagnostics, even on devices where traditional storage
> > like eMMC or NAND may not survive abrupt power loss.
> > 
> > In its current form, PSCRR focuses on software-reported reasons. However,
> > the framework is also designed with future extensibility in mind and could
> > serve as a central frontend for exposing hardware-reported reset reasons
> > from sources such as PMICs, watchdogs, or SoC-specific registers.
> > 
> > This version does not yet integrate such hardware-based reporting.
> 
> This adds quite some complex code for a very specific problem while
> mostly ignoring the common case of hardware reported reasons. I think
> having at least one hardware-reported reasons (e.g. WDOG) is mandatory
> to make sure the framework can really handle it.

The initial purpose of the PSCRR framework is to record power state
change reasons such as *undervoltage* or *regulator failures*, which can
be detected and stored by software just before shutdown. Supporting
hardware-reported reset reasons (like watchdog, PMIC, or SoC registers)
is possible by design — the interface allows it — but it’s not something
I consider mandatory for the initial version.

I’d prefer to focus on solving one concrete problem at a time, and the
current use case is already well-supported with software-side logic.

> I also see you extended power_on_reason.h and included it here

You're right — the `#include <linux/power/power_on_reason.h>` is no
longer used directly in `pscrr.c` and will be removed in the next
revision.

> but
> don't actually use the defined strings at all. Also you create the
> new enum, but don't handle the existing reasons and just add the new
> codes you need instead of making sure things are properly in-sync :(

Could you please clarify what exactly you mean here? The strings defined
in `power_on_reason.h` are still used indirectly via
`psc_reason_to_str()`, which is called in the reboot core and used by
PSCRR for logging (e.g., in `pscrr_reboot_notifier()` and
`pscrr_core_init()`). If your concern is about the enum values
themselves or something else, I'd appreciate a bit more detail.

> > [...]
> 
> > +struct pscrr_core {
> > +	struct mutex lock;
> > +	struct pscrr_backend *backend;
> > +	/* Kobject for sysfs */
> > +	struct kobject *kobj;
> > +	struct notifier_block reboot_nb;
> > +} g_pscrr = {
> > +	.lock = __MUTEX_INITIALIZER(g_pscrr.lock),
> > +};
> 
> Apart from the highlevel problems I have with this:
> 
> g_pscrr can be static
> 
> -- Sebastian

Best regards,  
Oleksij

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |