* [PATCH 0/3] thermal: Fix/cleanup error paths in __thermal_cooling_device_register()
@ 2023-01-18 8:38 Viresh Kumar
2023-01-18 8:38 ` [PATCH V4 1/3] thermal: core: call put_device() only after device_register() fails Viresh Kumar
` (2 more replies)
0 siblings, 3 replies; 13+ messages in thread
From: Viresh Kumar @ 2023-01-18 8:38 UTC (permalink / raw)
To: Rafael J. Wysocki, Amit Kucheria, Daniel Lezcano, Viresh Kumar,
Yang Yingliang, Zhang Rui
Cc: linux-pm, Vincent Guittot, Caleb Connolly, linux-kernel,
Rafael J. Wysocki
Hi,
The error paths in __thermal_cooling_device_register() are trying to free
un-acquired resources, fix that and clean it up a bit.
Caleb, can you please test this? I have just build tested it for now.
--
Viresh
Viresh Kumar (3):
thermal: core: call put_device() only after device_register() fails
thermal: core: Move cdev cleanup to thermal_release()
thermal: core: Use device_unregister() instead of device_del/put()
drivers/thermal/thermal_core.c | 29 +++++++++++++++--------------
1 file changed, 15 insertions(+), 14 deletions(-)
--
2.31.1.272.g89b43f80a514
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH V4 1/3] thermal: core: call put_device() only after device_register() fails
2023-01-18 8:38 [PATCH 0/3] thermal: Fix/cleanup error paths in __thermal_cooling_device_register() Viresh Kumar
@ 2023-01-18 8:38 ` Viresh Kumar
2023-01-18 19:57 ` Frank Rowand
` (3 more replies)
2023-01-18 8:38 ` [PATCH 2/3] thermal: core: Move cdev cleanup to thermal_release() Viresh Kumar
2023-01-18 8:38 ` [PATCH 3/3] thermal: core: Use device_unregister() instead of device_del/put() Viresh Kumar
2 siblings, 4 replies; 13+ messages in thread
From: Viresh Kumar @ 2023-01-18 8:38 UTC (permalink / raw)
To: Rafael J. Wysocki, Daniel Lezcano, Amit Kucheria, Zhang Rui,
Yang Yingliang, Viresh Kumar
Cc: linux-pm, Vincent Guittot, Caleb Connolly, Rafael J. Wysocki,
linux-kernel
put_device() shouldn't be called before a prior call to
device_register(). __thermal_cooling_device_register() doesn't follow
that properly and needs fixing. Also
thermal_cooling_device_destroy_sysfs() is getting called unnecessarily
on few error paths.
Fix all this by placing the calls at the right place.
Based on initial work done by Caleb Connolly.
Fixes: 4748f9687caa ("thermal: core: fix some possible name leaks in error paths")
Fixes: c408b3d1d9bb ("thermal: Validate new state in cur_state_store()")
Reported-by: Caleb Connolly <caleb.connolly@linaro.org>
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
For v6.2-rc.
V3->V4:
- The first three versions were sent by Caleb.
- The new version fixes the current bugs, without looking to optimize the
code any further, which is done separately in the next two patches.
drivers/thermal/thermal_core.c | 13 ++++++++++---
1 file changed, 10 insertions(+), 3 deletions(-)
diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
index f17ab2316dbd..77bd47d976a2 100644
--- a/drivers/thermal/thermal_core.c
+++ b/drivers/thermal/thermal_core.c
@@ -909,15 +909,20 @@ __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) {
+ kfree(cdev->type);
thermal_cooling_device_destroy_sysfs(cdev);
- goto out_kfree_type;
+ goto out_ida_remove;
}
+
ret = device_register(&cdev->device);
if (ret)
goto out_kfree_type;
@@ -943,6 +948,8 @@ __thermal_cooling_device_register(struct device_node *np,
thermal_cooling_device_destroy_sysfs(cdev);
kfree(cdev->type);
put_device(&cdev->device);
+
+ /* thermal_release() takes care of the rest */
cdev = NULL;
out_ida_remove:
ida_free(&thermal_cdev_ida, id);
--
2.31.1.272.g89b43f80a514
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH 2/3] thermal: core: Move cdev cleanup to thermal_release()
2023-01-18 8:38 [PATCH 0/3] thermal: Fix/cleanup error paths in __thermal_cooling_device_register() Viresh Kumar
2023-01-18 8:38 ` [PATCH V4 1/3] thermal: core: call put_device() only after device_register() fails Viresh Kumar
@ 2023-01-18 8:38 ` Viresh Kumar
2023-01-19 8:25 ` Yang Yingliang
2023-01-18 8:38 ` [PATCH 3/3] thermal: core: Use device_unregister() instead of device_del/put() Viresh Kumar
2 siblings, 1 reply; 13+ messages in thread
From: Viresh Kumar @ 2023-01-18 8:38 UTC (permalink / raw)
To: Rafael J. Wysocki, Daniel Lezcano, Amit Kucheria, Zhang Rui
Cc: Viresh Kumar, linux-pm, Vincent Guittot, Caleb Connolly,
Yang Yingliang, linux-kernel
thermal_release() already frees cdev, let it do rest of the cleanup as
well in order to simplify the error paths in
__thermal_cooling_device_register().
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
drivers/thermal/thermal_core.c | 33 ++++++++++++++-------------------
1 file changed, 14 insertions(+), 19 deletions(-)
diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
index 77bd47d976a2..1fb109a97ff6 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);
}
}
@@ -909,23 +912,21 @@ __thermal_cooling_device_register(struct device_node *np,
cdev->devdata = devdata;
ret = cdev->ops->get_max_state(cdev, &cdev->max_state);
- if (ret) {
- kfree(cdev->type);
- goto out_ida_remove;
- }
+ if (ret)
+ goto out_cdev_type;
thermal_cooling_device_setup_sysfs(cdev);
ret = dev_set_name(&cdev->device, "cooling_device%d", cdev->id);
- if (ret) {
- kfree(cdev->type);
- thermal_cooling_device_destroy_sysfs(cdev);
- goto out_ida_remove;
- }
+ if (ret)
+ goto out_cooling_dev;
ret = device_register(&cdev->device);
- if (ret)
- goto out_kfree_type;
+ if (ret) {
+ /* thermal_release() handles rest of the cleanup */
+ put_device(&cdev->device);
+ return ERR_PTR(ret);
+ }
/* Add 'this' new cdev to the global cdev list */
mutex_lock(&thermal_list_lock);
@@ -944,13 +945,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);
-
- /* thermal_release() takes care of the rest */
- cdev = NULL;
out_ida_remove:
ida_free(&thermal_cdev_ida, id);
out_kfree_cdev:
@@ -1111,10 +1109,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);
}
EXPORT_SYMBOL_GPL(thermal_cooling_device_unregister);
--
2.31.1.272.g89b43f80a514
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH 3/3] thermal: core: Use device_unregister() instead of device_del/put()
2023-01-18 8:38 [PATCH 0/3] thermal: Fix/cleanup error paths in __thermal_cooling_device_register() Viresh Kumar
2023-01-18 8:38 ` [PATCH V4 1/3] thermal: core: call put_device() only after device_register() fails Viresh Kumar
2023-01-18 8:38 ` [PATCH 2/3] thermal: core: Move cdev cleanup to thermal_release() Viresh Kumar
@ 2023-01-18 8:38 ` Viresh Kumar
2023-01-19 8:25 ` Yang Yingliang
2 siblings, 1 reply; 13+ messages in thread
From: Viresh Kumar @ 2023-01-18 8:38 UTC (permalink / raw)
To: Rafael J. Wysocki, Daniel Lezcano, Amit Kucheria, Zhang Rui
Cc: Viresh Kumar, linux-pm, Vincent Guittot, Caleb Connolly,
Yang Yingliang, linux-kernel
Lets not open code device_unregister() unnecessarily.
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
drivers/thermal/thermal_core.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
index 1fb109a97ff6..9fb37c5d9c4f 100644
--- a/drivers/thermal/thermal_core.c
+++ b/drivers/thermal/thermal_core.c
@@ -1109,8 +1109,7 @@ void thermal_cooling_device_unregister(struct thermal_cooling_device *cdev)
mutex_unlock(&thermal_list_lock);
- device_del(&cdev->device);
- put_device(&cdev->device);
+ device_unregister(&cdev->device);
}
EXPORT_SYMBOL_GPL(thermal_cooling_device_unregister);
--
2.31.1.272.g89b43f80a514
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH V4 1/3] thermal: core: call put_device() only after device_register() fails
2023-01-18 8:38 ` [PATCH V4 1/3] thermal: core: call put_device() only after device_register() fails Viresh Kumar
@ 2023-01-18 19:57 ` Frank Rowand
2023-01-18 19:58 ` Rafael J. Wysocki
` (2 subsequent siblings)
3 siblings, 0 replies; 13+ messages in thread
From: Frank Rowand @ 2023-01-18 19:57 UTC (permalink / raw)
To: Viresh Kumar, Rafael J. Wysocki, Daniel Lezcano, Amit Kucheria,
Zhang Rui, Yang Yingliang
Cc: linux-pm, Vincent Guittot, Caleb Connolly, Rafael J. Wysocki,
linux-kernel
On 1/18/23 02:38, Viresh Kumar wrote:
> put_device() shouldn't be called before a prior call to
> device_register(). __thermal_cooling_device_register() doesn't follow
> that properly and needs fixing. Also
> thermal_cooling_device_destroy_sysfs() is getting called unnecessarily
> on few error paths.
>
> Fix all this by placing the calls at the right place.
>
> Based on initial work done by Caleb Connolly.
>
> Fixes: 4748f9687caa ("thermal: core: fix some possible name leaks in error paths")
> Fixes: c408b3d1d9bb ("thermal: Validate new state in cur_state_store()")
> Reported-by: Caleb Connolly <caleb.connolly@linaro.org>
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
> For v6.2-rc.
>
> V3->V4:
> - The first three versions were sent by Caleb.
> - The new version fixes the current bugs, without looking to optimize the
> code any further, which is done separately in the next two patches.
>
> drivers/thermal/thermal_core.c | 13 ++++++++++---
> 1 file changed, 10 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
> index f17ab2316dbd..77bd47d976a2 100644
> --- a/drivers/thermal/thermal_core.c
> +++ b/drivers/thermal/thermal_core.c
> @@ -909,15 +909,20 @@ __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) {
> + kfree(cdev->type);
> thermal_cooling_device_destroy_sysfs(cdev);
> - goto out_kfree_type;
> + goto out_ida_remove;
> }
> +
> ret = device_register(&cdev->device);
> if (ret)
> goto out_kfree_type;
> @@ -943,6 +948,8 @@ __thermal_cooling_device_register(struct device_node *np,
> thermal_cooling_device_destroy_sysfs(cdev);
> kfree(cdev->type);
> put_device(&cdev->device);
> +
> + /* thermal_release() takes care of the rest */
> cdev = NULL;
> out_ida_remove:
> ida_free(&thermal_cdev_ida, id);
My testing:
Applied on top of v6.2-rc1
The configuration is qcom_defconfig
The system is a Qualcomm Dragon 8074
The two WARNING stack traces no longer occur after applying the patch.
Tested-by: Frank Rowand <frowand.list@gmail.com>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH V4 1/3] thermal: core: call put_device() only after device_register() fails
2023-01-18 8:38 ` [PATCH V4 1/3] thermal: core: call put_device() only after device_register() fails Viresh Kumar
2023-01-18 19:57 ` Frank Rowand
@ 2023-01-18 19:58 ` Rafael J. Wysocki
2023-01-19 5:16 ` Viresh Kumar
2023-01-19 8:13 ` Yang Yingliang
2023-01-19 15:02 ` Caleb Connolly
3 siblings, 1 reply; 13+ messages in thread
From: Rafael J. Wysocki @ 2023-01-18 19:58 UTC (permalink / raw)
To: Viresh Kumar
Cc: Rafael J. Wysocki, Daniel Lezcano, Amit Kucheria, Zhang Rui,
Yang Yingliang, linux-pm, Vincent Guittot, Caleb Connolly,
Rafael J. Wysocki, linux-kernel
On Wed, Jan 18, 2023 at 9:38 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> put_device() shouldn't be called before a prior call to
> device_register(). __thermal_cooling_device_register() doesn't follow
> that properly and needs fixing. Also
> thermal_cooling_device_destroy_sysfs() is getting called unnecessarily
> on few error paths.
>
> Fix all this by placing the calls at the right place.
>
> Based on initial work done by Caleb Connolly.
>
> Fixes: 4748f9687caa ("thermal: core: fix some possible name leaks in error paths")
> Fixes: c408b3d1d9bb ("thermal: Validate new state in cur_state_store()")
> Reported-by: Caleb Connolly <caleb.connolly@linaro.org>
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
OK, so I think that this patch is needed for 6.2 and the other two may
be queued up for later (they do depend on this one, though, of
course). Is my understanding correct?
> ---
> For v6.2-rc.
>
> V3->V4:
> - The first three versions were sent by Caleb.
> - The new version fixes the current bugs, without looking to optimize the
> code any further, which is done separately in the next two patches.
>
> drivers/thermal/thermal_core.c | 13 ++++++++++---
> 1 file changed, 10 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
> index f17ab2316dbd..77bd47d976a2 100644
> --- a/drivers/thermal/thermal_core.c
> +++ b/drivers/thermal/thermal_core.c
> @@ -909,15 +909,20 @@ __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) {
> + kfree(cdev->type);
> thermal_cooling_device_destroy_sysfs(cdev);
> - goto out_kfree_type;
> + goto out_ida_remove;
> }
> +
> ret = device_register(&cdev->device);
> if (ret)
> goto out_kfree_type;
> @@ -943,6 +948,8 @@ __thermal_cooling_device_register(struct device_node *np,
> thermal_cooling_device_destroy_sysfs(cdev);
> kfree(cdev->type);
> put_device(&cdev->device);
> +
> + /* thermal_release() takes care of the rest */
> cdev = NULL;
> out_ida_remove:
> ida_free(&thermal_cdev_ida, id);
> --
> 2.31.1.272.g89b43f80a514
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH V4 1/3] thermal: core: call put_device() only after device_register() fails
2023-01-18 19:58 ` Rafael J. Wysocki
@ 2023-01-19 5:16 ` Viresh Kumar
2023-01-19 20:09 ` Rafael J. Wysocki
0 siblings, 1 reply; 13+ messages in thread
From: Viresh Kumar @ 2023-01-19 5:16 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Daniel Lezcano, Amit Kucheria, Zhang Rui, Yang Yingliang,
linux-pm, Vincent Guittot, Caleb Connolly, Rafael J. Wysocki,
linux-kernel
On 18-01-23, 20:58, Rafael J. Wysocki wrote:
> On Wed, Jan 18, 2023 at 9:38 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
> >
> > put_device() shouldn't be called before a prior call to
> > device_register(). __thermal_cooling_device_register() doesn't follow
> > that properly and needs fixing. Also
> > thermal_cooling_device_destroy_sysfs() is getting called unnecessarily
> > on few error paths.
> >
> > Fix all this by placing the calls at the right place.
> >
> > Based on initial work done by Caleb Connolly.
> >
> > Fixes: 4748f9687caa ("thermal: core: fix some possible name leaks in error paths")
> > Fixes: c408b3d1d9bb ("thermal: Validate new state in cur_state_store()")
> > Reported-by: Caleb Connolly <caleb.connolly@linaro.org>
> > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
>
> OK, so I think that this patch is needed for 6.2 and the other two may
> be queued up for later (they do depend on this one, though, of
> course). Is my understanding correct?
Right.
--
viresh
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH V4 1/3] thermal: core: call put_device() only after device_register() fails
2023-01-18 8:38 ` [PATCH V4 1/3] thermal: core: call put_device() only after device_register() fails Viresh Kumar
2023-01-18 19:57 ` Frank Rowand
2023-01-18 19:58 ` Rafael J. Wysocki
@ 2023-01-19 8:13 ` Yang Yingliang
2023-01-19 15:02 ` Caleb Connolly
3 siblings, 0 replies; 13+ messages in thread
From: Yang Yingliang @ 2023-01-19 8:13 UTC (permalink / raw)
To: Viresh Kumar, Rafael J. Wysocki, Daniel Lezcano, Amit Kucheria,
Zhang Rui
Cc: linux-pm, Vincent Guittot, Caleb Connolly, Rafael J. Wysocki,
linux-kernel
On 2023/1/18 16:38, Viresh Kumar wrote:
> put_device() shouldn't be called before a prior call to
> device_register(). __thermal_cooling_device_register() doesn't follow
> that properly and needs fixing. Also
> thermal_cooling_device_destroy_sysfs() is getting called unnecessarily
> on few error paths.
>
> Fix all this by placing the calls at the right place.
>
> Based on initial work done by Caleb Connolly.
>
> Fixes: 4748f9687caa ("thermal: core: fix some possible name leaks in error paths")
> Fixes: c408b3d1d9bb ("thermal: Validate new state in cur_state_store()")
> Reported-by: Caleb Connolly <caleb.connolly@linaro.org>
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
Reviewed-by: Yang Yingliang <yangyingliang@huawei.com>
> For v6.2-rc.
>
> V3->V4:
> - The first three versions were sent by Caleb.
> - The new version fixes the current bugs, without looking to optimize the
> code any further, which is done separately in the next two patches.
>
> drivers/thermal/thermal_core.c | 13 ++++++++++---
> 1 file changed, 10 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
> index f17ab2316dbd..77bd47d976a2 100644
> --- a/drivers/thermal/thermal_core.c
> +++ b/drivers/thermal/thermal_core.c
> @@ -909,15 +909,20 @@ __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) {
> + kfree(cdev->type);
> thermal_cooling_device_destroy_sysfs(cdev);
> - goto out_kfree_type;
> + goto out_ida_remove;
> }
> +
> ret = device_register(&cdev->device);
> if (ret)
> goto out_kfree_type;
> @@ -943,6 +948,8 @@ __thermal_cooling_device_register(struct device_node *np,
> thermal_cooling_device_destroy_sysfs(cdev);
> kfree(cdev->type);
> put_device(&cdev->device);
> +
> + /* thermal_release() takes care of the rest */
> cdev = NULL;
> out_ida_remove:
> ida_free(&thermal_cdev_ida, id);
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/3] thermal: core: Move cdev cleanup to thermal_release()
2023-01-18 8:38 ` [PATCH 2/3] thermal: core: Move cdev cleanup to thermal_release() Viresh Kumar
@ 2023-01-19 8:25 ` Yang Yingliang
0 siblings, 0 replies; 13+ messages in thread
From: Yang Yingliang @ 2023-01-19 8:25 UTC (permalink / raw)
To: Viresh Kumar, Rafael J. Wysocki, Daniel Lezcano, Amit Kucheria,
Zhang Rui
Cc: linux-pm, Vincent Guittot, Caleb Connolly, linux-kernel
On 2023/1/18 16:38, Viresh Kumar wrote:
> thermal_release() already frees cdev, let it do rest of the cleanup as
> well in order to simplify the error paths in
> __thermal_cooling_device_register().
>
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
Reviewed-by: Yang Yingliang <yangyingliang@huawei.com>
> drivers/thermal/thermal_core.c | 33 ++++++++++++++-------------------
> 1 file changed, 14 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
> index 77bd47d976a2..1fb109a97ff6 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);
> }
> }
> @@ -909,23 +912,21 @@ __thermal_cooling_device_register(struct device_node *np,
> cdev->devdata = devdata;
>
> ret = cdev->ops->get_max_state(cdev, &cdev->max_state);
> - if (ret) {
> - kfree(cdev->type);
> - goto out_ida_remove;
> - }
> + if (ret)
> + goto out_cdev_type;
>
> thermal_cooling_device_setup_sysfs(cdev);
>
> ret = dev_set_name(&cdev->device, "cooling_device%d", cdev->id);
> - if (ret) {
> - kfree(cdev->type);
> - thermal_cooling_device_destroy_sysfs(cdev);
> - goto out_ida_remove;
> - }
> + if (ret)
> + goto out_cooling_dev;
>
> ret = device_register(&cdev->device);
> - if (ret)
> - goto out_kfree_type;
> + if (ret) {
> + /* thermal_release() handles rest of the cleanup */
> + put_device(&cdev->device);
> + return ERR_PTR(ret);
> + }
>
> /* Add 'this' new cdev to the global cdev list */
> mutex_lock(&thermal_list_lock);
> @@ -944,13 +945,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);
> -
> - /* thermal_release() takes care of the rest */
> - cdev = NULL;
> out_ida_remove:
> ida_free(&thermal_cdev_ida, id);
> out_kfree_cdev:
> @@ -1111,10 +1109,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);
> }
> EXPORT_SYMBOL_GPL(thermal_cooling_device_unregister);
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 3/3] thermal: core: Use device_unregister() instead of device_del/put()
2023-01-18 8:38 ` [PATCH 3/3] thermal: core: Use device_unregister() instead of device_del/put() Viresh Kumar
@ 2023-01-19 8:25 ` Yang Yingliang
0 siblings, 0 replies; 13+ messages in thread
From: Yang Yingliang @ 2023-01-19 8:25 UTC (permalink / raw)
To: Viresh Kumar, Rafael J. Wysocki, Daniel Lezcano, Amit Kucheria,
Zhang Rui
Cc: linux-pm, Vincent Guittot, Caleb Connolly, linux-kernel
On 2023/1/18 16:38, Viresh Kumar wrote:
> Lets not open code device_unregister() unnecessarily.
>
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
Reviewed-by: Yang Yingliang <yangyingliang@huawei.com>
> drivers/thermal/thermal_core.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
> index 1fb109a97ff6..9fb37c5d9c4f 100644
> --- a/drivers/thermal/thermal_core.c
> +++ b/drivers/thermal/thermal_core.c
> @@ -1109,8 +1109,7 @@ void thermal_cooling_device_unregister(struct thermal_cooling_device *cdev)
>
> mutex_unlock(&thermal_list_lock);
>
> - device_del(&cdev->device);
> - put_device(&cdev->device);
> + device_unregister(&cdev->device);
> }
> EXPORT_SYMBOL_GPL(thermal_cooling_device_unregister);
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH V4 1/3] thermal: core: call put_device() only after device_register() fails
2023-01-18 8:38 ` [PATCH V4 1/3] thermal: core: call put_device() only after device_register() fails Viresh Kumar
` (2 preceding siblings ...)
2023-01-19 8:13 ` Yang Yingliang
@ 2023-01-19 15:02 ` Caleb Connolly
3 siblings, 0 replies; 13+ messages in thread
From: Caleb Connolly @ 2023-01-19 15:02 UTC (permalink / raw)
To: Viresh Kumar, Rafael J. Wysocki, Daniel Lezcano, Amit Kucheria,
Zhang Rui, Yang Yingliang
Cc: linux-pm, Vincent Guittot, Rafael J. Wysocki, linux-kernel
On 18/01/2023 08:38, Viresh Kumar wrote:
> put_device() shouldn't be called before a prior call to
> device_register(). __thermal_cooling_device_register() doesn't follow
> that properly and needs fixing. Also
> thermal_cooling_device_destroy_sysfs() is getting called unnecessarily
> on few error paths.
>
> Fix all this by placing the calls at the right place.
>
> Based on initial work done by Caleb Connolly.
>
> Fixes: 4748f9687caa ("thermal: core: fix some possible name leaks in error paths")
> Fixes: c408b3d1d9bb ("thermal: Validate new state in cur_state_store()")
> Reported-by: Caleb Connolly <caleb.connolly@linaro.org>
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
Tested-by: Caleb Connolly <caleb.connolly@linaro.org>
Thanks for sending this, with this I no longer hit the splats when
get_max_state() fails.
> ---
> For v6.2-rc.
>
> V3->V4:
> - The first three versions were sent by Caleb.
> - The new version fixes the current bugs, without looking to optimize the
> code any further, which is done separately in the next two patches.
>
> drivers/thermal/thermal_core.c | 13 ++++++++++---
> 1 file changed, 10 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
> index f17ab2316dbd..77bd47d976a2 100644
> --- a/drivers/thermal/thermal_core.c
> +++ b/drivers/thermal/thermal_core.c
> @@ -909,15 +909,20 @@ __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) {
> + kfree(cdev->type);
> thermal_cooling_device_destroy_sysfs(cdev);
> - goto out_kfree_type;
> + goto out_ida_remove;
> }
> +
> ret = device_register(&cdev->device);
> if (ret)
> goto out_kfree_type;
> @@ -943,6 +948,8 @@ __thermal_cooling_device_register(struct device_node *np,
> thermal_cooling_device_destroy_sysfs(cdev);
> kfree(cdev->type);
> put_device(&cdev->device);
> +
> + /* thermal_release() takes care of the rest */
> cdev = NULL;
> out_ida_remove:
> ida_free(&thermal_cdev_ida, id);
--
Kind Regards,
Caleb (they/them)
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH V4 1/3] thermal: core: call put_device() only after device_register() fails
2023-01-19 5:16 ` Viresh Kumar
@ 2023-01-19 20:09 ` Rafael J. Wysocki
2023-01-24 19:26 ` Rafael J. Wysocki
0 siblings, 1 reply; 13+ messages in thread
From: Rafael J. Wysocki @ 2023-01-19 20:09 UTC (permalink / raw)
To: Viresh Kumar
Cc: Rafael J. Wysocki, Daniel Lezcano, Amit Kucheria, Zhang Rui,
Yang Yingliang, linux-pm, Vincent Guittot, Caleb Connolly,
Rafael J. Wysocki, linux-kernel
On Thu, Jan 19, 2023 at 6:16 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> On 18-01-23, 20:58, Rafael J. Wysocki wrote:
> > On Wed, Jan 18, 2023 at 9:38 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
> > >
> > > put_device() shouldn't be called before a prior call to
> > > device_register(). __thermal_cooling_device_register() doesn't follow
> > > that properly and needs fixing. Also
> > > thermal_cooling_device_destroy_sysfs() is getting called unnecessarily
> > > on few error paths.
> > >
> > > Fix all this by placing the calls at the right place.
> > >
> > > Based on initial work done by Caleb Connolly.
> > >
> > > Fixes: 4748f9687caa ("thermal: core: fix some possible name leaks in error paths")
> > > Fixes: c408b3d1d9bb ("thermal: Validate new state in cur_state_store()")
> > > Reported-by: Caleb Connolly <caleb.connolly@linaro.org>
> > > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> >
> > OK, so I think that this patch is needed for 6.2 and the other two may
> > be queued up for later (they do depend on this one, though, of
> > course). Is my understanding correct?
>
> Right.
OK, applied as 6.2-rc material and I'll get to the other two when this goes in.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH V4 1/3] thermal: core: call put_device() only after device_register() fails
2023-01-19 20:09 ` Rafael J. Wysocki
@ 2023-01-24 19:26 ` Rafael J. Wysocki
0 siblings, 0 replies; 13+ messages in thread
From: Rafael J. Wysocki @ 2023-01-24 19:26 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Viresh Kumar, Daniel Lezcano, Amit Kucheria, Zhang Rui,
Yang Yingliang, linux-pm, Vincent Guittot, Caleb Connolly,
Rafael J. Wysocki, linux-kernel
On Thu, Jan 19, 2023 at 9:09 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> On Thu, Jan 19, 2023 at 6:16 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
> >
> > On 18-01-23, 20:58, Rafael J. Wysocki wrote:
> > > On Wed, Jan 18, 2023 at 9:38 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
> > > >
> > > > put_device() shouldn't be called before a prior call to
> > > > device_register(). __thermal_cooling_device_register() doesn't follow
> > > > that properly and needs fixing. Also
> > > > thermal_cooling_device_destroy_sysfs() is getting called unnecessarily
> > > > on few error paths.
> > > >
> > > > Fix all this by placing the calls at the right place.
> > > >
> > > > Based on initial work done by Caleb Connolly.
> > > >
> > > > Fixes: 4748f9687caa ("thermal: core: fix some possible name leaks in error paths")
> > > > Fixes: c408b3d1d9bb ("thermal: Validate new state in cur_state_store()")
> > > > Reported-by: Caleb Connolly <caleb.connolly@linaro.org>
> > > > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> > >
> > > OK, so I think that this patch is needed for 6.2 and the other two may
> > > be queued up for later (they do depend on this one, though, of
> > > course). Is my understanding correct?
> >
> > Right.
>
> OK, applied as 6.2-rc material and I'll get to the other two when this goes in.
Patches [2-3/3] from this series have been applied as 6.3 material now, thanks!
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2023-01-24 19:26 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-01-18 8:38 [PATCH 0/3] thermal: Fix/cleanup error paths in __thermal_cooling_device_register() Viresh Kumar
2023-01-18 8:38 ` [PATCH V4 1/3] thermal: core: call put_device() only after device_register() fails Viresh Kumar
2023-01-18 19:57 ` Frank Rowand
2023-01-18 19:58 ` Rafael J. Wysocki
2023-01-19 5:16 ` Viresh Kumar
2023-01-19 20:09 ` Rafael J. Wysocki
2023-01-24 19:26 ` Rafael J. Wysocki
2023-01-19 8:13 ` Yang Yingliang
2023-01-19 15:02 ` Caleb Connolly
2023-01-18 8:38 ` [PATCH 2/3] thermal: core: Move cdev cleanup to thermal_release() Viresh Kumar
2023-01-19 8:25 ` Yang Yingliang
2023-01-18 8:38 ` [PATCH 3/3] thermal: core: Use device_unregister() instead of device_del/put() Viresh Kumar
2023-01-19 8:25 ` Yang Yingliang
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.