[PATCH 1/2] regmap: add regmap_might_sleep()

[PATCH 1/2] regmap: add regmap_might_sleep()

[Michael Walle]

Sometimes a driver needs to know if the underlying regmap could sleep.
For example, consider the gpio-regmap driver which needs to fill the
gpiochip->can_sleep property.

It might be possible to pass this information via the
gpio_regmap_config, but this has the following drawbacks. First, that
property is redundant and both places might contratict each other. And
secondly, the driver might not even know the type of the regmap because
it just gets an opaque pointer by querying the device tree.

Signed-off-by: Michael Walle <michael@walle.cc>
---
 drivers/base/regmap/regmap.c | 11 +++++++++++
 include/linux/regmap.h       |  7 +++++++
 2 files changed, 18 insertions(+)

diff --git a/drivers/base/regmap/regmap.c b/drivers/base/regmap/regmap.c
index 297e95be25b3..f0661ff6bdcf 100644
--- a/drivers/base/regmap/regmap.c
+++ b/drivers/base/regmap/regmap.c
@@ -1847,6 +1847,17 @@ size_t regmap_get_raw_write_max(struct regmap *map)
 }
 EXPORT_SYMBOL_GPL(regmap_get_raw_write_max);
 
+/**
+ * regmap_might_sleep - Returns whether a regmap access might sleep.
+ *
+ * @map: Map to check.
+ */
+bool regmap_might_sleep(struct regmap *map)
+{
+	return map->can_sleep;
+}
+EXPORT_SYMBOL_GPL(regmap_might_sleep);
+
 static int _regmap_bus_formatted_write(void *context, unsigned int reg,
 				       unsigned int val)
 {
diff --git a/include/linux/regmap.h b/include/linux/regmap.h
index f87a11a5cc4a..955fbe61557a 100644
--- a/include/linux/regmap.h
+++ b/include/linux/regmap.h
@@ -1079,6 +1079,7 @@ int regmap_reinit_cache(struct regmap *map,
 			const struct regmap_config *config);
 struct regmap *dev_get_regmap(struct device *dev, const char *name);
 struct device *regmap_get_device(struct regmap *map);
+bool regmap_might_sleep(struct regmap *map);
 int regmap_write(struct regmap *map, unsigned int reg, unsigned int val);
 int regmap_write_async(struct regmap *map, unsigned int reg, unsigned int val);
 int regmap_raw_write(struct regmap *map, unsigned int reg,
@@ -1816,6 +1817,12 @@ static inline struct device *regmap_get_device(struct regmap *map)
 	return NULL;
 }
 
+static inline bool regmap_might_sleep(struct regmap *map)
+{
+	WARN_ONCE(1, "regmap API is disabled");
+	return true;
+}
+
 #endif
 
 #endif
-- 
2.20.1

[PATCH 2/2] gpio: regmap: use new regmap_might_sleep()

[Michael Walle]

Now that the regmap can be queried whether it might sleep, we can get
rid of the conservative setting "can_sleep = true". New drivers which
want to use gpio-regmap and can access the registers memory-mapped won't
have the restriction that their consumers have to use the
gpiod_*cansleep() variants anymore.

Signed-off-by: Michael Walle <michael@walle.cc>
---
 drivers/gpio/gpio-regmap.c | 9 +--------
 1 file changed, 1 insertion(+), 8 deletions(-)

diff --git a/drivers/gpio/gpio-regmap.c b/drivers/gpio/gpio-regmap.c
index 134cedf151a7..706f6c34696e 100644
--- a/drivers/gpio/gpio-regmap.c
+++ b/drivers/gpio/gpio-regmap.c
@@ -259,14 +259,7 @@ struct gpio_regmap *gpio_regmap_register(const struct gpio_regmap_config *config
 	chip->of_node = to_of_node(config->fwnode);
 #endif /* CONFIG_OF_GPIO */
 
-	/*
-	 * If our regmap is fast_io we should probably set can_sleep to false.
-	 * Right now, the regmap doesn't save this property, nor is there any
-	 * access function for it.
-	 * The only regmap type which uses fast_io is regmap-mmio. For now,
-	 * assume a safe default of true here.
-	 */
-	chip->can_sleep = true;
+	chip->can_sleep = regmap_might_sleep(gpio->regmap);
 
 	chip->get = gpio_regmap_get;
 	if (gpio->reg_set_base && gpio->reg_clr_base)
-- 
2.20.1

Re: [PATCH 1/2] regmap: add regmap_might_sleep()

[Mark Brown]

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

On Fri, Apr 30, 2021 at 03:06:44PM +0200, Michael Walle wrote:

> Sometimes a driver needs to know if the underlying regmap could sleep.
> For example, consider the gpio-regmap driver which needs to fill the
> gpiochip->can_sleep property.

Whatever is creating the regmap really ought to know what device it's
dealing with...

> It might be possible to pass this information via the
> gpio_regmap_config, but this has the following drawbacks. First, that
> property is redundant and both places might contratict each other. And
> secondly, the driver might not even know the type of the regmap because
> it just gets an opaque pointer by querying the device tree.

If it's a generic GPIO driver from a code correctness point of view it's
always got a risk of sleeping...

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

Re: [PATCH 1/2] regmap: add regmap_might_sleep()

[Michael Walle]

Am 2021-04-30 17:19, schrieb Mark Brown:
> On Fri, Apr 30, 2021 at 03:06:44PM +0200, Michael Walle wrote:
> 
>> Sometimes a driver needs to know if the underlying regmap could sleep.
>> For example, consider the gpio-regmap driver which needs to fill the
>> gpiochip->can_sleep property.
> 
> Whatever is creating the regmap really ought to know what device it's
> dealing with...

But creating and using the regmap are two seperate things, no? Consider
the gpio-sl28cpld. It will just use whatever regmap the parent has 
created.
How would it know what type of regmap it is?

>> It might be possible to pass this information via the
>> gpio_regmap_config, but this has the following drawbacks. First, that
>> property is redundant and both places might contratict each other. And
>> secondly, the driver might not even know the type of the regmap 
>> because
>> it just gets an opaque pointer by querying the device tree.
> 
> If it's a generic GPIO driver from a code correctness point of view 
> it's
> always got a risk of sleeping...

I can't follow you here.

-michael

Re: [PATCH 1/2] regmap: add regmap_might_sleep()

[Mark Brown]

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

On Fri, Apr 30, 2021 at 06:01:49PM +0200, Michael Walle wrote:
> Am 2021-04-30 17:19, schrieb Mark Brown:

> > Whatever is creating the regmap really ought to know what device it's
> > dealing with...

> But creating and using the regmap are two seperate things, no? Consider
> the gpio-sl28cpld. It will just use whatever regmap the parent has created.
> How would it know what type of regmap it is?

But that's a driver for a specific device AFAICT which looks like it's
only got an I2C binding on the MFD so the driver knows that it's for a
device that's on a bus that's going to sleep and doesn't need to infer
anything?  This looks like the common case I'd expect where there's no
variation.

> > > It might be possible to pass this information via the
> > > gpio_regmap_config, but this has the following drawbacks. First, that
> > > property is redundant and both places might contratict each other. And
> > > secondly, the driver might not even know the type of the regmap
> > > because
> > > it just gets an opaque pointer by querying the device tree.

> > If it's a generic GPIO driver from a code correctness point of view it's
> > always got a risk of sleeping...

> I can't follow you here.

If users happen to end up with a map flagged as fast they can work on
the whatever driver uses this stuff and not realise they're breaking
other users of the same driver that end up with slow I/O.  The whole
point of the flag in GPIO is AIUI warnings to help with that case.

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

Re: [PATCH 1/2] regmap: add regmap_might_sleep()

[Michael Walle]

Am 2021-04-30 19:26, schrieb Mark Brown:
> On Fri, Apr 30, 2021 at 06:01:49PM +0200, Michael Walle wrote:
>> Am 2021-04-30 17:19, schrieb Mark Brown:
> 
>> > Whatever is creating the regmap really ought to know what device it's
>> > dealing with...
> 
>> But creating and using the regmap are two seperate things, no? 
>> Consider
>> the gpio-sl28cpld. It will just use whatever regmap the parent has 
>> created.
>> How would it know what type of regmap it is?
> 
> But that's a driver for a specific device AFAICT which looks like it's
> only got an I2C binding on the MFD so the driver knows that it's for a
> device that's on a bus that's going to sleep and doesn't need to infer
> anything?  This looks like the common case I'd expect where there's no
> variation.

You are right, at the moment this driver only has an I2C binding. But
the idea was that this IP block and driver can be reused behind any
kind of bridge; I2C, SPI or MMIO. Actually, I had the impression
that all you need to do to convert it to MMIO is to replace the
"kontron,sl28cpld" compatible with a "syscon" compatible. But it isn't
that easy. Anyway, the idea is that you don't need to change anything
in the gpio-sl28cpld driver, just change the parent. But if we can't
ask the regmap what type it is, then we'll have to modify the
gpio-sl28cpld driver and we will have to figure it out by some other
means.

>> > > It might be possible to pass this information via the
>> > > gpio_regmap_config, but this has the following drawbacks. First, that
>> > > property is redundant and both places might contratict each other. And
>> > > secondly, the driver might not even know the type of the regmap
>> > > because
>> > > it just gets an opaque pointer by querying the device tree.
> 
>> > If it's a generic GPIO driver from a code correctness point of view it's
>> > always got a risk of sleeping...
> 
>> I can't follow you here.
> 
> If users happen to end up with a map flagged as fast they can work on
> the whatever driver uses this stuff and not realise they're breaking
> other users of the same driver that end up with slow I/O.  The whole
> point of the flag in GPIO is AIUI warnings to help with that case.

Hm, but as of now, the only thing which makes the gpio-regmap driver
slow i/o is the regmap itself.

-michael

Re: [PATCH 1/2] regmap: add regmap_might_sleep()

[Mark Brown]

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

On Sat, May 01, 2021 at 12:10:16AM +0200, Michael Walle wrote:
> Am 2021-04-30 19:26, schrieb Mark Brown:

> > But that's a driver for a specific device AFAICT which looks like it's
> > only got an I2C binding on the MFD so the driver knows that it's for a
> > device that's on a bus that's going to sleep and doesn't need to infer
> > anything?  This looks like the common case I'd expect where there's no
> > variation.

> You are right, at the moment this driver only has an I2C binding. But
> the idea was that this IP block and driver can be reused behind any
> kind of bridge; I2C, SPI or MMIO. Actually, I had the impression

Is this actually a way people are building hardware though?

> that all you need to do to convert it to MMIO is to replace the
> "kontron,sl28cpld" compatible with a "syscon" compatible. But it isn't
> that easy. Anyway, the idea is that you don't need to change anything
> in the gpio-sl28cpld driver, just change the parent. But if we can't
> ask the regmap what type it is, then we'll have to modify the
> gpio-sl28cpld driver and we will have to figure it out by some other
> means.

Well, you don't need to change anything at all - the driver will work
perfectly fine if it's flagging up the GPIOs as potentially sleeping
even if they end up not actually sleeping.

> > If users happen to end up with a map flagged as fast they can work on
> > the whatever driver uses this stuff and not realise they're breaking
> > other users of the same driver that end up with slow I/O.  The whole
> > point of the flag in GPIO is AIUI warnings to help with that case.

> Hm, but as of now, the only thing which makes the gpio-regmap driver
> slow i/o is the regmap itself.

Surely it's just a case of the device that's creating the gpio regmap
setting a flag when it instantiates it?  It's just one more thing that
the parent knows about the device.  This doesn't seem insurmountable.

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

Re: [PATCH 1/2] regmap: add regmap_might_sleep()

[Michael Walle]

Am 2021-05-06 14:43, schrieb Mark Brown:
> On Sat, May 01, 2021 at 12:10:16AM +0200, Michael Walle wrote:
>> Am 2021-04-30 19:26, schrieb Mark Brown:
> 
>> > But that's a driver for a specific device AFAICT which looks like it's
>> > only got an I2C binding on the MFD so the driver knows that it's for a
>> > device that's on a bus that's going to sleep and doesn't need to infer
>> > anything?  This looks like the common case I'd expect where there's no
>> > variation.
> 
>> You are right, at the moment this driver only has an I2C binding. But
>> the idea was that this IP block and driver can be reused behind any
>> kind of bridge; I2C, SPI or MMIO. Actually, I had the impression
> 
> Is this actually a way people are building hardware though?

Granted, this might be a special case because the driver is for an
IP inside CPLD/FPGA, thus you could easily switch the bridge.

>> that all you need to do to convert it to MMIO is to replace the
>> "kontron,sl28cpld" compatible with a "syscon" compatible. But it isn't
>> that easy. Anyway, the idea is that you don't need to change anything
>> in the gpio-sl28cpld driver, just change the parent. But if we can't
>> ask the regmap what type it is, then we'll have to modify the
>> gpio-sl28cpld driver and we will have to figure it out by some other
>> means.
> 
> Well, you don't need to change anything at all - the driver will work
> perfectly fine if it's flagging up the GPIOs as potentially sleeping
> even if they end up not actually sleeping.

Unless you need the non-sleeping version for a gpio.

>> > If users happen to end up with a map flagged as fast they can work on
>> > the whatever driver uses this stuff and not realise they're breaking
>> > other users of the same driver that end up with slow I/O.  The whole
>> > point of the flag in GPIO is AIUI warnings to help with that case.
> 
>> Hm, but as of now, the only thing which makes the gpio-regmap driver
>> slow i/o is the regmap itself.
> 
> Surely it's just a case of the device that's creating the gpio regmap
> setting a flag when it instantiates it?  It's just one more thing that
> the parent knows about the device.  This doesn't seem insurmountable.

No its not. It just seemed like it is way easier to just ask the regmap.

-michael

Re: [PATCH 1/2] regmap: add regmap_might_sleep()

[Mark Brown]

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

On Thu, May 06, 2021 at 03:35:26PM +0200, Michael Walle wrote:
> Am 2021-05-06 14:43, schrieb Mark Brown:

> > Surely it's just a case of the device that's creating the gpio regmap
> > setting a flag when it instantiates it?  It's just one more thing that
> > the parent knows about the device.  This doesn't seem insurmountable.

> No its not. It just seemed like it is way easier to just ask the regmap.

I'd rather cause a small amount of hoop jumping for one or two users
than open up an API that feels like it's going to encourage dodgy usage,
there's already enough problems with things like regulator_get_optional()
and this feels like it's going down similar roads.

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

[PATCH 1/2] regmap: add regmap_might_sleep()

[Michael Walle]

With the dawn of MMIO gpio-regmap users, it is desirable to let
gpio-regmap ask the regmap if it might sleep during an access so
it can pass that information to gpiochip. Add a new regmap_might_sleep()
to query the regmap.

Signed-off-by: Michael Walle <michael@walle.cc>
---
 drivers/base/regmap/regmap.c | 13 +++++++++++++
 include/linux/regmap.h       |  7 +++++++
 2 files changed, 20 insertions(+)

diff --git a/drivers/base/regmap/regmap.c b/drivers/base/regmap/regmap.c
index c6d6d53e8cd3..d12d669157f2 100644
--- a/drivers/base/regmap/regmap.c
+++ b/drivers/base/regmap/regmap.c
@@ -3486,6 +3486,19 @@ int regmap_get_reg_stride(struct regmap *map)
 }
 EXPORT_SYMBOL_GPL(regmap_get_reg_stride);
 
+/**
+ * regmap_might_sleep() - Returns whether a regmap access might sleep.
+ *
+ * @map: Register map to operate on.
+ *
+ * Returns true if an access to the register might sleep, else false.
+ */
+bool regmap_might_sleep(struct regmap *map)
+{
+	return map->can_sleep;
+}
+EXPORT_SYMBOL_GPL(regmap_might_sleep);
+
 int regmap_parse_val(struct regmap *map, const void *buf,
 			unsigned int *val)
 {
diff --git a/include/linux/regmap.h b/include/linux/regmap.h
index ca3434dca3a0..3faf5d5dbb26 100644
--- a/include/linux/regmap.h
+++ b/include/linux/regmap.h
@@ -1219,6 +1219,7 @@ static inline int regmap_write_bits(struct regmap *map, unsigned int reg,
 int regmap_get_val_bytes(struct regmap *map);
 int regmap_get_max_register(struct regmap *map);
 int regmap_get_reg_stride(struct regmap *map);
+bool regmap_might_sleep(struct regmap *map);
 int regmap_async_complete(struct regmap *map);
 bool regmap_can_raw_write(struct regmap *map);
 size_t regmap_get_raw_read_max(struct regmap *map);
@@ -1905,6 +1906,12 @@ static inline int regmap_get_reg_stride(struct regmap *map)
 	return -EINVAL;
 }
 
+static inline bool regmap_might_sleep(struct regmap *map)
+{
+	WARN_ONCE(1, "regmap API is disabled");
+	return true;
+}
+
 static inline int regcache_sync(struct regmap *map)
 {
 	WARN_ONCE(1, "regmap API is disabled");
-- 
2.30.2

Re: [PATCH 1/2] regmap: add regmap_might_sleep()

[Mark Brown]

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

On Mon, Nov 21, 2022 at 04:08:42PM +0100, Michael Walle wrote:
> With the dawn of MMIO gpio-regmap users, it is desirable to let
> gpio-regmap ask the regmap if it might sleep during an access so
> it can pass that information to gpiochip. Add a new regmap_might_sleep()
> to query the regmap.

The following changes since commit 9abf2313adc1ca1b6180c508c25f22f9395cc780:

  Linux 6.1-rc1 (2022-10-16 15:36:24 -0700)

are available in the Git repository at:

  https://git.kernel.org/pub/scm/linux/kernel/git/broonie/regmap.git tags/regmap-might-sleep

for you to fetch changes up to a6d99022e56e8c1ddc4c75895ed9e3ce5da88453:

  regmap: add regmap_might_sleep() (2022-11-22 12:23:17 +0000)

----------------------------------------------------------------
regmap: Add regmap_might_sleep()

Add an interface allowing generic users to determine if a regmap might
use sleeping operations.

----------------------------------------------------------------
Michael Walle (1):
      regmap: add regmap_might_sleep()

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

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