regmap: make REGCACHE_NONE maps return error on regcache_sync
Subsystems (2)
RFC PATCH 1/1 regmap: make REGCACHE_NONE maps return error on regcache_sync
---
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(+)
drivers/base/regmap/regcache.c +6 -0
@@ -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);
Signature · 1 line
2.1.4
Mark Brown
maintainerQuoted 10 lines · Nariman Poushin
Signed-off-by Nariman Poushin <[email protected]>---
regcache currently causes aBUG_ONif cache_sync/sync_region is called on a map withcache_typeREGCACHE_NONE. This is not consistent with the behaviour of regcache_read/write which currently just return-ENOSYSand only throws aBUG_ONif thecache_typeis 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 aWARN_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.
Quoted 12 lines · Mark Brown
Quoted 10 lines · Nariman Poushin
Signed-off-by Nariman Poushin <[email protected]>---
regcache currently causes aBUG_ONif cache_sync/sync_region is called on a map withcache_typeREGCACHE_NONE. This is not consistent with the behaviour of regcache_read/write which currently just return-ENOSYSand only throws aBUG_ONif thecache_typeis 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 aWARN_ON.
Quoted 3 lines
Please submit patches in the format covered in SubmittingPatches, the changelog goes before the signoff.
Will do, apologies.
Quoted 7 lines
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
Quoted 27 lines · Nariman Poushin
Quoted 12 lines · Mark Brown
Quoted 10 lines · Nariman Poushin
Signed-off-by Nariman Poushin <[email protected]>---
regcache currently causes aBUG_ONif cache_sync/sync_region is called on a map withcache_typeREGCACHE_NONE. This is not consistent with the behaviour of regcache_read/write which currently just return-ENOSYSand only throws aBUG_ONif thecache_typeis 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 aWARN_ON.Quoted 3 lines
Please submit patches in the format covered in SubmittingPatches, the changelog goes before the signoff.
Will do, apologies.
Quoted 7 lines
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.
Quoted 1 line
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
Quoted 7 lines
Thanks
Nariman
_______________________________________________
patches mailing list
[email protected]
http://opensource.wolfsonmicro.com/cgi-bin/mailman/listinfo/patches
Mark Brown
maintainerQuoted 2 lines · Nariman Poushin
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.
Quoted 3 lines
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.