Re: [PATCH v3] thermal/core: fix error paths in __thermal_cooling_device_register()

[Caleb Connolly <caleb.connolly@linaro.org>]

On 17/01/2023 13:39, Rafael J. Wysocki wrote:
> On Tue, Jan 17, 2023 at 5:36 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
>>
>> On 16-01-23, 20:38, Yang Yingliang wrote:
>>> The 'cdev' will be freed after calling put_device(), it can not be accessed
>>> anymore.
>>
>> I surely missed the class's release callback :(
>>
>> How about this then, it clears this in a better way, i.e. what entity
>> is responsible for what exactly. This can be divided in a few patches
>> for sure.
> 
> It looks good to me, but then I haven't caught the release bug too.

it's a fun one:

https://lore.kernel.org/linux-pm/20230101174342.58351-1-caleb.connolly@linaro.org/

I don't see any issues with this suggested patch, however I don't think
I could comfortably attach my SoB to it given the larger code reordering
and my complete lack of experience with this subsystem.

Would a Tested-by be acceptable?
> 
>> diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
>> index f17ab2316dbd..5555bf827a0f 100644
>> --- a/drivers/thermal/thermal_core.c
>> +++ b/drivers/thermal/thermal_core.c
>> @@ -774,6 +774,9 @@ static void thermal_release(struct device *dev)
>>         } else if (!strncmp(dev_name(dev), "cooling_device",
>>                             sizeof("cooling_device") - 1)) {
>>                 cdev = to_cooling_device(dev);
>> +               thermal_cooling_device_destroy_sysfs(cdev);
>> +               kfree(cdev->type);
>> +               ida_free(&thermal_cdev_ida, cdev->id);
>>                 kfree(cdev);
>>         }
>>  }
>> @@ -910,17 +913,18 @@ __thermal_cooling_device_register(struct device_node *np,
>>
>>         ret = cdev->ops->get_max_state(cdev, &cdev->max_state);
>>         if (ret)
>> -               goto out_kfree_type;
>> +               goto out_cdev_type;
>>
>>         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_cooling_device_destroy_sysfs(cdev);
>> -               goto out_kfree_type;
>> +               /* thermal_release() handles rest of the cleanup */
>> +               put_device(&cdev->device);
>> +               return ret;
>>         }
>> -       ret = device_register(&cdev->device);
>> -       if (ret)
>> -               goto out_kfree_type;
>>
>>         /* Add 'this' new cdev to the global cdev list */
>>         mutex_lock(&thermal_list_lock);
>> @@ -939,11 +943,10 @@ __thermal_cooling_device_register(struct device_node *np,
>>
>>         return cdev;
>>
>> -out_kfree_type:
>> +out_cooling_dev:
>>         thermal_cooling_device_destroy_sysfs(cdev);
>> +out_cdev_type:
>>         kfree(cdev->type);
>> -       put_device(&cdev->device);
>> -       cdev = NULL;
>>  out_ida_remove:
>>         ida_free(&thermal_cdev_ida, id);
>>  out_kfree_cdev:
>> @@ -1104,11 +1107,7 @@ void thermal_cooling_device_unregister(struct thermal_cooling_device *cdev)
>>
>>         mutex_unlock(&thermal_list_lock);
>>
>> -       ida_free(&thermal_cdev_ida, cdev->id);
>> -       device_del(&cdev->device);
>> -       thermal_cooling_device_destroy_sysfs(cdev);
>> -       kfree(cdev->type);
>> -       put_device(&cdev->device);
>> +       device_unregister(&cdev->device);
>>  }
>>  EXPORT_SYMBOL_GPL(thermal_cooling_device_unregister);
>>
>> --
>> viresh

-- 
Kind Regards,
Caleb (they/them)