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

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

[Caleb Connolly]

There is in invalid call to thermal_cooling_device_destroy_sysfs() and
another to put_device() in the error paths here. Fix them.

Fixes: c408b3d1d9bb ("thermal: Validate new state in cur_state_store()")
Signed-off-by: Caleb Connolly <caleb.connolly@linaro.org>
---
Changes since v2:
 * Rework and simplify into one patch following Yang's suggestions.

V2: https://lore.kernel.org/all/20230103171811.204196-1-caleb.connolly@linaro.org/
---
 drivers/thermal/thermal_core.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
index f17ab2316dbd..321d2a6300c4 100644
--- a/drivers/thermal/thermal_core.c
+++ b/drivers/thermal/thermal_core.c
@@ -909,15 +909,16 @@ __thermal_cooling_device_register(struct device_node *np,
 	cdev->devdata = devdata;
 
 	ret = cdev->ops->get_max_state(cdev, &cdev->max_state);
-	if (ret)
-		goto out_kfree_type;
+	if (ret) {
+		kfree(cdev->type);
+		goto out_ida_remove;
+	}
 
 	thermal_cooling_device_setup_sysfs(cdev);
 	ret = dev_set_name(&cdev->device, "cooling_device%d", cdev->id);
-	if (ret) {
-		thermal_cooling_device_destroy_sysfs(cdev);
+	if (ret)
 		goto out_kfree_type;
-	}
+
 	ret = device_register(&cdev->device);
 	if (ret)
 		goto out_kfree_type;
-- 
2.39.0

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

[Luca Weiss]

On Donnerstag, 12. Jänner 2023 16:47:20 CET Caleb Connolly wrote:
> There is in invalid call to thermal_cooling_device_destroy_sysfs() and
> another to put_device() in the error paths here. Fix them.
> 
> Fixes: c408b3d1d9bb ("thermal: Validate new state in cur_state_store()")
> Signed-off-by: Caleb Connolly <caleb.connolly@linaro.org>

Fixes attached warnings on boot on qcom-apq8026-lg-lenok (with smbb driver)

Tested-by: Luca Weiss <luca@z3ntu.xyz>

[    2.095494] qcom-smbb fc4cf000.spmi:pm8226@0:charger@1000: Initializing SMBB rev 1
[    2.097362] ------------[ cut here ]------------
[    2.103582] WARNING: CPU: 0 PID: 1 at lib/kobject.c:718 kobject_put+0xcc/0x130
[    2.108393] kobject: '(null)' ((ptrval)): is not initialized, yet kobject_put() is being called.
[    2.115406] Modules linked in:
[    2.124340] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 6.2.0-rc3-00011-gd6a0fe8a370f #229
[    2.127151] Hardware name: Generic DT based system
[    2.135394]  unwind_backtrace from show_stack+0x10/0x14
[    2.139988]  show_stack from dump_stack_lvl+0x40/0x4c
[    2.145111]  dump_stack_lvl from __warn+0x78/0x158
[    2.150317]  __warn from warn_slowpath_fmt+0x98/0xc8
[    2.155006]  warn_slowpath_fmt from kobject_put+0xcc/0x130
[    2.160128]  kobject_put from __thermal_cooling_device_register.part.0+0xf8/0x388
[    2.165432]  __thermal_cooling_device_register.part.0 from __power_supply_register+0x43c/0x534
[    2.172986]  __power_supply_register from devm_power_supply_register+0x54/0x90
[    2.181489]  devm_power_supply_register from smbb_charger_probe+0x278/0x614
[    2.188690]  smbb_charger_probe from platform_probe+0x5c/0xb0
[    2.195546]  platform_probe from really_probe+0xc8/0x2ec
[    2.201447]  really_probe from __driver_probe_device+0x84/0xe4
[    2.206830]  __driver_probe_device from driver_probe_device+0x30/0x104
[    2.212474]  driver_probe_device from __driver_attach+0x90/0x174
[    2.218983]  __driver_attach from bus_for_each_dev+0x7c/0xc4
[    2.225145]  bus_for_each_dev from bus_add_driver+0x164/0x1f0
[    2.230789]  bus_add_driver from driver_register+0x88/0x11c
[    2.236429]  driver_register from do_one_initcall+0x5c/0x274
[    2.241812]  do_one_initcall from kernel_init_freeable+0x1a8/0x204
[    2.247717]  kernel_init_freeable from kernel_init+0x18/0x130
[    2.253704]  kernel_init from ret_from_fork+0x14/0x2c
[    2.259517] Exception stack(0xd0815fb0 to 0xd0815ff8)
[    2.264555] 5fa0:                                     00000000 00000000 00000000 00000000
[    2.269604] 5fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
[    2.277762] 5fe0: 00000000 00000000 00000000 00000000 00000013 00000000
[    2.286024] ---[ end trace 0000000000000000 ]---
[    2.292369] ------------[ cut here ]------------
[    2.297253] WARNING: CPU: 0 PID: 1 at lib/refcount.c:28 __thermal_cooling_device_register.part.0+0xf8/0x388
[    2.301844] refcount_t: underflow; use-after-free.
[    2.311337] Modules linked in:
[    2.316178] CPU: 0 PID: 1 Comm: swapper/0 Tainted: G        W          6.2.0-rc3-00011-gd6a0fe8a370f #229
[    2.319168] Hardware name: Generic DT based system
[    2.328789]  unwind_backtrace from show_stack+0x10/0x14
[    2.333478]  show_stack from dump_stack_lvl+0x40/0x4c
[    2.338598]  dump_stack_lvl from __warn+0x78/0x158
[    2.343806]  __warn from warn_slowpath_fmt+0x98/0xc8
[    2.348494]  warn_slowpath_fmt from __thermal_cooling_device_register.part.0+0xf8/0x388
[    2.353626]  __thermal_cooling_device_register.part.0 from __power_supply_register+0x43c/0x534
[    2.361354]  __power_supply_register from devm_power_supply_register+0x54/0x90
[    2.370030]  devm_power_supply_register from smbb_charger_probe+0x278/0x614
[    2.377231]  smbb_charger_probe from platform_probe+0x5c/0xb0
[    2.384086]  platform_probe from really_probe+0xc8/0x2ec
[    2.389987]  really_probe from __driver_probe_device+0x84/0xe4
[    2.395371]  __driver_probe_device from driver_probe_device+0x30/0x104
[    2.401015]  driver_probe_device from __driver_attach+0x90/0x174
[    2.407524]  __driver_attach from bus_for_each_dev+0x7c/0xc4
[    2.413686]  bus_for_each_dev from bus_add_driver+0x164/0x1f0
[    2.419329]  bus_add_driver from driver_register+0x88/0x11c
[    2.424971]  driver_register from do_one_initcall+0x5c/0x274
[    2.430353]  do_one_initcall from kernel_init_freeable+0x1a8/0x204
[    2.436257]  kernel_init_freeable from kernel_init+0x18/0x130
[    2.442245]  kernel_init from ret_from_fork+0x14/0x2c
[    2.448059] Exception stack(0xd0815fb0 to 0xd0815ff8)
[    2.453097] 5fa0:                                     00000000 00000000 00000000 00000000
[    2.458145] 5fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
[    2.466303] 5fe0: 00000000 00000000 00000000 00000000 00000013 00000000
[    2.474560] ---[ end trace 0000000000000000 ]---
[    2.481478] qcom-smbb fc4cf000.spmi:pm8226@0:charger@1000: failed to register USB power supply
[    2.485838] qcom-smbb: probe of fc4cf000.spmi:pm8226@0:charger@1000 failed with error -11

Regards
Luca

> ---
> Changes since v2:
>  * Rework and simplify into one patch following Yang's suggestions.
> 
> V2:
> https://lore.kernel.org/all/20230103171811.204196-1-caleb.connolly@linaro.o
> rg/ ---
>  drivers/thermal/thermal_core.c | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
> index f17ab2316dbd..321d2a6300c4 100644
> --- a/drivers/thermal/thermal_core.c
> +++ b/drivers/thermal/thermal_core.c
> @@ -909,15 +909,16 @@ __thermal_cooling_device_register(struct device_node
> *np, cdev->devdata = devdata;
> 
>  	ret = cdev->ops->get_max_state(cdev, &cdev->max_state);
> -	if (ret)
> -		goto out_kfree_type;
> +	if (ret) {
> +		kfree(cdev->type);
> +		goto out_ida_remove;
> +	}
> 
>  	thermal_cooling_device_setup_sysfs(cdev);
>  	ret = dev_set_name(&cdev->device, "cooling_device%d", cdev->id);
> -	if (ret) {
> -		thermal_cooling_device_destroy_sysfs(cdev);
> +	if (ret)
>  		goto out_kfree_type;
> -	}
> +
>  	ret = device_register(&cdev->device);
>  	if (ret)
>  		goto out_kfree_type;

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

[Viresh Kumar]

On 12-01-23, 15:47, Caleb Connolly wrote:
> There is in invalid call to thermal_cooling_device_destroy_sysfs() and
> another to put_device() in the error paths here. Fix them.
> 
> Fixes: c408b3d1d9bb ("thermal: Validate new state in cur_state_store()")
> Signed-off-by: Caleb Connolly <caleb.connolly@linaro.org>
> ---
> Changes since v2:
>  * Rework and simplify into one patch following Yang's suggestions.
> 
> V2: https://lore.kernel.org/all/20230103171811.204196-1-caleb.connolly@linaro.org/
> ---
>  drivers/thermal/thermal_core.c | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)

Replied to an earlier version by mistake. Should it be like this ?

diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
index f17ab2316dbd..18db011d4d68 100644
--- a/drivers/thermal/thermal_core.c
+++ b/drivers/thermal/thermal_core.c
@@ -910,17 +910,15 @@ __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) {
-               thermal_cooling_device_destroy_sysfs(cdev);
-               goto out_kfree_type;
-       }
+       if (ret)
+               goto out_cooling_dev;
        ret = device_register(&cdev->device);
        if (ret)
-               goto out_kfree_type;
+               goto out_put_device;

        /* Add 'this' new cdev to the global cdev list */
        mutex_lock(&thermal_list_lock);
@@ -939,11 +937,12 @@ __thermal_cooling_device_register(struct device_node *np,

        return cdev;

-out_kfree_type:
+out_put_device:
+       put_device(&cdev->device);
+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:

-- 
viresh

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

[Caleb Connolly]

On 16/01/2023 04:23, Viresh Kumar wrote:
> On 12-01-23, 15:47, Caleb Connolly wrote:
>> There is in invalid call to thermal_cooling_device_destroy_sysfs() and
>> another to put_device() in the error paths here. Fix them.
>>
>> Fixes: c408b3d1d9bb ("thermal: Validate new state in cur_state_store()")
>> Signed-off-by: Caleb Connolly <caleb.connolly@linaro.org>
>> ---
>> Changes since v2:
>>  * Rework and simplify into one patch following Yang's suggestions.
>>
>> V2: https://lore.kernel.org/all/20230103171811.204196-1-caleb.connolly@linaro.org/
>> ---
>>  drivers/thermal/thermal_core.c | 11 ++++++-----
>>  1 file changed, 6 insertions(+), 5 deletions(-)
> 
> Replied to an earlier version by mistake. Should it be like this ?

apologies for all the subject rewording.

I'm not 100% clear on the benefits/drawbacks to the different
approaches, am I right in thinking the improvement here (compared to my
patch) is in giving each error condition it's own path so there isn't
any ambiguity?
> 
> diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
> index f17ab2316dbd..18db011d4d68 100644
> --- a/drivers/thermal/thermal_core.c
> +++ b/drivers/thermal/thermal_core.c
> @@ -910,17 +910,15 @@ __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) {
> -               thermal_cooling_device_destroy_sysfs(cdev);
> -               goto out_kfree_type;
> -       }
> +       if (ret)
> +               goto out_cooling_dev;
>         ret = device_register(&cdev->device);
>         if (ret)
> -               goto out_kfree_type;
> +               goto out_put_device;
> 
>         /* Add 'this' new cdev to the global cdev list */
>         mutex_lock(&thermal_list_lock);
> @@ -939,11 +937,12 @@ __thermal_cooling_device_register(struct device_node *np,
> 
>         return cdev;
> 
> -out_kfree_type:
> +out_put_device:
> +       put_device(&cdev->device);
> +out_cooling_dev:
>         thermal_cooling_device_destroy_sysfs(cdev);
> +out_cdev_type:
As far as I can tell this is give or take the same as the two patches I
had in v2 [1]?
>         kfree(cdev->type);
> -       put_device(&cdev->device);
> -       cdev = NULL;
Is removing the explicit null assignment here intentional?

>  out_ida_remove:
>         ida_free(&thermal_cdev_ida, id);
>  out_kfree_cdev:
> 

I'm happy to go ahead an submit a v4 based on this.

[1]:
https://lore.kernel.org/all/20230103171811.204196-1-caleb.connolly@linaro.org/

-- 
Kind Regards,
Caleb (they/them)

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

[Viresh Kumar]

On 16-01-23, 12:22, Caleb Connolly wrote:
> I'm not 100% clear on the benefits/drawbacks to the different
> approaches, am I right in thinking the improvement here (compared to my
> patch) is in giving each error condition it's own path so there isn't
> any ambiguity?

There was still confusion about what should be called when, like
device_put() can't be called with just dev_set_name(), etc. Also I
made a mistake which Yang mentioned,

I have proposed a different way of fixing this all for once, lets see
how that goes.

-- 
viresh

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

[Yang Yingliang]

On 2023/1/16 12:23, Viresh Kumar wrote:
> On 12-01-23, 15:47, Caleb Connolly wrote:
>> There is in invalid call to thermal_cooling_device_destroy_sysfs() and
>> another to put_device() in the error paths here. Fix them.
>>
>> Fixes: c408b3d1d9bb ("thermal: Validate new state in cur_state_store()")
>> Signed-off-by: Caleb Connolly <caleb.connolly@linaro.org>
>> ---
>> Changes since v2:
>>   * Rework and simplify into one patch following Yang's suggestions.
>>
>> V2: https://lore.kernel.org/all/20230103171811.204196-1-caleb.connolly@linaro.org/
>> ---
>>   drivers/thermal/thermal_core.c | 11 ++++++-----
>>   1 file changed, 6 insertions(+), 5 deletions(-)
> Replied to an earlier version by mistake. Should it be like this ?
>
> diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
> index f17ab2316dbd..18db011d4d68 100644
> --- a/drivers/thermal/thermal_core.c
> +++ b/drivers/thermal/thermal_core.c
> @@ -910,17 +910,15 @@ __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) {
> -               thermal_cooling_device_destroy_sysfs(cdev);
> -               goto out_kfree_type;
> -       }
> +       if (ret)
> +               goto out_cooling_dev;
>          ret = device_register(&cdev->device);
>          if (ret)
> -               goto out_kfree_type;
> +               goto out_put_device;
>
>          /* Add 'this' new cdev to the global cdev list */
>          mutex_lock(&thermal_list_lock);
> @@ -939,11 +937,12 @@ __thermal_cooling_device_register(struct device_node *np,
>
>          return cdev;
>
> -out_kfree_type:
> +out_put_device:
> +       put_device(&cdev->device);
> +out_cooling_dev:
>          thermal_cooling_device_destroy_sysfs(cdev);
> +out_cdev_type:
>          kfree(cdev->type);
The 'cdev' will be freed after calling put_device(), it can not be 
accessed anymore.

Thanks,
Yang
> -       put_device(&cdev->device);
> -       cdev = NULL;
>   out_ida_remove:
>          ida_free(&thermal_cdev_ida, id);
>   out_kfree_cdev:
>

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

[Viresh Kumar]

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.

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

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

[Rafael J. Wysocki]

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.

> 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

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

[Caleb Connolly]

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)

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

[Viresh Kumar]

On 17-01-23, 18:11, Caleb Connolly wrote:
> 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?

Sure, lemme send the patch(es) formally then.

-- 
viresh

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

[Yang Yingliang]

On 2023/1/12 23:47, Caleb Connolly wrote:
> There is in invalid call to thermal_cooling_device_destroy_sysfs() and
> another to put_device() in the error paths here. Fix them.
>
> Fixes: c408b3d1d9bb ("thermal: Validate new state in cur_state_store()")
> Signed-off-by: Caleb Connolly <caleb.connolly@linaro.org>
> ---
> Changes since v2:
>   * Rework and simplify into one patch following Yang's suggestions.
>
> V2: https://lore.kernel.org/all/20230103171811.204196-1-caleb.connolly@linaro.org/
> ---
>   drivers/thermal/thermal_core.c | 11 ++++++-----
>   1 file changed, 6 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
> index f17ab2316dbd..321d2a6300c4 100644
> --- a/drivers/thermal/thermal_core.c
> +++ b/drivers/thermal/thermal_core.c
> @@ -909,15 +909,16 @@ __thermal_cooling_device_register(struct device_node *np,
>   	cdev->devdata = devdata;
>   
>   	ret = cdev->ops->get_max_state(cdev, &cdev->max_state);
> -	if (ret)
> -		goto out_kfree_type;
> +	if (ret) {
> +		kfree(cdev->type);
> +		goto out_ida_remove;
> +	}
>   
>   	thermal_cooling_device_setup_sysfs(cdev);
>   	ret = dev_set_name(&cdev->device, "cooling_device%d", cdev->id);
> -	if (ret) {
> -		thermal_cooling_device_destroy_sysfs(cdev);
> +	if (ret)
>   		goto out_kfree_type;
> -	}
> +
Before device_initialize() which is called in device_register(), we can 
not call put_device().
So we can not use 'out_kfree_type' lable before calling device_register().

How about like fix it this:
diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
index f17ab2316dbd..b57a6db0efdf 100644
--- a/drivers/thermal/thermal_core.c
+++ b/drivers/thermal/thermal_core.c
@@ -909,14 +909,17 @@ __thermal_cooling_device_register(struct 
device_node *np,
         cdev->devdata = devdata;

         ret = cdev->ops->get_max_state(cdev, &cdev->max_state);
-       if (ret)
-               goto out_kfree_type;
+       if (ret) {
+               kfree(cdev->type);
+               goto out_ida_remove;
+       }

         thermal_cooling_device_setup_sysfs(cdev);
         ret = dev_set_name(&cdev->device, "cooling_device%d", cdev->id);
         if (ret) {
                 thermal_cooling_device_destroy_sysfs(cdev);
-               goto out_kfree_type;
+               kfree(cdev->type);
+               goto out_ida_remove;
         }
         ret = device_register(&cdev->device);
         if (ret)

Thanks,
Yang
>   	ret = device_register(&cdev->device);
>   	if (ret)
>   		goto out_kfree_type;