[lm-sensors] [PATCH 0/5] acpi_power_meter: clean up duplicate code

[lm-sensors] [PATCH 0/5] acpi_power_meter: clean up duplicate code

[Kyle McMartin]

From: Kyle McMartin <kyle@mcmartin.ca>

acpi_power_meter currently duplicates a lot of code between the rw and
ro sensor templates unnecessarily. The attached five part series cleans
things up to use the same code path for both, deciding which is which by
the use of the .set field.

Additionally, (imho at least) it makes the struct initializers a bit
more pretty and uses a macro to reduce the repetition of each member.

It's been compile tested across the series for bisectability, but I do
not currently have access to any device which uses this driver for
testing.

regards, Kyle McMartin

Kyle McMartin (5):
  acpi_power_meter: use the same struct {rw,ro}_sensor_template for
    both
  acpi_power_meter: use a {RW,RO}_SENSOR_TEMPLATE macro to clean things
    up
  acpi_power_meter: remove duplicate code between
    register_{ro,rw}_attrs
  acpi_power_meter: drop meter_rw_attrs, use common meter_attrs
  acpi_power_meter: clean up code around setup_attrs

 drivers/hwmon/acpi_power_meter.c |  165 +++++++++++++++++---------------------
 1 files changed, 72 insertions(+), 93 deletions(-)

-- 
1.7.7.6


_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH 0/5] acpi_power_meter: clean up duplicate code

[Guenter Roeck]

On Mon, Apr 02, 2012 at 02:18:59PM -0400, Kyle McMartin wrote:
> From: Kyle McMartin <kyle@mcmartin.ca>
> 
> acpi_power_meter currently duplicates a lot of code between the rw and
> ro sensor templates unnecessarily. The attached five part series cleans
> things up to use the same code path for both, deciding which is which by
> the use of the .set field.
> 
> Additionally, (imho at least) it makes the struct initializers a bit
> more pretty and uses a macro to reduce the repetition of each member.
> 
> It's been compile tested across the series for bisectability, but I do
> not currently have access to any device which uses this driver for
> testing.
> 
> regards, Kyle McMartin
> 
> Kyle McMartin (5):
>   acpi_power_meter: use the same struct {rw,ro}_sensor_template for
>     both
>   acpi_power_meter: use a {RW,RO}_SENSOR_TEMPLATE macro to clean things
>     up
>   acpi_power_meter: remove duplicate code between
>     register_{ro,rw}_attrs
>   acpi_power_meter: drop meter_rw_attrs, use common meter_attrs
>   acpi_power_meter: clean up code around setup_attrs
> 
>  drivers/hwmon/acpi_power_meter.c |  165 +++++++++++++++++---------------------
>  1 files changed, 72 insertions(+), 93 deletions(-)
> 
Hi Kyle,

nice cleanup series.

Couple of comments / questions. That won't prevent me from applying the series, just some things
to think about.

- You could have used a single macro instead of two, with _set set to NULL for read-only attributes.
- Why not just use SENSOR_DEVICE_ATTR, struct attribute, and sysfs_create_group/sysfs_remove_group ?
- I think there may be a race condition in acpi_power_meter_notify(), in the METER_NOTIFY_CONFIG path.
  It frees some data, re-allocates it, then removes the attributes and re-creates them. If someone
  were to read the power1_model_number attribute after the call to free_capabilities, show_str()
  might potentially access freed memory.

Thanks,
Guenter

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH 0/5] acpi_power_meter: clean up duplicate code

[Kyle McMartin]

On Tue, Apr 03, 2012 at 08:39:18AM -0700, Guenter Roeck wrote:
> nice cleanup series.
> 
> Couple of comments / questions. That won't prevent me from applying the series, just some things
> to think about.
> 
> - You could have used a single macro instead of two, with _set set to NULL for read-only attributes.
> - Why not just use SENSOR_DEVICE_ATTR, struct attribute, and sysfs_create_group/sysfs_remove_group ?
> - I think there may be a race condition in acpi_power_meter_notify(), in the METER_NOTIFY_CONFIG path.
>   It frees some data, re-allocates it, then removes the attributes and re-creates them. If someone
>   were to read the power1_model_number attribute after the call to free_capabilities, show_str()
>   might potentially access freed memory.
> 

Thanks for the review, Guenter. I'll take a look at some of these
other fixes this week.

regards, Kyle

> Thanks,
> Guenter
> 

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH 0/5] acpi_power_meter: clean up duplicate code

[Guenter Roeck]

On Mon, 2012-04-02 at 14:18 -0400, Kyle McMartin wrote:
> From: Kyle McMartin <kyle@mcmartin.ca>
> 
> acpi_power_meter currently duplicates a lot of code between the rw and
> ro sensor templates unnecessarily. The attached five part series cleans
> things up to use the same code path for both, deciding which is which by
> the use of the .set field.
> 
> Additionally, (imho at least) it makes the struct initializers a bit
> more pretty and uses a macro to reduce the repetition of each member.
> 
> It's been compile tested across the series for bisectability, but I do
> not currently have access to any device which uses this driver for
> testing.
> 
> regards, Kyle McMartin
> 
> Kyle McMartin (5):
>   acpi_power_meter: use the same struct {rw,ro}_sensor_template for
>     both
>   acpi_power_meter: use a {RW,RO}_SENSOR_TEMPLATE macro to clean things
>     up
>   acpi_power_meter: remove duplicate code between
>     register_{ro,rw}_attrs
>   acpi_power_meter: drop meter_rw_attrs, use common meter_attrs
>   acpi_power_meter: clean up code around setup_attrs
> 
>  drivers/hwmon/acpi_power_meter.c |  165 +++++++++++++++++---------------------
>  1 files changed, 72 insertions(+), 93 deletions(-)
> 
Series applied to -next.

Thanks,
Guenter



_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors