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

[Oleksij Rempel]

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.

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.

Would you agree that this approach keeps things maintainable while allowing 
for future extensibility? Also, do you have a preference for naming the 
subdirectory (`providers/`, `sources/`, etc.)?

Thanks again for your feedback!

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 |