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

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

[Roel Kluin]

Make txx9wdt_probe() clean up after errors.

Signed-off-by: Roel Kluin <roel.kluin@gmail.com>
---
Any comments?

diff --git a/drivers/watchdog/txx9wdt.c b/drivers/watchdog/txx9wdt.c
index 6adab77..1e15de3 100644
--- a/drivers/watchdog/txx9wdt.c
+++ b/drivers/watchdog/txx9wdt.c
@@ -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);

Re: [PATCH] txx9wdt.c:52: Fix txx9wdt_probe() cleaning up after errors

[Wim Van Sebroeck]

Hi Roel,

> Make txx9wdt_probe() clean up after errors.
> 
> Signed-off-by: Roel Kluin <roel.kluin@gmail.com>

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

Kind regards,
Wim.

Re: [PATCH] txx9wdt.c:52: Fix txx9wdt_probe() cleaning up after errors

[Atsushi Nemoto]

On Tue, 25 Aug 2009 11:20:26 +0200, Roel Kluin <roel.kluin@gmail.com> wrote:
> Make txx9wdt_probe() clean up after errors.
> 
> Signed-off-by: Roel Kluin <roel.kluin@gmail.com>
> ---
> 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

Re: [PATCH] txx9wdt.c:52: Fix txx9wdt_probe() cleaning up after errors

[Wim Van Sebroeck]

Hi Atsushi,

> > Make txx9wdt_probe() clean up after errors.
> > 
> > Signed-off-by: Roel Kluin <roel.kluin@gmail.com>
> > ---
> > 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.

Re: [PATCH] txx9wdt.c:52: Fix txx9wdt_probe() cleaning up after errors

[Atsushi Nemoto]

On Thu, 27 Aug 2009 22:13:17 +0200, Wim Van Sebroeck <wim@iguana.be> wrote:
> > > Make txx9wdt_probe() clean up after errors.
> > > 
> > > Signed-off-by: Roel Kluin <roel.kluin@gmail.com>
> > > ---
> > > 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...

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

Re: [PATCH] txx9wdt.c:52: Fix txx9wdt_probe() cleaning up after errors

[Wim Van Sebroeck]

Hi Atsushi,

> On Thu, 27 Aug 2009 22:13:17 +0200, Wim Van Sebroeck <wim@iguana.be> wrote:
> > > > Make txx9wdt_probe() clean up after errors.
> > > > 
> > > > Signed-off-by: Roel Kluin <roel.kluin@gmail.com>
> > > > ---
> > > > 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...
> 
> 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.

> 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.