thermal: Fix/cleanup error paths in __thermal_cooling_device_register()

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 <[email protected]>

---

  drivers/thermal/thermal_core.c | 33 ++++++++++++++-------------------
  1 file changed, 14 insertions(+), 19 deletions(-)

drivers/thermal/thermal_core.c +14 -19

index 77bd47d976a2..1fb109a97ff6 100644
@@ -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);

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 <[email protected]>

Signed-off-by Viresh Kumar <[email protected]>

---
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(-)

drivers/thermal/thermal_core.c +8 -3

index f17ab2316dbd..77bd47d976a2 100644
@@ -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;

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 <[email protected]>

Signed-off-by Viresh Kumar <[email protected]>

---
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(-)

drivers/thermal/thermal_core.c +10 -4

index f17ab2316dbd..77bd47d976a2 100644
@@ -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

Quoted 11 lines · Viresh Kumar

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.

Quoted 4 lines

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 <[email protected]>

Signed-off-by Viresh Kumar <[email protected]>

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?

Quoted 17 lines · Rafael J. Wysocki

Quoted 15 lines · Viresh Kumar

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 <[email protected]>

Signed-off-by Viresh Kumar <[email protected]>

Quoted 3 lines

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.

Quoted 19 lines · Viresh Kumar

Quoted 17 lines · Rafael J. Wysocki

Quoted 15 lines · Viresh Kumar

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 <[email protected]>

Signed-off-by Viresh Kumar <[email protected]>

Quoted 5 lines

Quoted 3 lines

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.

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 <[email protected]>

Signed-off-by Viresh Kumar <[email protected]>

---

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(-)

drivers/thermal/thermal_core.c +10 -3

index f17ab2316dbd..77bd47d976a2 100644
@@ -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);

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 <[email protected]>

Signed-off-by Viresh Kumar <[email protected]>

---
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(-)

drivers/thermal/thermal_core.c +10 -3

index f17ab2316dbd..77bd47d976a2 100644
@@ -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);

Lets not open code device_unregister() unnecessarily.

Signed-off-by Viresh Kumar <[email protected]>

---

  drivers/thermal/thermal_core.c | 3 +--
  1 file changed, 1 insertion(+), 2 deletions(-)

drivers/thermal/thermal_core.c +1 -2

index 1fb109a97ff6..9fb37c5d9c4f 100644
@@ -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);