linux-kernel - Re: [PATCH v6 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] [thread-next>] [day] [month] [year] [list]

Message-ID: <58e19481-530c-4465-aec5-1f44462eaf5f@gmail.com>
Date: Fri, 14 Mar 2025 14:22:40 +0200
From: Matti Vaittinen <mazziesaccount@...il.com>
To: Oleksij Rempel <o.rempel@...gutronix.de>,
 Sebastian Reichel <sre@...nel.org>,
 Srinivas Kandagatla <srinivas.kandagatla@...aro.org>,
 Benson Leung <bleung@...omium.org>, Tzung-Bi Shih <tzungbi@...nel.org>,
 Daniel Lezcano <daniel.lezcano@...aro.org>
Cc: 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 v6 3/7] power: reset: Introduce PSCR Recording Framework
 for Non-Volatile Storage

Hi deee Ho Oleksij,

On 14/03/2025 13:36, Oleksij Rempel wrote:
> This commit introduces the Power State Change Reasons Recording (PSCRR)
> framework into the kernel. The framework is vital for systems where
> PMICs or watchdogs cannot provide information on power state changes. It
> stores reasons for system shutdowns and reboots, like under-voltage or
> software-triggered events, in non-volatile hardware storage. This
> approach is essential for postmortem analysis in scenarios where
> traditional storage methods (block devices, RAM) are not feasible. The
> framework aids bootloaders and early-stage system components in recovery
> decision-making, although it does not cover resets caused by hardware
> issues like system freezes or watchdog timeouts.
> 
> Signed-off-by: Oleksij Rempel <o.rempel@...gutronix.de>

I see you're already at v6, so I am probably slightly late... I think I 
hadn't noticed this before. Thus, feel free to treat my comments as mere 
suggestions.

All in all, I do like this series. Looks mostly very good to me :) Just 
wondering if we could utilize this same for standardizing reading the 
reset reason registers which are included in many PMICs?

> ---

...

> +int pscrr_core_init(const struct pscrr_backend_ops *ops)
> +{
> +	enum psc_reason stored_val = PSCR_UNKNOWN;
> +	int ret;
> +
> +	mutex_lock(&pscrr_lock);
> +
> +	if (g_pscrr) {
> +		pr_err("PSCRR: Core is already initialized!\n");
> +		ret = -EBUSY;
> +		goto err_unlock;
> +	}
> +
> +	if (!ops->read_reason || !ops->write_reason) {
> +		pr_err("PSCRR: Backend must provide read and write callbacks\n");

Why both are required?

I can easily envision integrating the some PMIC's 'boot reason' register 
reading to the PSCRR. Benefit would be that user-space could use this 
same interface when reading the reset reason on a system where reason is 
stored using mechanisms provided by this series - and when reset reason 
is automatically stored by the HW (for example to a PMIC).

In a PMIC case the write_reason might not be needed, right?

> +		ret = -EINVAL;
> +		goto err_unlock;
> +	}
> +
> +	g_pscrr = kzalloc(sizeof(*g_pscrr), GFP_KERNEL);
> +	if (!g_pscrr) {
> +		ret = -ENOMEM;
> +		goto err_unlock;
> +	}
> +
> +	g_pscrr->ops = ops;
> +	g_pscrr->last_boot_reason = PSCR_UNKNOWN;
> +
> +	ret = ops->read_reason(&stored_val);
> +	if (!ret) {
> +		g_pscrr->last_boot_reason = stored_val;
> +		pr_info("PSCRR: Initial read_reason: %d (%s)\n",
> +			stored_val, psc_reason_to_str(stored_val));
> +	} else {
> +		pr_warn("PSCRR: read_reason failed, err=%pe\n",
> +			ERR_PTR(ret));
> +	}

...

> +/**
> + * struct pscrr_backend_ops - Backend operations for storing power state change
> + *			      reasons.
> + *
> + * This structure defines the interface for backend implementations that handle
> + * the persistent storage of power state change reasons. Different backends
> + * (e.g., NVMEM, EEPROM, battery-backed RAM) can implement these operations to
> + * store and retrieve shutdown reasons across reboots.

Maybe we should support / mention also a case where the PMIC driver 
could just register the read-callback and provide the reset reason 
stored by the PMIC to users. In this case the write_reason might not be 
needed.

> + *
> + * @write_reason: Function pointer to store the specified `psc_reason` in
> + *		  persistent storage. This function is called before a reboot
> + *		  to record the last power state change reason.
> + * @read_reason:  Function pointer to retrieve the last stored `psc_reason`
> + *		  from persistent storage. This function is called at boot to
> + *		  restore the shutdown reason.
> + */
> +struct pscrr_backend_ops {
> +	int (*write_reason)(enum psc_reason reason);
> +	int (*read_reason)(enum psc_reason *reason);
> +};

Yours,
	-- Matti