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

[Greg KH]

Overall, no real issues, just some very minor ones:

On Wed, Jun 18, 2025 at 02:02:51PM +0200, Oleksij Rempel wrote:
> + * Sysfs Interface:
> + * ----------------
> + *   /sys/kernel/pscrr/reason       - Read/write current power state change
> + *				      reason
> + *   /sys/kernel/pscrr/reason_boot  - Read-only last recorded reason from
> + *				      previous boot

The sysfs documentation is in the ABI file, so it's not needed here.

> +static int pscrr_reboot_notifier(struct notifier_block *nb,
> +				 unsigned long action, void *unused)
> +{
> +	struct pscrr_backend *backend;
> +	int ret;
> +
> +	guard(g_pscrr)(&g_pscrr);
> +
> +	backend = g_pscrr.backend;
> +
> +	if (!backend || !backend->ops || !backend->ops->write_reason)
> +		return NOTIFY_DONE;
> +
> +	ret = backend->ops->write_reason(get_psc_reason());
> +	if (ret) {
> +		pr_err("PSCRR: Failed to store reason %d (%s) at reboot, err=%pe\n",
> +		       get_psc_reason(), psc_reason_to_str(get_psc_reason()),
> +		       ERR_PTR(ret));
> +	} else {
> +		pr_info("PSCRR: Stored reason %d (%s) at reboot.\n",
> +			get_psc_reason(), psc_reason_to_str(get_psc_reason()));

Why print anything?  If this works properly, it should be quiet, right?

> +static ssize_t reason_show(struct kobject *kobj, struct kobj_attribute *attr,
> +			   char *buf)
> +{
> +	struct pscrr_backend *backend;
> +	enum psc_reason r;
> +
> +	guard(g_pscrr)(&g_pscrr);
> +
> +	backend = g_pscrr.backend;
> +
> +	if (!backend || !backend->ops)
> +		return scnprintf(buf, PAGE_SIZE, "No backend registered\n");

So a string, or an int will be returned?  That's crazy, just return an
error here, -ENODEV?

> +
> +	/* If the backend can read from hardware, do so. Otherwise, use our cached value. */
> +	if (backend->ops->read_reason) {
> +		if (backend->ops->read_reason(&r) == 0) {
> +			/* Also update our cached value for consistency */
> +			set_psc_reason(r);
> +		} else {
> +			/* If read fails, fallback to cached. */
> +			r = get_psc_reason();
> +		}
> +	} else {
> +		r = get_psc_reason();
> +	}
> +
> +	return scnprintf(buf, PAGE_SIZE, "%d\n", r);

sysfs files should use sysfs_emit() so you don't get people sending you
patches later on to convert it :)

> +static ssize_t reason_store(struct kobject *kobj, struct kobj_attribute *attr,
> +			    const char *buf, size_t count)
> +{
> +	struct pscrr_backend *backend;
> +	long val;
> +	int ret;
> +
> +	guard(g_pscrr)(&g_pscrr);
> +
> +	backend = g_pscrr.backend;
> +
> +	if (!backend || !backend->ops || !backend->ops->write_reason)
> +		return -ENODEV;
> +
> +	ret = kstrtol(buf, 10, &val);
> +	if (ret)
> +		return ret;
> +
> +	if (val > U32_MAX)
> +		return -ERANGE;
> +
> +	if (val < PSCR_UNKNOWN || val > PSCR_MAX_REASON)
> +		/*
> +		 * Log a warning, but still attempt to write the value. In
> +		 * case the backend can handle it, we don't want to block it.
> +		 */
> +		pr_warn("PSCRR: writing unknown reason %ld (out of range)\n",
> +			val);

Do not let userspace cause a DoS of kernel log messages just because it
sent you an invalid data range.

> +static struct kobj_attribute reason_attr = __ATTR(reason, 0644, reason_show,
> +						  reason_store);

__ATTR_RW(), right?  If not, why not?

> +static struct kobj_attribute reason_boot_attr =
> +	__ATTR(reason_boot, 0444, reason_boot_show, NULL); /* Read-only */

__ATTR_RO(), right?  Then no comment is needed :)

> +int pscrr_core_init(const struct pscrr_backend_ops *ops)
> +{
> +	enum psc_reason stored_val = PSCR_UNKNOWN;
> +	struct pscrr_backend *backend;
> +	int ret;
> +
> +	guard(g_pscrr)(&g_pscrr);
> +
> +	backend = g_pscrr.backend;
> +
> +	if (backend) {
> +		pr_err("PSCRR: Core is already initialized!\n");

All of the "PSCRR:" stuff should just be set with pr_fmt being defined
at the top of this file, don't put it everywhere manually.

> +		return -EBUSY;
> +	}
> +
> +	if (!ops->read_reason) {
> +		pr_err("PSCRR: Backend must provide read callbacks\n");
> +		return -EINVAL;
> +	}
> +
> +	backend = kzalloc(sizeof(*backend), GFP_KERNEL);
> +	if (!backend)
> +		return -ENOMEM;
> +
> +	backend->ops = ops;
> +	backend->last_boot_reason = PSCR_UNKNOWN;
> +	g_pscrr.backend = backend;
> +
> +	ret = ops->read_reason(&stored_val);
> +	if (!ret) {
> +		backend->last_boot_reason = stored_val;
> +		pr_info("PSCRR: Initial read_reason: %d (%s)\n",
> +			stored_val, psc_reason_to_str(stored_val));

When code works properly, it should be quiet.  Don't spam the boot log
please.

> +	ret = sysfs_create_group(g_pscrr.kobj, &pscrr_attr_group);
> +	if (ret) {
> +		pr_err("PSCRR: Failed to create sysfs group, err=%pe\n",
> +		       ERR_PTR(ret));
> +		goto err_kobj_put;
> +	}
> +
> +	pr_info("PSCRR: initialized successfully.\n");

Same here, be quiet.

> +void pscrr_core_exit(void)
> +{
> +	guard(g_pscrr)(&g_pscrr);
> +
> +	if (!g_pscrr.backend)
> +		return;
> +
> +	if (g_pscrr.kobj) {
> +		sysfs_remove_group(g_pscrr.kobj, &pscrr_attr_group);
> +		kobject_put(g_pscrr.kobj);
> +	}
> +
> +	unregister_reboot_notifier(&g_pscrr.reboot_nb);
> +
> +	kfree(g_pscrr.backend);
> +	g_pscrr.backend = NULL;
> +
> +	pr_info("PSCRR: exited.\n");

Same here, please be quiet.

thanks,

greg k-h