Re: [PATCH] lm87: Use hwmon to create the sysfs groups

[Jason Gunthorpe <jgunthorpe@obsidianresearch.com>]

On Tue, Oct 25, 2016 at 06:50:01PM -0700, Guenter Roeck wrote:
> >+
> >+	const struct attribute_group *attr_groups[5];
> 
> There can be up to 5 active groups total, so this array needs to have
> 6 entries (it needs to be NULL_terminated).

Agh right, I forgot about that. I will send a v2

> > {
> > 	struct lm87_data *data = i2c_get_clientdata(client);
> >
> >-	hwmon_device_unregister(data->hwmon_dev);
> >-	lm87_remove_files(client);
> >-
> > 	lm87_write_value(client, LM87_REG_CONFIG, data->config);
> 
> Strictly speaking this is now racy: The sysfs attributes still exist here,
> meaning the chip can still be accessed. Consider using devm_add_action();
> then you can drop the remove function entirely, and simplify error cleanup
> in the probe function.

I guess, but that is very obscure. If you unload the driver while
monitoring it with sysfs and the boot firmware left the chip in the
low power state then a sysfs right might race and return garbage..

I can switch it to use hwmon_device_register_with_groups and put the
hwmon_device_unregister back - I'm not sure wrapping the write in devm
would be a simplification..

Thanks,
Jason