[PATCH v7 0/3] hwspinlock: convert to XArray and add debug capabilities

[PATCH v7 0/3] hwspinlock: convert to XArray and add debug capabilities

[Wolfram Sang]

Changes since v6 are only a rebase to linux-next as of 20260911 and a
typo in the subject of patch 2 (was "list of mailboxes" instead of "list
of locks").

I would really love to get this applied. Since there have not been any
reviewers lately, maybe it is time to put it into -next and see what
happens. I will be there to fix things. Or we just revert if it goes all
wrong. But Matthew is the creator of XArray, I tested this as much as I
could and I took the Sashiko reviews into account. I don't know what to
do else. But I can't continue fixing the real issues in the hwspinlock
subsystem unless this series is applied :/ Here is the old coverletter
for more details:

I will not add more functionality to upcoming versions of this series,
only address review comments. I do have a list of issues I still want to
fix in hwspinlock, but only once this series is applied. So, I hope we
can upstream this soon.

Renesas R-Car SoCs have their spinlocks inside a unit called MFIS. Up to
R-Car Gen4, there was only one MFIS unit on the SoC. Gen5, though, has
multiple instances and, thus, multiple spinlock providers. The spinlocks
are meant for specific cases (AP<->AP, AP<->RT, AP<->SCP...). For
development on these systems, it is helpful to have an overview of
registered spinlocks in debugfs. Also, for debugging the hwspinlock
core, it is helpful to change lock states from userspace to trigger
corner cases.  This series adds support for all that. The first patch
converts the subsystem from the deprecated radix-tree to the easier to
handle XArray (Thank you again, Matthew!). The second patch adds the
summary in debugfs based on the new XArray implementation. The third
patch implements the low level debug interface.

A branch for testing is here:

git://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git renesas/hwspinlock/xarray

It has been tested on a SparrowHawk board (R-Car V4H) with one MFIS
instance and an Ironhide board (R-Car X5H) with two MFIS instances.

Looking forward to comments.


Matthew Wilcox (Oracle) (1):
  hwspinlock: Convert to XArray

Wolfram Sang (2):
  hwspinlock: add list of locks to debugfs
  hwspinlock: add low level debug capabilities

 drivers/hwspinlock/hwspinlock_core.c | 369 ++++++++++++++++++++-------
 1 file changed, 270 insertions(+), 99 deletions(-)

-- 
2.53.0

[PATCH v7 1/3] hwspinlock: Convert to XArray

[Wolfram Sang]

From: "Matthew Wilcox (Oracle)" <willy@infradead.org>

The radix tree is deprecated.  The XArray uses the same data structure
with a nicer interface.  The hwspinlock_tree_lock is not needed as the
spinlock built into the XArray is sufficient for all these cases after
we split a larger critical section in hwspin_lock_request_specific()
into two smaller ones.

Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
Co-developed-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 drivers/hwspinlock/hwspinlock_core.c | 170 +++++++++++----------------
 1 file changed, 71 insertions(+), 99 deletions(-)

diff --git a/drivers/hwspinlock/hwspinlock_core.c b/drivers/hwspinlock/hwspinlock_core.c
index a509b73da190..22e6c7690f1a 100644
--- a/drivers/hwspinlock/hwspinlock_core.c
+++ b/drivers/hwspinlock/hwspinlock_core.c
@@ -16,7 +16,7 @@
 #include <linux/types.h>
 #include <linux/err.h>
 #include <linux/jiffies.h>
-#include <linux/radix-tree.h>
+#include <linux/xarray.h>
 #include <linux/hwspinlock.h>
 #include <linux/pm_runtime.h>
 #include <linux/mutex.h>
@@ -27,35 +27,21 @@
 /* retry delay used in atomic context */
 #define HWSPINLOCK_RETRY_DELAY_US	100
 
-/* radix tree tags */
-#define HWSPINLOCK_UNUSED	(0) /* tags an hwspinlock as unused */
+/* XArray search mark */
+#define HWSPINLOCK_UNUSED	XA_MARK_0 /* marks a hwspinlock as unused */
 
 /*
- * A radix tree is used to maintain the available hwspinlock instances.
+ * An XArray is used to maintain the available hwspinlock instances.
  * The tree associates hwspinlock pointers with their integer key id,
  * and provides easy-to-use API which makes the hwspinlock core code simple
  * and easy to read.
  *
- * Radix trees are quick on lookups, and reasonably efficient in terms of
+ * XArrays are quick on lookups, and reasonably efficient in terms of
  * storage, especially with high density usages such as this framework
  * requires (a continuous range of integer keys, beginning with zero, is
- * used as the ID's of the hwspinlock instances).
- *
- * The radix tree API supports tagging items in the tree, which this
- * framework uses to mark unused hwspinlock instances (see the
- * HWSPINLOCK_UNUSED tag above). As a result, the process of querying the
- * tree, looking for an unused hwspinlock instance, is now reduced to a
- * single radix tree API call.
+ * used as the ID of the hwspinlock instances).
  */
-static RADIX_TREE(hwspinlock_tree, GFP_KERNEL);
-
-/*
- * Synchronization of access to the tree is achieved using this mutex,
- * as the radix-tree API requires that users provide all synchronisation.
- * A mutex is needed because we're using non-atomic radix tree allocations.
- */
-static DEFINE_MUTEX(hwspinlock_tree_lock);
-
+static DEFINE_XARRAY(hwspinlocks);
 
 /**
  * __hwspin_trylock() - attempt to lock a specific hwspinlock
@@ -369,10 +355,9 @@ of_hwspin_lock_simple_xlate(const struct of_phandle_args *hwlock_spec)
  */
 int of_hwspin_lock_get_id(struct device_node *np, int index)
 {
+	XA_STATE(xas, &hwspinlocks, 0);
 	struct of_phandle_args args;
 	struct hwspinlock *hwlock;
-	struct radix_tree_iter iter;
-	void **slot;
 	int id;
 	int ret;
 
@@ -389,15 +374,9 @@ int of_hwspin_lock_get_id(struct device_node *np, int index)
 	/* Find the hwspinlock device: we need its base_id */
 	ret = -EPROBE_DEFER;
 	rcu_read_lock();
-	radix_tree_for_each_slot(slot, &hwspinlock_tree, &iter, 0) {
-		hwlock = radix_tree_deref_slot(slot);
-		if (unlikely(!hwlock))
+	xas_for_each(&xas, hwlock, ULONG_MAX) {
+		if (xas_retry(&xas, hwlock))
 			continue;
-		if (radix_tree_deref_retry(hwlock)) {
-			slot = radix_tree_iter_retry(&iter);
-			continue;
-		}
-
 		if (device_match_of_node(hwlock->bank->dev, args.np)) {
 			ret = 0;
 			break;
@@ -452,51 +431,50 @@ EXPORT_SYMBOL_GPL(of_hwspin_lock_get_id_byname);
 
 static int hwspin_lock_register_single(struct hwspinlock *hwlock, int id)
 {
-	struct hwspinlock *tmp;
+	XA_STATE(xas, &hwspinlocks, id);
+	struct hwspinlock *existing;
 	int ret;
 
-	mutex_lock(&hwspinlock_tree_lock);
-
-	ret = radix_tree_insert(&hwspinlock_tree, id, hwlock);
-	if (ret) {
-		if (ret == -EEXIST)
+	do {
+		xas_lock(&xas);
+		existing = xas_load(&xas);
+		if (existing) {
 			pr_err("hwspinlock id %d already exists!\n", id);
-		goto out;
-	}
-
-	/* mark this hwspinlock as available */
-	tmp = radix_tree_tag_set(&hwspinlock_tree, id, HWSPINLOCK_UNUSED);
+			xas_set_err(&xas, -EBUSY);
+		}
+		xas_store(&xas, hwlock);
 
-	/* self-sanity check which should never fail */
-	WARN_ON(tmp != hwlock);
+		/* mark this hwspinlock as available */
+		xas_set_mark(&xas, HWSPINLOCK_UNUSED);
+		ret = xas_error(&xas);
+		xas_unlock(&xas);
+	} while (xas_nomem(&xas, GFP_KERNEL));
 
-out:
-	mutex_unlock(&hwspinlock_tree_lock);
 	return ret;
 }
 
 static struct hwspinlock *hwspin_lock_unregister_single(unsigned int id)
 {
+	XA_STATE(xas, &hwspinlocks, id);
 	struct hwspinlock *hwlock = NULL;
-	int ret;
+	bool unused;
 
-	mutex_lock(&hwspinlock_tree_lock);
+	xas_lock(&xas);
+	xas_load(&xas);
 
-	/* make sure the hwspinlock is not in use (tag is set) */
-	ret = radix_tree_tag_get(&hwspinlock_tree, id, HWSPINLOCK_UNUSED);
-	if (ret == 0) {
+	/* make sure the hwspinlock is not in use (mark is set) */
+	unused = xas_get_mark(&xas, HWSPINLOCK_UNUSED);
+	if (!unused) {
 		pr_err("hwspinlock %d still in use (or not present)\n", id);
 		goto out;
 	}
 
-	hwlock = radix_tree_delete(&hwspinlock_tree, id);
-	if (!hwlock) {
-		pr_err("failed to delete hwspinlock %d\n", id);
-		goto out;
-	}
+	hwlock = xas_store(&xas, NULL);
+	if (!hwlock)
+		pr_err("hwspinlock %d already deleted\n", id);
 
 out:
-	mutex_unlock(&hwspinlock_tree_lock);
+	xas_unlock(&xas);
 	return hwlock;
 }
 
@@ -666,20 +644,18 @@ int devm_hwspin_lock_register(struct device *dev,
 EXPORT_SYMBOL_GPL(devm_hwspin_lock_register);
 
 /**
- * __hwspin_lock_request() - tag an hwspinlock as used and power it up
+ * hwspin_lock_prepare() - prepare a hwspinlock
  * @hwlock: the target hwspinlock
  *
  * This is an internal function that prepares an hwspinlock instance
- * before it is given to the user. The function assumes that
- * hwspinlock_tree_lock is taken.
+ * before it is given to the user.
  *
  * Returns: %0 or positive to indicate success, and a negative value to
  * indicate an error (with the appropriate error code)
  */
-static int __hwspin_lock_request(struct hwspinlock *hwlock)
+static int hwspin_lock_prepare(struct hwspinlock *hwlock)
 {
 	struct device *dev = hwlock->bank->dev;
-	struct hwspinlock *tmp;
 	int ret;
 
 	/* prevent underlying implementation from being removed */
@@ -697,16 +673,7 @@ static int __hwspin_lock_request(struct hwspinlock *hwlock)
 		return ret;
 	}
 
-	ret = 0;
-
-	/* mark hwspinlock as used, should not fail */
-	tmp = radix_tree_tag_clear(&hwspinlock_tree, hwlock_to_id(hwlock),
-							HWSPINLOCK_UNUSED);
-
-	/* self-sanity check that should never fail */
-	WARN_ON(tmp != hwlock);
-
-	return ret;
+	return 0;
 }
 
 /**
@@ -724,36 +691,43 @@ static int __hwspin_lock_request(struct hwspinlock *hwlock)
  */
 struct hwspinlock *hwspin_lock_request_specific(unsigned int id)
 {
+	XA_STATE(xas, &hwspinlocks, id);
 	struct hwspinlock *hwlock;
+	bool unused;
 	int ret;
 
-	mutex_lock(&hwspinlock_tree_lock);
+	xas_lock(&xas);
 
-	/* make sure this hwspinlock exists */
-	hwlock = radix_tree_lookup(&hwspinlock_tree, id);
+	hwlock = xas_load(&xas);
 	if (!hwlock) {
+		xas_unlock(&xas);
 		pr_warn("hwspinlock %u does not exist\n", id);
-		goto out;
+		return NULL;
 	}
 
 	/* sanity check (this shouldn't happen) */
 	WARN_ON(hwlock_to_id(hwlock) != id);
 
-	/* make sure this hwspinlock is unused */
-	ret = radix_tree_tag_get(&hwspinlock_tree, id, HWSPINLOCK_UNUSED);
-	if (ret == 0) {
+	unused = xas_get_mark(&xas, HWSPINLOCK_UNUSED);
+	if (!unused) {
+		xas_unlock(&xas);
 		pr_warn("hwspinlock %u is already in use\n", id);
-		hwlock = NULL;
-		goto out;
+		return NULL;
 	}
 
-	/* mark as used and power up */
-	ret = __hwspin_lock_request(hwlock);
-	if (ret < 0)
-		hwlock = NULL;
+	xas_clear_mark(&xas, HWSPINLOCK_UNUSED);
+	xas_unlock(&xas);
+
+	ret = hwspin_lock_prepare(hwlock);
+	if (ret < 0) {
+		xas_lock(&xas);
+		xas_set(&xas, hwlock_to_id(hwlock));
+		xas_load(&xas);
+		xas_set_mark(&xas, HWSPINLOCK_UNUSED);
+		xas_unlock(&xas);
+		return NULL;
+	}
 
-out:
-	mutex_unlock(&hwspinlock_tree_lock);
 	return hwlock;
 }
 EXPORT_SYMBOL_GPL(hwspin_lock_request_specific);
@@ -772,9 +746,10 @@ EXPORT_SYMBOL_GPL(hwspin_lock_request_specific);
  */
 int hwspin_lock_free(struct hwspinlock *hwlock)
 {
+	XA_STATE(xas, &hwspinlocks, 0);
 	struct device *dev;
-	struct hwspinlock *tmp;
-	int ret;
+	bool unused;
+	int ret = 0;
 
 	if (!hwlock) {
 		pr_err("invalid hwlock\n");
@@ -782,12 +757,13 @@ int hwspin_lock_free(struct hwspinlock *hwlock)
 	}
 
 	dev = hwlock->bank->dev;
-	mutex_lock(&hwspinlock_tree_lock);
+	xas_lock(&xas);
+	xas_set(&xas, hwlock_to_id(hwlock));
+	xas_load(&xas);
 
 	/* make sure the hwspinlock is used */
-	ret = radix_tree_tag_get(&hwspinlock_tree, hwlock_to_id(hwlock),
-							HWSPINLOCK_UNUSED);
-	if (ret == 1) {
+	unused = xas_get_mark(&xas, HWSPINLOCK_UNUSED);
+	if (unused) {
 		dev_err(dev, "%s: hwlock is already free\n", __func__);
 		dump_stack();
 		ret = -EINVAL;
@@ -798,16 +774,12 @@ int hwspin_lock_free(struct hwspinlock *hwlock)
 	pm_runtime_put(dev);
 
 	/* mark this hwspinlock as available */
-	tmp = radix_tree_tag_set(&hwspinlock_tree, hwlock_to_id(hwlock),
-							HWSPINLOCK_UNUSED);
-
-	/* sanity check (this shouldn't happen) */
-	WARN_ON(tmp != hwlock);
+	xas_set_mark(&xas, HWSPINLOCK_UNUSED);
 
 	module_put(dev->driver->owner);
 
 out:
-	mutex_unlock(&hwspinlock_tree_lock);
+	xas_unlock(&xas);
 	return ret;
 }
 EXPORT_SYMBOL_GPL(hwspin_lock_free);
-- 
2.53.0

[PATCH v7 2/3] hwspinlock: add list of locks to debugfs

[Wolfram Sang]

To help debugging, offer a list of registered hwspinlocks in debugfs. It
will also print the responsible hwspinlock_device and if the spinlock is
currently used. It does not show if the lock is taken because of
side-effects. Often, reading a lock means actually taking it.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 drivers/hwspinlock/hwspinlock_core.c | 67 ++++++++++++++++++++++++++++
 1 file changed, 67 insertions(+)

diff --git a/drivers/hwspinlock/hwspinlock_core.c b/drivers/hwspinlock/hwspinlock_core.c
index 22e6c7690f1a..947b4d4fa153 100644
--- a/drivers/hwspinlock/hwspinlock_core.c
+++ b/drivers/hwspinlock/hwspinlock_core.c
@@ -9,6 +9,7 @@
 
 #define pr_fmt(fmt)    "%s: " fmt, __func__
 
+#include <linux/debugfs.h>
 #include <linux/delay.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
@@ -21,6 +22,7 @@
 #include <linux/pm_runtime.h>
 #include <linux/mutex.h>
 #include <linux/of.h>
+#include <linux/seq_file.h>
 
 #include "hwspinlock_internal.h"
 
@@ -860,5 +862,70 @@ struct hwspinlock *devm_hwspin_lock_request_specific(struct device *dev,
 }
 EXPORT_SYMBOL_GPL(devm_hwspin_lock_request_specific);
 
+#ifdef CONFIG_DEBUG_FS
+static void *hwspin_lock_seq_start(struct seq_file *s, loff_t *ppos)
+{
+	unsigned long index = *ppos;
+	struct hwspinlock *hwlock;
+
+	rcu_read_lock();
+	hwlock = xa_find(&hwspinlocks, &index, ULONG_MAX, XA_PRESENT);
+	*ppos = index;
+
+	return hwlock;
+}
+
+static void *hwspin_lock_seq_next(struct seq_file *s, void *v, loff_t *ppos)
+{
+	/* Increase ppos here to avoid endless loops. Don't use xa_find_after() */
+	unsigned long index = *ppos + 1;
+	struct hwspinlock *hwlock;
+
+	hwlock = xa_find(&hwspinlocks, &index, ULONG_MAX, XA_PRESENT);
+	*ppos = index;
+
+	return hwlock;
+}
+
+static void hwspin_lock_seq_stop(struct seq_file *s, void *v)
+{
+	rcu_read_unlock();
+}
+
+static int hwspin_lock_seq_show(struct seq_file *s, void *v)
+{
+	struct hwspinlock *hwlock = v;
+	bool unused = xa_get_mark(&hwspinlocks, s->index, HWSPINLOCK_UNUSED);
+
+	seq_printf(s, "%4llu:\t%s\t%s\n", s->index, unused ? "free" : "in use",
+		   dev_name(hwlock->bank->dev));
+	return 0;
+}
+
+static const struct seq_operations hwspinlock_sops = {
+	.start = hwspin_lock_seq_start,
+	.next = hwspin_lock_seq_next,
+	.stop = hwspin_lock_seq_stop,
+	.show = hwspin_lock_seq_show,
+};
+DEFINE_SEQ_ATTRIBUTE(hwspinlock);
+
+/*
+ * subsys_initcall() is used here but controllers may already have been
+ * registered earlier or will be later. The rationale is that debugfs is
+ * accessed only late, i.e. from userspace. So, files created here must make no
+ * assumptions about initcall ordering.
+ */
+static int __init hwspin_lock_init(void)
+{
+	struct dentry *hwspinlock_debugfs = debugfs_create_dir("hwspinlock", NULL);
+
+	debugfs_create_file("hwspinlock_summary", 0444, hwspinlock_debugfs,
+			    NULL, &hwspinlock_fops);
+	return 0;
+}
+subsys_initcall(hwspin_lock_init);
+#endif	/* DEBUG_FS */
+
 MODULE_DESCRIPTION("Hardware spinlock interface");
 MODULE_AUTHOR("Ohad Ben-Cohen <ohad@wizery.com>");
-- 
2.53.0

[PATCH v7 3/3] hwspinlock: add low level debug capabilities

[Wolfram Sang]

While developing the hwspinlock core or a device driver, it can be
really helpful to change states from userspace. Add a debugsfs interface
to request, free, lock, unlock, and test hwspinlocks. Because this is a
potentially dangerous interface, it can only be enabled via source code.
Similar to writeable attributes for clocks where this idea stems from.
The created files take a single id for a single lock. To test all locks
in a system, one could use this oneliner while in the debugfs directory:

for l in $(cut -d: -f1 hwspinlock_summary); do echo $l > test_lock; done

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 drivers/hwspinlock/hwspinlock_core.c | 132 +++++++++++++++++++++++++++
 1 file changed, 132 insertions(+)

diff --git a/drivers/hwspinlock/hwspinlock_core.c b/drivers/hwspinlock/hwspinlock_core.c
index 947b4d4fa153..6fb2670873a2 100644
--- a/drivers/hwspinlock/hwspinlock_core.c
+++ b/drivers/hwspinlock/hwspinlock_core.c
@@ -910,6 +910,120 @@ static const struct seq_operations hwspinlock_sops = {
 };
 DEFINE_SEQ_ATTRIBUTE(hwspinlock);
 
+/*
+ * This can be dangerous, therefore don't provide any real compile time
+ * configuration option for this feature.
+ * People who want to use this will need to modify the source code directly.
+ */
+#undef HWSPINLOCK_LOW_LEVEL_DEBUG
+#ifdef HWSPINLOCK_LOW_LEVEL_DEBUG
+
+static int fops_request_set(void *data, u64 id)
+{
+	if (id > UINT_MAX)
+		return -EINVAL;
+
+	return hwspin_lock_request_specific(id) ? 0 : -ENOENT;
+}
+DEFINE_DEBUGFS_ATTRIBUTE(fops_request, NULL, fops_request_set, "%llu\n");
+
+static int fops_free_set(void *data, u64 id)
+{
+	struct hwspinlock *hwlock;
+
+	if (id > UINT_MAX)
+		return -EINVAL;
+
+	hwlock = xa_load(&hwspinlocks, id);
+
+	return hwlock ? hwspin_lock_free(hwlock) : -ENOENT;
+}
+DEFINE_DEBUGFS_ATTRIBUTE(fops_free, NULL, fops_free_set, "%llu\n");
+
+/*
+ * It is intentionally allowed to lock/free hwspinlocks which have not been
+ * requested before. Like error injection, these inconsistent states can be
+ * very useful for debugging. Be aware that you might need another properly
+ * requested lock on a provider to ensure that its clocks etc are enabled!
+ */
+static int fops_trylock_raw_set(void *data, u64 id)
+{
+	struct hwspinlock *hwlock;
+
+	if (id > UINT_MAX)
+		return -EINVAL;
+
+	hwlock = xa_load(&hwspinlocks, id);
+
+	return hwlock ? hwspin_trylock_raw(hwlock) : -ENOENT;
+}
+DEFINE_DEBUGFS_ATTRIBUTE(fops_trylock_raw, NULL, fops_trylock_raw_set, "%llu\n");
+
+static int fops_unlock_raw_set(void *data, u64 id)
+{
+	struct hwspinlock *hwlock;
+
+	if (id > UINT_MAX)
+		return -EINVAL;
+
+	hwlock = xa_load(&hwspinlocks, id);
+	if (hwlock)
+		hwspin_unlock_raw(hwlock);
+
+	return 0;
+}
+DEFINE_DEBUGFS_ATTRIBUTE(fops_unlock_raw, NULL, fops_unlock_raw_set, "%llu\n");
+
+static int fops_test_set(void *data, u64 id)
+{
+	struct hwspinlock *hwlock;
+	int ret;
+
+	if (id > UINT_MAX)
+		return -EINVAL;
+
+	hwlock = hwspin_lock_request_specific(id);
+	if (!hwlock)
+		return -ENOENT;
+
+	/* Try twice to see if unlocking was also successful */
+	for (int i = 0; i < 2; i++) {
+		ret = hwspin_trylock_raw(hwlock);
+		if (ret) {
+			hwspin_lock_free(hwlock);
+			return ret;
+		}
+
+		ret = hwspin_trylock_raw(hwlock);
+		if (ret != -EBUSY) {
+			if (ret == 0)
+				hwspin_unlock_raw(hwlock);
+			hwspin_lock_free(hwlock);
+			return -EACCES;
+		}
+
+		hwspin_unlock_raw(hwlock);
+	}
+
+	ret = hwspin_lock_free(hwlock);
+	if (ret)
+		return ret;
+
+	pr_info("Successfully tested lock %llu\n", id);
+	return 0;
+}
+DEFINE_DEBUGFS_ATTRIBUTE(fops_test, NULL, fops_test_set, "%llu\n");
+
+static void hwspin_lock_low_level_debug_init(struct dentry *rootdir)
+{
+	debugfs_create_file("request", 0200, rootdir, NULL, &fops_request);
+	debugfs_create_file("free", 0200, rootdir, NULL, &fops_free);
+	debugfs_create_file("lock", 0200, rootdir, NULL, &fops_trylock_raw);
+	debugfs_create_file("unlock", 0200, rootdir, NULL, &fops_unlock_raw);
+	debugfs_create_file("test_lock", 0200, rootdir, NULL, &fops_test);
+}
+#endif
+
 /*
  * subsys_initcall() is used here but controllers may already have been
  * registered earlier or will be later. The rationale is that debugfs is
@@ -922,6 +1036,24 @@ static int __init hwspin_lock_init(void)
 
 	debugfs_create_file("hwspinlock_summary", 0444, hwspinlock_debugfs,
 			    NULL, &hwspinlock_fops);
+
+#ifdef HWSPINLOCK_LOW_LEVEL_DEBUG
+	pr_warn("**********************************************************\n");
+	pr_warn("**   NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE   **\n");
+	pr_warn("**                                                      **\n");
+	pr_warn("**    Low Level Debug for hwspinlocks is compiled in!   **\n");
+	pr_warn("**                                                      **\n");
+	pr_warn("** This means safety, security, stability can be easily **\n");
+	pr_warn("** compromised from userspace!                          **\n");
+	pr_warn("**                                                      **\n");
+	pr_warn("** If you see this message and you are not debugging    **\n");
+	pr_warn("** the kernel, report this immediately to your vendor!  **\n");
+	pr_warn("**                                                      **\n");
+	pr_warn("**   NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE   **\n");
+	pr_warn("**********************************************************\n");
+	hwspin_lock_low_level_debug_init(hwspinlock_debugfs);
+#endif
+
 	return 0;
 }
 subsys_initcall(hwspin_lock_init);
-- 
2.53.0

Re: [PATCH v7 2/3] hwspinlock: add list of locks to debugfs

[Geert Uytterhoeven]

Hi Wolfram,

On Sat, 12 Sept 2026 at 21:20, Wolfram Sang
<wsa+renesas@sang-engineering.com> wrote:
> To help debugging, offer a list of registered hwspinlocks in debugfs. It
> will also print the responsible hwspinlock_device and if the spinlock is
> currently used. It does not show if the lock is taken because of
> side-effects. Often, reading a lock means actually taking it.
>
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

Thanks for your patch!

> --- a/drivers/hwspinlock/hwspinlock_core.c
> +++ b/drivers/hwspinlock/hwspinlock_core.c
> @@ -860,5 +862,70 @@ struct hwspinlock *devm_hwspin_lock_request_specific(struct device *dev,
>  }
>  EXPORT_SYMBOL_GPL(devm_hwspin_lock_request_specific);
>
> +#ifdef CONFIG_DEBUG_FS
> +static void *hwspin_lock_seq_start(struct seq_file *s, loff_t *ppos)
> +{
> +       unsigned long index = *ppos;
> +       struct hwspinlock *hwlock;
> +
> +       rcu_read_lock();
> +       hwlock = xa_find(&hwspinlocks, &index, ULONG_MAX, XA_PRESENT);
> +       *ppos = index;
> +
> +       return hwlock;
> +}
> +
> +static void *hwspin_lock_seq_next(struct seq_file *s, void *v, loff_t *ppos)
> +{
> +       /* Increase ppos here to avoid endless loops. Don't use xa_find_after() */

Do you mean xa_find_after() may loop forever?

> +       unsigned long index = *ppos + 1;
> +       struct hwspinlock *hwlock;
> +
> +       hwlock = xa_find(&hwspinlocks, &index, ULONG_MAX, XA_PRESENT);
> +       *ppos = index;
> +
> +       return hwlock;
> +}

> +static int hwspin_lock_seq_show(struct seq_file *s, void *v)
> +{
> +       struct hwspinlock *hwlock = v;
> +       bool unused = xa_get_mark(&hwspinlocks, s->index, HWSPINLOCK_UNUSED);
> +
> +       seq_printf(s, "%4llu:\t%s\t%s\n", s->index, unused ? "free" : "in use",

For easier parsing the output, you may want to use a string without
spaces, e.g. "busy", "active", ...

> +                  dev_name(hwlock->bank->dev));
> +       return 0;
> +}

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v7 3/3] hwspinlock: add low level debug capabilities

[Geert Uytterhoeven]

Hi Wolfram,

Thanks for your patch!

On Sat, 12 Sept 2026 at 21:19, Wolfram Sang
<wsa+renesas@sang-engineering.com> wrote:
> While developing the hwspinlock core or a device driver, it can be
> really helpful to change states from userspace. Add a debugsfs interface

debugfs

> to request, free, lock, unlock, and test hwspinlocks. Because this is a
> potentially dangerous interface, it can only be enabled via source code.

... code,

> Similar to writeable attributes for clocks where this idea stems from.

similar ...

> The created files take a single id for a single lock. To test all locks
> in a system, one could use this oneliner while in the debugfs directory:
>
> for l in $(cut -d: -f1 hwspinlock_summary); do echo $l > test_lock; done
>
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

> --- a/drivers/hwspinlock/hwspinlock_core.c
> +++ b/drivers/hwspinlock/hwspinlock_core.c

> +static void hwspin_lock_low_level_debug_init(struct dentry *rootdir)
> +{
> +       debugfs_create_file("request", 0200, rootdir, NULL, &fops_request);
> +       debugfs_create_file("free", 0200, rootdir, NULL, &fops_free);
> +       debugfs_create_file("lock", 0200, rootdir, NULL, &fops_trylock_raw);
> +       debugfs_create_file("unlock", 0200, rootdir, NULL, &fops_unlock_raw);
> +       debugfs_create_file("test_lock", 0200, rootdir, NULL, &fops_test);

Perhaps just "test"?

> +}
> +#endif
> +
>  /*
>   * subsys_initcall() is used here but controllers may already have been
>   * registered earlier or will be later. The rationale is that debugfs is

The rest LGTM, so
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v7 2/3] hwspinlock: add list of locks to debugfs

[Geert Uytterhoeven]

On Wed, 16 Sept 2026 at 16:01, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> On Sat, 12 Sept 2026 at 21:20, Wolfram Sang
> <wsa+renesas@sang-engineering.com> wrote:
> > To help debugging, offer a list of registered hwspinlocks in debugfs. It
> > will also print the responsible hwspinlock_device and if the spinlock is
> > currently used. It does not show if the lock is taken because of
> > side-effects. Often, reading a lock means actually taking it.
> >
> > Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

I have been using this since quite a while on R-Car X5H Ironhide:
Tested-by: Geert Uytterhoeven <geert+renesas@glider.be>

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v7 2/3] hwspinlock: add list of locks to debugfs

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 180 bytes --]

> 
> I have been using this since quite a while on R-Car X5H Ironhide:
> Tested-by: Geert Uytterhoeven <geert+renesas@glider.be>

So, this applies for patch 1 as well then?


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v7 2/3] hwspinlock: add list of locks to debugfs

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 883 bytes --]

> > +static void *hwspin_lock_seq_next(struct seq_file *s, void *v, loff_t *ppos)
> > +{
> > +       /* Increase ppos here to avoid endless loops. Don't use xa_find_after() */
> 
> Do you mean xa_find_after() may loop forever?

Ehrm, it's been a while, I need to look this up. Which means the comment
is not explanationary enough :/

> > +static int hwspin_lock_seq_show(struct seq_file *s, void *v)
> > +{
> > +       struct hwspinlock *hwlock = v;
> > +       bool unused = xa_get_mark(&hwspinlocks, s->index, HWSPINLOCK_UNUSED);
> > +
> > +       seq_printf(s, "%4llu:\t%s\t%s\n", s->index, unused ? "free" : "in use",
> 
> For easier parsing the output, you may want to use a string without
> spaces, e.g. "busy", "active", ...

I get the idea, but haven't found a nice word yet. "Busy" could be
interpreted as "lock taken". I'll think some more about it.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v7 3/3] hwspinlock: add low level debug capabilities

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1247 bytes --]


> > While developing the hwspinlock core or a device driver, it can be
> > really helpful to change states from userspace. Add a debugsfs interface
> 
> debugfs

Yes.

> 
> > to request, free, lock, unlock, and test hwspinlocks. Because this is a
> > potentially dangerous interface, it can only be enabled via source code.
> 
> ... code,
> 
> > Similar to writeable attributes for clocks where this idea stems from.
> 
> similar ...

You mean I should merge the two sentences into one?

> > +static void hwspin_lock_low_level_debug_init(struct dentry *rootdir)
> > +{
> > +       debugfs_create_file("request", 0200, rootdir, NULL, &fops_request);
> > +       debugfs_create_file("free", 0200, rootdir, NULL, &fops_free);
> > +       debugfs_create_file("lock", 0200, rootdir, NULL, &fops_trylock_raw);
> > +       debugfs_create_file("unlock", 0200, rootdir, NULL, &fops_unlock_raw);
> > +       debugfs_create_file("test_lock", 0200, rootdir, NULL, &fops_test);
> 
> Perhaps just "test"?

Hmm, too generic for my taste. In fact, I think I should maybe rename it
to "test_single_lock" to make it more precise...

> The rest LGTM, so
> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

Thank you!


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v7 2/3] hwspinlock: add list of locks to debugfs

[Geert Uytterhoeven]

Hi Wolfram,

On Wed, 16 Sept 2026 at 23:38, Wolfram Sang
<wsa+renesas@sang-engineering.com> wrote:
> > I have been using this since quite a while on R-Car X5H Ironhide:
> > Tested-by: Geert Uytterhoeven <geert+renesas@glider.be>
>
> So, this applies for patch 1 as well then?

I don't think just booting on a system with a single spinlock
counts as a full Tested-by for such a complex conversion.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v7 2/3] hwspinlock: add list of locks to debugfs

[Geert Uytterhoeven]

Hi Wolfram,

On Wed, 16 Sept 2026 at 23:40, Wolfram Sang
<wsa+renesas@sang-engineering.com> wrote:
> > > +static void *hwspin_lock_seq_next(struct seq_file *s, void *v, loff_t *ppos)
> > > +{
> > > +       /* Increase ppos here to avoid endless loops. Don't use xa_find_after() */
> >
> > Do you mean xa_find_after() may loop forever?
>
> Ehrm, it's been a while, I need to look this up. Which means the comment
> is not explanationary enough :/

Yeah, I'd expect xa_find_after() to do exactly what you want...

> > > +static int hwspin_lock_seq_show(struct seq_file *s, void *v)
> > > +{
> > > +       struct hwspinlock *hwlock = v;
> > > +       bool unused = xa_get_mark(&hwspinlocks, s->index, HWSPINLOCK_UNUSED);
> > > +
> > > +       seq_printf(s, "%4llu:\t%s\t%s\n", s->index, unused ? "free" : "in use",
> >
> > For easier parsing the output, you may want to use a string without
> > spaces, e.g. "busy", "active", ...
>
> I get the idea, but haven't found a nice word yet. "Busy" could be
> interpreted as "lock taken". I'll think some more about it.

"used", "bound", "unavailable", "occupied", "reserved", "in_use", "inuse", ...

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v7 3/3] hwspinlock: add low level debug capabilities

[Geert Uytterhoeven]

Hi Wolfram,

On Wed, 16 Sept 2026 at 23:44, Wolfram Sang
<wsa+renesas@sang-engineering.com> wrote:
> > > to request, free, lock, unlock, and test hwspinlocks. Because this is a
> > > potentially dangerous interface, it can only be enabled via source code.
> >
> > ... code,
> >
> > > Similar to writeable attributes for clocks where this idea stems from.
> >
> > similar ...
>
> You mean I should merge the two sentences into one?

Yes, as the second "sentence" does not have a verb.

> > > +static void hwspin_lock_low_level_debug_init(struct dentry *rootdir)
> > > +{
> > > +       debugfs_create_file("request", 0200, rootdir, NULL, &fops_request);
> > > +       debugfs_create_file("free", 0200, rootdir, NULL, &fops_free);
> > > +       debugfs_create_file("lock", 0200, rootdir, NULL, &fops_trylock_raw);
> > > +       debugfs_create_file("unlock", 0200, rootdir, NULL, &fops_unlock_raw);
> > > +       debugfs_create_file("test_lock", 0200, rootdir, NULL, &fops_test);
> >
> > Perhaps just "test"?
>
> Hmm, too generic for my taste. In fact, I think I should maybe rename it
> to "test_single_lock" to make it more precise...

"test_single"? All others don't have "lock" in their name.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v7 2/3] hwspinlock: add list of locks to debugfs

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 249 bytes --]


> I don't think just booting on a system with a single spinlock
> counts as a full Tested-by for such a complex conversion.

Your choice. There would be, however, enough chances to break the
conversion so that even a single lock would not work ;)


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v7 3/3] hwspinlock: add low level debug capabilities

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 877 bytes --]


> > You mean I should merge the two sentences into one?
> 
> Yes, as the second "sentence" does not have a verb.

Pfff, okay.

> > > > +       debugfs_create_file("request", 0200, rootdir, NULL, &fops_request);
> > > > +       debugfs_create_file("free", 0200, rootdir, NULL, &fops_free);
> > > > +       debugfs_create_file("lock", 0200, rootdir, NULL, &fops_trylock_raw);
> > > > +       debugfs_create_file("unlock", 0200, rootdir, NULL, &fops_unlock_raw);
> > > > +       debugfs_create_file("test_lock", 0200, rootdir, NULL, &fops_test);
> > >
> > > Perhaps just "test"?
> >
> > Hmm, too generic for my taste. In fact, I think I should maybe rename it
> > to "test_single_lock" to make it more precise...
> 
> "test_single"? All others don't have "lock" in their name.

Hmm, I'd rather add 'lock' to request/free. Only lock_lock sounds
strange :)


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v7 2/3] hwspinlock: add list of locks to debugfs

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 721 bytes --]


> > > Do you mean xa_find_after() may loop forever?
> >
> > Ehrm, it's been a while, I need to look this up. Which means the comment
> > is not explanationary enough :/
> 
> Yeah, I'd expect xa_find_after() to do exactly what you want...

So, the reason is that xa_find_after() only upadtes ppos if something is
found. If nothing is found, it will in deed loop forever staying on the
same ppos. Will update the comment.

> > I get the idea, but haven't found a nice word yet. "Busy" could be
> > interpreted as "lock taken". I'll think some more about it.
> 
> "used", "bound", "unavailable", "occupied", "reserved", "in_use", "inuse", ...

After sleeping over it, I will simply go with "free/used".


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]