drivers/hwmon/f75375s.c contains the following code:
<-- snip -->
...
static ssize_t set_pwm_mode(struct device *dev, struct device_attribute *attr,
const char *buf, size_t count)
{
...
if (val != 0 || val != 1 || data->kind == f75373)
return -EINVAL;
...
<-- snip -->
I'm not sure what exactly was intended, but it was for sure not intended
to always return -EINVAL...
Spotted by the Coverity checker.
cu
Adrian
--
"Is there not promise of rain?" Ling Tan asked suddenly out
of the darkness. There had been need of rain for many days.
"Only a promise," Lao Er said.
Pearl S. Buck - Dragon Seed
> <-- snip -->
>
> ...
> static ssize_t set_pwm_mode(struct device *dev, struct device_attribute *attr,
> const char *buf, size_t count)
> {
> ...
> if (val != 0 || val != 1 || data->kind == f75373)
> return -EINVAL;
> ...
>
> <-- snip -->
> I'm not sure what exactly was intended, but it was for sure not intended
> to always return -EINVAL...
Aiee. val should be 1 or 0, and kind must not be f75373.
Signed-off-by: Riku Voipio <[email protected]>
---
drivers/hwmon/f75375s.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/drivers/hwmon/f75375s.c b/drivers/hwmon/f75375s.c
index f1df57a..885bfe9 100644
--- a/drivers/hwmon/f75375s.c
+++ b/drivers/hwmon/f75375s.c
@@ -344,7 +344,7 @@ static ssize_t set_pwm_mode(struct device *dev, struct device_attribute *attr,
int val = simple_strtoul(buf, NULL, 10);
u8 conf = 0;
- if (val != 0 || val != 1 || data->kind == f75373)
+ if (!(val == 0 || val == 1 ) || data->kind == f75373)
return -EINVAL;
mutex_lock(&data->update_lock);
--
1.5.3.1
Hi:
* Riku Voipio <[email protected]> [2007-10-17 23:45:08 +0300]:
> > <-- snip -->
> >
> > ...
> > static ssize_t set_pwm_mode(struct device *dev, struct device_attribute *attr,
> > const char *buf, size_t count)
> > {
> > ...
> > if (val != 0 || val != 1 || data->kind == f75373)
> > return -EINVAL;
> > ...
> >
> > <-- snip -->
>
> > I'm not sure what exactly was intended, but it was for sure not intended
> > to always return -EINVAL...
>
> Aiee. val should be 1 or 0, and kind must not be f75373.
>
> Signed-off-by: Riku Voipio <[email protected]>
> ---
> drivers/hwmon/f75375s.c | 2 +-
> 1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/hwmon/f75375s.c b/drivers/hwmon/f75375s.c
> index f1df57a..885bfe9 100644
> --- a/drivers/hwmon/f75375s.c
> +++ b/drivers/hwmon/f75375s.c
> @@ -344,7 +344,7 @@ static ssize_t set_pwm_mode(struct device *dev, struct device_attribute *attr,
> int val = simple_strtoul(buf, NULL, 10);
> u8 conf = 0;
>
> - if (val != 0 || val != 1 || data->kind == f75373)
> + if (!(val == 0 || val == 1 ) || data->kind == f75373)
> return -EINVAL;
>
> mutex_lock(&data->update_lock);
> --
> 1.5.3.1
That patch doesn't apply here, so I applied this:
commit 805763cd743f2aed41dc61a55569fa43cf1f240c
Author: Riku Voipio <[email protected]>
Date: Thu Oct 18 09:29:53 2007 -0400
hwmon: (f75375s) fix pwm mode setting
Spotted by the Coverity checker. (Thanks Adrian Bunk)
Signed-off-by: Riku Voipio <[email protected]>
Signed-off-by: Mark M. Hoffman <[email protected]>
diff --git a/drivers/hwmon/f75375s.c b/drivers/hwmon/f75375s.c
index 13a0413..59a3470 100644
--- a/drivers/hwmon/f75375s.c
+++ b/drivers/hwmon/f75375s.c
@@ -323,7 +323,7 @@ static ssize_t set_pwm_mode(struct device *dev, struct device_attribute *attr,
int val = simple_strtoul(buf, NULL, 10);
u8 conf = 0;
- if (val != 0 || val != 1 || data->kind == f75373)
+ if (!(val == 0 || val == 1) || data->kind == f75373)
return -EINVAL;
mutex_lock(&data->update_lock);
Thanks & regards,
--
Mark M. Hoffman
[email protected]
Hi Mark, hi Riku,
On Thu, 18 Oct 2007 09:37:44 -0400, Mark M. Hoffman wrote:
> That patch doesn't apply here, so I applied this:
>
> commit 805763cd743f2aed41dc61a55569fa43cf1f240c
> Author: Riku Voipio <[email protected]>
> Date: Thu Oct 18 09:29:53 2007 -0400
>
> hwmon: (f75375s) fix pwm mode setting
>
> Spotted by the Coverity checker. (Thanks Adrian Bunk)
>
> Signed-off-by: Riku Voipio <[email protected]>
> Signed-off-by: Mark M. Hoffman <[email protected]>
>
> diff --git a/drivers/hwmon/f75375s.c b/drivers/hwmon/f75375s.c
> index 13a0413..59a3470 100644
> --- a/drivers/hwmon/f75375s.c
> +++ b/drivers/hwmon/f75375s.c
> @@ -323,7 +323,7 @@ static ssize_t set_pwm_mode(struct device *dev, struct device_attribute *attr,
> int val = simple_strtoul(buf, NULL, 10);
> u8 conf = 0;
>
> - if (val != 0 || val != 1 || data->kind == f75373)
> + if (!(val == 0 || val == 1) || data->kind == f75373)
> return -EINVAL;
>
> mutex_lock(&data->update_lock);
BTW, that's the wrong way to do it. If the F75373S doesn't support
changing the PWM mode, then the sysfs attribute in question should be
read-only for this chip type. Making it writable and returning an error
on write is confusing.
Riku, can you please submit a patch fixing this? The attribute should
be declared read-only, and then you can use sysfs_chmod_file() to
change it to read-write where supported. Take a look at the w83781d
driver for an example.
Thanks,
--
Jean Delvare
On Fri, Oct 19, 2007 at 02:37:54PM +0200, Jean Delvare wrote:
> Riku, can you please submit a patch fixing this? The attribute should
> be declared read-only, and then you can use sysfs_chmod_file() to
> change it to read-write where supported.
Thanks, this was good suggestion. Patch attached.
> Take a look at the w83781d
> driver for an example.
Btw, I think your example code has a indentation bug:
if (kind != w83781d)
err = sysfs_chmod_file(&dev->kobj,
&sensor_dev_attr_temp3_alarm.dev_attr.attr,
S_IRUGO | S_IWUSR);
if (err)
return err;
--
"rm -rf" only sounds scary if you don't have backups
Hi Riku:
* Riku Voipio <[email protected]> [2007-10-24 14:50:34 +0300]:
> On Fri, Oct 19, 2007 at 02:37:54PM +0200, Jean Delvare wrote:
> > Riku, can you please submit a patch fixing this? The attribute should
> > be declared read-only, and then you can use sysfs_chmod_file() to
> > change it to read-write where supported.
>
> Thanks, this was good suggestion. Patch attached.
No patch?
--
Mark M. Hoffman
[email protected]
Hi Riku,
On Wed, 24 Oct 2007 14:50:34 +0300, Riku Voipio wrote:
> On Fri, Oct 19, 2007 at 02:37:54PM +0200, Jean Delvare wrote:
> > Take a look at the w83781d
> > driver for an example.
>
> Btw, I think your example code has a indentation bug:
>
> if (kind != w83781d)
> err = sysfs_chmod_file(&dev->kobj,
> &sensor_dev_attr_temp3_alarm.dev_attr.attr,
> S_IRUGO | S_IWUSR);
> if (err)
> return err;
Indentation is correct, but curly braces are missing! Nice catch,
thanks for reporting. It happens to be harmless in this specific case,
but it still needs fixing. I'll submit a patch shortly.
--
Jean Delvare
On Wed, Oct 24, 2007 at 10:25:29PM -0400, Mark M. Hoffman wrote:
> * Riku Voipio <[email protected]> [2007-10-24 14:50:34 +0300]:
> > On Fri, Oct 19, 2007 at 02:37:54PM +0200, Jean Delvare wrote:
> > > Riku, can you please submit a patch fixing this? The attribute should
> > > be declared read-only, and then you can use sysfs_chmod_file() to
> > > change it to read-write where supported.
> > Thanks, this was good suggestion. Patch attached.
> No patch?
Let's try again..
--
"rm -rf" only sounds scary if you don't have backups
Hi Riku,
On Thu, 25 Oct 2007 14:48:14 +0300, Riku Voipio wrote:
> On Wed, Oct 24, 2007 at 10:25:29PM -0400, Mark M. Hoffman wrote:
> > * Riku Voipio <[email protected]> [2007-10-24 14:50:34 +0300]:
> > > On Fri, Oct 19, 2007 at 02:37:54PM +0200, Jean Delvare wrote:
> > > > Riku, can you please submit a patch fixing this? The attribute should
> > > > be declared read-only, and then you can use sysfs_chmod_file() to
> > > > change it to read-write where supported.
>
> > > Thanks, this was good suggestion. Patch attached.
>
> > No patch?
>
> Let's try again..
> Fix value check in set_pwm_mode(). Instead of checking for
> chip variant there, make pwmX_mode sysfs nodes only writable
> on f75375 variant.
>
> Signed-off-by: Riku Voipio <[email protected]>
> ---
> drivers/hwmon/f75375s.c | 19 ++++++++++++++++---
> 1 files changed, 16 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/hwmon/f75375s.c b/drivers/hwmon/f75375s.c
> index 13a0413..d3b7932 100644
> --- a/drivers/hwmon/f75375s.c
> +++ b/drivers/hwmon/f75375s.c
> @@ -323,7 +323,7 @@ static ssize_t set_pwm_mode(struct device *dev, struct device_attribute *attr,
> int val = simple_strtoul(buf, NULL, 10);
> u8 conf = 0;
>
> - if (val != 0 || val != 1 || data->kind == f75373)
> + if (!(val == 0 || val == 1))
> return -EINVAL;
>
> mutex_lock(&data->update_lock);
> @@ -529,13 +529,13 @@ static SENSOR_DEVICE_ATTR(pwm1, S_IRUGO|S_IWUSR,
> show_pwm, set_pwm, 0);
> static SENSOR_DEVICE_ATTR(pwm1_enable, S_IRUGO|S_IWUSR,
> show_pwm_enable, set_pwm_enable, 0);
> -static SENSOR_DEVICE_ATTR(pwm1_mode, S_IRUGO|S_IWUSR,
> +static SENSOR_DEVICE_ATTR(pwm1_mode, S_IRUGO,
> show_pwm_mode, set_pwm_mode, 0);
> static SENSOR_DEVICE_ATTR(pwm2, S_IRUGO | S_IWUSR,
> show_pwm, set_pwm, 1);
> static SENSOR_DEVICE_ATTR(pwm2_enable, S_IRUGO|S_IWUSR,
> show_pwm_enable, set_pwm_enable, 1);
> -static SENSOR_DEVICE_ATTR(pwm2_mode, S_IRUGO|S_IWUSR,
> +static SENSOR_DEVICE_ATTR(pwm2_mode, S_IRUGO,
> show_pwm_mode, set_pwm_mode, 1);
>
> static struct attribute *f75375_attributes[] = {
> @@ -655,6 +655,19 @@ static int f75375_detect(struct i2c_adapter *adapter, int address, int kind)
> if ((err = sysfs_create_group(&client->dev.kobj, &f75375_group)))
> goto exit_detach;
>
> + if (kind == f75375) {
> + err = sysfs_chmod_file(&client->dev.kobj,
> + &sensor_dev_attr_pwm1_mode.dev_attr.attr,
> + S_IRUGO | S_IWUSR);
> + if (err)
> + goto exit_detach;
> + err = sysfs_chmod_file(&client->dev.kobj,
> + &sensor_dev_attr_pwm2_mode.dev_attr.attr,
> + S_IRUGO | S_IWUSR);
> + if (err)
> + goto exit_detach;
> + }
> +
> data->hwmon_dev = hwmon_device_register(&client->dev);
> if (IS_ERR(data->hwmon_dev)) {
> err = PTR_ERR(data->hwmon_dev);
Patch looks correct, however it doesn't apply on top of Mark's tree. I
was able to get it to apply by reverting "(f75375s) fix pwm mode
setting" first, but then the build fails. Presumably the other f75375s
patches interact badly. Can you please respin this patch on top of
Mark's tree (i.e. on top the the 4 other f75375s patches you sent since
the -rc1 merge)? Thanks.
--
Jean Delvare
On Fri, Oct 26, 2007 at 10:36:47AM +0200, Jean Delvare wrote:
> Patch looks correct, however it doesn't apply on top of Mark's tree. I
> was able to get it to apply by reverting "(f75375s) fix pwm mode
> setting" first, but then the build fails. Presumably the other f75375s
> patches interact badly. Can you please respin this patch on top of
> Mark's tree (i.e. on top the the 4 other f75375s patches you sent since
> the -rc1 merge)? Thanks.
The surrounding code had wandered to another function, so it's suprising
it applied at all. Here's respin.
--
"rm -rf" only sounds scary if you don't have backups
On Fri, 26 Oct 2007 14:14:23 +0300, Riku Voipio wrote:
> On Fri, Oct 26, 2007 at 10:36:47AM +0200, Jean Delvare wrote:
> > Patch looks correct, however it doesn't apply on top of Mark's tree. I
> > was able to get it to apply by reverting "(f75375s) fix pwm mode
> > setting" first, but then the build fails. Presumably the other f75375s
> > patches interact badly. Can you please respin this patch on top of
> > Mark's tree (i.e. on top the the 4 other f75375s patches you sent since
> > the -rc1 merge)? Thanks.
>
> The surrounding code had wandered to another function, so it's suprising
> it applied at all. Here's respin.
Patch looks OK, thanks.
Acked-by: Jean Delvare <[email protected]>
Riku, I have 14 patches posted to the lm-sensors list pending for
review. Would you mind reviewing one of them? Thanks.
--
Jean Delvare
Hi:
* Riku Voipio <[email protected]> [2007-10-26 14:14:23 +0300]:
> On Fri, Oct 26, 2007 at 10:36:47AM +0200, Jean Delvare wrote:
> > Patch looks correct, however it doesn't apply on top of Mark's tree. I
> > was able to get it to apply by reverting "(f75375s) fix pwm mode
> > setting" first, but then the build fails. Presumably the other f75375s
> > patches interact badly. Can you please respin this patch on top of
> > Mark's tree (i.e. on top the the 4 other f75375s patches you sent since
> > the -rc1 merge)? Thanks.
>
> The surrounding code had wandered to another function, so it's suprising
> it applied at all. Here's respin.
>
> --
> "rm -rf" only sounds scary if you don't have backups
> >From 4de69e3ab5b5833cddb503f0dcb2a3ccc2d5b328 Mon Sep 17 00:00:00 2001
> From: Riku Voipio <[email protected]>
> Date: Fri, 26 Oct 2007 13:53:50 +0300
> Subject: [PATCH] hwmon (f75375s) fix buggy if() properly
>
> Fix value check in set_pwm_mode(). Instead of checking for
> chip variant there, make pwmX_mode sysfs nodes only writable
> on f75375 variant.
>
> Signed-off-by: Riku Voipio <[email protected]>
> ---
> drivers/hwmon/f75375s.c | 19 ++++++++++++++++---
> 1 files changed, 16 insertions(+), 3 deletions(-)
>
Applied to hwmon-2.6.git/testing, thanks.
--
Mark M. Hoffman
[email protected]