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

From: Matti Vaittinen
Date: Fri Mar 14 2025 - 08:22:57 EST


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@xxxxxxxxxxxxxx>

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