[PROPOSAL] hwspinlock: remove 'unregistering hwspinlock devices' from the core

[PROPOSAL] hwspinlock: remove 'unregistering hwspinlock devices' from the core

[Wolfram Sang]

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

Hi all,

TLDR: I think that unregistering hwspinlock devices is broken currently. A
correct solution is complex. My intermediate proposal is to simply remove
"unregistering" support until someone actually needs it.

Longer read (I still try to keep it short):

Problem
=======

I recently worked on hwspinlock device allocation being done in the core,
rather than in drivers. This lead to a lengthy Sashiko review[1]. The comments
about leaking the new allocation are correct. But the underlying problems were
in the hwspinlock core even before my changes. See the current unregister
function:

566 int hwspin_lock_unregister(struct hwspinlock_device *bank)
567 {
568         struct hwspinlock *hwlock, *tmp;
569         int i;
570 
571         for (i = 0; i < bank->num_locks; i++) {
572                 hwlock = &bank->lock[i];
573 
574                 tmp = hwspin_lock_unregister_single(bank->base_id + i);
575                 if (!tmp)
576                         return -EBUSY;
577 
578                 /* self-sanity check that should never fail */
579                 WARN_ON(tmp != hwlock);
580         }
581 
582         return 0;
583 }

Let's say lock 5 is in use. Then, locks 0-4 will get unregistered. With lock 5,
-EBUSY will be returned. Two problems now:

1) The only caller of this function (devm_hwspin_lock_unregister) ignores the
   errno anyhow. All locks after 5 are leaked.

2) Even if the errno was considered, a later try to unregister again will fail
   immediately because lock 0 is no longer present.

Correct solution
================

As I see it, the correct solution is to decouple the lifetime of a single lock
from the lifetime of a hwspinlock device. It reminds me of a similar problem
the I2C subsystem has. There, we also still need to decouple the lifetime of an
I2C adapter from the lifetime of its users. Given the experience from I2C, this
is a complex and time-consuming job. I, at least, don't have this bandwidth.

Simple intermediate solution?
=============================

Unlike I2C, I wonder if the hwspinlock subsystem really needs to unregister its
devices. All current users are IP cores within the SoC, not hot-pluggable. This
is unlikely to change in the near future. My proposal is now to see hwspinlock
devices more like critical clocks which can be probed, but never removed. That
would mean for now, the unregister parts in the hwspinlock core can just be
discarded.

Benefits
========

- no false promises made (unregistering doesn't really work)
- simplifies the subsystem (less maintainer burden, especially given the
  rising amount of AI generated bug reports)
- simplifies further refactoring like device allocation in core
- no oneway. Proper solution can be added any time if needed
- I have bandwidth for this intermediate approach

Looking forward to comments. I hope I am still seeing the forest for the trees.

   Wolfram

[1] https://sashiko.dev/#/patchset/20260319105947.6237-1-wsa%2Brenesas%40sang-engineering.com

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