* Re: [lm-sensors] [PATCH v2 4/4] hwmon: (f75375s) Catch some attempts to write to r/o registers
2012-02-28 21:15 [lm-sensors] [PATCH v2 4/4] hwmon: (f75375s) Catch some attempts to write to r/o registers Nikolaus Schulz
@ 2012-02-28 22:23 ` Guenter Roeck
2012-03-02 16:32 ` Guenter Roeck
` (4 subsequent siblings)
5 siblings, 0 replies; 7+ messages in thread
From: Guenter Roeck @ 2012-02-28 22:23 UTC (permalink / raw)
To: lm-sensors
On Tue, Feb 28, 2012 at 04:15:54PM -0500, Nikolaus Schulz wrote:
> It makes no sense to attempt to manually configure the fan in auto mode,
> or set the duty cycle directly in closed loop mode. The corresponding
> registers are then read-only. If the user tries it nonetheless, error out
> with EINVAL instead of silently doing nothing.
>
> Signed-off-by: Nikolaus Schulz <mail@microschulz.de>
Same as before - we can either hold the patch for 3.4, or split it into two parts.
Let me know which way you prefer.
Thanks,
Guenter
> ---
> drivers/hwmon/f75375s.c | 27 +++++++++++++++++++++++++++
> 1 files changed, 27 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/hwmon/f75375s.c b/drivers/hwmon/f75375s.c
> index 3fee82dc..5374501 100644
> --- a/drivers/hwmon/f75375s.c
> +++ b/drivers/hwmon/f75375s.c
> @@ -278,6 +278,21 @@ static bool duty_mode_enabled(u8 pwm_enable)
> }
> }
>
> +static bool auto_mode_enabled(u8 pwm_enable)
> +{
> + switch (pwm_enable) {
> + case 0: /* Manual, duty mode (full speed) */
> + case 1: /* Manual, duty mode */
> + case 3: /* Manual, speed mode */
> + return false;
> + case 2: /* Auto, speed mode */
> + case 4: /* Auto, duty mode */
> + return true;
> + default:
> + BUG();
> + }
> +}
> +
> static ssize_t set_fan_min(struct device *dev, struct device_attribute *attr,
> const char *buf, size_t count)
> {
> @@ -311,6 +326,11 @@ static ssize_t set_fan_target(struct device *dev, struct device_attribute *attr,
> if (err < 0)
> return err;
>
> + if (auto_mode_enabled(data->pwm_enable[nr]))
> + return -EINVAL;
> + if (data->kind = f75387 && duty_mode_enabled(data->pwm_enable[nr]))
> + return -EINVAL;
> +
> mutex_lock(&data->update_lock);
> data->fan_target[nr] = rpm_to_reg(val);
> f75375_write16(client, F75375_REG_FAN_EXP(nr), data->fan_target[nr]);
> @@ -331,6 +351,10 @@ static ssize_t set_pwm(struct device *dev, struct device_attribute *attr,
> if (err < 0)
> return err;
>
> + if (auto_mode_enabled(data->pwm_enable[nr]) ||
> + !duty_mode_enabled(data->pwm_enable[nr]))
> + return -EINVAL;
> +
> mutex_lock(&data->update_lock);
> data->pwm[nr] = SENSORS_LIMIT(val, 0, 255);
> f75375_write_pwm(client, nr, data->pwm[nr]);
> @@ -792,6 +816,9 @@ static void f75375_init(struct i2c_client *client, struct f75375_data *data,
> set_pwm_enable_direct(client, 0, f75375s_pdata->pwm_enable[0]);
> set_pwm_enable_direct(client, 1, f75375s_pdata->pwm_enable[1]);
> for (nr = 0; nr < 2; nr++) {
> + if (auto_mode_enabled(f75375s_pdata->pwm_enable[nr]) ||
> + !duty_mode_enabled(f75375s_pdata->pwm_enable[nr]))
> + continue;
> data->pwm[nr] = SENSORS_LIMIT(f75375s_pdata->pwm[nr], 0, 255);
> f75375_write_pwm(client, nr, data->pwm[nr]);
> }
> --
> 1.7.9.1
>
_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [lm-sensors] [PATCH v2 4/4] hwmon: (f75375s) Catch some attempts to write to r/o registers
2012-02-28 21:15 [lm-sensors] [PATCH v2 4/4] hwmon: (f75375s) Catch some attempts to write to r/o registers Nikolaus Schulz
2012-02-28 22:23 ` Guenter Roeck
@ 2012-03-02 16:32 ` Guenter Roeck
2012-03-02 21:59 ` Nikolaus Schulz
` (3 subsequent siblings)
5 siblings, 0 replies; 7+ messages in thread
From: Guenter Roeck @ 2012-03-02 16:32 UTC (permalink / raw)
To: lm-sensors
On Tue, 2012-02-28 at 16:15 -0500, Nikolaus Schulz wrote:
> It makes no sense to attempt to manually configure the fan in auto mode,
> or set the duty cycle directly in closed loop mode. The corresponding
> registers are then read-only. If the user tries it nonetheless, error out
> with EINVAL instead of silently doing nothing.
>
> Signed-off-by: Nikolaus Schulz <mail@microschulz.de>
Applied (with minor formatting cleanup) to -next.
Thanks,
Guenter
_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [lm-sensors] [PATCH v2 4/4] hwmon: (f75375s) Catch some attempts to write to r/o registers
2012-02-28 21:15 [lm-sensors] [PATCH v2 4/4] hwmon: (f75375s) Catch some attempts to write to r/o registers Nikolaus Schulz
2012-02-28 22:23 ` Guenter Roeck
2012-03-02 16:32 ` Guenter Roeck
@ 2012-03-02 21:59 ` Nikolaus Schulz
2012-03-02 22:06 ` Guenter Roeck
` (2 subsequent siblings)
5 siblings, 0 replies; 7+ messages in thread
From: Nikolaus Schulz @ 2012-03-02 21:59 UTC (permalink / raw)
To: lm-sensors
On Tue, Feb 28, 2012 at 02:23:55PM -0800, Guenter Roeck wrote:
> On Tue, Feb 28, 2012 at 04:15:54PM -0500, Nikolaus Schulz wrote:
> > It makes no sense to attempt to manually configure the fan in auto mode,
> > or set the duty cycle directly in closed loop mode. The corresponding
> > registers are then read-only. If the user tries it nonetheless, error out
> > with EINVAL instead of silently doing nothing.
> >
> > Signed-off-by: Nikolaus Schulz <mail@microschulz.de>
>
> Same as before - we can either hold the patch for 3.4, or split it into two parts.
> Let me know which way you prefer.
I see that this patch can indeed be split.
> > ---
> > drivers/hwmon/f75375s.c | 27 +++++++++++++++++++++++++++
> > 1 files changed, 27 insertions(+), 0 deletions(-)
> >
> > diff --git a/drivers/hwmon/f75375s.c b/drivers/hwmon/f75375s.c
> > index 3fee82dc..5374501 100644
> > --- a/drivers/hwmon/f75375s.c
> > +++ b/drivers/hwmon/f75375s.c
> > @@ -278,6 +278,21 @@ static bool duty_mode_enabled(u8 pwm_enable)
> > }
> > }
> >
> > +static bool auto_mode_enabled(u8 pwm_enable)
> > +{
> > + switch (pwm_enable) {
> > + case 0: /* Manual, duty mode (full speed) */
> > + case 1: /* Manual, duty mode */
> > + case 3: /* Manual, speed mode */
> > + return false;
> > + case 2: /* Auto, speed mode */
> > + case 4: /* Auto, duty mode */
> > + return true;
> > + default:
> > + BUG();
> > + }
> > +}
> > +
> > static ssize_t set_fan_min(struct device *dev, struct device_attribute *attr,
> > const char *buf, size_t count)
> > {
> > @@ -311,6 +326,11 @@ static ssize_t set_fan_target(struct device *dev, struct device_attribute *attr,
> > if (err < 0)
> > return err;
> >
> > + if (auto_mode_enabled(data->pwm_enable[nr]))
> > + return -EINVAL;
This is harmless as the register is r/o in this case, so the change need
not go into v3.3.
> > + if (data->kind = f75387 && duty_mode_enabled(data->pwm_enable[nr]))
> > + return -EINVAL;
> > +
This is blocking a dangerous operation, so it is a good candidate for
v3.3.
> > mutex_lock(&data->update_lock);
> > data->fan_target[nr] = rpm_to_reg(val);
> > f75375_write16(client, F75375_REG_FAN_EXP(nr), data->fan_target[nr]);
> > @@ -331,6 +351,10 @@ static ssize_t set_pwm(struct device *dev, struct device_attribute *attr,
> > if (err < 0)
> > return err;
> >
> > + if (auto_mode_enabled(data->pwm_enable[nr]) ||
> > + !duty_mode_enabled(data->pwm_enable[nr]))
> > + return -EINVAL;
> > +
This is combination of the two cases above. The check for auto mode is
again only cosmetic, but the check for duty mode is blocking a dangerous
operation for the F75387.
> > mutex_lock(&data->update_lock);
> > data->pwm[nr] = SENSORS_LIMIT(val, 0, 255);
> > f75375_write_pwm(client, nr, data->pwm[nr]);
> > @@ -792,6 +816,9 @@ static void f75375_init(struct i2c_client *client, struct f75375_data *data,
> > set_pwm_enable_direct(client, 0, f75375s_pdata->pwm_enable[0]);
> > set_pwm_enable_direct(client, 1, f75375s_pdata->pwm_enable[1]);
> > for (nr = 0; nr < 2; nr++) {
> > + if (auto_mode_enabled(f75375s_pdata->pwm_enable[nr]) ||
> > + !duty_mode_enabled(f75375s_pdata->pwm_enable[nr]))
> > + continue;
This is again combining a cosmetic and a safety check.
> > data->pwm[nr] = SENSORS_LIMIT(f75375s_pdata->pwm[nr], 0, 255);
> > f75375_write_pwm(client, nr, data->pwm[nr]);
> > }
> > --
> > 1.7.9.1
How shall we proceed here? I can split this, but then I see that you
have already applied the current patch to -next.
-- Nikolaus
_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [lm-sensors] [PATCH v2 4/4] hwmon: (f75375s) Catch some attempts to write to r/o registers
2012-02-28 21:15 [lm-sensors] [PATCH v2 4/4] hwmon: (f75375s) Catch some attempts to write to r/o registers Nikolaus Schulz
` (2 preceding siblings ...)
2012-03-02 21:59 ` Nikolaus Schulz
@ 2012-03-02 22:06 ` Guenter Roeck
2012-03-02 22:20 ` Nikolaus Schulz
2012-03-03 0:30 ` Guenter Roeck
5 siblings, 0 replies; 7+ messages in thread
From: Guenter Roeck @ 2012-03-02 22:06 UTC (permalink / raw)
To: lm-sensors
On Fri, 2012-03-02 at 16:59 -0500, Nikolaus Schulz wrote:
[ ... ]
>
> How shall we proceed here? I can split this, but then I see that you
> have already applied the current patch to -next.
>
With the other patch applied to 3.3, it made sense to apply this entire
patch to 3.3 as well. So I did, and already sent it off to Linus.
Guenter
_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [lm-sensors] [PATCH v2 4/4] hwmon: (f75375s) Catch some attempts to write to r/o registers
2012-02-28 21:15 [lm-sensors] [PATCH v2 4/4] hwmon: (f75375s) Catch some attempts to write to r/o registers Nikolaus Schulz
` (3 preceding siblings ...)
2012-03-02 22:06 ` Guenter Roeck
@ 2012-03-02 22:20 ` Nikolaus Schulz
2012-03-03 0:30 ` Guenter Roeck
5 siblings, 0 replies; 7+ messages in thread
From: Nikolaus Schulz @ 2012-03-02 22:20 UTC (permalink / raw)
To: lm-sensors
On Fri, Mar 02, 2012 at 02:06:15PM -0800, Guenter Roeck wrote:
> On Fri, 2012-03-02 at 16:59 -0500, Nikolaus Schulz wrote:
> > How shall we proceed here? I can split this, but then I see that you
> > have already applied the current patch to -next.
> >
> With the other patch applied to 3.3, it made sense to apply this entire
> patch to 3.3 as well. So I did, and already sent it off to Linus.
Excellent. What a relief! I was really worried about this. Thank you.
Nikolaus
_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [lm-sensors] [PATCH v2 4/4] hwmon: (f75375s) Catch some attempts to write to r/o registers
2012-02-28 21:15 [lm-sensors] [PATCH v2 4/4] hwmon: (f75375s) Catch some attempts to write to r/o registers Nikolaus Schulz
` (4 preceding siblings ...)
2012-03-02 22:20 ` Nikolaus Schulz
@ 2012-03-03 0:30 ` Guenter Roeck
5 siblings, 0 replies; 7+ messages in thread
From: Guenter Roeck @ 2012-03-03 0:30 UTC (permalink / raw)
To: lm-sensors
On Fri, Mar 02, 2012 at 05:20:14PM -0500, Nikolaus Schulz wrote:
> On Fri, Mar 02, 2012 at 02:06:15PM -0800, Guenter Roeck wrote:
> > On Fri, 2012-03-02 at 16:59 -0500, Nikolaus Schulz wrote:
> > > How shall we proceed here? I can split this, but then I see that you
> > > have already applied the current patch to -next.
> > >
> > With the other patch applied to 3.3, it made sense to apply this entire
> > patch to 3.3 as well. So I did, and already sent it off to Linus.
>
> Excellent. What a relief! I was really worried about this. Thank you.
>
... and thanks for your persistence.
Guenter
_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
^ permalink raw reply [flat|nested] 7+ messages in thread