[PATCH] Fix i2c module parameter permissions for read/write

[PATCH] Fix i2c module parameter permissions for read/write

[Jon Smirl]

The permissions of i2c module parameters were set to zero making the
parameters invisible and unsettable from the kernel command line. This
patch changes the permissions to the standard 0644 read/write.

Signed-off-by: Jon Smirl <jonsmirl@gmail.com>
---

diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index 8033e6b..395e430 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -588,7 +588,7 @@ union i2c_smbus_data {
 #define I2C_CLIENT_MODULE_PARM(var,desc) \
   static unsigned short var[I2C_CLIENT_MAX_OPTS] = I2C_CLIENT_DEFAULTS; \
   static unsigned int var##_num; \
-  module_param_array(var, short, &var##_num, 0); \
+  module_param_array(var, short, &var##_num, 0644); \
   MODULE_PARM_DESC(var,desc)

 #define I2C_CLIENT_MODULE_PARM_FORCE(name)                             \


-- 
Jon Smirl
jonsmirl@gmail.com

Re: [i2c] [PATCH] Fix i2c module parameter permissions for read/write

[Jean Delvare]

Hi John,

On Sun, 4 Nov 2007 00:55:15 -0400, Jon Smirl wrote:
> The permissions of i2c module parameters were set to zero making the
> parameters invisible and unsettable from the kernel command line. This
> patch changes the permissions to the standard 0644 read/write.

These permissions have nothing to do with the kernel command line. They
define whether a file is created in sysfs for the respective module
parameter. Even with permissions set to 0, you can still set the value
in question from the kernel command line with <module>.<parm>=<value>.

Also, there's no such think as "standard 0644" permissions. For some
parameters, it doesn't make much sense to expose them in sysfs. For
others it doesn't make sense (or could even be dangerous) to make them
writable. Each parameter has its own requirements.

> 
> Signed-off-by: Jon Smirl <jonsmirl@gmail.com>
> ---
> 
> diff --git a/include/linux/i2c.h b/include/linux/i2c.h
> index 8033e6b..395e430 100644
> --- a/include/linux/i2c.h
> +++ b/include/linux/i2c.h
> @@ -588,7 +588,7 @@ union i2c_smbus_data {
>  #define I2C_CLIENT_MODULE_PARM(var,desc) \
>    static unsigned short var[I2C_CLIENT_MAX_OPTS] = I2C_CLIENT_DEFAULTS; \
>    static unsigned int var##_num; \
> -  module_param_array(var, short, &var##_num, 0); \
> +  module_param_array(var, short, &var##_num, 0644); \
>    MODULE_PARM_DESC(var,desc)
> 
>  #define I2C_CLIENT_MODULE_PARM_FORCE(name)                             \
> 
> 


-- 
Jean Delvare

Re: [i2c] [PATCH] Fix i2c module parameter permissions for read/write

[Jon Smirl]

On 11/4/07, Jean Delvare <khali@linux-fr.org> wrote:
> Hi John,
>
> On Sun, 4 Nov 2007 00:55:15 -0400, Jon Smirl wrote:
> > The permissions of i2c module parameters were set to zero making the
> > parameters invisible and unsettable from the kernel command line. This
> > patch changes the permissions to the standard 0644 read/write.
>
> These permissions have nothing to do with the kernel command line. They
> define whether a file is created in sysfs for the respective module
> parameter. Even with permissions set to 0, you can still set the value
> in question from the kernel command line with <module>.<parm>=<value>.

I figured out later by inserting printks into the driver that they
were being set from the command line, but without changing the
permissions there was no way to read them to verify.  The permissions
should at least be set to 0444 to allow them to be read.

IMHO the I2C_CLIENT_INSMOD macros could use some rework since they use
side effects to set variables without being explicit about it.

The real problem was unrelated to this, it was an off by one error in
the i2c bus numbering code of my embedded processor. That made the
parameters not work since I was setting them on a different bus.

Changed to 0444.

Signed-off-by: Jon Smirl <jonsmirl@gmail.com>
---

diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index 8033e6b..395e430 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -588,7 +588,7 @@ union i2c_smbus_data {
 #define I2C_CLIENT_MODULE_PARM(var,desc) \
  static unsigned short var[I2C_CLIENT_MAX_OPTS] = I2C_CLIENT_DEFAULTS; \
  static unsigned int var##_num; \
-  module_param_array(var, short, &var##_num, 0); \
+  module_param_array(var, short, &var##_num, 0444); \
  MODULE_PARM_DESC(var,desc)

 #define I2C_CLIENT_MODULE_PARM_FORCE(name)                             \


-- 
Jon Smirl
jonsmirl@gmail.com

Re: [i2c] [PATCH] Fix i2c module parameter permissions for read/write

[Jean Delvare]

Hi Jon,

On Sun, 4 Nov 2007 08:39:49 -0500, Jon Smirl wrote:
> I figured out later by inserting printks into the driver that they
> were being set from the command line, but without changing the
> permissions there was no way to read them to verify.  The permissions
> should at least be set to 0444 to allow them to be read.

There's nothing to "verify" - the code works as intended, that's about
it.

> IMHO the I2C_CLIENT_INSMOD macros could use some rework since they use
> side effects to set variables without being explicit about it.

I don't understand what you mean here. Can you please be more specific?

> The real problem was unrelated to this, it was an off by one error in
> the i2c bus numbering code of my embedded processor. That made the
> parameters not work since I was setting them on a different bus.
> 
> Changed to 0444.

I'd rather not do that. The i2c core uses I2C_CLIENT_MODULE_PARM()
extensively and I don't really want to create dozens of additional
sysfs files for no good reason. If you want your own parameter to be
readable (or even writable if it makes sense - you didn't show the
code) I'd rather suggest adding a parameter to I2C_CLIENT_MODULE_PARM()
to let the caller decide what mode the attribute should have, setting
the mode to 0 for all the core parameters.

That being said, this whole set of macros in i2c.h is a complete mess
and with David Brownell's work on the i2c-core, using them should
become less necessary over time. Ultimately I'd be happy to get rid of
them completely.

Thanks,
-- 
Jean Delvare