Message-ID: <2025071631-henna-synthesis-9961@gregkh>
Date: Wed, 16 Jul 2025 14:51:34 +0200
From: Greg KH <gregkh@...uxfoundation.org>
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: Oleksij Rempel <o.rempel@...gutronix.de>, 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>,
	Matti Vaittinen <mazziesaccount@...il.com>,
	Ahmad Fatoum <a.fatoum@...gutronix.de>,
	Andrew Morton <akpm@...ux-foundation.org>,
	chrome-platform@...ts.linux.dev
Subject: Re: [PATCH v11 6/7] power: reset: add PSCR NVMEM Driver for
 Recording Power State Change Reasons

On Wed, Jun 18, 2025 at 02:02:54PM +0200, Oleksij Rempel wrote:
> This driver utilizes the Power State Change Reasons Recording (PSCRR)
> framework to store specific power state change information, such as
> shutdown or reboot reasons, into a designated non-volatile memory
> (NVMEM) cell.
> 
> Signed-off-by: Oleksij Rempel <o.rempel@...gutronix.de>
> Tested-by: Francesco Valla <francesco@...la.it>
> ---
> changes v6:
> - rename pscr_reason to psc_reason
> changes v5:
> - avoid a build against NVMEM=m
> changes v4:
> - remove devicetree dependencies
> ---
>  drivers/power/reset/Kconfig       |  22 +++
>  drivers/power/reset/Makefile      |   1 +
>  drivers/power/reset/pscrr-nvmem.c | 254 ++++++++++++++++++++++++++++++
>  3 files changed, 277 insertions(+)
>  create mode 100644 drivers/power/reset/pscrr-nvmem.c
> 
> diff --git a/drivers/power/reset/Kconfig b/drivers/power/reset/Kconfig
> index 69e038e20731..3affef932e4d 100644
> --- a/drivers/power/reset/Kconfig
> +++ b/drivers/power/reset/Kconfig
> @@ -354,3 +354,25 @@ menuconfig PSCRR
>  	  be recorded unless hardware provides the reset cause.
>  
>  	  If unsure, say N.
> +
> +if PSCRR
> +
> +config PSCRR_NVMEM
> +	tristate "Generic NVMEM-based Power State Change Reason Recorder"
> +	depends on NVMEM || !NVMEM
> +	help
> +	  This option enables support for storing power state change reasons
> +	  (such as shutdown, reboot, or power failure events) into a designated
> +	  NVMEM (Non-Volatile Memory) cell.
> +
> +	  This feature allows embedded systems to retain power transition
> +	  history even after a full system restart or power loss. It is useful
> +	  for post-mortem debugging, automated recovery strategies, and
> +	  improving system reliability.
> +
> +	  The NVMEM cell used for storing these reasons can be dynamically
> +	  configured via module parameters.

Module parameters?  Why?

> +
> +	  If unsure, say N.
> +
> +endif
> diff --git a/drivers/power/reset/Makefile b/drivers/power/reset/Makefile
> index 025da19cb335..cc9008c8bb02 100644
> --- a/drivers/power/reset/Makefile
> +++ b/drivers/power/reset/Makefile
> @@ -34,6 +34,7 @@ obj-$(CONFIG_POWER_RESET_SYSCON) += syscon-reboot.o
>  obj-$(CONFIG_POWER_RESET_SYSCON_POWEROFF) += syscon-poweroff.o
>  obj-$(CONFIG_POWER_RESET_RMOBILE) += rmobile-reset.o
>  obj-$(CONFIG_PSCRR) += pscrr.o
> +obj-$(CONFIG_PSCRR_NVMEM) += pscrr-nvmem.o
>  obj-$(CONFIG_REBOOT_MODE) += reboot-mode.o
>  obj-$(CONFIG_SYSCON_REBOOT_MODE) += syscon-reboot-mode.o
>  obj-$(CONFIG_POWER_RESET_SC27XX) += sc27xx-poweroff.o
> diff --git a/drivers/power/reset/pscrr-nvmem.c b/drivers/power/reset/pscrr-nvmem.c
> new file mode 100644
> index 000000000000..7d02d989893f
> --- /dev/null
> +++ b/drivers/power/reset/pscrr-nvmem.c
> @@ -0,0 +1,254 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * pscrr_nvmem.c - PSCRR backend for storing shutdown reasons in small NVMEM
> + *		   cells
> + *
> + * This backend provides a way to persist power state change reasons in a
> + * non-volatile memory (NVMEM) cell, ensuring that reboot causes can be
> + * analyzed post-mortem. Unlike traditional logging to eMMC or NAND, which
> + * may be unreliable during power failures, this approach allows storing
> + * reboot reasons in small, fast-access storage like RTC scratchpads, EEPROM,
> + * or FRAM.
> + *
> + * The module allows dynamic configuration of the NVMEM device and cell
> + * via module parameters:
> + *
> + * Example usage:
> + *   modprobe pscrr-nvmem nvmem_name=pcf85063_nvram0 cell_name=pscr@0,0

Ugh, no, this isn't the 1990's anymore.  Module parameters don't make
sense for dynamic systems with multiple devices and names and the like.
Please use a proper api for this instead of this static one.

> +/*
> + * Module parameters:
> + *   nvmem_name: Name of the NVMEM device (e.g. "pcf85063_nvram0").
> + *   cell_name : Sysfs name of the cell on that device (e.g. "pscr@0,0").
> + */
> +static char *nvmem_name;
> +module_param(nvmem_name, charp, 0444);
> +MODULE_PARM_DESC(nvmem_name, "Name of the NVMEM device (e.g. pcf85063_nvram0)");
> +
> +static char *cell_name;
> +module_param(cell_name, charp, 0444);
> +MODULE_PARM_DESC(cell_name, "Sysfs name of the NVMEM cell (e.g. pscr@0,0)");

Again, don't.  Please don't.

You now only have one name, and one device.  WHat happens to the next
system that has multiple ones?  And who is enforcing this arbitrary
naming scheme?  Why is that now a user/kernel abi that can never change?

> +	pr_info("PSCRR-nvmem: Loaded (nvmem=%s, cell=%s), can store 0..%zu\n",
> +		nvmem_name, cell_name, priv->max_val);

Again, when code works, it is quiet.

> +static void __exit pscrr_nvmem_exit(void)
> +{
> +	pscrr_core_exit();
> +
> +	if (priv) {
> +		if (priv->cell) {
> +			nvmem_cell_put(priv->cell);
> +			priv->cell = NULL;
> +		}
> +		if (priv->nvmem) {
> +			nvmem_device_put(priv->nvmem);
> +			priv->nvmem = NULL;
> +		}
> +		kfree(priv);
> +		priv = NULL;
> +	}
> +
> +	pr_info("pscrr-nvmem: Unloaded\n");

Again, please be quiet.

And use pr_fmt().

thanks,

greg k-h

Powered by blists - more mailing lists