On Mon, Apr 02, 2012 at 02:18:59PM -0400, Kyle McMartin wrote:
> From: Kyle McMartin <kyle@xxxxxxxxxxx>
>
> 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@xxxxxxxxxxxxxx
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
