[PATCH 0/2] hwspinlock: add summary in debugfs

[PATCH 0/2] hwspinlock: add summary in debugfs

[Wolfram Sang]

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. This series adds support for that. One
preparational patch, and one patch with the desired functionality.

A branch for testing is here:

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

It has been tested on a SparrowHawk board (R-Car V4H) and an Ironhide
board (R-Car X5H).

Looking forward to comments.

Wolfram Sang (2):
  hwspinlock: reverse logic for used channels
  hwspinlock: add summary in debugfs

 drivers/hwspinlock/hwspinlock_core.c | 110 +++++++++++++++++++++++----
 1 file changed, 95 insertions(+), 15 deletions(-)

-- 
2.51.0

[PATCH 1/2] hwspinlock: reverse logic for used channels

[Wolfram Sang]

Having a flag set for an unused channel is irritating. It became
especially confusing while developing debugfs support for this
subsystem. Inverse the logic which makes the current and the future code
easier to follow.

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

diff --git a/drivers/hwspinlock/hwspinlock_core.c b/drivers/hwspinlock/hwspinlock_core.c
index cc8e952a6772..7aa597a28eec 100644
--- a/drivers/hwspinlock/hwspinlock_core.c
+++ b/drivers/hwspinlock/hwspinlock_core.c
@@ -28,7 +28,7 @@
 #define HWSPINLOCK_RETRY_DELAY_US	100
 
 /* radix tree tags */
-#define HWSPINLOCK_UNUSED	(0) /* tags an hwspinlock as unused */
+#define HWSPINLOCK_USED	(0)
 
 /*
  * A radix tree is used to maintain the available hwspinlock instances.
@@ -42,8 +42,8 @@
  * 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
+ * framework uses to mark used hwspinlock instances (see the
+ * HWSPINLOCK_USED 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.
  */
@@ -465,7 +465,7 @@ static int hwspin_lock_register_single(struct hwspinlock *hwlock, int id)
 	}
 
 	/* mark this hwspinlock as available */
-	tmp = radix_tree_tag_set(&hwspinlock_tree, id, HWSPINLOCK_UNUSED);
+	tmp = radix_tree_tag_clear(&hwspinlock_tree, id, HWSPINLOCK_USED);
 
 	/* self-sanity check which should never fail */
 	WARN_ON(tmp != hwlock);
@@ -482,9 +482,9 @@ static struct hwspinlock *hwspin_lock_unregister_single(unsigned int id)
 
 	mutex_lock(&hwspinlock_tree_lock);
 
-	/* 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 */
+	ret = radix_tree_tag_get(&hwspinlock_tree, id, HWSPINLOCK_USED);
+	if (ret) {
 		pr_err("hwspinlock %d still in use (or not present)\n", id);
 		goto out;
 	}
@@ -700,8 +700,8 @@ static int __hwspin_lock_request(struct hwspinlock *hwlock)
 	ret = 0;
 
 	/* mark hwspinlock as used, should not fail */
-	tmp = radix_tree_tag_clear(&hwspinlock_tree, hwlock_to_id(hwlock),
-							HWSPINLOCK_UNUSED);
+	tmp = radix_tree_tag_set(&hwspinlock_tree, hwlock_to_id(hwlock),
+				 HWSPINLOCK_USED);
 
 	/* self-sanity check that should never fail */
 	WARN_ON(tmp != hwlock);
@@ -740,8 +740,8 @@ struct hwspinlock *hwspin_lock_request_specific(unsigned int id)
 	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) {
+	ret = radix_tree_tag_get(&hwspinlock_tree, id, HWSPINLOCK_USED);
+	if (ret) {
 		pr_warn("hwspinlock %u is already in use\n", id);
 		hwlock = NULL;
 		goto out;
@@ -786,8 +786,8 @@ int hwspin_lock_free(struct hwspinlock *hwlock)
 
 	/* make sure the hwspinlock is used */
 	ret = radix_tree_tag_get(&hwspinlock_tree, hwlock_to_id(hwlock),
-							HWSPINLOCK_UNUSED);
-	if (ret == 1) {
+				 HWSPINLOCK_USED);
+	if (!ret) {
 		dev_err(dev, "%s: hwlock is already free\n", __func__);
 		dump_stack();
 		ret = -EINVAL;
@@ -798,8 +798,8 @@ 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);
+	tmp = radix_tree_tag_clear(&hwspinlock_tree, hwlock_to_id(hwlock),
+				   HWSPINLOCK_USED);
 
 	/* sanity check (this shouldn't happen) */
 	WARN_ON(tmp != hwlock);
-- 
2.51.0

Re: [PATCH 1/2] hwspinlock: reverse logic for used channels

[Wolfram Sang]

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


> > @@ -42,8 +42,8 @@
> >   * 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
> > + * framework uses to mark used hwspinlock instances (see the
> > + * HWSPINLOCK_USED 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.
> >   */
> 
> [Severity: Low]
> Is the documentation internally contradictory here? Because the tag is now
> exclusively applied to USED locks, it seems impossible to query for an
> UNUSED lock with a single tag-based API call.

Can reword to "getting the hwspinlock state". The core doesn't offer to
look for just some unused hwlock. Only specific locks are looked for and
their state is queried.

> [Severity: High]
> This isn't a bug introduced by this patch, but does

Ignored.

> > @@ -482,9 +482,9 @@ static struct hwspinlock *hwspin_lock_unregister_single(unsigned int id)
> >  
> >  	mutex_lock(&hwspinlock_tree_lock);
> >  
> > -	/* 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 */
> > +	ret = radix_tree_tag_get(&hwspinlock_tree, id, HWSPINLOCK_USED);
> > +	if (ret) {
> >  		pr_err("hwspinlock %d still in use (or not present)\n", id);
> >  		goto out;
> >  	}
> 
> [Severity: Low]
> Is the "(or not present)" portion of this error message unreachable?

Can reword.

> > @@ -798,8 +798,8 @@ 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);
> > +	tmp = radix_tree_tag_clear(&hwspinlock_tree, hwlock_to_id(hwlock),
> > +				   HWSPINLOCK_USED);
> >  
> >  	/* sanity check (this shouldn't happen) */
> >  	WARN_ON(tmp != hwlock);
> 
> [Severity: Medium]
> Does hwspin_lock_free() now incorrectly return 1 instead of 0 on success?

The quoted code is the wrong block, but the issue is correct. Will fix.

> [Severity: Critical]
> This is a pre-existing issue, but does accessing hwlock->bank->num_locks

Ignored.

> [Severity: High]
> This is a pre-existing issue, but does hwspin_lock_unregister() fail to
> rollback on an error?

...

> [Severity: Critical]
> This is a pre-existing issue, but does devm_hwspin_lock_unreg() ignore the
> -EBUSY return value from hwspin_lock_unregister()?

Unregistering has _many_ problems. This will be addressed in a seperate
patch series.

   Wolfram


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

Re: [PATCH 1/2] hwspinlock: reverse logic for used channels

[Wolfram Sang]

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


On Fri, Jun 19, 2026 at 07:06:34AM +0200, Wolfram Sang wrote:
> 
> > > @@ -42,8 +42,8 @@
> > >   * 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
> > > + * framework uses to mark used hwspinlock instances (see the
> > > + * HWSPINLOCK_USED 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.
> > >   */
> > 
> > [Severity: Low]
> > Is the documentation internally contradictory here? Because the tag is now
> > exclusively applied to USED locks, it seems impossible to query for an
> > UNUSED lock with a single tag-based API call.
> 
> Can reword to "getting the hwspinlock state". The core doesn't offer to
> look for just some unused hwlock. Only specific locks are looked for and
> their state is queried.

On second thought, this still shows the original intention of using an
UNUSED flag instead of a USED flag. I will probably drop this patch in
v2. It is less intrusive and probably more future proof.


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

[PATCH 2/2] hwspinlock: add summary in debugfs

[Wolfram Sang]

Add a subsystem entry in debugfs and place a summary file there. It
informs about registered locks, if they are in use, and to which device
they belong. The state of the lock itself is usually not accessible
without modifying the state, so there is no plan to add support for
that.

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

diff --git a/drivers/hwspinlock/hwspinlock_core.c b/drivers/hwspinlock/hwspinlock_core.c
index 7aa597a28eec..dbfedc47abc5 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"
 
@@ -888,5 +890,83 @@ struct hwspinlock *devm_hwspin_lock_request_specific(struct device *dev,
 }
 EXPORT_SYMBOL_GPL(devm_hwspin_lock_request_specific);
 
+#ifdef CONFIG_DEBUG_FS
+struct hwspinlock_seq_iterator {
+	void __rcu **slot;
+	struct radix_tree_iter iter;
+};
+
+static void *hwspinlock_seq_start(struct seq_file *s, loff_t *ppos)
+{
+	struct hwspinlock_seq_iterator *hwsp_seq_iter = kzalloc(sizeof(*hwsp_seq_iter), GFP_KERNEL);
+
+	seq_puts(s, "id\tstatus\tdevice\n");
+
+	if (!hwsp_seq_iter)
+		return NULL;
+
+	mutex_lock(&hwspinlock_tree_lock);
+	radix_tree_iter_init(&hwsp_seq_iter->iter, *ppos);
+	hwsp_seq_iter->slot = radix_tree_next_chunk(&hwspinlock_tree, &hwsp_seq_iter->iter, 0);
+
+	return hwsp_seq_iter->slot ? hwsp_seq_iter : NULL;
+}
+
+static void *hwspinlock_seq_next(struct seq_file *s, void *v, loff_t *ppos)
+{
+	struct hwspinlock_seq_iterator *hwsp_seq_iter = v;
+
+	++*ppos;
+
+	hwsp_seq_iter->slot = radix_tree_next_slot(hwsp_seq_iter->slot, &hwsp_seq_iter->iter, 0);
+	if (!hwsp_seq_iter->slot)
+		hwsp_seq_iter->slot = radix_tree_next_chunk(&hwspinlock_tree, &hwsp_seq_iter->iter, 0);
+
+	return hwsp_seq_iter->slot ? hwsp_seq_iter : NULL;
+}
+
+static void hwspinlock_seq_stop(struct seq_file *s, void *v)
+{
+	kfree(v);
+	mutex_unlock(&hwspinlock_tree_lock);
+}
+
+static int hwspinlock_seq_show(struct seq_file *s, void *v)
+{
+	struct hwspinlock_seq_iterator *hwsp_seq_iter = v;
+	unsigned long id = hwsp_seq_iter->iter.index;
+	struct hwspinlock *hwlock;
+	int used;
+
+	used = radix_tree_tag_get(&hwspinlock_tree, id, HWSPINLOCK_USED);
+	hwlock = radix_tree_deref_slot(hwsp_seq_iter->slot);
+	seq_printf(s, "%4lu:\t%s\t%s\n", id, used ? "in use" : "free", dev_name(hwlock->bank->dev));
+	return 0;
+}
+
+static const struct seq_operations hwspinlock_sops = {
+	.start = hwspinlock_seq_start,
+	.next = hwspinlock_seq_next,
+	.stop = hwspinlock_seq_stop,
+	.show = hwspinlock_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 hwspinlock_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(hwspinlock_init);
+#endif	/* DEBUG_FS */
+
 MODULE_DESCRIPTION("Hardware spinlock interface");
 MODULE_AUTHOR("Ohad Ben-Cohen <ohad@wizery.com>");
-- 
2.51.0

Re: [PATCH 2/2] hwspinlock: add summary in debugfs

[Wolfram Sang]

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

> > +static void *hwspinlock_seq_start(struct seq_file *s, loff_t *ppos)
> > +{
> > +	struct hwspinlock_seq_iterator *hwsp_seq_iter = kzalloc(sizeof(*hwsp_seq_iter), GFP_KERNEL);
> > +
> > +	seq_puts(s, "id\tstatus\tdevice\n");
> 
> [Severity: Medium]
> Will this unconditionally print the table header on every chunked read?
> 
> Because the seq_file framework calls start() at the beginning of every read()
> syscall or when reallocating an internal buffer, this header might be
> redundantly injected into the middle of the output.
> 
> Should it be printed only when *ppos == 0, or handled in show() using
> SEQ_START_TOKEN?

Yes, I will do the latter.

> > +
> > +	if (!hwsp_seq_iter)
> > +		return NULL;
> 
> [Severity: Medium]
> Should this return ERR_PTR(-ENOMEM) instead of NULL?

Yes.

> [Severity: High]
> Can this early return cause an unbalanced mutex unlock?

Yes.

> > +static void *hwspinlock_seq_next(struct seq_file *s, void *v, loff_t *ppos)
> > +{
> > +	struct hwspinlock_seq_iterator *hwsp_seq_iter = v;
> > +
> > +	++*ppos;
> 
> [Severity: High]
> Does sequentially incrementing *ppos cause infinite loops or duplicate entries
> on chunked reads?
> 
> In hwspinlock_seq_start(), *ppos is passed to radix_tree_iter_init() to
> search the sparse radix tree. However, *ppos is incremented sequentially
> here.
> 
> Because registered hwspinlock IDs (the radix tree keys) are often sparse,
> *ppos quickly desynchronizes from the actual keys. If a read() operation
> resumes, seq_start() might search the radix tree starting from a sequential
> *ppos value much smaller than the actual next key, re-printing already output
> keys.

Probably true, will test.

> 
> > +
> > +	hwsp_seq_iter->slot = radix_tree_next_slot(hwsp_seq_iter->slot, &hwsp_seq_iter->iter, 0);
> > +	if (!hwsp_seq_iter->slot)
> > +		hwsp_seq_iter->slot = radix_tree_next_chunk(&hwspinlock_tree, &hwsp_seq_iter->iter, 0);
> > +
> > +	return hwsp_seq_iter->slot ? hwsp_seq_iter : NULL;
> 
> [Severity: High]
> Does returning NULL here cause a memory leak of the sequence iterator?
> 
> When iteration reaches the end of the radix tree, hwspinlock_seq_next()
> returns NULL to signal EOF.
> 
> The seq_file framework then updates its internal iterator pointer to NULL and
> calls hwspinlock_seq_stop() with v=NULL. Since stop() expects to free the
> iterator, calling kfree(NULL) means the originally allocated iterator is lost
> and never freed.

Uh, yes, overlooked that.

> > +}
> > +
> > +static void hwspinlock_seq_stop(struct seq_file *s, void *v)
> > +{
> > +	kfree(v);
> > +	mutex_unlock(&hwspinlock_tree_lock);
> > +}
> > +
> > +static int hwspinlock_seq_show(struct seq_file *s, void *v)
> > +{
> > +	struct hwspinlock_seq_iterator *hwsp_seq_iter = v;
> > +	unsigned long id = hwsp_seq_iter->iter.index;
> > +	struct hwspinlock *hwlock;
> > +	int used;
> > +
> > +	used = radix_tree_tag_get(&hwspinlock_tree, id, HWSPINLOCK_USED);
> > +	hwlock = radix_tree_deref_slot(hwsp_seq_iter->slot);
> 
> [Severity: High]
> Does calling radix_tree_deref_slot() without an active RCU read lock trigger
> a lockdep warning?

True. For such cases, radix_tree_deref_slot_protected() exists. But it
only targets spinlocks not mutexes. Will think about it.


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