[PATCH] regmap: allow to disable all locking mechanisms

[PATCH] regmap: allow to disable all locking mechanisms

[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.

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 <brgl@bgdev.pl>
---
 drivers/base/regmap/regmap.c | 9 ++++++++-
 include/linux/regmap.h       | 5 +++++
 2 files changed, 13 insertions(+), 1 deletion(-)

diff --git a/drivers/base/regmap/regmap.c b/drivers/base/regmap/regmap.c
index 8d516a9bfc01..72917b2fc10e 100644
--- a/drivers/base/regmap/regmap.c
+++ b/drivers/base/regmap/regmap.c
@@ -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;
diff --git a/include/linux/regmap.h b/include/linux/regmap.h
index 15eddc1353ba..072a90229e34 100644
--- a/include/linux/regmap.h
+++ b/include/linux/regmap.h
@@ -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;
-- 
2.15.1

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

[Mark Brown]

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

>From c9b41fcf272b4926b373d21c2b83dfe374313780 Mon Sep 17 00:00:00 2001
From: Bartosz Golaszewski <brgl@bgdev.pl>
Date: Wed, 6 Dec 2017 15:26:21 +0100
Subject: [PATCH] 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 <brgl@bgdev.pl>
Signed-off-by: Mark Brown <broonie@kernel.org>
---
 drivers/base/regmap/regmap.c | 9 ++++++++-
 include/linux/regmap.h       | 5 +++++
 2 files changed, 13 insertions(+), 1 deletion(-)

diff --git a/drivers/base/regmap/regmap.c b/drivers/base/regmap/regmap.c
index 8d516a9bfc01..72917b2fc10e 100644
--- a/drivers/base/regmap/regmap.c
+++ b/drivers/base/regmap/regmap.c
@@ -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;
diff --git a/include/linux/regmap.h b/include/linux/regmap.h
index 15eddc1353ba..072a90229e34 100644
--- a/include/linux/regmap.h
+++ b/include/linux/regmap.h
@@ -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;
-- 
2.15.0

Re: [PATCH] regmap: allow to disable all locking mechanisms

[Andy Shevchenko]

On Wed, Dec 6, 2017 at 4:26 PM, Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> 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.

> +static void regmap_lock_unlock_empty(void *__map)

..._none()?


> +{
> +
> +}
> +
>  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?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] regmap: allow to disable all locking mechanisms

[Bartosz Golaszewski]

2017-12-10 14:10 GMT+01:00 Andy Shevchenko <andy.shevchenko@gmail.com>:
> On Wed, Dec 6, 2017 at 4:26 PM, Bartosz Golaszewski <brgl@bgdev.pl> wrote:
>> 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.
>
>> +static void regmap_lock_unlock_empty(void *__map)
>
> ..._none()?
>

Too late, Mark already applied it.

>
>> +{
>> +
>> +}
>> +
>>  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?

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

Re: [PATCH] regmap: allow to disable all locking mechanisms

[Andy Shevchenko]

On Sun, Dec 10, 2017 at 5:14 PM, Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> 2017-12-10 14:10 GMT+01:00 Andy Shevchenko <andy.shevchenko@gmail.com>:
>> On Wed, Dec 6, 2017 at 4:26 PM, Bartosz Golaszewski <brgl@bgdev.pl> wrote:
>>> 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.
>>
>>> +static void regmap_lock_unlock_empty(void *__map)
>>
>> ..._none()?
>>
>
> Too late, Mark already applied it.

Ah, Mark always works at speed of light!

>> 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).
>From those the latter looks better to me and IIUC you went that way.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] regmap: allow to disable all locking mechanisms

[Mark Brown]

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

On Tue, Dec 12, 2017 at 01:12:05PM +0200, Andy Shevchenko wrote:
> On Sun, Dec 10, 2017 at 5:14 PM, Bartosz Golaszewski <brgl@bgdev.pl> wrote:

> >>> +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.

> >> 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).
> From those the latter looks better to me and IIUC you went that way.

Yup.

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

Re: [PATCH] regmap: allow to disable all locking mechanisms

[Lars-Peter Clausen]

On 12/06/2017 03:26 PM, Bartosz Golaszewski wrote:
[...]
> + * @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.

Re: [PATCH] regmap: allow to disable all locking mechanisms

[Mark Brown]

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

On Tue, Dec 12, 2017 at 12:42:47PM +0100, Lars-Peter Clausen wrote:
> On 12/06/2017 03:26 PM, Bartosz Golaszewski wrote:

> > + * @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.

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

Re: [PATCH] regmap: allow to disable all locking mechanisms

[Bartosz Golaszewski]

2017-12-12 13:18 GMT+01:00 Mark Brown <broonie@kernel.org>:
> On Tue, Dec 12, 2017 at 12:42:47PM +0100, Lars-Peter Clausen wrote:
>> On 12/06/2017 03:26 PM, Bartosz Golaszewski wrote:
>
>> > + * @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.

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

Re: [PATCH] regmap: allow to disable all locking mechanisms

[Mark Brown]

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

On Tue, Dec 12, 2017 at 01:21:46PM +0100, Bartosz Golaszewski wrote:
> 2017-12-12 13:18 GMT+01:00 Mark Brown <broonie@kernel.org>:

> > 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.

> 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.

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