Prev: hwmon: Add support for W83667HG-B to w83627ehf driver
Next: omfs: fix memory leak
From: Jean Delvare on 4 Jul 2010 07:30
Hi Guenter,

On Sat, 3 Jul 2010 15:13:21 -0700, Guenter Roeck wrote:
> - Moved fan pwm register array pointers into per-instance data.
> - Only read fan pwm data for installed/supported fans.
> - Update fan max output and fan step output information from data in registers.
> - Create max_output and step_output attribute files only if respective
> fan pwm registers exist.
>
> Signed-off-by: Guenter Roeck <guenter.roeck(a)ericsson.com>
> ---
> drivers/hwmon/w83627ehf.c | 59 ++++++++++++++++++++++++++++++++++++++++++--
> 1 files changed, 56 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/hwmon/w83627ehf.c b/drivers/hwmon/w83627ehf.c
> index 0dcaba9..e01a3e9 100644
> --- a/drivers/hwmon/w83627ehf.c
> +++ b/drivers/hwmon/w83627ehf.c
> @@ -277,6 +277,11 @@ struct w83627ehf_data {
> struct device *hwmon_dev;
> struct mutex lock;
>
> + const u8 *REG_FAN_START_OUTPUT;
> + const u8 *REG_FAN_STOP_OUTPUT;
> + const u8 *REG_FAN_MAX_OUTPUT;
> + const u8 *REG_FAN_STEP_OUTPUT;
> +
> struct mutex update_lock;
> char valid; /* !=0 if following fields are valid */
> unsigned long last_updated; /* In jiffies */
> @@ -524,7 +529,10 @@ static struct w83627ehf_data *w83627ehf_update_device(struct device *dev)
> }
> }
>
> - for (i = 0; i < 4; i++) {
> + for (i = 0; i < data->pwm_num; i++) {
> + if (!(data->has_fan & (1 << i)))
> + continue;

I'm skeptical. data->has_fan refers to fan inputs, not fan outputs.
There is no guarantee that pwm1 is mapped to fan1, etc., so you can't
use data->has_fan to discard fan outputs.

> +
> /* pwmcfg, tolerance mapped for i=0, i=1 to same reg */
> if (i != 1) {
> pwmcfg = w83627ehf_read_value(data,
> @@ -546,6 +554,17 @@ static struct w83627ehf_data *w83627ehf_update_device(struct device *dev)
> W83627EHF_REG_FAN_STOP_OUTPUT[i]);
> data->fan_stop_time[i] = w83627ehf_read_value(data,
> W83627EHF_REG_FAN_STOP_TIME[i]);
> +
> + if (data->REG_FAN_MAX_OUTPUT[i] != 0xff)
> + data->fan_max_output[i] =
> + w83627ehf_read_value(data,
> + data->REG_FAN_MAX_OUTPUT[i]);
> +
> + if (data->REG_FAN_STEP_OUTPUT[i] != 0xff)
> + data->fan_step_output[i] =
> + w83627ehf_read_value(data,
> + data->REG_FAN_STEP_OUTPUT[i]);
> +

Huuu, wait a minute, So these values were supposedly exposed to
user-space, but were never read from their respective registers? So
this is a plain bug you're fixing?

Now I'm wondering, how useful is a feature that has been broken forever
and nobody ever complained?

> data->target_temp[i] =
> w83627ehf_read_value(data,
> W83627EHF_REG_TARGET[i]) &
> @@ -1126,7 +1145,7 @@ store_##reg(struct device *dev, struct device_attribute *attr, \
> u32 val = SENSORS_LIMIT(simple_strtoul(buf, NULL, 10), 1, 255); \
> mutex_lock(&data->update_lock); \
> data->reg[nr] = val; \
> - w83627ehf_write_value(data, W83627EHF_REG_##REG[nr], val); \
> + w83627ehf_write_value(data, data->REG_##REG[nr], val); \
> mutex_unlock(&data->update_lock); \
> return count; \
> }
> @@ -1206,12 +1225,26 @@ static struct sensor_device_attribute sda_sf3_arrays[] = {
> store_fan_stop_output, 1),
> SENSOR_ATTR(pwm3_stop_output, S_IWUSR | S_IRUGO, show_fan_stop_output,
> store_fan_stop_output, 2),
> +};
>
> - /* pwm1 and pwm3 don't support max and step settings */
> +
> +/*
> + * pwm1 and pwm3 don't support max and step settings on all chips.
> + * Need to check support while generating/removing attribute files.
> + */
> +static struct sensor_device_attribute sda_sf3_max_step_arrays[] = {
> + SENSOR_ATTR(pwm1_max_output, S_IWUSR | S_IRUGO, show_fan_max_output,
> + store_fan_max_output, 0),
> + SENSOR_ATTR(pwm1_step_output, S_IWUSR | S_IRUGO, show_fan_step_output,
> + store_fan_step_output, 0),
> SENSOR_ATTR(pwm2_max_output, S_IWUSR | S_IRUGO, show_fan_max_output,
> store_fan_max_output, 1),
> SENSOR_ATTR(pwm2_step_output, S_IWUSR | S_IRUGO, show_fan_step_output,
> store_fan_step_output, 1),
> + SENSOR_ATTR(pwm3_max_output, S_IWUSR | S_IRUGO, show_fan_max_output,
> + store_fan_max_output, 2),
> + SENSOR_ATTR(pwm3_step_output, S_IWUSR | S_IRUGO, show_fan_step_output,
> + store_fan_step_output, 2),
> };
>
> static ssize_t
> @@ -1235,6 +1268,12 @@ static void w83627ehf_device_remove_files(struct device *dev)
>
> for (i = 0; i < ARRAY_SIZE(sda_sf3_arrays); i++)
> device_remove_file(dev, &sda_sf3_arrays[i].dev_attr);
> + for (i = 0; i < ARRAY_SIZE(sda_sf3_max_step_arrays); i++) {
> + struct sensor_device_attribute *attr =
> + &sda_sf3_max_step_arrays[i];
> + if (data->REG_FAN_STEP_OUTPUT[attr->index] != 0xff)
> + device_remove_file(dev, &attr->dev_attr);
> + }
> for (i = 0; i < ARRAY_SIZE(sda_sf3_arrays_fan4); i++)
> device_remove_file(dev, &sda_sf3_arrays_fan4[i].dev_attr);
> for (i = 0; i < data->in_num; i++) {
> @@ -1352,6 +1391,11 @@ static int __devinit w83627ehf_probe(struct platform_device *pdev)
> data->in6_skip = !data->temp3_disable;
> }
>
> + data->REG_FAN_START_OUTPUT = W83627EHF_REG_FAN_START_OUTPUT;
> + data->REG_FAN_STOP_OUTPUT = W83627EHF_REG_FAN_STOP_OUTPUT;
> + data->REG_FAN_MAX_OUTPUT = W83627EHF_REG_FAN_MAX_OUTPUT;
> + data->REG_FAN_STEP_OUTPUT = W83627EHF_REG_FAN_STEP_OUTPUT;
> +
> /* Initialize the chip */
> w83627ehf_init_device(data);
>
> @@ -1440,6 +1484,15 @@ static int __devinit w83627ehf_probe(struct platform_device *pdev)
> &sda_sf3_arrays[i].dev_attr)))
> goto exit_remove;
>
> + for (i = 0; i < ARRAY_SIZE(sda_sf3_max_step_arrays); i++) {
> + struct sensor_device_attribute *attr =
> + &sda_sf3_max_step_arrays[i];
> + if (data->REG_FAN_STEP_OUTPUT[attr->index] != 0xff) {
> + err = device_create_file(dev, &attr->dev_attr);
> + if (err)
> + goto exit_remove;
> + }
> + }
> /* if fan4 is enabled create the sf3 files for it */
> if ((data->has_fan & (1 << 3)) && data->pwm_num >= 4)
> for (i = 0; i < ARRAY_SIZE(sda_sf3_arrays_fan4); i++) {


Nice cleanup overall.

--
Jean Delvare
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Guenter Roeck on 4 Jul 2010 10:00
Hi Jean,

On Sun, Jul 04, 2010 at 07:24:42AM -0400, Jean Delvare wrote:
> Hi Guenter,
>
> On Sat, 3 Jul 2010 15:13:21 -0700, Guenter Roeck wrote:
> > - Moved fan pwm register array pointers into per-instance data.
> > - Only read fan pwm data for installed/supported fans.
> > - Update fan max output and fan step output information from data in registers.
> > - Create max_output and step_output attribute files only if respective
> > fan pwm registers exist.
> >
> > Signed-off-by: Guenter Roeck <guenter.roeck(a)ericsson.com>
> > ---
> > drivers/hwmon/w83627ehf.c | 59 ++++++++++++++++++++++++++++++++++++++++++--
> > 1 files changed, 56 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/hwmon/w83627ehf.c b/drivers/hwmon/w83627ehf.c
> > index 0dcaba9..e01a3e9 100644
> > --- a/drivers/hwmon/w83627ehf.c
> > +++ b/drivers/hwmon/w83627ehf.c
> > @@ -277,6 +277,11 @@ struct w83627ehf_data {
> > struct device *hwmon_dev;
> > struct mutex lock;
> >
> > + const u8 *REG_FAN_START_OUTPUT;
> > + const u8 *REG_FAN_STOP_OUTPUT;
> > + const u8 *REG_FAN_MAX_OUTPUT;
> > + const u8 *REG_FAN_STEP_OUTPUT;
> > +
> > struct mutex update_lock;
> > char valid; /* !=0 if following fields are valid */
> > unsigned long last_updated; /* In jiffies */
> > @@ -524,7 +529,10 @@ static struct w83627ehf_data *w83627ehf_update_device(struct device *dev)
> > }
> > }
> >
> > - for (i = 0; i < 4; i++) {
> > + for (i = 0; i < data->pwm_num; i++) {
> > + if (!(data->has_fan & (1 << i)))
> > + continue;
>
> I'm skeptical. data->has_fan refers to fan inputs, not fan outputs.
> There is no guarantee that pwm1 is mapped to fan1, etc., so you can't
> use data->has_fan to discard fan outputs.
>
Maybe, but they are treated as 1:1 for fan4 when creating the pwm files,
and the pwm files for fan4 are not created if the matching bit isn't set.
This applies to all chips supported by this driver. And for fan1..3, the
mapping is supposed to be 1:1 per the driver documentation.

I figured that it does not make sense to read the registers for a non-existing
attribute file.

> > +
> > /* pwmcfg, tolerance mapped for i=0, i=1 to same reg */
> > if (i != 1) {
> > pwmcfg = w83627ehf_read_value(data,
> > @@ -546,6 +554,17 @@ static struct w83627ehf_data *w83627ehf_update_device(struct device *dev)
> > W83627EHF_REG_FAN_STOP_OUTPUT[i]);
> > data->fan_stop_time[i] = w83627ehf_read_value(data,
> > W83627EHF_REG_FAN_STOP_TIME[i]);
> > +
> > + if (data->REG_FAN_MAX_OUTPUT[i] != 0xff)
> > + data->fan_max_output[i] =
> > + w83627ehf_read_value(data,
> > + data->REG_FAN_MAX_OUTPUT[i]);
> > +
> > + if (data->REG_FAN_STEP_OUTPUT[i] != 0xff)
> > + data->fan_step_output[i] =
> > + w83627ehf_read_value(data,
> > + data->REG_FAN_STEP_OUTPUT[i]);
> > +
>
> Huuu, wait a minute, So these values were supposedly exposed to
> user-space, but were never read from their respective registers? So
> this is a plain bug you're fixing?
>
This one, yes.

> Now I'm wondering, how useful is a feature that has been broken forever
> and nobody ever complained?
>
No idea, really.

> > data->target_temp[i] =
> > w83627ehf_read_value(data,
> > W83627EHF_REG_TARGET[i]) &
> > @@ -1126,7 +1145,7 @@ store_##reg(struct device *dev, struct device_attribute *attr, \
> > u32 val = SENSORS_LIMIT(simple_strtoul(buf, NULL, 10), 1, 255); \
> > mutex_lock(&data->update_lock); \
> > data->reg[nr] = val; \
> > - w83627ehf_write_value(data, W83627EHF_REG_##REG[nr], val); \
> > + w83627ehf_write_value(data, data->REG_##REG[nr], val); \
> > mutex_unlock(&data->update_lock); \
> > return count; \
> > }
> > @@ -1206,12 +1225,26 @@ static struct sensor_device_attribute sda_sf3_arrays[] = {
> > store_fan_stop_output, 1),
> > SENSOR_ATTR(pwm3_stop_output, S_IWUSR | S_IRUGO, show_fan_stop_output,
> > store_fan_stop_output, 2),
> > +};
> >
> > - /* pwm1 and pwm3 don't support max and step settings */
> > +
> > +/*
> > + * pwm1 and pwm3 don't support max and step settings on all chips.
> > + * Need to check support while generating/removing attribute files.
> > + */
> > +static struct sensor_device_attribute sda_sf3_max_step_arrays[] = {
> > + SENSOR_ATTR(pwm1_max_output, S_IWUSR | S_IRUGO, show_fan_max_output,
> > + store_fan_max_output, 0),
> > + SENSOR_ATTR(pwm1_step_output, S_IWUSR | S_IRUGO, show_fan_step_output,
> > + store_fan_step_output, 0),
> > SENSOR_ATTR(pwm2_max_output, S_IWUSR | S_IRUGO, show_fan_max_output,
> > store_fan_max_output, 1),
> > SENSOR_ATTR(pwm2_step_output, S_IWUSR | S_IRUGO, show_fan_step_output,
> > store_fan_step_output, 1),
> > + SENSOR_ATTR(pwm3_max_output, S_IWUSR | S_IRUGO, show_fan_max_output,
> > + store_fan_max_output, 2),
> > + SENSOR_ATTR(pwm3_step_output, S_IWUSR | S_IRUGO, show_fan_step_output,
> > + store_fan_step_output, 2),
> > };
> >
> > static ssize_t
> > @@ -1235,6 +1268,12 @@ static void w83627ehf_device_remove_files(struct device *dev)
> >
> > for (i = 0; i < ARRAY_SIZE(sda_sf3_arrays); i++)
> > device_remove_file(dev, &sda_sf3_arrays[i].dev_attr);
> > + for (i = 0; i < ARRAY_SIZE(sda_sf3_max_step_arrays); i++) {
> > + struct sensor_device_attribute *attr =
> > + &sda_sf3_max_step_arrays[i];
> > + if (data->REG_FAN_STEP_OUTPUT[attr->index] != 0xff)
> > + device_remove_file(dev, &attr->dev_attr);
> > + }
> > for (i = 0; i < ARRAY_SIZE(sda_sf3_arrays_fan4); i++)
> > device_remove_file(dev, &sda_sf3_arrays_fan4[i].dev_attr);
> > for (i = 0; i < data->in_num; i++) {
> > @@ -1352,6 +1391,11 @@ static int __devinit w83627ehf_probe(struct platform_device *pdev)
> > data->in6_skip = !data->temp3_disable;
> > }
> >
> > + data->REG_FAN_START_OUTPUT = W83627EHF_REG_FAN_START_OUTPUT;
> > + data->REG_FAN_STOP_OUTPUT = W83627EHF_REG_FAN_STOP_OUTPUT;
> > + data->REG_FAN_MAX_OUTPUT = W83627EHF_REG_FAN_MAX_OUTPUT;
> > + data->REG_FAN_STEP_OUTPUT = W83627EHF_REG_FAN_STEP_OUTPUT;
> > +
> > /* Initialize the chip */
> > w83627ehf_init_device(data);
> >
> > @@ -1440,6 +1484,15 @@ static int __devinit w83627ehf_probe(struct platform_device *pdev)
> > &sda_sf3_arrays[i].dev_attr)))
> > goto exit_remove;
> >
> > + for (i = 0; i < ARRAY_SIZE(sda_sf3_max_step_arrays); i++) {
> > + struct sensor_device_attribute *attr =
> > + &sda_sf3_max_step_arrays[i];
> > + if (data->REG_FAN_STEP_OUTPUT[attr->index] != 0xff) {
> > + err = device_create_file(dev, &attr->dev_attr);
> > + if (err)
> > + goto exit_remove;
> > + }
> > + }
> > /* if fan4 is enabled create the sf3 files for it */
> > if ((data->has_fan & (1 << 3)) && data->pwm_num >= 4)
> > for (i = 0; i < ARRAY_SIZE(sda_sf3_arrays_fan4); i++) {
>
>
> Nice cleanup overall.
>
Thanks

Guenter
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Jean Delvare on 5 Jul 2010 08:00
Hi Guenter,

On Sun, 4 Jul 2010 06:50:10 -0700, Guenter Roeck wrote:
> On Sun, Jul 04, 2010 at 07:24:42AM -0400, Jean Delvare wrote:
> > I'm skeptical. data->has_fan refers to fan inputs, not fan outputs.
> > There is no guarantee that pwm1 is mapped to fan1, etc., so you can't
> > use data->has_fan to discard fan outputs.
>
> Maybe, but they are treated as 1:1 for fan4 when creating the pwm files,
> and the pwm files for fan4 are not created if the matching bit isn't set.
> This applies to all chips supported by this driver. And for fan1..3, the
> mapping is supposed to be 1:1 per the driver documentation.

The driver documentation was written by ourselves, I wouldn't trust it ;)

Oh well, I hadn't noticed that sysfs file creation already assumes a
1:1 fan to pwm mapping. I am still unsure if it is correct, but it might
be good enough on practice, and your patch isn't changing it anyway, so
it's not the time to discuss it.

> I figured that it does not make sense to read the registers for a non-existing
> attribute file.

I agree that we want to skip register reads when possible (not that it
matters as much as when reading over slow SMBus though.)

I have no further objections to your patch, so I'll apply it now. Then
I'll publish the patches and update the stand-alone driver, and we can
call for testers.

--
Jean Delvare
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
 | 
Pages: 1
Prev: hwmon: Add support for W83667HG-B to w83627ehf driver
Next: omfs: fix memory leak