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