Re: [PATCH v2 0/4] hwspinlock: add summary in debugfs

[Matthew Wilcox]

On Mon, Jun 29, 2026 at 10:57:14AM +0200, Wolfram Sang wrote:
> Okay, seems to work so far. Thank you again! Will merge your patch into
> my series with your credits. Now I just need to wrap XArray into struct
> seq_operations. Seems no one has needed that in the kernel so far.

Huh.  I thought I had done that at some point.  But it was pre-pandemic
that I was looking at it so maybe I either never did it or I never sent
it out.

> > @@ -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>
> 
> According to some quick grepping, there are 102 users of XArray
> including this header and 423 users which are not including this header.
> Do you think this is a useful improvement to add the header directly
> (per subsystem to keep the number of patches limited)?

Our header files are a mess.  Trying to fix tham and keep them fixed
is a Sisyphean exercise.  Unlike our Greek hero, I have stopped trying.

> > -	void **slot;
> 
> Great, this obsoletes a fix concerning RCU annotations I have sent
> previously!

Yes, this was one of the things I hated about the radix tree API.
When designing the XArray API, I wrapped the rcu annotations safely
inside the XA_STATE() so users didn't need to care.  I'm glad you like it.

> > @@ -389,15 +375,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))
> > -			continue;
> > -		if (radix_tree_deref_retry(hwlock)) {
> > -			slot = radix_tree_iter_retry(&iter);
> > +	xas_for_each(&xas, hwlock, ULONG_MAX) {
> > +		if (xas_retry(&xas, hwlock))
> 
> So, the unlikely(!hwlock) case cannot happen with XArray?

That's right.  The iterator uses hwlock == NULL as the iteration
termination condition.  It skips over the NULL slots for you and only
returns the entries in the array which are present.  There are other
ways to iterate over each slot in the array (but we have very few users
of them and they've never been worth wrapping up into an iterator).

> > -	ret = radix_tree_tag_get(&hwspinlock_tree, id, HWSPINLOCK_UNUSED);
> > +	ret = xas_get_mark(&xas, HWSPINLOCK_UNUSED);
> 
> xas_get_mark() returns bool, so I will update the code to match that.
> Makes it more readable, too, IMO.

Thanks!

> The rest I could understand, I think. Looks much leaner, in deed. Will
> keep you in the loop once my next iteration is ready.

Fantastic!  I'll take the liberty of replying to your next email here
too ...

> In hwspin_lock_request_specific(), the spinlock is taken, then:
> 
> hwspin_lock_request_specific()
> 
> -> __hwspin_lock_request()
>   -> pm_runtime_get_sync()
>     -> __pm_runtime_resume()
> 
> This starts with:
> 
> 	might_sleep_if(!(rpmflags & RPM_ASYNC) && !dev->power.irq_safe &&
> 			dev->power.runtime_status != RPM_ACTIVE);
> 
> Isn't this a problem?

Ah, er, maybe?  I seem to have overlooked this.  I mean, if that warning
doesn't trigger, than it's not a problem, right?  Assuming you have the
applicable debugging config turned on.

Assuming that we don't want to call pm_runtime_get_sync() under the
spinlock (and maybe for cleanliness we shouldn't anyway?), I would clear
the HWSPINLOCK_UNUSED mark in hwspin_lock_request_specific(), drop the
lock, then if __hwspin_lock_request() fails, set the UNUSED mark again.