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

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

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.