[PATCH] regmap: Add Kconfig option for debugfs register writes

[PATCH] regmap: Add Kconfig option for debugfs register writes

[Markus Pargmann]

To enable writing of registers through the regmap debugfs interface, it
was necessary to alter the regmap-debugfs.c source code. This is not
really practical.

As this is a powerful tool for debugging, this patch creates an expert
kconfig option for this function. This makes it easier to enable
register writing in the kernel.

This patch also fixes the file mode when the regmap debugfs write code is
compiled.

Cc: Dimitris Papastamos <dp@opensource.wolfsonmicro.com>
Signed-off-by: Markus Pargmann <mpa@pengutronix.de>
---
 drivers/base/regmap/Kconfig          | 14 ++++++++++++++
 drivers/base/regmap/regmap-debugfs.c | 16 ++++++++++------
 2 files changed, 24 insertions(+), 6 deletions(-)

diff --git a/drivers/base/regmap/Kconfig b/drivers/base/regmap/Kconfig
index 4251570610c9..ce86600dbe75 100644
--- a/drivers/base/regmap/Kconfig
+++ b/drivers/base/regmap/Kconfig
@@ -23,3 +23,17 @@ config REGMAP_MMIO
 
 config REGMAP_IRQ
 	bool
+
+config REGMAP_ALLOW_WRITE_DEBUGFS
+	default n
+	depends on DEBUG_FS
+	depends on EXPERT
+	depends on REGMAP
+	bool "Regmap, allow direct register writes through debugfs"
+	help
+	  This option enables regmap debugfs write support. If this is enabled,
+	  you can directly write registers using the registers file in the
+	  regmap debugfs.
+
+	  This is potentially dangerous. Do not enable this option unless you
+	  know what you are doing.
diff --git a/drivers/base/regmap/regmap-debugfs.c b/drivers/base/regmap/regmap-debugfs.c
index 45d812c0ea77..12906f2bdf00 100644
--- a/drivers/base/regmap/regmap-debugfs.c
+++ b/drivers/base/regmap/regmap-debugfs.c
@@ -261,13 +261,10 @@ 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
+#if IS_ENABLED(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
- * for this feature, people who want to use this will need to modify
- * the source code directly.
+ * PMICs, therefore this function can only be selected in expert kconfig mode.
  */
 static ssize_t regmap_map_write_file(struct file *file,
 				     const char __user *user_buf,
@@ -512,7 +509,14 @@ void regmap_debugfs_init(struct regmap *map, const char *name)
 			    map, &regmap_reg_ranges_fops);
 
 	if (map->max_register || regmap_readable(map, 0)) {
-		debugfs_create_file("registers", 0400, map->debugfs,
+		unsigned int registers_mode;
+
+		if (IS_ENABLED(CONFIG_REGMAP_ALLOW_WRITE_DEBUGFS))
+			registers_mode = 0600;
+		else
+			registers_mode = 0400;
+
+		debugfs_create_file("registers", registers_mode, map->debugfs,
 				    map, &regmap_map_fops);
 		debugfs_create_file("access", 0400, map->debugfs,
 				    map, &regmap_access_fops);
-- 
2.0.0

[PATCH] regmap: Add Kconfig option for debugfs register writes

[David Rientjes]

On Tue, 17 Jun 2014, Markus Pargmann wrote:

> To enable writing of registers through the regmap debugfs interface, it
> was necessary to alter the regmap-debugfs.c source code. This is not
> really practical.
> 
> As this is a powerful tool for debugging, this patch creates an expert
> kconfig option for this function. This makes it easier to enable
> register writing in the kernel.
> 
> This patch also fixes the file mode when the regmap debugfs write code is
> compiled.
> 

Forcing users to enable CONFIG_EXPERT for this doesn't seem appropriate, 
it enables things by default and has really become a separate config of 
its own.  You might want to consider removing this dependency unless you 
have a strong reason for keeping it.

> Cc: Dimitris Papastamos <dp@opensource.wolfsonmicro.com>
> Signed-off-by: Markus Pargmann <mpa@pengutronix.de>
> ---
>  drivers/base/regmap/Kconfig          | 14 ++++++++++++++
>  drivers/base/regmap/regmap-debugfs.c | 16 ++++++++++------
>  2 files changed, 24 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/base/regmap/Kconfig b/drivers/base/regmap/Kconfig
> index 4251570610c9..ce86600dbe75 100644
> --- a/drivers/base/regmap/Kconfig
> +++ b/drivers/base/regmap/Kconfig
> @@ -23,3 +23,17 @@ config REGMAP_MMIO
>  
>  config REGMAP_IRQ
>  	bool
> +
> +config REGMAP_ALLOW_WRITE_DEBUGFS
> +	default n
> +	depends on DEBUG_FS
> +	depends on EXPERT
> +	depends on REGMAP
> +	bool "Regmap, allow direct register writes through debugfs"
> +	help
> +	  This option enables regmap debugfs write support. If this is enabled,
> +	  you can directly write registers using the registers file in the
> +	  regmap debugfs.
> +
> +	  This is potentially dangerous. Do not enable this option unless you
> +	  know what you are doing.
> diff --git a/drivers/base/regmap/regmap-debugfs.c b/drivers/base/regmap/regmap-debugfs.c
> index 45d812c0ea77..12906f2bdf00 100644
> --- a/drivers/base/regmap/regmap-debugfs.c
> +++ b/drivers/base/regmap/regmap-debugfs.c
> @@ -261,13 +261,10 @@ 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
> +#if IS_ENABLED(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
> - * for this feature, people who want to use this will need to modify
> - * the source code directly.
> + * PMICs, therefore this function can only be selected in expert kconfig mode.
>   */
>  static ssize_t regmap_map_write_file(struct file *file,
>  				     const char __user *user_buf,
> @@ -512,7 +509,14 @@ void regmap_debugfs_init(struct regmap *map, const char *name)
>  			    map, &regmap_reg_ranges_fops);
>  
>  	if (map->max_register || regmap_readable(map, 0)) {
> -		debugfs_create_file("registers", 0400, map->debugfs,
> +		unsigned int registers_mode;
> +
> +		if (IS_ENABLED(CONFIG_REGMAP_ALLOW_WRITE_DEBUGFS))
> +			registers_mode = 0600;
> +		else
> +			registers_mode = 0400;
> +
> +		debugfs_create_file("registers", registers_mode, map->debugfs,
>  				    map, &regmap_map_fops);
>  		debugfs_create_file("access", 0400, map->debugfs,
>  				    map, &regmap_access_fops);

[PATCH] regmap: Add Kconfig option for debugfs register writes

[Markus Pargmann]

Hi,

On Tue, Jun 17, 2014 at 12:56:54PM -0700, David Rientjes wrote:
> On Tue, 17 Jun 2014, Markus Pargmann wrote:
> 
> > To enable writing of registers through the regmap debugfs interface, it
> > was necessary to alter the regmap-debugfs.c source code. This is not
> > really practical.
> > 
> > As this is a powerful tool for debugging, this patch creates an expert
> > kconfig option for this function. This makes it easier to enable
> > register writing in the kernel.
> > 
> > This patch also fixes the file mode when the regmap debugfs write code is
> > compiled.
> > 
> 
> Forcing users to enable CONFIG_EXPERT for this doesn't seem appropriate, 
> it enables things by default and has really become a separate config of 
> its own.  You might want to consider removing this dependency unless you 
> have a strong reason for keeping it.

I only created the dependency on CONFIG_EXPERT to make it clear that
this option is only for people who know what they are doing. I can
remove that dependency if that is OK for everyone?

Thanks,

Markus

> 
> > Cc: Dimitris Papastamos <dp@opensource.wolfsonmicro.com>
> > Signed-off-by: Markus Pargmann <mpa@pengutronix.de>
> > ---
> >  drivers/base/regmap/Kconfig          | 14 ++++++++++++++
> >  drivers/base/regmap/regmap-debugfs.c | 16 ++++++++++------
> >  2 files changed, 24 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/base/regmap/Kconfig b/drivers/base/regmap/Kconfig
> > index 4251570610c9..ce86600dbe75 100644
> > --- a/drivers/base/regmap/Kconfig
> > +++ b/drivers/base/regmap/Kconfig
> > @@ -23,3 +23,17 @@ config REGMAP_MMIO
> >  
> >  config REGMAP_IRQ
> >  	bool
> > +
> > +config REGMAP_ALLOW_WRITE_DEBUGFS
> > +	default n
> > +	depends on DEBUG_FS
> > +	depends on EXPERT
> > +	depends on REGMAP
> > +	bool "Regmap, allow direct register writes through debugfs"
> > +	help
> > +	  This option enables regmap debugfs write support. If this is enabled,
> > +	  you can directly write registers using the registers file in the
> > +	  regmap debugfs.
> > +
> > +	  This is potentially dangerous. Do not enable this option unless you
> > +	  know what you are doing.
> > diff --git a/drivers/base/regmap/regmap-debugfs.c b/drivers/base/regmap/regmap-debugfs.c
> > index 45d812c0ea77..12906f2bdf00 100644
> > --- a/drivers/base/regmap/regmap-debugfs.c
> > +++ b/drivers/base/regmap/regmap-debugfs.c
> > @@ -261,13 +261,10 @@ 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
> > +#if IS_ENABLED(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
> > - * for this feature, people who want to use this will need to modify
> > - * the source code directly.
> > + * PMICs, therefore this function can only be selected in expert kconfig mode.
> >   */
> >  static ssize_t regmap_map_write_file(struct file *file,
> >  				     const char __user *user_buf,
> > @@ -512,7 +509,14 @@ void regmap_debugfs_init(struct regmap *map, const char *name)
> >  			    map, &regmap_reg_ranges_fops);
> >  
> >  	if (map->max_register || regmap_readable(map, 0)) {
> > -		debugfs_create_file("registers", 0400, map->debugfs,
> > +		unsigned int registers_mode;
> > +
> > +		if (IS_ENABLED(CONFIG_REGMAP_ALLOW_WRITE_DEBUGFS))
> > +			registers_mode = 0600;
> > +		else
> > +			registers_mode = 0400;
> > +
> > +		debugfs_create_file("registers", registers_mode, map->debugfs,
> >  				    map, &regmap_map_fops);
> >  		debugfs_create_file("access", 0400, map->debugfs,
> >  				    map, &regmap_access_fops);
> 

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140617/6811b4a4/attachment.sig>

[PATCH] regmap: Add Kconfig option for debugfs register writes

[Mark Brown]

On Tue, Jun 17, 2014 at 09:36:35PM +0200, Markus Pargmann wrote:

> To enable writing of registers through the regmap debugfs interface, it
> was necessary to alter the regmap-debugfs.c source code. This is not
> really practical.

> As this is a powerful tool for debugging, this patch creates an expert
> kconfig option for this function. This makes it easier to enable
> register writing in the kernel.

This is deliberately hard to enable in order to make it clear that it
should never, ever be used in production as a bodge around writing
drivers and generally in order to reduce the risk to the rest of the
system.  Enabling this will typically give userspace full access to
components like PMICs which have the potential to cause physical damage
to the system if misused.

Anyone with a use for this is most likely building their own kernels
anyway and if a user modifies the code to enable it is clear that it is
their responsibility to deal with the fallout.

> This patch also fixes the file mode when the regmap debugfs write code is
> compiled.

Seperate changes in separate patches, especially don't put bugfixes in
the same patch as features.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140618/85db496e/attachment.sig>

[PATCH] regmap: Add Kconfig option for debugfs register writes

[Markus Pargmann]

Hi,

On Wed, Jun 18, 2014 at 01:11:27AM +0100, Mark Brown wrote:
> On Tue, Jun 17, 2014 at 09:36:35PM +0200, Markus Pargmann wrote:
> 
> > To enable writing of registers through the regmap debugfs interface, it
> > was necessary to alter the regmap-debugfs.c source code. This is not
> > really practical.
> 
> > As this is a powerful tool for debugging, this patch creates an expert
> > kconfig option for this function. This makes it easier to enable
> > register writing in the kernel.
> 
> This is deliberately hard to enable in order to make it clear that it
> should never, ever be used in production as a bodge around writing
> drivers and generally in order to reduce the risk to the rest of the
> system.  Enabling this will typically give userspace full access to
> components like PMICs which have the potential to cause physical damage
> to the system if misused.

The userspace already has full access to all those registers if you
want. For example memory mapped registeres can be written through
/dev/mem. I2C registers can easily be changed using i2c-tools or the
/dev/i2c-* devices. So you are already able to write a userspace driver
if you really want to.

I think this kconfig option would make it easier to debug hardware or
driver issues, as it is much more comfortable to have a clean kconfig
option.

> 
> Anyone with a use for this is most likely building their own kernels
> anyway and if a user modifies the code to enable it is clear that it is
> their responsibility to deal with the fallout.
> 
> > This patch also fixes the file mode when the regmap debugfs write code is
> > compiled.
> 
> Seperate changes in separate patches, especially don't put bugfixes in
> the same patch as features.

Ok, I will split this to a seperate patch

Thanks,

Markus

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140618/bb6e745d/attachment.sig>

[PATCH] regmap: Add Kconfig option for debugfs register writes

[Mark Brown]

On Wed, Jun 18, 2014 at 11:01:59PM +0200, Markus Pargmann wrote:
> On Wed, Jun 18, 2014 at 01:11:27AM +0100, Mark Brown wrote:

> > This is deliberately hard to enable in order to make it clear that it
> > should never, ever be used in production as a bodge around writing
> > drivers and generally in order to reduce the risk to the rest of the
> > system.  Enabling this will typically give userspace full access to
> > components like PMICs which have the potential to cause physical damage
> > to the system if misused.

> The userspace already has full access to all those registers if you
> want. For example memory mapped registeres can be written through
> /dev/mem. I2C registers can easily be changed using i2c-tools or the
> /dev/i2c-* devices. So you are already able to write a userspace driver
> if you really want to.

Right, but in all cases except for /dev/mem the API ensures that you
can't simultaneously run a kernel and userspace driver.  It's not the
userspace driver bit that's the problem but rather having both at the
same time.

> I think this kconfig option would make it easier to debug hardware or
> driver issues, as it is much more comfortable to have a clean kconfig
> option.

Well, a major goal of doing things this way is to make it seem wrong to
be using this too much!  :P
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140624/3d643e17/attachment-0001.sig>