Linux Power Management development
 help / color / mirror / Atom feed
* [PATCH] thermal/core: Split __thermal_cooling_device_register() into two functions
@ 2026-05-05 14:44 Daniel Lezcano
  2026-05-07  8:29 ` Daniel Lezcano
  2026-05-07 10:02 ` Lukasz Luba
  0 siblings, 2 replies; 8+ messages in thread
From: Daniel Lezcano @ 2026-05-05 14:44 UTC (permalink / raw)
  To: rafael; +Cc: rui.zhang, lukasz.luba, daniel.lezcano, linux-pm, linux-kernel

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)
+		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)
-		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;
 }
 
 /**
-- 
2.43.0


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH] thermal/core: Split __thermal_cooling_device_register() into two functions
  2026-05-05 14:44 [PATCH] thermal/core: Split __thermal_cooling_device_register() into two functions Daniel Lezcano
@ 2026-05-07  8:29 ` Daniel Lezcano
  2026-05-07 10:02 ` Lukasz Luba
  1 sibling, 0 replies; 8+ messages in thread
From: Daniel Lezcano @ 2026-05-07  8:29 UTC (permalink / raw)
  To: rafael; +Cc: rui.zhang, lukasz.luba, linux-pm, linux-kernel


Hi Rafael,

On 5/5/26 16: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>
> ---
Are you fine with this change ?

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] thermal/core: Split __thermal_cooling_device_register() into two functions
  2026-05-05 14:44 [PATCH] thermal/core: Split __thermal_cooling_device_register() into two functions Daniel Lezcano
  2026-05-07  8:29 ` Daniel Lezcano
@ 2026-05-07 10:02 ` Lukasz Luba
  2026-05-07 18:26   ` Daniel Lezcano
  1 sibling, 1 reply; 8+ messages in thread
From: Lukasz Luba @ 2026-05-07 10:02 UTC (permalink / raw)
  To: Daniel Lezcano; +Cc: rui.zhang, linux-pm, linux-kernel, rafael

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


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] thermal/core: Split __thermal_cooling_device_register() into two functions
  2026-05-07 10:02 ` Lukasz Luba
@ 2026-05-07 18:26   ` Daniel Lezcano
  2026-05-08 10:35     ` Lukasz Luba
  0 siblings, 1 reply; 8+ messages in thread
From: Daniel Lezcano @ 2026-05-07 18:26 UTC (permalink / raw)
  To: Lukasz Luba; +Cc: rui.zhang, linux-pm, linux-kernel, rafael

On 5/7/26 12:02, Lukasz Luba wrote:
> 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>
>> ---

[ ... ]

>> +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.

Hmm, I wondering if we can invert dev_set_name() and 
thermal_cooling_device_setup_sysfs() ?


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] thermal/core: Split __thermal_cooling_device_register() into two functions
  2026-05-07 18:26   ` Daniel Lezcano
@ 2026-05-08 10:35     ` Lukasz Luba
  2026-05-08 11:25       ` Daniel Lezcano
  0 siblings, 1 reply; 8+ messages in thread
From: Lukasz Luba @ 2026-05-08 10:35 UTC (permalink / raw)
  To: Daniel Lezcano; +Cc: rui.zhang, linux-pm, linux-kernel, rafael



On 5/7/26 19:26, Daniel Lezcano wrote:
> On 5/7/26 12:02, Lukasz Luba wrote:
>> 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>
>>> ---
> 
> [ ... ]
> 
>>> +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.
> 
> Hmm, I wondering if we can invert dev_set_name() and 
> thermal_cooling_device_setup_sysfs() ?
> 


I've checked that. It looks like the guarded 'if()'
in thermal_release which needs a dev_name() to clean-up the memory i
an issue in this case.

When this dev_set_name() fails, we won't go into the
clean up code to free cdev->type, ida, cdev...

I would call them explicitly in case of 'if(ret)'
error from dev_set_name()...
Then the rest could end in case of error with that new
'put_device()' logic at the bottom.
Would you agree?

A different option would be to refactor thermal_release()
and somehow recognize the cooling device not based on name.

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] thermal/core: Split __thermal_cooling_device_register() into two functions
  2026-05-08 10:35     ` Lukasz Luba
@ 2026-05-08 11:25       ` Daniel Lezcano
  2026-05-08 12:08         ` Rafael J. Wysocki
  0 siblings, 1 reply; 8+ messages in thread
From: Daniel Lezcano @ 2026-05-08 11:25 UTC (permalink / raw)
  To: Lukasz Luba; +Cc: rui.zhang, linux-pm, linux-kernel, rafael

On 5/8/26 12:35, Lukasz Luba wrote:
> 
> 
> On 5/7/26 19:26, Daniel Lezcano wrote:
>> On 5/7/26 12:02, Lukasz Luba wrote:

[ ... ]

> I would call them explicitly in case of 'if(ret)'
> error from dev_set_name()...
> Then the rest could end in case of error with that new
> 'put_device()' logic at the bottom.
> Would you agree?
> 
> A different option would be to refactor thermal_release()
> and somehow recognize the cooling device not based on name.

This mechanism is not adequate.

thermal_release should not be used this way, neither it should be needed.

We should have:

	thermal_class = class_create("thermal");
	if (IS_ERR(thermal_class))
		...

No thermal_release needed.


But,

static void cooling_dev_release(struct device *dev)
{
	thermal_cooling_device *cdev;

	cdev = to_cooling_device(dev);
	thermal_cooling_device_destroy_sysfs(cdev);
	kfree_const(cdev->type);
	ida_free(&thermal_cdev_ida, cdev->id);
	kfree(cdev);
}

static void tz_dev_release(struct device *dev)
{
	thermal_zone_device *tz;

	tz = to_thermal_zone(dev);
	thermal_zone_destroy_device_groups(tz);
	thermal_set_governor(tz, NULL);
	ida_destroy(&tz->ida);
	mutex_destroy(&tz->lock);
	complete(&tz->removal);
}

In thermal_cooling_device_add()
{
	...
	cdev->device.release = cooling_dev_release
	...
}


In thermal_zone_device_register_with_trips()
{
	...
	tz->device.release = tz_dev_release
	...
}


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] thermal/core: Split __thermal_cooling_device_register() into two functions
  2026-05-08 11:25       ` Daniel Lezcano
@ 2026-05-08 12:08         ` Rafael J. Wysocki
  2026-05-08 12:40           ` Daniel Lezcano
  0 siblings, 1 reply; 8+ messages in thread
From: Rafael J. Wysocki @ 2026-05-08 12:08 UTC (permalink / raw)
  To: Daniel Lezcano; +Cc: Lukasz Luba, rui.zhang, linux-pm, linux-kernel, rafael

On Fri, May 8, 2026 at 1:25 PM Daniel Lezcano
<daniel.lezcano@oss.qualcomm.com> wrote:
>
> On 5/8/26 12:35, Lukasz Luba wrote:
> >
> >
> > On 5/7/26 19:26, Daniel Lezcano wrote:
> >> On 5/7/26 12:02, Lukasz Luba wrote:
>
> [ ... ]
>
> > I would call them explicitly in case of 'if(ret)'
> > error from dev_set_name()...
> > Then the rest could end in case of error with that new
> > 'put_device()' logic at the bottom.
> > Would you agree?
> >
> > A different option would be to refactor thermal_release()
> > and somehow recognize the cooling device not based on name.
>
> This mechanism is not adequate.
>
> thermal_release should not be used this way, neither it should be needed.
>
> We should have:
>
>         thermal_class = class_create("thermal");
>         if (IS_ERR(thermal_class))
>                 ...
>
> No thermal_release needed.
>
>
> But,
>
> static void cooling_dev_release(struct device *dev)
> {
>         thermal_cooling_device *cdev;
>
>         cdev = to_cooling_device(dev);
>         thermal_cooling_device_destroy_sysfs(cdev);
>         kfree_const(cdev->type);
>         ida_free(&thermal_cdev_ida, cdev->id);
>         kfree(cdev);
> }
>
> static void tz_dev_release(struct device *dev)
> {
>         thermal_zone_device *tz;
>
>         tz = to_thermal_zone(dev);
>         thermal_zone_destroy_device_groups(tz);
>         thermal_set_governor(tz, NULL);
>         ida_destroy(&tz->ida);
>         mutex_destroy(&tz->lock);
>         complete(&tz->removal);
> }
>
> In thermal_cooling_device_add()
> {
>         ...
>         cdev->device.release = cooling_dev_release
>         ...
> }
>
>
> In thermal_zone_device_register_with_trips()
> {
>         ...
>         tz->device.release = tz_dev_release
>         ...
> }

Right, so why don't we clean that up to start with?

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] thermal/core: Split __thermal_cooling_device_register() into two functions
  2026-05-08 12:08         ` Rafael J. Wysocki
@ 2026-05-08 12:40           ` Daniel Lezcano
  0 siblings, 0 replies; 8+ messages in thread
From: Daniel Lezcano @ 2026-05-08 12:40 UTC (permalink / raw)
  To: Rafael J. Wysocki; +Cc: Lukasz Luba, rui.zhang, linux-pm, linux-kernel

On 5/8/26 14:08, Rafael J. Wysocki wrote:
> On Fri, May 8, 2026 at 1:25 PM Daniel Lezcano
> <daniel.lezcano@oss.qualcomm.com> wrote:

[ ... ]

>> In thermal_zone_device_register_with_trips()
>> {
>>          ...
>>          tz->device.release = tz_dev_release
>>          ...
>> }
> 
> Right, so why don't we clean that up to start with?

On the way ... :)

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2026-05-08 12:40 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-05-05 14:44 [PATCH] thermal/core: Split __thermal_cooling_device_register() into two functions Daniel Lezcano
2026-05-07  8:29 ` Daniel Lezcano
2026-05-07 10:02 ` Lukasz Luba
2026-05-07 18:26   ` Daniel Lezcano
2026-05-08 10:35     ` Lukasz Luba
2026-05-08 11:25       ` Daniel Lezcano
2026-05-08 12:08         ` Rafael J. Wysocki
2026-05-08 12:40           ` Daniel Lezcano

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox