regmap: allow to disable all locking mechanisms

PATCH In review Bartosz Golaszewski 4 participants 10 messages Dec 6, 2017 – Dec 12, 2017

Subsystems (2)

Driver core Register map abstraction

lore

Share & tools

lore

Bartosz Golaszewski

author

2017-12-06 14:26 UTC #0 lore raw

PATCH 1/1 regmap: allow to disable all locking mechanisms

We have a use case in the at24 EEPROM driver (recently converted to using regmap instead of raw i2c/smbus calls) where we read from/write to the regmap in a loop, while protecting the entire loop with a mutex.

Currently this implicitly makes us use two mutexes - one in the driver and one in regmap. While browsing the code for similar use cases I noticed a significant number of places where locking *seems* redundant.

Allow users to completely disable any locking mechanisms in regmap config.

Signed-off-by Bartosz Golaszewski <[email protected]>

---
 drivers/base/regmap/regmap.c | 9 ++++++++-
 include/linux/regmap.h       | 5 +++++
 2 files changed, 13 insertions(+), 1 deletion(-)

drivers/base/regmap/regmap.c +8 -1

index 8d516a9bfc01..72917b2fc10e 100644
@@ -459,6 +459,11 @@ static void regmap_unlock_hwlock_irqrestore(void *__map)
 }
 #endif
 
+static void regmap_lock_unlock_empty(void *__map)
+{
+
+}
+
 static void regmap_lock_mutex(void *__map)
 {
 	struct regmap *map = __map;

@@ -669,7 +674,9 @@ struct regmap *__regmap_init(struct device *dev,
 		goto err;
 	}
 
-	if (config->lock && config->unlock) {
+	if (config->disable_locking) {
+		map->lock = map->unlock = regmap_lock_unlock_empty;
+	} else if (config->lock && config->unlock) {
 		map->lock = config->lock;
 		map->unlock = config->unlock;
 		map->lock_arg = config->lock_arg;

include/linux/regmap.h +5 -0

index 15eddc1353ba..072a90229e34 100644
@@ -264,6 +264,9 @@ typedef void (*regmap_unlock)(void *);
  *                field is NULL but precious_table (see below) is not, the
  *                check is performed on such table (a register is precious if
  *                it belongs to one of the ranges specified by precious_table).
+ * @disable_locking: This regmap is either protected by external means or
+ *                   is guaranteed not be be accessed from multiple threads.
+ *                   Don't use any locking mechanisms.
  * @lock:	  Optional lock callback (overrides regmap's default lock
  *		  function, based on spinlock or mutex).
  * @unlock:	  As above for unlocking.

@@ -333,6 +336,8 @@ struct regmap_config {
 	bool (*readable_reg)(struct device *dev, unsigned int reg);
 	bool (*volatile_reg)(struct device *dev, unsigned int reg);
 	bool (*precious_reg)(struct device *dev, unsigned int reg);
+
+	bool disable_locking;
 	regmap_lock lock;
 	regmap_unlock unlock;
 	void *lock_arg;

Signature · 1 line

2.15.1

Mark Brown

maintainer

2017-12-06 15:33 UTC #1 lore raw

Applied "regmap: allow to disable all locking mechanisms" to the regmap tree

The patch

regmap: allow to disable all locking mechanisms

has been applied to the regmap tree at

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

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

Quoted 1 line

From c9b41fcf272b4926b373d21c2b83dfe374313780 Mon Sep 17 00:00:00 2001

From: Bartosz Golaszewski <[email protected]>
Date: Wed, 6 Dec 2017 15:26:21 +0100
Subject: [PATCH] regmap: allow to disable all locking mechanisms

Allow users to completely disable any locking mechanisms in regmap config.

Signed-off-by Bartosz Golaszewski <[email protected]>

Signed-off-by Mark Brown <[email protected]>

---
 drivers/base/regmap/regmap.c | 9 ++++++++-
 include/linux/regmap.h       | 5 +++++
 2 files changed, 13 insertions(+), 1 deletion(-)

drivers/base/regmap/regmap.c +8 -1

index 8d516a9bfc01..72917b2fc10e 100644
@@ -459,6 +459,11 @@ static void regmap_unlock_hwlock_irqrestore(void *__map)
 }
 #endif
 
+static void regmap_lock_unlock_empty(void *__map)
+{
+
+}
+
 static void regmap_lock_mutex(void *__map)
 {
 	struct regmap *map = __map;

@@ -669,7 +674,9 @@ struct regmap *__regmap_init(struct device *dev,
 		goto err;
 	}
 
-	if (config->lock && config->unlock) {
+	if (config->disable_locking) {
+		map->lock = map->unlock = regmap_lock_unlock_empty;
+	} else if (config->lock && config->unlock) {
 		map->lock = config->lock;
 		map->unlock = config->unlock;
 		map->lock_arg = config->lock_arg;

include/linux/regmap.h +5 -0

index 15eddc1353ba..072a90229e34 100644
@@ -264,6 +264,9 @@ typedef void (*regmap_unlock)(void *);
  *                field is NULL but precious_table (see below) is not, the
  *                check is performed on such table (a register is precious if
  *                it belongs to one of the ranges specified by precious_table).
+ * @disable_locking: This regmap is either protected by external means or
+ *                   is guaranteed not be be accessed from multiple threads.
+ *                   Don't use any locking mechanisms.
  * @lock:	  Optional lock callback (overrides regmap's default lock
  *		  function, based on spinlock or mutex).
  * @unlock:	  As above for unlocking.

@@ -333,6 +336,8 @@ struct regmap_config {
 	bool (*readable_reg)(struct device *dev, unsigned int reg);
 	bool (*volatile_reg)(struct device *dev, unsigned int reg);
 	bool (*precious_reg)(struct device *dev, unsigned int reg);
+
+	bool disable_locking;
 	regmap_lock lock;
 	regmap_unlock unlock;
 	void *lock_arg;

Signature · 1 line

2.15.0

Andy Shevchenko

2017-12-10 13:10 UTC #2 lore raw

In reply to Bartosz Golaszewski

Quoted 5 lines · Bartosz Golaszewski

We have a use case in the at24 EEPROM driver (recently converted to using regmap instead of raw i2c/smbus calls) where we read from/write to the regmap in a loop, while protecting the entire loop with a mutex.

Quoted 8 lines

Currently this implicitly makes us use two mutexes - one in the driver and one in regmap. While browsing the code for similar use cases I noticed a significant number of places where locking *seems* redundant.
Allow users to completely disable any locking mechanisms in regmap config.
+static void regmap_lock_unlock_empty(void *__map)

..._none()?

drivers/base/regmap/regmap.c static void regmap_unlock_hwlock_irqrestore(void *__map)

Quoted 10 lines

drivers/base/regmap/regmap.c +7 -1

+{
+
+}
+
 static void regmap_lock_mutex(void *__map)
 
-       if (config->lock && config->unlock) {
+       if (config->disable_locking) {
+               map->lock = map->unlock = regmap_lock_unlock_empty;
+       } else if (config->lock && config->unlock) {

Why not to introduce positive switch, namely
bool mutex_lock; // choose better name
and assign ..._none() by default?

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Bartosz Golaszewski

author

2017-12-10 15:14 UTC #3 lore raw

2017-12-10 14:10 GMT+01:00 Andy Shevchenko <[email protected]>:

Quoted 10 lines · Bartosz Golaszewski

Quoted 9 lines
We have a use case in the at24 EEPROM driver (recently converted to using regmap instead of raw i2c/smbus calls) where we read from/write to the regmap in a loop, while protecting the entire loop with a mutex.
Currently this implicitly makes us use two mutexes - one in the driver and one in regmap. While browsing the code for similar use cases I noticed a significant number of places where locking *seems* redundant.

Quoted 7 lines

Quoted 4 lines
Allow users to completely disable any locking mechanisms in regmap config.
+static void regmap_lock_unlock_empty(void *__map)
..._none()?

Too late, Mark already applied it.

Quoted 7 lines

Quoted 6 lines
drivers/base/regmap/regmap.c +4 -0
+{
+
+}
+
 static void regmap_lock_mutex(void *__map)

Quoted 8 lines

drivers/base/regmap/regmap.c struct regmap *__regmap_init(struct device *dev, · line 679
Quoted 4 lines
drivers/base/regmap/regmap.c +3 -1
-       if (config->lock && config->unlock) {
+       if (config->disable_locking) {
+               map->lock = map->unlock = regmap_lock_unlock_empty;
+       } else if (config->lock && config->unlock) {
Why not to introduce positive switch, namely
bool mutex_lock; // choose better name
and assign ..._none() by default?

Because we don't want to break all the existing regmaps, if map->lock or map->unlock is empty, regmap core decides internally whether to use a mutex or a spinlock.

Best regards,
Bartosz Golaszewski

Andy Shevchenko

2017-12-12 11:12 UTC #4 lore raw

Quoted 14 lines · Bartosz Golaszewski

2017-12-10 14:10 GMT+01:00 Andy Shevchenko <[email protected]>:
Quoted 13 lines · Bartosz Golaszewski
Quoted 11 lines
We have a use case in the at24 EEPROM driver (recently converted to using regmap instead of raw i2c/smbus calls) where we read from/write to the regmap in a loop, while protecting the entire loop with a mutex.
Currently this implicitly makes us use two mutexes - one in the driver and one in regmap. While browsing the code for similar use cases I noticed a significant number of places where locking *seems* redundant.
Allow users to completely disable any locking mechanisms in regmap config.

Quoted 6 lines

Quoted 4 lines
Quoted 1 line
+static void regmap_lock_unlock_empty(void *__map)
..._none()?
Too late, Mark already applied it.

Ah, Mark always works at speed of light!

Quoted 7 lines

Quoted 3 lines
Why not to introduce positive switch, namely
bool mutex_lock; // choose better name
and assign ..._none() by default?
Because we don't want to break all the existing regmaps, if map->lock or map->unlock is empty, regmap core decides internally whether to use a mutex or a spinlock.

Good point.
So, it means the options like: nomutex (false — mutex is in use) or nolock (true — disable locking).

Quoted 1 line

From those the latter looks better to me and IIUC you went that way.

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Mark Brown

maintainer

2017-12-12 11:25 UTC #5 lore raw

Quoted 9 lines · Andy Shevchenko

Quoted 5 lines · Bartosz Golaszewski
Quoted 3 lines
Quoted 1 line
+static void regmap_lock_unlock_empty(void *__map)
..._none()?
Too late, Mark already applied it.
Ah, Mark always works at speed of light!

An incremental patch is always possible.

Quoted 4 lines

Quoted 3 lines
Quoted 3 lines
Why not to introduce positive switch, namely
bool mutex_lock; // choose better name
and assign ..._none() by default?

Quoted 8 lines

Quoted 3 lines
Because we don't want to break all the existing regmaps, if map->lock or map->unlock is empty, regmap core decides internally whether to use a mutex or a spinlock.
Good point.
So, it means the options like: nomutex (false — mutex is in use) or nolock (true — disable locking).
From those the latter looks better to me and IIUC you went that way.

Yup.

Lars-Peter Clausen

2017-12-12 11:42 UTC #6 lore raw

In reply to Bartosz Golaszewski

On 12/06/2017 03:26 PM, Bartosz Golaszewski wrote:
[...]

Quoted 2 lines

+ * @disable_locking: This regmap is either protected by external means or
+ * is guaranteed not be be accessed from multiple threads.

To guarantee this you need to make sure that a regmap instance with this flag set does not register a debugfs interface. debugfs file access happens from a userspace process and hence could (and probably) is in a different thread than the driver regmap access.

Mark Brown

maintainer

2017-12-12 12:18 UTC #7 lore raw

Quoted 9 lines · Lars-Peter Clausen

Quoted 2 lines · Bartosz Golaszewski
+ * @disable_locking: This regmap is either protected by external means or
+ * is guaranteed not be be accessed from multiple threads.
To guarantee this you need to make sure that a regmap instance with this flag set does not register a debugfs interface. debugfs file access happens from a userspace process and hence could (and probably) is in a different thread than the driver regmap access.

That's true, and the debugfs is world readable too so we can't just say it's up to root to be sensible.

Bartosz Golaszewski

author

2017-12-12 12:21 UTC #8 lore raw

2017-12-12 13:18 GMT+01:00 Mark Brown <[email protected]>:

Quoted 6 lines · Lars-Peter Clausen

Quoted 4 lines · Bartosz Golaszewski
Quoted 3 lines
+ * @disable_locking: This regmap is either protected by external means or
+ * is guaranteed not be be accessed from multiple threads.

Quoted 7 lines

Quoted 4 lines
To guarantee this you need to make sure that a regmap instance with this flag set does not register a debugfs interface. debugfs file access happens from a userspace process and hence could (and probably) is in a different thread than the driver regmap access.
That's true, and the debugfs is world readable too so we can't just say it's up to root to be sensible.

This lock is only taken when writing to cache_only and cache_bypass though.

This shouldn't cause serious trouble but yeah, we need to find a better solution.

Maybe the right approach would be to provide users with locking routines (regmap_lock()/regmap_unlock()) and then add a boolean variable to the config (e.g. bool manual_locking) which would indicate to regmap that it shouldn't take the lock in most cases (except for debugfs) as this would be manually done by the regmap owner?

Or possibly a separate lock for map->cache_bypass/map->cache_only?

Thanks,
Bartosz

Mark Brown

maintainer

2017-12-12 15:56 UTC #9 lore raw

Quoted 6 lines · Bartosz Golaszewski

2017-12-12 13:18 GMT+01:00 Mark Brown <[email protected]>:
Quoted 2 lines
That's true, and the debugfs is world readable too so we can't just say it's up to root to be sensible.
This lock is only taken when writing to cache_only and cache_bypass though.

No, that's not the case at all. Most obviously userspace can also initiate read operations from debugfs.

Quoted 5 lines

Maybe the right approach would be to provide users with locking routines (regmap_lock()/regmap_unlock()) and then add a boolean variable to the config (e.g. bool manual_locking) which would indicate to regmap that it shouldn't take the lock in most cases (except for debugfs) as this would be manually done by the regmap owner?

The most obvious thing is just to do what Lars suggested and suppress the creation of the debugfs entries for the device when locking is disabled.