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

[Lukasz Luba]

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@xxxxxxxxxx>
> > > Signed-off-by: Daniel Lezcano <daniel.lezcano@xxxxxxxxxxxxxxxx>
> > > ---
>
> [ ... ]
>
> > > +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.