[RFC][PATCH] regmap: make REGCACHE_NONE maps return error on regcache_sync

[RFC][PATCH] regmap: make REGCACHE_NONE maps return error on regcache_sync

[Nariman Poushin]

Signed-off-by: Nariman Poushin <nariman@opensource.wolfsonmicro.com>
---
 regcache currently causes a BUG_ON if cache_sync/sync_region is
 called on a map with cache_type REGCACHE_NONE. This is not
 consistent with the behaviour of regcache_read/write which
 currently just return -ENOSYS and only throws a BUG_ON if
 the cache_type is something that _should_ have cache ops,
 but doesn't. Sure your device might not work, it but doesn't
 seem right to panic the kernel. The other option I suppose
 is to change it to a WARN_ON.

 Thanks
 Nariman

 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 7eb7b3b..992aba3 100644
--- a/drivers/base/regmap/regcache.c
+++ b/drivers/base/regmap/regcache.c
@@ -303,6 +303,9 @@ int regcache_sync(struct regmap *map)
 	const char *name;
 	unsigned int bypass;
 
+	if (map->cache_type == REGCACHE_NONE)
+		return -ENODEV;
+
 	BUG_ON(!map->cache_ops);
 
 	map->lock(map->lock_arg);
@@ -371,6 +374,9 @@ int regcache_sync_region(struct regmap *map, unsigned int min,
 	const char *name;
 	unsigned int bypass;
 
+	if (map->cache_type == REGCACHE_NONE)
+		return -ENODEV;
+
 	BUG_ON(!map->cache_ops);
 
 	map->lock(map->lock_arg);
-- 
2.1.4

Re: [RFC][PATCH] regmap: make REGCACHE_NONE maps return error on regcache_sync

[Mark Brown]

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

On Fri, May 08, 2015 at 10:55:37AM +0100, Nariman Poushin wrote:
> Signed-off-by: Nariman Poushin <nariman@opensource.wolfsonmicro.com>
> ---
>  regcache currently causes a BUG_ON if cache_sync/sync_region is
>  called on a map with cache_type REGCACHE_NONE. This is not
>  consistent with the behaviour of regcache_read/write which
>  currently just return -ENOSYS and only throws a BUG_ON if
>  the cache_type is something that _should_ have cache ops,
>  but doesn't. Sure your device might not work, it but doesn't
>  seem right to panic the kernel. The other option I suppose
>  is to change it to a WARN_ON.

Please submit patches in the format covered in SubmittingPatches, the
changelog goes before the signoff.  

The reason this is so loud is that while it's reasonable that generic
code could end up triggering a write it's difficult to see any way in
which a sync could be triggered on a device without a cache without it
being an obvious bug.  Since people frequently don't bother checking
return codes loud log messages are our only real way of reporting this,
given where syncs tend to happen it's not likely to happen in an obscure
code path that won't get seen.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [RFC][PATCH] regmap: make REGCACHE_NONE maps return error on regcache_sync

[Nariman Poushin]

On Fri, May 08, 2015 at 11:20:19AM +0100, Mark Brown wrote:
> On Fri, May 08, 2015 at 10:55:37AM +0100, Nariman Poushin wrote:
> > Signed-off-by: Nariman Poushin <nariman@opensource.wolfsonmicro.com>
> > ---
> >  regcache currently causes a BUG_ON if cache_sync/sync_region is
> >  called on a map with cache_type REGCACHE_NONE. This is not
> >  consistent with the behaviour of regcache_read/write which
> >  currently just return -ENOSYS and only throws a BUG_ON if
> >  the cache_type is something that _should_ have cache ops,
> >  but doesn't. Sure your device might not work, it but doesn't
> >  seem right to panic the kernel. The other option I suppose
> >  is to change it to a WARN_ON.
> 
> Please submit patches in the format covered in SubmittingPatches, the
> changelog goes before the signoff.  
>

Will do, apologies.
 
> The reason this is so loud is that while it's reasonable that generic
> code could end up triggering a write it's difficult to see any way in
> which a sync could be triggered on a device without a cache without it
> being an obvious bug.  Since people frequently don't bother checking
> return codes loud log messages are our only real way of reporting this,
> given where syncs tend to happen it's not likely to happen in an obscure
> code path that won't get seen.

Fair enough, that makes sense.

Thanks
Nariman

Re: [RFC][PATCH] regmap: make REGCACHE_NONE maps return error on regcache_sync

[Nariman Poushin]

On Fri, May 08, 2015 at 11:34:54AM +0100, Nariman Poushin wrote:
> On Fri, May 08, 2015 at 11:20:19AM +0100, Mark Brown wrote:
> > On Fri, May 08, 2015 at 10:55:37AM +0100, Nariman Poushin wrote:
> > > Signed-off-by: Nariman Poushin <nariman@opensource.wolfsonmicro.com>
> > > ---
> > >  regcache currently causes a BUG_ON if cache_sync/sync_region is
> > >  called on a map with cache_type REGCACHE_NONE. This is not
> > >  consistent with the behaviour of regcache_read/write which
> > >  currently just return -ENOSYS and only throws a BUG_ON if
> > >  the cache_type is something that _should_ have cache ops,
> > >  but doesn't. Sure your device might not work, it but doesn't
> > >  seem right to panic the kernel. The other option I suppose
> > >  is to change it to a WARN_ON.
> > 
> > Please submit patches in the format covered in SubmittingPatches, the
> > changelog goes before the signoff.  
> >
> 
> Will do, apologies.
>  
> > The reason this is so loud is that while it's reasonable that generic
> > code could end up triggering a write it's difficult to see any way in
> > which a sync could be triggered on a device without a cache without it
> > being an obvious bug.  Since people frequently don't bother checking
> > return codes loud log messages are our only real way of reporting this,
> > given where syncs tend to happen it's not likely to happen in an obscure
> > code path that won't get seen.
> 
> Fair enough, that makes sense.

In light of this:
https://lkml.org/lkml/2015/6/7/193

Would it be prudent to now  make these WARN_ON? It seems like it's not
worth taking the kernel down in the case where you are trying to sync to
a non-existent cache for a peripheral. Of course it could be that the 
particular write _not_ going in to the cache is catastrophic for some
other reason, but it still seems a bit severe and given the discussion
above not the intended usage for BUG_ON. It doesn't feel like this
satisfies the test:

"The *ONLY* acceptable reason for a BUG_ON() is if the machine is dead
anyway because of some major internal corruption."

Anyway, just a thought. Not sure if this constitutes a "content-less
bump", I just read something that I thought may change our previous
assumptions, if it does, apologies.

Thanks
Nariman
> 
> Thanks
> Nariman
> _______________________________________________
> patches mailing list
> patches@opensource.wolfsonmicro.com
> http://opensource.wolfsonmicro.com/cgi-bin/mailman/listinfo/patches

Re: [RFC][PATCH] regmap: make REGCACHE_NONE maps return error on regcache_sync

[Mark Brown]

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

On Mon, Jun 08, 2015 at 03:16:41PM +0100, Nariman Poushin wrote:

> In light of this:
> https://lkml.org/lkml/2015/6/7/193

Please provide human readable descriptions for things, people don't
always read online and it at least means going to another program to
figure out what's being discussed.

> Would it be prudent to now  make these WARN_ON? It seems like it's not
> worth taking the kernel down in the case where you are trying to sync to
> a non-existent cache for a peripheral. Of course it could be that the 

This is nothing new on Linus' part, really - as he himself says.  We're
being cautious here because we are often controlling devices like PMICs
and battery chargers where we're worried about physically damaging the
system if we get things too wrong, one thing that might possibly have
been happening here is that we've got very confused about what we're
doing with a critical system component.  There's certainly a case for
having some handling that's less severe but it does need to be something
that people notice and which is conservative about interacting with the
device after it noticed the problem.

In any case, if you want to propose a code change then please submit a
patch.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]