Re: [PATCH] thermal/core: Split __thermal_cooling_device_register() into two functions

[Lukasz Luba <lukasz.luba@arm.com>]

Hi Daniel,

On 5/5/26 15:44, Daniel Lezcano wrote:
> In preparation for the upcoming changes separating OF and non-OF code,
> split __thermal_cooling_device_register() into allocation and addition
> phases.
> 
> This allows moving the device node assignment out of the core
> initialization path.
> 
> This change is not a trivial split. The lifetime of the cooling device
> is managed by the device core through put_device(), which triggers
> thermal_release() to free all associated resources.
> 
> With the introduction of thermal_cooling_device_alloc(), the allocation
> path must mirror what thermal_release() undoes. In contrast,
> thermal_cooling_device_add() must not perform any rollback and relies
> on put_device() for cleanup on error paths. This avoids both double
> free and resource leaks.
> 
> As part of this rework, add the missing device_initialize() call when
> allocating the cooling device.
> 
> Suggested-by: Rafael J. Wysocki <rafael@kernel.org>
> Signed-off-by: Daniel Lezcano <daniel.lezcano@oss.qualcomm.com>
> ---
>   drivers/thermal/thermal_core.c | 117 ++++++++++++++++++++-------------
>   1 file changed, 73 insertions(+), 44 deletions(-)
> 
> diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
> index 664a4cc95199..fd4c61197d1e 100644
> --- a/drivers/thermal/thermal_core.c
> +++ b/drivers/thermal/thermal_core.c
> @@ -1040,29 +1040,10 @@ static void thermal_cooling_device_init_complete(struct thermal_cooling_device *
>   		thermal_zone_cdev_bind(tz, cdev);
>   }
>   
> -/**
> - * __thermal_cooling_device_register() - register a new thermal cooling device
> - * @np:		a pointer to a device tree node.
> - * @type:	the thermal cooling device type.
> - * @devdata:	device private data.
> - * @ops:	standard thermal cooling devices callbacks.
> - *
> - * This interface function adds a new thermal cooling device (fan/processor/...)
> - * to /sys/class/thermal/ folder as cooling_device[0-*]. It tries to bind itself
> - * to all the thermal zone devices registered at the same time.
> - * It also gives the opportunity to link the cooling device to a device tree
> - * node, so that it can be bound to a thermal zone created out of device tree.
> - *
> - * Return: a pointer to the created struct thermal_cooling_device or an
> - * ERR_PTR. Caller must check return value with IS_ERR*() helpers.
> - */
>   static struct thermal_cooling_device *
> -__thermal_cooling_device_register(struct device_node *np,
> -				  const char *type, void *devdata,
> -				  const struct thermal_cooling_device_ops *ops)
> +thermal_cooling_device_alloc(const char *type, const struct thermal_cooling_device_ops *ops)
>   {
>   	struct thermal_cooling_device *cdev;
> -	unsigned long current_state;
>   	int ret;
>   
>   	if (!ops || !ops->get_max_state || !ops->get_cur_state ||
> @@ -1076,6 +1057,8 @@ __thermal_cooling_device_register(struct device_node *np,
>   	if (!cdev)
>   		return ERR_PTR(-ENOMEM);
>   
> +	cdev->ops = ops;
> +
>   	ret = ida_alloc(&thermal_cdev_ida, GFP_KERNEL);
>   	if (ret < 0)
>   		goto out_kfree_cdev;
> @@ -1087,17 +1070,36 @@ __thermal_cooling_device_register(struct device_node *np,
>   		goto out_ida_remove;
>   	}
>   
> +	return cdev;
> +
> +out_ida_remove:
> +	ida_free(&thermal_cdev_ida, cdev->id);
> +out_kfree_cdev:
> +	kfree(cdev);
> +	return ERR_PTR(ret);
> +}
> +
> +static int thermal_cooling_device_add(struct thermal_cooling_device *cdev, void *devdata)
> +{
> +	unsigned long current_state;
> +	int ret;
> +
>   	mutex_init(&cdev->lock);
>   	INIT_LIST_HEAD(&cdev->thermal_instances);
> -	cdev->np = np;
> -	cdev->ops = ops;
>   	cdev->updated = false;
>   	cdev->device.class = &thermal_class;
> +	device_initialize(&cdev->device);
>   	cdev->devdata = devdata;
>   
> +	thermal_cooling_device_setup_sysfs(cdev);
> +
> +	ret = dev_set_name(&cdev->device, "cooling_device%d", cdev->id);
> +	if (ret)

In case of error here, when the cdev->device won't have this name,
would thermal_release() still be able to call the cleanup
for the sysfs bits?
IMHO the check 'if()' there might bite us and we might not free
the sysfs allocated memory.


> +		goto out_put_device;
> +
>   	ret = cdev->ops->get_max_state(cdev, &cdev->max_state);
>   	if (ret)
> -		goto out_cdev_type;
> +		goto out_put_device;
>   
>   	/*
>   	 * The cooling device's current state is only needed for debug
> @@ -1111,35 +1113,62 @@ __thermal_cooling_device_register(struct device_node *np,
>   	if (ret)
>   		current_state = ULONG_MAX;
>   
> -	thermal_cooling_device_setup_sysfs(cdev);
> -
> -	ret = dev_set_name(&cdev->device, "cooling_device%d", cdev->id);
> -	if (ret)

In this current code case, we explicitly call the
thermal_cooling_device_destroy_sysfs() which will definitely clean-up
all stuff.


> -		goto out_cooling_dev;
> -
>   	ret = device_register(&cdev->device);
> -	if (ret) {
> -		/* thermal_release() handles rest of the cleanup */
> -		put_device(&cdev->device);
> -		return ERR_PTR(ret);
> -	}
> +	if (ret)
> +		goto out_put_device;
>   
>   	if (current_state <= cdev->max_state)
>   		thermal_debug_cdev_add(cdev, current_state);
>   
>   	thermal_cooling_device_init_complete(cdev);
>   
> -	return cdev;
> +	return 0;
>   
> -out_cooling_dev:
> -	thermal_cooling_device_destroy_sysfs(cdev);
> -out_cdev_type:
> -	kfree_const(cdev->type);
> -out_ida_remove:
> -	ida_free(&thermal_cdev_ida, cdev->id);
> -out_kfree_cdev:
> -	kfree(cdev);
> -	return ERR_PTR(ret);
> +out_put_device:
> +	/*
> +	 * The device core will release the memory via
> +	 * thermal_release() after put_device() is called in the error
> +	 * path
> +	 */
> +	put_device(&cdev->device);
> +	return ret;
> +}
> +
> +/**
> + * __thermal_cooling_device_register() - register a new thermal cooling device
> + * @np:		a pointer to a device tree node.
> + * @type:	the thermal cooling device type.
> + * @devdata:	device private data.
> + * @ops:	standard thermal cooling devices callbacks.
> + *
> + * This interface function adds a new thermal cooling device (fan/processor/...)
> + * to /sys/class/thermal/ folder as cooling_device[0-*]. It tries to bind itself
> + * to all the thermal zone devices registered at the same time.
> + * It also gives the opportunity to link the cooling device to a device tree
> + * node, so that it can be bound to a thermal zone created out of device tree.
> + *
> + * Return: a pointer to the created struct thermal_cooling_device or an
> + * ERR_PTR. Caller must check return value with IS_ERR*() helpers.
> + */
> +static struct thermal_cooling_device *
> +__thermal_cooling_device_register(struct device_node *np,
> +				  const char *type, void *devdata,
> +				  const struct thermal_cooling_device_ops *ops)
> +{
> +	struct thermal_cooling_device *cdev;
> +	int ret;
> +
> +	cdev = thermal_cooling_device_alloc(type, ops);
> +	if (IS_ERR(cdev))
> +		return cdev;
> +
> +	cdev->np = np;
> +
> +	ret = thermal_cooling_device_add(cdev, devdata);
> +	if (ret)
> +		return ERR_PTR(ret);
> +
> +	return cdev;
>   }
>   
>   /**

The rest looks OK

Regards,
Lukasz