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

[Matti Vaittinen]

On 14/03/2025 15:58, Oleksij Rempel wrote:
> Hi Matti,
>
> Thanks for your feedback and for taking the time to review this patch series!
>
> On Fri, Mar 14, 2025 at 02:22:40PM +0200, Matti Vaittinen wrote:
> > 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?
>   
> I agree that PMICs could be valuable sources of reset reasons, and integrating
> them into PSCRR makes sense. However, this introduces new challenges when
> multiple providers exist on the same system, each reporting different power
> state change reasons.

I see that now. I didn't think of having multiple sources of this 
information at the same time, but I agree it is a valid setup. For 
example an under voltage warning triggering software reset could leave a 
reason in PSCRR non volatile memory, while it wouldn't leave a trace in 
a PMIC. More severe under voltage on same system might trigger the 
hardware shutdown by the PMIC, which would leave the reason in PMIC but 
not necessarily to the non volatile memory (depending on the amount 
backup-energy). So yes, we might need multiple sources to get a good 
picture. Thanks for pointing this out to me.

> Handling Multiple Read-Only Providers (PMIC, Firmware, etc.):
> - If we have multiple sources (e.g., PMIC, firmware logs, NVMEM-based storage),
>    we need to define how to handle conflicting or differing reset reasons.
> - Using priorities may work in simple cases but is unlikely to scale well
>    across different platforms.
> - A more flexible solution would be to expose all read-only providers
>    separately, rather than forcing one to override others.
>
> Potential UAPI and Sysfs Structure
> - The current sysfs API exposes:
>    - `/sys/kernel/pscrr/reason` → Default recorder
>    - `/sys/kernel/pscrr/reason_boot` → Last stored reason on default recorder
>       from before boot
> - If we introduce read-only providers (like PMICs), we may need a dedicated
>    subdirectory to keep them separate.
> - A possible structure:
>    ```
>    /sys/kernel/pscrr/
>    ├── reason         # Default recorder
>    ├── reason_boot    # Default recorder (before boot)
>    ├── providers/
>    │   ├── pmic0      # Read-only reset reason from PMIC
>    │   ├── firmware   # Reset reason from firmware logs
>    │   ├── another_provider
>    ```
> - This would allow user-space tools to query all available sources while keeping
>    the default recorder behavior intact.
>
> Next Steps
> - I propose keeping this patch series focused on the default PSCRR recorder.
> - Support for multiple read-only providers should be designed as a future
>    extension, possibly with an expanded sysfs API.

Makes sense. Better to get something ready than keep everything on the 
table. :) Still wonder if we should allow registering read-only 
providers, even if we supported only a single source? But, I am not 
insisting this, just doing a suggestion. Don't want to make "the 
perfect" to be the enemy of good. (Actually, when I think of it, in many 
cases I wouldn't want to even make the "good" to be enemy of "much 
better than nothing").

> Would you agree that this approach keeps things maintainable while allowing
> for future extensibility?

Sure.

> Also, do you have a preference for naming the
> subdirectory (`providers/`, `sources/`, etc.)?

I am bad with naming but both of those would work for me :)

Yours,
	-- Matti