Re:Re: [PATCH] thermal/core: Fix refcount bugs in __thermal_cooling_device_register()

["Liang He" <windhl@126.com>]

At 2022-07-17 06:03:46, "Daniel Lezcano" <daniel.lezcano@linaro.org> wrote:
>On 15/07/2022 19:14, Rafael J. Wysocki wrote:
>> On Thu, Jul 7, 2022 at 8:21 AM Liang He <windhl@126.com> wrote:
>
>[ ... ]
>
>>> -       cdev->np = np;
>>> +       cdev->np = of_node_get(np);
>>>          cdev->ops = ops;
>>>          cdev->updated = false;
>>>          cdev->device.class = &thermal_class;
>>> @@ -947,6 +947,7 @@ __thermal_cooling_device_register(struct device_node *np,
>>>          return cdev;
>>>
>>>   out_kfree_type:
>>> +       of_node_put(cdev->np);
>>>          thermal_cooling_device_destroy_sysfs(cdev);
>>>          kfree(cdev->type);
>>>          put_device(&cdev->device);
>>> @@ -1111,6 +1112,7 @@ void thermal_cooling_device_unregister(struct thermal_cooling_device *cdev)
>>>
>>>          mutex_unlock(&thermal_list_lock);
>>>
>>> +       of_node_put(cdev->np);
>> 
>> Could this be done right before the
>> thermal_cooling_device_destroy_sysfs() below?  Then the sequence would
>> be completely analogous to the error code path above.
>> 
>>>          ida_simple_remove(&thermal_cdev_ida, cdev->id);
>>>          device_del(&cdev->device);
>>>          thermal_cooling_device_destroy_sysfs(cdev);
>>> --
>> 
>> Overall, this looks like a genuine fix to me.
>> 
>> Daniel, what do you think?
>
>Yes, the of_node_put() is often missing when there is the for_each_xxx 
>OF API. But here the cdev->np is only used to compare pointers so used 
>as an identifier, not de-referenced just comparing the addresses.

>

Thanks, this is a good lesson that explains when there is no need
to refcount new reference.
So I think there is also no need to patch anything, right?

Thanks again,

Liang

>It is used to bind a thermal zone with a cooling device:
>
>thermal_of.c:                   if (tcbp->cooling_device == cdev->np) {
>thermal_of.c:                   if (tcbp->cooling_device == cdev->np) {
>
>That is probably why no refcount was taken on this device node.
>
>Moreover, this will go away with the series reworking the thermal-of I 
>sent last week

>
>>
>-- 
><http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
>
>Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
><http://twitter.com/#!/linaroorg> Twitter |
><http://www.linaro.org/linaro-blog/> Blog