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

[Matti Vaittinen]

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