2023-03-10 07:39:34

by Alexander Stein

[permalink] [raw]
Subject: [PATCH 1/2] regmap: cache: Do not panic for REGCACHE_NONE regmap

Most regcache operations do check for REGCACHE_NONE, before ensuring
doing BUG_ON(!map->cache_ops). The missing regcache_sync* functions
panic on REGCACHE_NONE regmaps instead. Add an early return for non-cached
ones.

Signed-off-by: Alexander Stein <[email protected]>
---
drivers/base/regmap/regcache.c | 6 ++++++
1 file changed, 6 insertions(+)

diff --git a/drivers/base/regmap/regcache.c b/drivers/base/regmap/regcache.c
index 362e043e26d8..b61763dbfc68 100644
--- a/drivers/base/regmap/regcache.c
+++ b/drivers/base/regmap/regcache.c
@@ -349,6 +349,9 @@ int regcache_sync(struct regmap *map)
const char *name;
bool bypass;

+ if (map->cache_type == REGCACHE_NONE)
+ return 0;
+
BUG_ON(!map->cache_ops);

map->lock(map->lock_arg);
@@ -418,6 +421,9 @@ int regcache_sync_region(struct regmap *map, unsigned int min,
const char *name;
bool bypass;

+ if (map->cache_type == REGCACHE_NONE)
+ return 0;
+
BUG_ON(!map->cache_ops);

map->lock(map->lock_arg);
--
2.34.1



2023-03-10 07:40:15

by Alexander Stein

[permalink] [raw]
Subject: [PATCH 2/2] regmap: cache: Fix return value

checkpatch.pl warned:
WARNING: ENOSYS means 'invalid syscall nr' and nothing else
Align the return value to regcache_drop_region().

Signed-off-by: Alexander Stein <[email protected]>
---
This warning popped up while initially returning ENOSYS in patch 1.

But I'm wondering if returning 0 for regcache_write is correct or not.
This might be a follow-up patch though.

drivers/base/regmap/regcache.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/base/regmap/regcache.c b/drivers/base/regmap/regcache.c
index b61763dbfc68..c13f5f8872ac 100644
--- a/drivers/base/regmap/regcache.c
+++ b/drivers/base/regmap/regcache.c
@@ -242,7 +242,7 @@ int regcache_read(struct regmap *map,
int ret;

if (map->cache_type == REGCACHE_NONE)
- return -ENOSYS;
+ return -EINVAL;

BUG_ON(!map->cache_ops);

--
2.34.1


2023-03-10 13:02:15

by Mark Brown

[permalink] [raw]
Subject: Re: [PATCH 1/2] regmap: cache: Do not panic for REGCACHE_NONE regmap

On Fri, Mar 10, 2023 at 08:39:10AM +0100, Alexander Stein wrote:
> Most regcache operations do check for REGCACHE_NONE, before ensuring
> doing BUG_ON(!map->cache_ops). The missing regcache_sync* functions
> panic on REGCACHE_NONE regmaps instead. Add an early return for non-cached
> ones.

Why would we be trying to do a regcache_sync() on a device with
no cache?


Attachments:
(No filename) (366.00 B)
signature.asc (488.00 B)
Download all attachments

2023-03-10 13:35:25

by Alexander Stein

[permalink] [raw]
Subject: Re: [PATCH 1/2] regmap: cache: Do not panic for REGCACHE_NONE regmap

Hi Mark,

Am Freitag, 10. M?rz 2023, 14:02:13 CET schrieb Mark Brown:
> * PGP Signed by an unknown key
>
> On Fri, Mar 10, 2023 at 08:39:10AM +0100, Alexander Stein wrote:
> > Most regcache operations do check for REGCACHE_NONE, before ensuring
> > doing BUG_ON(!map->cache_ops). The missing regcache_sync* functions
> > panic on REGCACHE_NONE regmaps instead. Add an early return for non-cached
> > ones.
>
> Why would we be trying to do a regcache_sync() on a device with
> no cache?

Indeed, that makes no sense. That's indicating a bug in a driver, but why do
we need to panic the kernel in this case?
On the other hand the same question applies to other regcache related
functions currently checking for non-cached maps. There is no common
behaviour:

panic:
* regcache_sync
* regcache_sync_region

returning -EINVAL:
* regcache_drop_region

returning -ENOSYS:
* regcache_read

returning success (0):
* regcache_write

early return (void return value):
* regcache_exit

Given all these possibilities I have no idea what's the right thing to do.

Best regards,
Alexander

--
TQ-Systems GmbH | M?hlstra?e 2, Gut Delling | 82229 Seefeld, Germany
Amtsgericht M?nchen, HRB 105018
Gesch?ftsf?hrer: Detlef Schneider, R?diger Stahl, Stefan Schneider
http://www.tq-group.com/



2023-03-10 14:22:31

by Mark Brown

[permalink] [raw]
Subject: Re: [PATCH 1/2] regmap: cache: Do not panic for REGCACHE_NONE regmap

On Fri, Mar 10, 2023 at 02:35:13PM +0100, Alexander Stein wrote:
> Am Freitag, 10. M?rz 2023, 14:02:13 CET schrieb Mark Brown:

> > Why would we be trying to do a regcache_sync() on a device with
> > no cache?

> Indeed, that makes no sense. That's indicating a bug in a driver, but why do
> we need to panic the kernel in this case?

You're trying to change this to silently ignore the call which
isn't going to make anything happy.

> On the other hand the same question applies to other regcache related
> functions currently checking for non-cached maps. There is no common
> behaviour:

> panic:
> * regcache_sync
> * regcache_sync_region

These are only ever triggered from a client driver, nothing in
regmap will ever sync the regmap without being explicitly asked
to.

> returning -ENOSYS:
> * regcache_read

> returning success (0):
> * regcache_write

> early return (void return value):
> * regcache_exit

These are all called transparently as part of the regmap core
regardless of if there is a cache, users never directly read or
write values to the cache.


Attachments:
(No filename) (1.05 kB)
signature.asc (488.00 B)
Download all attachments