txx9wdt.c:52: Fix txx9wdt_probe() cleaning up after errors

PATCH In review Roel Kluin 3 participants 6 messages Aug 25, 2009 – Aug 31, 2009 Watchdog device drivers

lore

Share & tools

lore

Roel Kluin

author

2009-08-25 09:15 UTC #0 lore raw

PATCH 1/1 txx9wdt.c:52: Fix txx9wdt_probe() cleaning up after errors

Make txx9wdt_probe() clean up after errors.

Signed-off-by Roel Kluin <[email protected]>

---
Any comments?

drivers/watchdog/txx9wdt.c +19 ~~-10~~

index 6adab77..1e15de3 100644
@@ -212,25 +212,31 @@ static int __init txx9wdt_probe(struct platform_device *dev)
 	}
 
 	res = platform_get_resource(dev, IORESOURCE_MEM, 0);
-	if (!res)
-		goto exit_busy;
+	if (!res) {
+		ret = -EBUSY;
+		goto exit;
+	}
 	if (!devm_request_mem_region(&dev->dev,
 				     res->start, res->end - res->start + 1,
-				     "txx9wdt"))
-		goto exit_busy;
+				     "txx9wdt")) {
+		ret = -EBUSY;
+		goto exit;
+	}
 	txx9wdt_reg = devm_ioremap(&dev->dev,
 				   res->start, res->end - res->start + 1);
-	if (!txx9wdt_reg)
-		goto exit_busy;
+	if (!txx9wdt_reg) {
+		ret = -EBUSY;
+		goto exit_release_mem;
+	}
 
 	ret = register_reboot_notifier(&txx9wdt_notifier);
 	if (ret)
-		goto exit;
+		goto exit_iounmap;
 
 	ret = misc_register(&txx9wdt_miscdev);
 	if (ret) {
 		unregister_reboot_notifier(&txx9wdt_notifier);
-		goto exit;
+		goto exit_iounmap;
 	}
 
 	printk(KERN_INFO "Hardware Watchdog Timer for TXx9: "

@@ -238,8 +244,11 @@ static int __init txx9wdt_probe(struct platform_device *dev)
 	       timeout, WD_MAX_TIMEOUT, nowayout);
 
 	return 0;
-exit_busy:
-	ret = -EBUSY;
+
+exit_iounmap:
+	devm_iounmap(&dev->dev, txx9wdt_reg);
+exit_release_mem:
+	devm_release_mem_region(&dev->dev, res->start, res->end - res->start + 1);
 exit:
 	if (txx9_imclk) {
 		clk_disable(txx9_imclk);

Wim Van Sebroeck

2009-08-25 20:08 UTC #1 lore raw

Hi Roel,

Quoted 3 lines

Make txx9wdt_probe() clean up after errors.
Signed-off-by Roel Kluin <[email protected]>

patch added to the linux-2.6-watchdog-next tree.

Kind regards,
Wim.

Atsushi Nemoto

2009-08-27 14:56 UTC #2 lore raw

In reply to Roel Kluin

Quoted 5 lines · Roel Kluin

Make txx9wdt_probe() clean up after errors.
Signed-off-by Roel Kluin <[email protected]>
---
Any comments?

Well, why is this patch needed? I intentionally omitted iounmap and release_mem_region because devres guarantee these resources are freed on probe failure. Or are there any leaks?

---
Atsushi Nemoto

Wim Van Sebroeck

2009-08-27 20:13 UTC #3 lore raw

Hi Atsushi,

Quoted 9 lines

Quoted 5 lines
Make txx9wdt_probe() clean up after errors.
Signed-off-by Roel Kluin <[email protected]>
---
Any comments?
Well, why is this patch needed? I intentionally omitted iounmap and release_mem_region because devres guarantee these resources are freed on probe failure. Or are there any leaks?

My opinion: a driver should do proper clean-up on probe failures because
1) this makes clean and robust code and shows that you know what you are doing
2) to avoid having to search for leaks if there are any. See the NULL pointer story...

Kind regards,
Wim.

Atsushi Nemoto

2009-08-28 14:29 UTC #4 lore raw

Quoted 6 lines · Wim Van Sebroeck

Quoted 6 lines
Quoted 5 lines
Make txx9wdt_probe() clean up after errors.
Signed-off-by Roel Kluin <[email protected]>
---
Any comments?

Quoted 7 lines

Quoted 3 lines
Well, why is this patch needed? I intentionally omitted iounmap and release_mem_region because devres guarantee these resources are freed on probe failure. Or are there any leaks?
My opinion: a driver should do proper clean-up on probe failures because
1) this makes clean and robust code and shows that you know what you are doing
2) to avoid having to search for leaks if there are any. See the NULL pointer story...

I think "proper clean-up" in this case is "do nothing for devres-managed resources". An example in Documentation/driver-model/devres.txt clearly say so. Actually most drivers do not call devm_iounmap in any case.

Also, the patch added devm_iounmap in failure path but not in exit path. This looks inconsistent.

---
Atsushi Nemoto

Wim Van Sebroeck

2009-08-31 12:02 UTC #5 lore raw

Hi Atsushi,

Quoted 11 lines · Wim Van Sebroeck

Quoted 10 lines
Quoted 9 lines
Quoted 5 lines
Make txx9wdt_probe() clean up after errors.
Signed-off-by Roel Kluin <[email protected]>
---
Any comments?
Well, why is this patch needed? I intentionally omitted iounmap and release_mem_region because devres guarantee these resources are freed on probe failure. Or are there any leaks?

Quoted 8 lines

Quoted 3 lines
My opinion: a driver should do proper clean-up on probe failures because
1) this makes clean and robust code and shows that you know what you are doing
2) to avoid having to search for leaks if there are any. See the NULL pointer story...
I think "proper clean-up" in this case is "do nothing for devres-managed resources". An example in Documentation/driver-model/devres.txt clearly say so. Actually most drivers do not call devm_iounmap in any case.

I will have a look at devres managed resources and than come back on this.

Quoted 2 lines

Also, the patch added devm_iounmap in failure path but not in exit path. This looks inconsistent.

Correct, if we use it in the probe error function then it should be used also in the exit function.

Kind regards,
Wim.