[PATCH RFC] regmap: maple: Provide lockdep (sub)class for maple tree's internal lock

[PATCH RFC] regmap: maple: Provide lockdep (sub)class for maple tree's internal lock

[Cristian Ciocaltea]

In some cases when using the maple tree register cache, the lockdep
validator might complain about invalid deadlocks:

[7.131886]  Possible interrupt unsafe locking scenario:

[7.131890]        CPU0                    CPU1
[7.131893]        ----                    ----
[7.131896]   lock(&mt->ma_lock);
[7.131904]                                local_irq_disable();
[7.131907]                                lock(rockchip_drm_vop2:3114:(&vop2_regmap_config)->lock);
[7.131916]                                lock(&mt->ma_lock);
[7.131925]   <Interrupt>
[7.131928]     lock(rockchip_drm_vop2:3114:(&vop2_regmap_config)->lock);
[7.131936]
                *** DEADLOCK ***

[7.131939] no locks held by swapper/0/0.
[7.131944]
               the shortest dependencies between 2nd lock and 1st lock:
[7.131950]  -> (&mt->ma_lock){+.+.}-{2:2} {
[7.131966]     HARDIRQ-ON-W at:
[7.131973]                       lock_acquire+0x200/0x330
[7.131986]                       _raw_spin_lock+0x50/0x70
[7.131998]                       regcache_maple_write+0x68/0xe0
[7.132010]                       regcache_write+0x6c/0x90
[7.132019]                       _regmap_read+0x19c/0x1d0
[7.132029]                       _regmap_update_bits+0xc0/0x148
[7.132038]                       regmap_update_bits_base+0x6c/0xa8
[7.132048]                       rk8xx_probe+0x22c/0x3d8
[7.132057]                       rk8xx_spi_probe+0x74/0x88
[7.132065]                       spi_probe+0xa8/0xe0

[...]

[7.132675]   }
[7.132678]   ... key      at: [<ffff800082943c20>] __key.0+0x0/0x10
[7.132691]   ... acquired at:
[7.132695]    _raw_spin_lock+0x50/0x70
[7.132704]    regcache_maple_write+0x68/0xe0
[7.132714]    regcache_write+0x6c/0x90
[7.132724]    _regmap_read+0x19c/0x1d0
[7.132732]    _regmap_update_bits+0xc0/0x148
[7.132741]    regmap_field_update_bits_base+0x74/0xb8
[7.132751]    vop2_plane_atomic_update+0x480/0x14d8 [rockchipdrm]
[7.132820]    drm_atomic_helper_commit_planes+0x1a0/0x320 [drm_kms_helper]

[...]

[7.135112] -> (rockchip_drm_vop2:3114:(&vop2_regmap_config)->lock){-...}-{2:2} {
[7.135130]    IN-HARDIRQ-W at:
[7.135136]                     lock_acquire+0x200/0x330
[7.135147]                     _raw_spin_lock_irqsave+0x6c/0x98
[7.135157]                     regmap_lock_spinlock+0x20/0x40
[7.135166]                     regmap_read+0x44/0x90
[7.135175]                     vop2_isr+0x90/0x290 [rockchipdrm]
[7.135225]                     __handle_irq_event_percpu+0x124/0x2d0

In the example above, the validator seems to get the scope of
dependencies wrong, since the regmap instance used in rk8xx-spi driver
has nothing to do with the instance from vop2.

Improve validation by sharing the regmap's lockdep class with the maple
tree's internal lock, while also providing a subclass for the latter.

Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
---
 drivers/base/regmap/internal.h       | 1 +
 drivers/base/regmap/regcache-maple.c | 3 +++
 drivers/base/regmap/regmap.c         | 1 +
 3 files changed, 5 insertions(+)

diff --git a/drivers/base/regmap/internal.h b/drivers/base/regmap/internal.h
index 83acccdc1008976de681396a300b78b701bbfa3c..bdb450436cbc53031c3d921f9f6067d26eca2549 100644
--- a/drivers/base/regmap/internal.h
+++ b/drivers/base/regmap/internal.h
@@ -59,6 +59,7 @@ struct regmap {
 			unsigned long raw_spinlock_flags;
 		};
 	};
+	struct lock_class_key *lock_key;
 	regmap_lock lock;
 	regmap_unlock unlock;
 	void *lock_arg; /* This is passed to lock/unlock functions */
diff --git a/drivers/base/regmap/regcache-maple.c b/drivers/base/regmap/regcache-maple.c
index a6a894aaf41bef56a95c92d27d88bef1631db83b..f3bbd771b1d7d48dab7e6a375bdf517673207e9c 100644
--- a/drivers/base/regmap/regcache-maple.c
+++ b/drivers/base/regmap/regcache-maple.c
@@ -317,6 +317,9 @@ static int regcache_maple_init(struct regmap *map)
 
 	mt_init(mt);
 
+	if (!mt_external_lock(mt) && map->lock_key)
+		lockdep_set_class_and_subclass(&mt->ma_lock, map->lock_key, 1);
+
 	for (i = 0; i < map->num_reg_defaults; i++) {
 		ret = regcache_maple_write(map,
 					   map->reg_defaults[i].reg,
diff --git a/drivers/base/regmap/regmap.c b/drivers/base/regmap/regmap.c
index 4ded93687c1f0ab3584947c61398cd6333b2e72c..53131a7ede0a6aad54bc85e970124f6b166a8010 100644
--- a/drivers/base/regmap/regmap.c
+++ b/drivers/base/regmap/regmap.c
@@ -745,6 +745,7 @@ struct regmap *__regmap_init(struct device *dev,
 						   lock_key, lock_name);
 		}
 		map->lock_arg = map;
+		map->lock_key = lock_key;
 	}
 
 	/*

---
base-commit: dec9255a128e19c5fcc3bdb18175d78094cc624d
change-id: 20241029-regmap-maple-lockdep-fix-f368ce6e7363

Re: [PATCH RFC] regmap: maple: Provide lockdep (sub)class for maple tree's internal lock

[Mark Brown]

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

On Tue, Oct 29, 2024 at 11:50:12PM +0200, Cristian Ciocaltea wrote:
> In some cases when using the maple tree register cache, the lockdep
> validator might complain about invalid deadlocks:

This doesn't apply against current code, please check and resend.

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

Re: [PATCH RFC] regmap: maple: Provide lockdep (sub)class for maple tree's internal lock

[Cristian Ciocaltea]

On 10/31/24 4:29 PM, Mark Brown wrote:
> On Tue, Oct 29, 2024 at 11:50:12PM +0200, Cristian Ciocaltea wrote:
>> In some cases when using the maple tree register cache, the lockdep
>> validator might complain about invalid deadlocks:
> 
> This doesn't apply against current code, please check and resend.

It was based on next-20241028 which contained commit 49a85851b14c
("regcache: Store values more directly in maple trees"), but that seems to
have been dropped recently.

I've sent v2 [1], which applies cleanly on both v6.12-rc5 and
next-20241031.

Thanks,
Cristian

[1] https://lore.kernel.org/lkml/20241031-regmap-maple-lockdep-fix-v2-1-06a3710f3623@collabora.com/

Re: [PATCH RFC] regmap: maple: Provide lockdep (sub)class for maple tree's internal lock

[Mark Brown]

On Tue, 29 Oct 2024 23:50:12 +0200, Cristian Ciocaltea wrote:
> In some cases when using the maple tree register cache, the lockdep
> validator might complain about invalid deadlocks:
> 
> [7.131886]  Possible interrupt unsafe locking scenario:
> 
> [7.131890]        CPU0                    CPU1
> [7.131893]        ----                    ----
> [7.131896]   lock(&mt->ma_lock);
> [7.131904]                                local_irq_disable();
> [7.131907]                                lock(rockchip_drm_vop2:3114:(&vop2_regmap_config)->lock);
> [7.131916]                                lock(&mt->ma_lock);
> [7.131925]   <Interrupt>
> [7.131928]     lock(rockchip_drm_vop2:3114:(&vop2_regmap_config)->lock);
> [7.131936]
>                 *** DEADLOCK ***
> 
> [...]

Applied to

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/regmap.git for-next

Thanks!

[1/1] regmap: maple: Provide lockdep (sub)class for maple tree's internal lock
      commit: 1ed9b927e7dd8b8cff13052efe212a8ff72ec51d

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark