* [PATCH] watchdog: fix error in probe() of s3c2410_wdt (reset at booting) @ 2012-01-13 5:14 MyungJoo Ham 2012-01-13 11:11 ` Russell King - ARM Linux 0 siblings, 1 reply; 6+ messages in thread From: MyungJoo Ham @ 2012-01-13 5:14 UTC (permalink / raw) To: linux-arm-kernel Probe function of s3c2410 watchdog calls request_irq before initializing required value (wdt_count). This incurs resetting watchdog counter value and watchdog-reboot during booting up. This patch addresses such an issue by calling request_irq later. Error handling in probe function and calling oder in remove function are also revised accordingly. Reported-by: Chanwoo Park <cw00.choi@samsung.com> Signed-off-by: MyungJoo Ham <myungjoo.ham@samsung.com> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com> --- drivers/watchdog/s3c2410_wdt.c | 29 +++++++++++++++-------------- 1 files changed, 15 insertions(+), 14 deletions(-) diff --git a/drivers/watchdog/s3c2410_wdt.c b/drivers/watchdog/s3c2410_wdt.c index a79e384..07bd768 100644 --- a/drivers/watchdog/s3c2410_wdt.c +++ b/drivers/watchdog/s3c2410_wdt.c @@ -342,22 +342,17 @@ static int __devinit s3c2410wdt_probe(struct platform_device *pdev) goto err_map; } - ret = request_irq(wdt_irq->start, s3c2410wdt_irq, 0, pdev->name, pdev); - if (ret != 0) { - dev_err(dev, "failed to install irq (%d)\n", ret); - goto err_map; - } - wdt_clock = clk_get(&pdev->dev, "watchdog"); if (IS_ERR(wdt_clock)) { dev_err(dev, "failed to find watchdog clock source\n"); ret = PTR_ERR(wdt_clock); - goto err_irq; + goto err_map; } clk_enable(wdt_clock); - if (s3c2410wdt_cpufreq_register() < 0) { + ret = s3c2410wdt_cpufreq_register(); + if (ret < 0) { printk(KERN_ERR PFX "failed to register cpufreq\n"); goto err_clk; } @@ -395,6 +390,12 @@ static int __devinit s3c2410wdt_probe(struct platform_device *pdev) s3c2410wdt_stop(&s3c2410_wdd); } + ret = request_irq(wdt_irq->start, s3c2410wdt_irq, 0, pdev->name, pdev); + if (ret != 0) { + dev_err(dev, "failed to install irq (%d)\n", ret); + goto err_reg; + } + /* print out a statement of readiness */ wtcon = readl(wdt_base + S3C2410_WTCON); @@ -406,6 +407,9 @@ static int __devinit s3c2410wdt_probe(struct platform_device *pdev) return 0; + err_reg: + watchdog_unregister_device(&s3c2410_wdd); + err_cpufreq: s3c2410wdt_cpufreq_deregister(); @@ -413,9 +417,6 @@ static int __devinit s3c2410wdt_probe(struct platform_device *pdev) clk_disable(wdt_clock); clk_put(wdt_clock); - err_irq: - free_irq(wdt_irq->start, pdev); - err_map: iounmap(wdt_base); @@ -428,6 +429,9 @@ static int __devinit s3c2410wdt_probe(struct platform_device *pdev) static int __devexit s3c2410wdt_remove(struct platform_device *dev) { + free_irq(wdt_irq->start, dev); + wdt_irq = NULL; + watchdog_unregister_device(&s3c2410_wdd); s3c2410wdt_cpufreq_deregister(); @@ -436,9 +440,6 @@ static int __devexit s3c2410wdt_remove(struct platform_device *dev) clk_put(wdt_clock); wdt_clock = NULL; - free_irq(wdt_irq->start, dev); - wdt_irq = NULL; - iounmap(wdt_base); release_mem_region(wdt_mem->start, resource_size(wdt_mem)); -- 1.7.4.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH] watchdog: fix error in probe() of s3c2410_wdt (reset at booting) 2012-01-13 5:14 [PATCH] watchdog: fix error in probe() of s3c2410_wdt (reset at booting) MyungJoo Ham @ 2012-01-13 11:11 ` Russell King - ARM Linux 2012-01-13 21:45 ` Wim Van Sebroeck 0 siblings, 1 reply; 6+ messages in thread From: Russell King - ARM Linux @ 2012-01-13 11:11 UTC (permalink / raw) To: linux-arm-kernel On Fri, Jan 13, 2012 at 02:14:23PM +0900, MyungJoo Ham wrote: > Probe function of s3c2410 watchdog calls request_irq before initializing > required value (wdt_count). This incurs resetting watchdog counter value > and watchdog-reboot during booting up. > > This patch addresses such an issue by calling request_irq later. > > Error handling in probe function and calling oder in remove function are > also revised accordingly. On the other hand, this patch violates the general principle that you set the device up _before_ you publish it to userspace. In the case where you add the watchdog device, but then the subsequent request_irq() fails, there is a window where the watchdog device could be opened and started. You then fall out through the error paths, and the module could be removed. Don't do this. Always setup the device first. Then publish it. Never the other way around. ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] watchdog: fix error in probe() of s3c2410_wdt (reset at booting) 2012-01-13 11:11 ` Russell King - ARM Linux @ 2012-01-13 21:45 ` Wim Van Sebroeck 2012-01-17 2:00 ` MyungJoo Ham 0 siblings, 1 reply; 6+ messages in thread From: Wim Van Sebroeck @ 2012-01-13 21:45 UTC (permalink / raw) To: linux-arm-kernel Hi, On Fri, Jan 13, 2012 at 11:11:17AM +0000, Russell King - ARM Linux wrote: > On Fri, Jan 13, 2012 at 02:14:23PM +0900, MyungJoo Ham wrote: > > Probe function of s3c2410 watchdog calls request_irq before initializing > > required value (wdt_count). This incurs resetting watchdog counter value > > and watchdog-reboot during booting up. > > > > This patch addresses such an issue by calling request_irq later. > > > > Error handling in probe function and calling oder in remove function are > > also revised accordingly. > > On the other hand, this patch violates the general principle that you set > the device up _before_ you publish it to userspace. > > In the case where you add the watchdog device, but then the subsequent > request_irq() fails, there is a window where the watchdog device could > be opened and started. You then fall out through the error paths, and > the module could be removed. > > Don't do this. Always setup the device first. Then publish it. Never > the other way around. Russell was faster in replying, but I would have given the same NAK: the registering of the watchdog means that userspace get access/control over the watchdog device. So the request_irq should be done before that. In your trouble description you wrote: "Probe function of s3c2410 watchdog calls request_irq before initializing required value (wdt_count)." Should we not fix the initialization of the wdt_count value instead of the request_irq? i will have a look at the code somewhere this weekend. Mvg, Wim. ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] watchdog: fix error in probe() of s3c2410_wdt (reset at booting) 2012-01-13 21:45 ` Wim Van Sebroeck @ 2012-01-17 2:00 ` MyungJoo Ham 2012-01-30 19:01 ` Wim Van Sebroeck 0 siblings, 1 reply; 6+ messages in thread From: MyungJoo Ham @ 2012-01-17 2:00 UTC (permalink / raw) To: linux-arm-kernel On Sat, Jan 14, 2012 at 6:45 AM, Wim Van Sebroeck <wim@iguana.be> wrote: > Hi, > > On Fri, Jan 13, 2012 at 11:11:17AM +0000, Russell King - ARM Linux wrote: > >> On Fri, Jan 13, 2012 at 02:14:23PM +0900, MyungJoo Ham wrote: >> > Probe function of s3c2410 watchdog calls request_irq before initializing >> > required value (wdt_count). This incurs resetting watchdog counter value >> > and watchdog-reboot during booting up. >> > >> > This patch addresses such an issue by calling request_irq later. >> > >> > Error handling in probe function and calling oder in remove function are >> > also revised accordingly. >> >> On the other hand, this patch violates the general principle that you set >> the device up _before_ you publish it to userspace. >> >> In the case where you add the watchdog device, but then the subsequent >> request_irq() fails, there is a window where the watchdog device could >> be opened and started. ?You then fall out through the error paths, and >> the module could be removed. >> >> Don't do this. ?Always setup the device first. ?Then publish it. ?Never >> the other way around. > > Russell was faster in replying, but I would have given the same NAK: > the registering of the watchdog means that userspace get access/control over > the watchdog device. So the request_irq should be done before that. > > In your trouble description you wrote: "Probe function of s3c2410 watchdog > calls request_irq before initializing required value (wdt_count)." > Should we not fix the initialization of the wdt_count value instead of the > request_irq? i will have a look at the code somewhere this weekend. > > Mvg, > Wim. > Yes, if we set wdt_count value properly before request_irq, it's fine. Anyway, as registering the device (watchdog_register_device()) may be done after request_irq without any problem, moving request_irq right before it, not after it should be fine. Thank you. Cheers! MyungJoo. -- MyungJoo Ham, Ph.D. Mobile Software Platform Lab, DMC Business, Samsung Electronics ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] watchdog: fix error in probe() of s3c2410_wdt (reset at booting) 2012-01-17 2:00 ` MyungJoo Ham @ 2012-01-30 19:01 ` Wim Van Sebroeck 2012-01-31 5:30 ` MyungJoo Ham 0 siblings, 1 reply; 6+ messages in thread From: Wim Van Sebroeck @ 2012-01-30 19:01 UTC (permalink / raw) To: linux-arm-kernel Hi, > Yes, if we set wdt_count value properly before request_irq, it's fine. > > Anyway, as registering the device (watchdog_register_device()) may be > done after request_irq without any problem, moving request_irq right > before it, not after it should be fine. So what that be attached patch then? Could you also test this patch? Kind regards, Wim. --- diff --git a/drivers/watchdog/s3c2410_wdt.c b/drivers/watchdog/s3c2410_wdt.c index 4bc3744..404172f 100644 --- a/drivers/watchdog/s3c2410_wdt.c +++ b/drivers/watchdog/s3c2410_wdt.c @@ -312,18 +312,26 @@ static int __devinit s3c2410wdt_probe(struct platform_device *pdev) dev = &pdev->dev; wdt_dev = &pdev->dev; - /* get the memory region for the watchdog timer */ - wdt_mem = platform_get_resource(pdev, IORESOURCE_MEM, 0); if (wdt_mem == NULL) { dev_err(dev, "no memory resource specified\n"); return -ENOENT; } + wdt_irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0); + if (wdt_irq == NULL) { + dev_err(dev, "no irq resource specified\n"); + ret = -ENOENT; + goto err; + } + + /* get the memory region for the watchdog timer */ + size = resource_size(wdt_mem); if (!request_mem_region(wdt_mem->start, size, pdev->name)) { dev_err(dev, "failed to get memory region\n"); - return -EBUSY; + ret = -EBUSY; + goto err; } wdt_base = ioremap(wdt_mem->start, size); @@ -335,29 +343,17 @@ static int __devinit s3c2410wdt_probe(struct platform_device *pdev) DBG("probe: mapped wdt_base=%p\n", wdt_base); - wdt_irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0); - if (wdt_irq == NULL) { - dev_err(dev, "no irq resource specified\n"); - ret = -ENOENT; - goto err_map; - } - - ret = request_irq(wdt_irq->start, s3c2410wdt_irq, 0, pdev->name, pdev); - if (ret != 0) { - dev_err(dev, "failed to install irq (%d)\n", ret); - goto err_map; - } - wdt_clock = clk_get(&pdev->dev, "watchdog"); if (IS_ERR(wdt_clock)) { dev_err(dev, "failed to find watchdog clock source\n"); ret = PTR_ERR(wdt_clock); - goto err_irq; + goto err_map; } clk_enable(wdt_clock); - if (s3c2410wdt_cpufreq_register() < 0) { + ret = s3c2410wdt_cpufreq_register(); + if (ret < 0) { printk(KERN_ERR PFX "failed to register cpufreq\n"); goto err_clk; } @@ -378,12 +374,18 @@ static int __devinit s3c2410wdt_probe(struct platform_device *pdev) "cannot start\n"); } + ret = request_irq(wdt_irq->start, s3c2410wdt_irq, 0, pdev->name, pdev); + if (ret != 0) { + dev_err(dev, "failed to install irq (%d)\n", ret); + goto err_cpufreq; + } + watchdog_set_nowayout(&s3c2410_wdd, nowayout); ret = watchdog_register_device(&s3c2410_wdd); if (ret) { dev_err(dev, "cannot register watchdog (%d)\n", ret); - goto err_cpufreq; + goto err_irq; } if (tmr_atboot && started == 0) { @@ -408,23 +410,26 @@ static int __devinit s3c2410wdt_probe(struct platform_device *pdev) return 0; + err_irq: + free_irq(wdt_irq->start, pdev); + err_cpufreq: s3c2410wdt_cpufreq_deregister(); err_clk: clk_disable(wdt_clock); clk_put(wdt_clock); - - err_irq: - free_irq(wdt_irq->start, pdev); + wdt_clock = NULL; err_map: iounmap(wdt_base); err_req: release_mem_region(wdt_mem->start, size); - wdt_mem = NULL; + err: + wdt_irq = NULL; + wdt_mem = NULL; return ret; } @@ -432,18 +437,18 @@ static int __devexit s3c2410wdt_remove(struct platform_device *dev) { watchdog_unregister_device(&s3c2410_wdd); + free_irq(wdt_irq->start, dev); + s3c2410wdt_cpufreq_deregister(); clk_disable(wdt_clock); clk_put(wdt_clock); wdt_clock = NULL; - free_irq(wdt_irq->start, dev); - wdt_irq = NULL; - iounmap(wdt_base); release_mem_region(wdt_mem->start, resource_size(wdt_mem)); + wdt_irq = NULL; wdt_mem = NULL; return 0; } ^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH] watchdog: fix error in probe() of s3c2410_wdt (reset at booting) 2012-01-30 19:01 ` Wim Van Sebroeck @ 2012-01-31 5:30 ` MyungJoo Ham 0 siblings, 0 replies; 6+ messages in thread From: MyungJoo Ham @ 2012-01-31 5:30 UTC (permalink / raw) To: linux-arm-kernel On Tue, Jan 31, 2012 at 4:01 AM, Wim Van Sebroeck <wim@iguana.be> wrote: > Hi, > >> Yes, if we set wdt_count value properly before request_irq, it's fine. >> >> Anyway, as registering the device (watchdog_register_device()) may be >> done after request_irq without any problem, moving request_irq right >> before it, not after it should be fine. > > So what that be attached patch then? Could you also test this patch? This patch looks fine and works. It's tested in Exynos4 machine. Acked-by: MyungJoo Ham <myungjoo.ham@samsung.com> Thanks. Cheers! MyungJoo. > > Kind regards, > Wim. > --- > diff --git a/drivers/watchdog/s3c2410_wdt.c b/drivers/watchdog/s3c2410_wdt.c > index 4bc3744..404172f 100644 > --- a/drivers/watchdog/s3c2410_wdt.c > +++ b/drivers/watchdog/s3c2410_wdt.c > @@ -312,18 +312,26 @@ static int __devinit s3c2410wdt_probe(struct platform_device *pdev) > ? ? ? ?dev = &pdev->dev; > ? ? ? ?wdt_dev = &pdev->dev; > > - ? ? ? /* get the memory region for the watchdog timer */ > - > ? ? ? ?wdt_mem = platform_get_resource(pdev, IORESOURCE_MEM, 0); > ? ? ? ?if (wdt_mem == NULL) { > ? ? ? ? ? ? ? ?dev_err(dev, "no memory resource specified\n"); > ? ? ? ? ? ? ? ?return -ENOENT; > ? ? ? ?} > > + ? ? ? wdt_irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0); > + ? ? ? if (wdt_irq == NULL) { > + ? ? ? ? ? ? ? dev_err(dev, "no irq resource specified\n"); > + ? ? ? ? ? ? ? ret = -ENOENT; > + ? ? ? ? ? ? ? goto err; > + ? ? ? } > + > + ? ? ? /* get the memory region for the watchdog timer */ > + > ? ? ? ?size = resource_size(wdt_mem); > ? ? ? ?if (!request_mem_region(wdt_mem->start, size, pdev->name)) { > ? ? ? ? ? ? ? ?dev_err(dev, "failed to get memory region\n"); > - ? ? ? ? ? ? ? return -EBUSY; > + ? ? ? ? ? ? ? ret = -EBUSY; > + ? ? ? ? ? ? ? goto err; > ? ? ? ?} > > ? ? ? ?wdt_base = ioremap(wdt_mem->start, size); > @@ -335,29 +343,17 @@ static int __devinit s3c2410wdt_probe(struct platform_device *pdev) > > ? ? ? ?DBG("probe: mapped wdt_base=%p\n", wdt_base); > > - ? ? ? wdt_irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0); > - ? ? ? if (wdt_irq == NULL) { > - ? ? ? ? ? ? ? dev_err(dev, "no irq resource specified\n"); > - ? ? ? ? ? ? ? ret = -ENOENT; > - ? ? ? ? ? ? ? goto err_map; > - ? ? ? } > - > - ? ? ? ret = request_irq(wdt_irq->start, s3c2410wdt_irq, 0, pdev->name, pdev); > - ? ? ? if (ret != 0) { > - ? ? ? ? ? ? ? dev_err(dev, "failed to install irq (%d)\n", ret); > - ? ? ? ? ? ? ? goto err_map; > - ? ? ? } > - > ? ? ? ?wdt_clock = clk_get(&pdev->dev, "watchdog"); > ? ? ? ?if (IS_ERR(wdt_clock)) { > ? ? ? ? ? ? ? ?dev_err(dev, "failed to find watchdog clock source\n"); > ? ? ? ? ? ? ? ?ret = PTR_ERR(wdt_clock); > - ? ? ? ? ? ? ? goto err_irq; > + ? ? ? ? ? ? ? goto err_map; > ? ? ? ?} > > ? ? ? ?clk_enable(wdt_clock); > > - ? ? ? if (s3c2410wdt_cpufreq_register() < 0) { > + ? ? ? ret = s3c2410wdt_cpufreq_register(); > + ? ? ? if (ret < 0) { > ? ? ? ? ? ? ? ?printk(KERN_ERR PFX "failed to register cpufreq\n"); > ? ? ? ? ? ? ? ?goto err_clk; > ? ? ? ?} > @@ -378,12 +374,18 @@ static int __devinit s3c2410wdt_probe(struct platform_device *pdev) > ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?"cannot start\n"); > ? ? ? ?} > > + ? ? ? ret = request_irq(wdt_irq->start, s3c2410wdt_irq, 0, pdev->name, pdev); > + ? ? ? if (ret != 0) { > + ? ? ? ? ? ? ? dev_err(dev, "failed to install irq (%d)\n", ret); > + ? ? ? ? ? ? ? goto err_cpufreq; > + ? ? ? } > + > ? ? ? ?watchdog_set_nowayout(&s3c2410_wdd, nowayout); > > ? ? ? ?ret = watchdog_register_device(&s3c2410_wdd); > ? ? ? ?if (ret) { > ? ? ? ? ? ? ? ?dev_err(dev, "cannot register watchdog (%d)\n", ret); > - ? ? ? ? ? ? ? goto err_cpufreq; > + ? ? ? ? ? ? ? goto err_irq; > ? ? ? ?} > > ? ? ? ?if (tmr_atboot && started == 0) { > @@ -408,23 +410,26 @@ static int __devinit s3c2410wdt_probe(struct platform_device *pdev) > > ? ? ? ?return 0; > > + err_irq: > + ? ? ? free_irq(wdt_irq->start, pdev); > + > ?err_cpufreq: > ? ? ? ?s3c2410wdt_cpufreq_deregister(); > > ?err_clk: > ? ? ? ?clk_disable(wdt_clock); > ? ? ? ?clk_put(wdt_clock); > - > - err_irq: > - ? ? ? free_irq(wdt_irq->start, pdev); > + ? ? ? wdt_clock = NULL; > > ?err_map: > ? ? ? ?iounmap(wdt_base); > > ?err_req: > ? ? ? ?release_mem_region(wdt_mem->start, size); > - ? ? ? wdt_mem = NULL; > > + err: > + ? ? ? wdt_irq = NULL; > + ? ? ? wdt_mem = NULL; > ? ? ? ?return ret; > ?} > > @@ -432,18 +437,18 @@ static int __devexit s3c2410wdt_remove(struct platform_device *dev) > ?{ > ? ? ? ?watchdog_unregister_device(&s3c2410_wdd); > > + ? ? ? free_irq(wdt_irq->start, dev); > + > ? ? ? ?s3c2410wdt_cpufreq_deregister(); > > ? ? ? ?clk_disable(wdt_clock); > ? ? ? ?clk_put(wdt_clock); > ? ? ? ?wdt_clock = NULL; > > - ? ? ? free_irq(wdt_irq->start, dev); > - ? ? ? wdt_irq = NULL; > - > ? ? ? ?iounmap(wdt_base); > > ? ? ? ?release_mem_region(wdt_mem->start, resource_size(wdt_mem)); > + ? ? ? wdt_irq = NULL; > ? ? ? ?wdt_mem = NULL; > ? ? ? ?return 0; > ?} -- MyungJoo Ham, Ph.D. Mobile Software Platform Lab, DMC Business, Samsung Electronics ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2012-01-31 5:30 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2012-01-13 5:14 [PATCH] watchdog: fix error in probe() of s3c2410_wdt (reset at booting) MyungJoo Ham 2012-01-13 11:11 ` Russell King - ARM Linux 2012-01-13 21:45 ` Wim Van Sebroeck 2012-01-17 2:00 ` MyungJoo Ham 2012-01-30 19:01 ` Wim Van Sebroeck 2012-01-31 5:30 ` MyungJoo Ham
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).