Re: [PATCH v1 09/10] hwmon: adt7x10: use devm_hwmon_device_register_with_info

[Guenter Roeck <linux@xxxxxxxxxxxx>]

On 12/21/21 4:39 AM, Cosmin Tanislav wrote:
> From: Cosmin Tanislav <cosmin.tanislav@xxxxxxxxxx>
>
> To simplify core driver remove function.
>
> Signed-off-by: Cosmin Tanislav <cosmin.tanislav@xxxxxxxxxx>
> ---
>   drivers/hwmon/adt7x10.c | 12 +++---------
>   1 file changed, 3 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/hwmon/adt7x10.c b/drivers/hwmon/adt7x10.c
> index dd4901299590..c03805c72906 100644
> --- a/drivers/hwmon/adt7x10.c
> +++ b/drivers/hwmon/adt7x10.c
> @@ -54,7 +54,6 @@
>   /* Each client has this additional data */
>   struct adt7x10_data {
>   	const struct adt7x10_ops *ops;
> -	struct device		*hwmon_dev;
>   	struct device		*bus_dev;
>   	struct mutex		update_lock;
>   	u8			config;
> @@ -430,8 +429,8 @@ int adt7x10_probe(struct device *dev, const char *name, int irq,
>   	if (ret)
>   		goto exit_restore;
>   
> -	hdev = hwmon_device_register_with_info(dev, name, data,
> -					       &adt7x10_chip_info, NULL);
> +	hdev = devm_hwmon_device_register_with_info(dev, name, data,
> +						    &adt7x10_chip_info, NULL);
>   
>   	if (IS_ERR(hdev)) {
>   		ret = PTR_ERR(hdev);
> @@ -445,15 +444,11 @@ int adt7x10_probe(struct device *dev, const char *name, int irq,
>   						IRQF_ONESHOT,
>   						dev_name(dev), hdev);
>   		if (ret)
> -			goto exit_hwmon_device_unregister;
> +			goto exit_restore;
>   	}
>   
> -	data->hwmon_dev = hdev;
> -
>   	return 0;
>   
> -exit_hwmon_device_unregister:
> -	hwmon_device_unregister(hdev);
>   exit_restore:
>   	adt7x10_write_byte(dev, ADT7X10_CONFIG, data->oldconfig);
>   	return ret;
> @@ -464,7 +459,6 @@ void adt7x10_remove(struct device *dev, int irq)
>   {
>   	struct adt7x10_data *data = dev_get_drvdata(dev);
>   
> -	hwmon_device_unregister(data->hwmon_dev);
>   	if (data->oldconfig != data->config)
>   		adt7x10_write_byte(dev, ADT7X10_CONFIG, data->oldconfig);

This doesn't work as-is because the hwmon device still exists at this point
and at least in theory userspace could still write into the device
after the old configuration was restored.

To fix this, you'll need a preceding patch to introduce adt7x10_restore_config()
or similar, and call it through devm_add_action_or_reset() after updating the
chip configuration. You can then drop the restore code from here and from
the exist_restore code in the probe function.
After that, you can use devm_hwmon_device_register_with_info() and drop the
remove function entirely.

Guenter