[PATCH] regmap: Add a config option for making regmap debugfs writable

[PATCH] regmap: Add a config option for making regmap debugfs writable

[Anatol Pomozov]

Instead of modifiying source code directly one should use config files.
It is the standard way to set compile-time options.

Signed-off-by: Anatol Pomozov <anatol.pomozov@gmail.com>
---
 drivers/base/regmap/Kconfig          | 6 ++++++
 drivers/base/regmap/regmap-debugfs.c | 5 ++---
 2 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/base/regmap/Kconfig b/drivers/base/regmap/Kconfig
index db9d00c3..579c8b5 100644
--- a/drivers/base/regmap/Kconfig
+++ b/drivers/base/regmap/Kconfig
@@ -9,6 +9,12 @@ config REGMAP
 	select IRQ_DOMAIN if REGMAP_IRQ
 	bool
 
+config REGMAP_ALLOW_WRITE_DEBUGFS
+	bool "Make regmap debugfs writable"
+	default n
+	select REGMAP
+	select DEBUG_FS
+
 config REGMAP_AC97
 	tristate
 
diff --git a/drivers/base/regmap/regmap-debugfs.c b/drivers/base/regmap/regmap-debugfs.c
index 3f0a7e2..3ea2e27 100644
--- a/drivers/base/regmap/regmap-debugfs.c
+++ b/drivers/base/regmap/regmap-debugfs.c
@@ -259,8 +259,7 @@ static ssize_t regmap_map_read_file(struct file *file, char __user *user_buf,
 				   count, ppos);
 }
 
-#undef REGMAP_ALLOW_WRITE_DEBUGFS
-#ifdef REGMAP_ALLOW_WRITE_DEBUGFS
+#ifdef CONFIG_REGMAP_ALLOW_WRITE_DEBUGFS
 /*
  * This can be dangerous especially when we have clients such as
  * PMICs, therefore don't provide any real compile time configuration option
@@ -595,7 +594,7 @@ void regmap_debugfs_init(struct regmap *map, const char *name)
 	if (map->max_register || regmap_readable(map, 0)) {
 		umode_t registers_mode;
 
-#if defined(REGMAP_ALLOW_WRITE_DEBUGFS)
+#ifdef CONFIG_REGMAP_ALLOW_WRITE_DEBUGFS
 		registers_mode = 0600;
 #else
 		registers_mode = 0400;
-- 
2.6.0.rc2.230.g3dd15c0

Re: [PATCH] regmap: Add a config option for making regmap debugfs writable

[Mark Brown]

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

On Tue, Oct 13, 2015 at 10:46:55AM -0700, Anatol Pomozov wrote:
> Instead of modifiying source code directly one should use config files.
> It is the standard way to set compile-time options.

This is deliberately not a Kconfig option because it is a terrible idea
to do this in production and making it either selectable or the default
is an invitation to abuse.  We want to place a barrier here so that
users know that this is something that they have taken a decision to
enable, not something that is in any way supported (this is also why we
taint the kernel when people do write).

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

Re: [PATCH] regmap: Add a config option for making regmap debugfs writable

[Anatol Pomozov]

Hi

On Tue, Oct 13, 2015 at 10:55 AM, Mark Brown <broonie@kernel.org> wrote:
> This is deliberately not a Kconfig option because it is a terrible idea
> to do this in production and making it either selectable or the default
> is an invitation to abuse.

What kind of abuse are you talking about?

Having an easy way of modifying chip registers is extremely useful
during bringup / driver development. And during device development
phase I regularly have situations when I need to change a register to
see if it fixes an issue. Sometimes I need to test it remotely when
users located at another end of the Earth.

Current kernel source suggests I need to modify regmap-debugfs.c
directly. But my kernel tree is shared by multiply products and some
of the products in production already. I do not want to enable
writable remap for production products. I would like to have a
per-product compile-time configuration and .config serves exactly this
purpose.


> We want to place a barrier here so that
> users know that this is something that they have taken a decision to
> enable, not something that is in any way supported (this is also why we
> taint the kernel when people do write).

Honestly I am not convinced. Why to put obstacles on a feature that is
very useful during development?

Re: [PATCH] regmap: Add a config option for making regmap debugfs writable

[kbuild test robot]

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

Hi Anatol,

[auto build test WARNING on regmap/for-next -- if it's inappropriate base, please suggest rules for selecting the more suitable base]

url:    https://github.com/0day-ci/linux/commits/Anatol-Pomozov/regmap-Add-a-config-option-for-making-regmap-debugfs-writable/20151014-014929
config: mn10300-allyesconfig (attached as .config)
reproduce:
        wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=mn10300 

All warnings (new ones prefixed by >>):

   In file included from include/linux/list.h:8:0,
                    from include/linux/preempt.h:10,
                    from include/linux/spinlock.h:50,
                    from include/linux/mmzone.h:7,
                    from include/linux/gfp.h:5,
                    from include/linux/slab.h:14,
                    from drivers/base/regmap/regmap-debugfs.c:13:
   drivers/base/regmap/regmap-debugfs.c: In function 'regmap_map_write_file':
   include/linux/kernel.h:722:17: warning: comparison of distinct pointer types lacks a cast
     (void) (&_min1 == &_min2);  \
                    ^
>> drivers/base/regmap/regmap-debugfs.c:280:13: note: in expansion of macro 'min'
     buf_size = min(count, (sizeof(buf)-1));
                ^

vim +/min +280 drivers/base/regmap/regmap-debugfs.c

09c6ecd3 Dimitris Papastamos 2012-02-22  264   * This can be dangerous especially when we have clients such as
09c6ecd3 Dimitris Papastamos 2012-02-22  265   * PMICs, therefore don't provide any real compile time configuration option
09c6ecd3 Dimitris Papastamos 2012-02-22  266   * for this feature, people who want to use this will need to modify
09c6ecd3 Dimitris Papastamos 2012-02-22  267   * the source code directly.
09c6ecd3 Dimitris Papastamos 2012-02-22  268   */
09c6ecd3 Dimitris Papastamos 2012-02-22  269  static ssize_t regmap_map_write_file(struct file *file,
09c6ecd3 Dimitris Papastamos 2012-02-22  270  				     const char __user *user_buf,
09c6ecd3 Dimitris Papastamos 2012-02-22  271  				     size_t count, loff_t *ppos)
09c6ecd3 Dimitris Papastamos 2012-02-22  272  {
09c6ecd3 Dimitris Papastamos 2012-02-22  273  	char buf[32];
09c6ecd3 Dimitris Papastamos 2012-02-22  274  	size_t buf_size;
09c6ecd3 Dimitris Papastamos 2012-02-22  275  	char *start = buf;
09c6ecd3 Dimitris Papastamos 2012-02-22  276  	unsigned long reg, value;
09c6ecd3 Dimitris Papastamos 2012-02-22  277  	struct regmap *map = file->private_data;
68e850d8 Dimitris Papastamos 2013-05-09  278  	int ret;
09c6ecd3 Dimitris Papastamos 2012-02-22  279  
09c6ecd3 Dimitris Papastamos 2012-02-22 @280  	buf_size = min(count, (sizeof(buf)-1));
09c6ecd3 Dimitris Papastamos 2012-02-22  281  	if (copy_from_user(buf, user_buf, buf_size))
09c6ecd3 Dimitris Papastamos 2012-02-22  282  		return -EFAULT;
09c6ecd3 Dimitris Papastamos 2012-02-22  283  	buf[buf_size] = 0;
09c6ecd3 Dimitris Papastamos 2012-02-22  284  
09c6ecd3 Dimitris Papastamos 2012-02-22  285  	while (*start == ' ')
09c6ecd3 Dimitris Papastamos 2012-02-22  286  		start++;
09c6ecd3 Dimitris Papastamos 2012-02-22  287  	reg = simple_strtoul(start, &start, 16);
09c6ecd3 Dimitris Papastamos 2012-02-22  288  	while (*start == ' ')

:::::: The code at line 280 was first introduced by commit
:::::: 09c6ecd394105c4864a0e409e181c9b1578c2a63 regmap: Add support for writing to regmap registers via debugfs

:::::: TO: Dimitris Papastamos <dp@opensource.wolfsonmicro.com>
:::::: CC: Mark Brown <broonie@opensource.wolfsonmicro.com>

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 36228 bytes --]

Re: [PATCH] regmap: Add a config option for making regmap debugfs writable

[Mark Brown]

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

On Tue, Oct 13, 2015 at 11:33:13AM -0700, Anatol Pomozov wrote:
> On Tue, Oct 13, 2015 at 10:55 AM, Mark Brown <broonie@kernel.org> wrote:

> > This is deliberately not a Kconfig option because it is a terrible idea
> > to do this in production and making it either selectable or the default
> > is an invitation to abuse.

> What kind of abuse are you talking about?

Using it as a standard interface to control systems in production rather
than having appropriate support in the relevant driver.

> Having an easy way of modifying chip registers is extremely useful
> during bringup / driver development. And during device development
> phase I regularly have situations when I need to change a register to
> see if it fixes an issue. Sometimes I need to test it remotely when
> users located at another end of the Earth.

This is exactly the sort of use case this feature is intended for, and
is the sort of situation where a custom kernel is not going to be any
kind of practical problem.

> Current kernel source suggests I need to modify regmap-debugfs.c
> directly. But my kernel tree is shared by multiply products and some
> of the products in production already. I do not want to enable
> writable remap for production products. I would like to have a
> per-product compile-time configuration and .config serves exactly this
> purpose.

Feel free to make that modification in your local tree if you want it,
I'm not going to take it for upstream.

> > We want to place a barrier here so that
> > users know that this is something that they have taken a decision to
> > enable, not something that is in any way supported (this is also why we
> > taint the kernel when people do write).

> Honestly I am not convinced. Why to put obstacles on a feature that is
> very useful during development?

We don't want people complaining when someone misprograms their PMIC or
battery charger in a production system because a debug feature got left
on by mistake (both components that frequently use regmap and both
components that have the capacity to physically damage the system), or
have someone decide that the way to tune their system is to turn on this
option and bang on the hardware from userspace bypassing the driver.
It's really handy for debug but it's terrible for system robustness and
stability.

The whole point is that this is only intended to be used during
development while modifying the kernel, if you're able to do that it's
not a meningful obstacle.

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

Re: [PATCH] regmap: Add a config option for making regmap debugfs writable

[Pavel Machek]

On Tue 2015-10-13 21:07:00, Mark Brown wrote:
> On Tue, Oct 13, 2015 at 11:33:13AM -0700, Anatol Pomozov wrote:
> > On Tue, Oct 13, 2015 at 10:55 AM, Mark Brown <broonie@kernel.org> wrote:
> 
> > > This is deliberately not a Kconfig option because it is a terrible idea
> > > to do this in production and making it either selectable or the default
> > > is an invitation to abuse.
> 
> > What kind of abuse are you talking about?
> 
> Using it as a standard interface to control systems in production rather
> than having appropriate support in the relevant driver.
> 
> > Having an easy way of modifying chip registers is extremely useful
> > during bringup / driver development. And during device development
> > phase I regularly have situations when I need to change a register to
> > see if it fixes an issue. Sometimes I need to test it remotely when
> > users located at another end of the Earth.
> 
> This is exactly the sort of use case this feature is intended for, and
> is the sort of situation where a custom kernel is not going to be any
> kind of practical problem.

Well.. we support iopl(0) and we have /dev/mem. I don't see how this
is different.

And yes, CONFIG_ option would be nice, so that this gets compile test
coverage....
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH] regmap: Add a config option for making regmap debugfs writable

[Mark Brown]

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

On Fri, Dec 04, 2015 at 05:37:09PM +0100, Pavel Machek wrote:
> On Tue 2015-10-13 21:07:00, Mark Brown wrote:

> > This is exactly the sort of use case this feature is intended for, and
> > is the sort of situation where a custom kernel is not going to be any
> > kind of practical problem.

> Well.. we support iopl(0) and we have /dev/mem. I don't see how this
> is different.

It's not clear to me that those are especially good ideas that we want
to encourage people to use.

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