linux-kernel - [RFC PATCH 1/2] regmap: add configurable lock class key for lockdep

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [thread-next>] [day] [month] [year] [list]
Message-Id: <1435224904-35517-1-git-send-email-drinkcat@chromium.org>
Date:	Thu, 25 Jun 2015 17:35:03 +0800
From:	Nicolas Boichat <drinkcat@...omium.org>
To:	Mark Brown <broonie@...nel.org>
Cc:	Mauro Carvalho Chehab <mchehab@....samsung.com>,
	Antti Palosaari <crope@....fi>, Ingo Molnar <mingo@...hat.com>,
	Arjan van de Ven <arjan@...ux.intel.com>,
	Lars-Peter Clausen <lars@...afoo.de>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	linux-kernel@...r.kernel.org, Bard Liao <bardliao@...ltek.com>,
	Oder Chiou <oder_chiou@...ltek.com>,
	Liam Girdwood <lgirdwood@...il.com>,
	Jaroslav Kysela <perex@...ex.cz>, Takashi Iwai <tiwai@...e.de>,
	alsa-devel@...a-project.org,
	Anatol Pomozov <anatol.pomozov@...il.com>
Subject: [RFC PATCH 1/2] regmap: add configurable lock class key for lockdep

From: Antti Palosaari <crope@....fi>

Lockdep validator complains about recursive locking and deadlock
when two different regmap instances are called in a nested order.
That happens anytime a regmap read/write call needs to access
another regmap.

This is because, for performance reason, lockdep groups all locks
initialized by the same mutex_init() in the same lock class.
Therefore all regmap mutexes are in the same lock class, leading
to lockdep "nested locking" warnings if a regmap accesses another
regmap. However, depending on the specifics of the driver, this
can be perfectly safe (e.g. if there is a clear hierarchy between
a "master" regmap that uses another "slave" regmap). In these
cases, the warning is false and should be silenced.

As a solution, add configuration option to pass custom lock class
key for lockdep validator, to be used in the regmap that needs to
access another regmap. This removes the need for uglier workarounds
in drivers, just to silence this warning (e.g. add custom mutex
lock/unlock functions).

Cc: Lars-Peter Clausen <lars@...afoo.de>
Cc: Mark Brown <broonie@...nel.org>
Signed-off-by: Antti Palosaari <crope@....fi>
[drinkcat@...omium.org: Reworded the commit message and regmap.h
documentation]
Signed-off-by: Nicolas Boichat <drinkcat@...omium.org>
---

I'm trying to revive Antti's patch [1], as we are hitting similar issue
with rt5677 driver. I only updated the commit message and documentation,
I kept both Signed-off-by and From lines, with a small note highlighting
my changes, let me know if that's not appropriate.

One issue that was raised by Mark at the time is that a per-regmap_config
lock class might not be enough (Mark mentioned clocks as an example). The
current implementation should be good enough as long as the clock regmaps
do not access each other. If this is a problem, maybe we should consider
replacing lockdep_lock_class_key by a boolean use_own_lock_class that would
allocate/use a per regmap instance lock class, or have devm_regmap_init
take an extra parameter specifying the lock class.

[1] https://www.mail-archive.com/linux-media%40vger.kernel.org/msg83490.html

 drivers/base/regmap/regmap.c | 3 +++
 include/linux/regmap.h       | 7 +++++++
 2 files changed, 10 insertions(+)

diff --git a/drivers/base/regmap/regmap.c b/drivers/base/regmap/regmap.c
index 6273ff0..f5d1131 100644
--- a/drivers/base/regmap/regmap.c
+++ b/drivers/base/regmap/regmap.c
@@ -560,6 +560,9 @@ struct regmap *regmap_init(struct device *dev,
 			mutex_init(&map->mutex);
 			map->lock = regmap_lock_mutex;
 			map->unlock = regmap_unlock_mutex;
+			if (config->lockdep_lock_class_key)
+				lockdep_set_class(&map->mutex,
+						config->lockdep_lock_class_key);
 		}
 		map->lock_arg = map;
 	}
diff --git a/include/linux/regmap.h b/include/linux/regmap.h
index 116655d..09aaaf5 100644
--- a/include/linux/regmap.h
+++ b/include/linux/regmap.h
@@ -135,6 +135,12 @@ typedef void (*regmap_unlock)(void *);
  * @lock_arg:	  this field is passed as the only argument of lock/unlock
  *		  functions (ignored in case regular lock/unlock functions
  *		  are not overridden).
+ * @lock_class_key: Custom lock class key for lockdep validator. Use that to
+ *                  silence false lockdep nested locking warning, when this
+ *                  regmap needs to access another regmap during read/write
+ *                  operations (directly in read/write functions, or
+ *                  indirectly, e.g. through bus accesses).
+ *                  Valid only when regmap default mutex locking is used.
  * @reg_read:	  Optional callback that if filled will be used to perform
  *           	  all the reads from the registers. Should only be provided for
  *		  devices whose read operation cannot be represented as a simple
@@ -198,6 +204,7 @@ struct regmap_config {
 	regmap_lock lock;
 	regmap_unlock unlock;
 	void *lock_arg;
+	struct lock_class_key *lockdep_lock_class_key;
 
 	int (*reg_read)(void *context, unsigned int reg, unsigned int *val);
 	int (*reg_write)(void *context, unsigned int reg, unsigned int val);
-- 
2.4.3.573.g4eafbef

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/