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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <aBRWrwY6YN0526SN@pengutronix.de>
Date: Fri, 2 May 2025 07:22:55 +0200
From: Oleksij Rempel <o.rempel@...gutronix.de>
To: Sebastian Reichel <sebastian.reichel@...labora.com>
Cc: Srinivas Kandagatla <srinivas.kandagatla@...aro.org>,
	Benson Leung <bleung@...omium.org>,
	Tzung-Bi Shih <tzungbi@...nel.org>,
	Daniel Lezcano <daniel.lezcano@...aro.org>,
	Matti Vaittinen <mazziesaccount@...il.com>, kernel@...gutronix.de,
	linux-kernel@...r.kernel.org, Liam Girdwood <lgirdwood@...il.com>,
	Mark Brown <broonie@...nel.org>,
	"Rafael J. Wysocki" <rafael@...nel.org>,
	Zhang Rui <rui.zhang@...el.com>, Lukasz Luba <lukasz.luba@....com>,
	linux-pm@...r.kernel.org,
	Søren Andersen <san@...v.dk>,
	Guenter Roeck <groeck@...omium.org>,
	Ahmad Fatoum <a.fatoum@...gutronix.de>,
	Andrew Morton <akpm@...ux-foundation.org>,
	chrome-platform@...ts.linux.dev
Subject: Re: [PATCH v9 3/7] power: reset: Introduce PSCR Recording Framework
 for Non-Volatile Storage

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 |