[PATCH 0/5] watchdog: dw_wdt: allow only one probe + various fixes

[PATCH 0/5] watchdog: dw_wdt: allow only one probe + various fixes

[Ian Abbott]

The dw_wdt watchdog driver supports a single DesignWare watchdog device
using a statically allocated variable 'dw_wdt' to manage it, but there
are some bits of hardware containing two such devices.  If the "probe"
is called for more than one device, the 'dw_wdt' variable gets
clobbered, generally resulting in a kernel crash.  Change it to reject
additional probed devices.

We probably don't expect the "remove" function to get called, but if it
does, make sure the Linux timer used by device gets unqueued.

The timer also gets unqueued if the watchdog device is closed by the
user process unexpectedly.  However, this is not currently done
asynchronously and the timer function normally requeues the timer.  If
it manages to requeue the timer, the next call of the timer function
will behave as though the close was expected and continue patting the
dog every half a second when it should have stopped doing so.  Unqueue
the timer synchronously to avoid that.

NOTE: There is another potential problem that I haven't bothered to fix.
If the "remove" function is called while the watchdog device is still
open, any in-progress or future file operations are likely to screw
things up.

1) watchdog: dw_wdt: prepare for more atomic bits
2) watchdog: dw_wdt: reject additional device probes
3) watchdog: dw_wdt: only unregister restart handler if registered
4) watchdog: dw_wdt: unqueue timer on device removal
5) watchdog: dw_wdt: unqueue timer synchronously on unexpected close

 drivers/watchdog/dw_wdt.c | 46 +++++++++++++++++++++++++++++++++++-----------
 1 file changed, 35 insertions(+), 11 deletions(-)

[PATCH 1/5] watchdog: dw_wdt: prepare for more atomic bits

[Ian Abbott]

Bit 0 of `dw_wdt.in_use` is tested and set atomically to indicate that
the watchdog has been opened exclusively.  The other bits are unused.
Some parts of the code also treat the whole of the `in_use` member as a
boolean value.

In preparation for making use of the unused bits, rename the `in_use`
member to `state`, define a new macro `DW_WDT_IN_USE` to identify bit 0
as the "in-use" bit, and replace conditional tests on the whole of
`in_use` with single-bit tests on the appropriate bit of `state`.

Signed-off-by: Ian Abbott <abbotti@mev.co.uk>
---
 drivers/watchdog/dw_wdt.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/watchdog/dw_wdt.c b/drivers/watchdog/dw_wdt.c
index 6ea0634..249ef09 100644
--- a/drivers/watchdog/dw_wdt.c
+++ b/drivers/watchdog/dw_wdt.c
@@ -59,10 +59,13 @@ MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started "
 
 #define WDT_TIMEOUT		(HZ / 2)
 
+/* dw_wdt.state bit numbers */
+#define DW_WDT_IN_USE		0
+
 static struct {
 	void __iomem		*regs;
 	struct clk		*clk;
-	unsigned long		in_use;
+	unsigned long		state;
 	unsigned long		next_heartbeat;
 	struct timer_list	timer;
 	int			expect_close;
@@ -160,7 +163,7 @@ static int dw_wdt_restart_handle(struct notifier_block *this,
 static void dw_wdt_ping(unsigned long data)
 {
 	if (time_before(jiffies, dw_wdt.next_heartbeat) ||
-	    (!nowayout && !dw_wdt.in_use)) {
+	    (!nowayout && !test_bit(DW_WDT_IN_USE, &dw_wdt.state))) {
 		dw_wdt_keepalive();
 		mod_timer(&dw_wdt.timer, jiffies + WDT_TIMEOUT);
 	} else
@@ -169,7 +172,7 @@ static void dw_wdt_ping(unsigned long data)
 
 static int dw_wdt_open(struct inode *inode, struct file *filp)
 {
-	if (test_and_set_bit(0, &dw_wdt.in_use))
+	if (test_and_set_bit(DW_WDT_IN_USE, &dw_wdt.state))
 		return -EBUSY;
 
 	/* Make sure we don't get unloaded. */
@@ -273,7 +276,7 @@ static long dw_wdt_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 
 static int dw_wdt_release(struct inode *inode, struct file *filp)
 {
-	clear_bit(0, &dw_wdt.in_use);
+	clear_bit(DW_WDT_IN_USE, &dw_wdt.state);
 
 	if (!dw_wdt.expect_close) {
 		del_timer(&dw_wdt.timer);
-- 
2.1.4

[PATCH 2/5] watchdog: dw_wdt: reject additional device probes

[Ian Abbott]

Certain Altera FPGAs have an ARM-based Hard Processor System (HPS) that
includes two DesignWare Watchdog Timer peripherals.  Problems occur if
both are enabled in the device tree as the "dw_wdt" driver only supports
a single device, using a static variable (`dw_wdt`) to hold its state.
Typically, the timer queue gets corrupted, leading to a kernel Oops.

Use bit 1 of `dw_wdt.state` to indicate that a device has already been
probed, and reject attempts to probe extra devices.

Signed-off-by: Ian Abbott <abbotti@mev.co.uk>
---
 drivers/watchdog/dw_wdt.c | 25 ++++++++++++++++++++-----
 1 file changed, 20 insertions(+), 5 deletions(-)

diff --git a/drivers/watchdog/dw_wdt.c b/drivers/watchdog/dw_wdt.c
index 249ef09..cece633 100644
--- a/drivers/watchdog/dw_wdt.c
+++ b/drivers/watchdog/dw_wdt.c
@@ -61,6 +61,7 @@ MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started "
 
 /* dw_wdt.state bit numbers */
 #define DW_WDT_IN_USE		0
+#define DW_WDT_PROBED		1
 
 static struct {
 	void __iomem		*regs;
@@ -335,17 +336,26 @@ static int dw_wdt_drv_probe(struct platform_device *pdev)
 	int ret;
 	struct resource *mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 
+	if (test_and_set_bit(DW_WDT_PROBED, &dw_wdt.state)) {
+		pr_err("This driver only supports one device\n");
+		return -ENODEV;
+	}
+
 	dw_wdt.regs = devm_ioremap_resource(&pdev->dev, mem);
-	if (IS_ERR(dw_wdt.regs))
-		return PTR_ERR(dw_wdt.regs);
+	if (IS_ERR(dw_wdt.regs)) {
+		ret = PTR_ERR(dw_wdt.regs);
+		goto out_unmark_probed;
+	}
 
 	dw_wdt.clk = devm_clk_get(&pdev->dev, NULL);
-	if (IS_ERR(dw_wdt.clk))
-		return PTR_ERR(dw_wdt.clk);
+	if (IS_ERR(dw_wdt.clk)) {
+		ret = PTR_ERR(dw_wdt.clk);
+		goto out_unmark_probed;
+	}
 
 	ret = clk_prepare_enable(dw_wdt.clk);
 	if (ret)
-		return ret;
+		goto out_unmark_probed;
 
 	ret = misc_register(&dw_wdt_miscdev);
 	if (ret)
@@ -366,6 +376,9 @@ static int dw_wdt_drv_probe(struct platform_device *pdev)
 out_disable_clk:
 	clk_disable_unprepare(dw_wdt.clk);
 
+out_unmark_probed:
+	clear_bit(DW_WDT_PROBED, &dw_wdt.state);
+
 	return ret;
 }
 
@@ -377,6 +390,8 @@ static int dw_wdt_drv_remove(struct platform_device *pdev)
 
 	clk_disable_unprepare(dw_wdt.clk);
 
+	clear_bit(DW_WDT_PROBED, &dw_wdt.state);
+
 	return 0;
 }
 
-- 
2.1.4

[PATCH 3/5] watchdog: dw_wdt: only unregister restart handler if registered

[Ian Abbott]

If the platform driver "probe" function fails to register a restart
handler, it issues a warning and continues regardless.  The
corresponding "remove" function unregisters the restart handler
regardless.

Use bit 2 of `dw_wdt.state` to indicate whether the restart handler was
registered successfully in the "probe" function, and only unregister it
in the "remove" function if so.

Signed-off-by: Ian Abbott <abbotti@mev.co.uk>
---
 drivers/watchdog/dw_wdt.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/watchdog/dw_wdt.c b/drivers/watchdog/dw_wdt.c
index cece633..1cd877b 100644
--- a/drivers/watchdog/dw_wdt.c
+++ b/drivers/watchdog/dw_wdt.c
@@ -62,6 +62,7 @@ MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started "
 /* dw_wdt.state bit numbers */
 #define DW_WDT_IN_USE		0
 #define DW_WDT_PROBED		1
+#define DW_WDT_RESTART_HANDLER	2
 
 static struct {
 	void __iomem		*regs;
@@ -366,6 +367,8 @@ static int dw_wdt_drv_probe(struct platform_device *pdev)
 	ret = register_restart_handler(&dw_wdt.restart_handler);
 	if (ret)
 		pr_warn("cannot register restart handler\n");
+	else
+		set_bit(DW_WDT_RESTART_HANDLER, &dw_wdt.state);
 
 	dw_wdt_set_next_heartbeat();
 	setup_timer(&dw_wdt.timer, dw_wdt_ping, 0);
@@ -384,7 +387,8 @@ out_unmark_probed:
 
 static int dw_wdt_drv_remove(struct platform_device *pdev)
 {
-	unregister_restart_handler(&dw_wdt.restart_handler);
+	if (test_and_clear_bit(DW_WDT_RESTART_HANDLER, &dw_wdt.state))
+		unregister_restart_handler(&dw_wdt.restart_handler);
 
 	misc_deregister(&dw_wdt_miscdev);
 
-- 
2.1.4

[PATCH 4/5] watchdog: dw_wdt: unqueue timer on device removal

[Ian Abbott]

The watchdog device's timer may be queued when the platform driver
"remove" function is called.  Call `del_timer_sync` to remove it from
the queue.

Signed-off-by: Ian Abbott <abbotti@mev.co.uk>
---
 drivers/watchdog/dw_wdt.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/watchdog/dw_wdt.c b/drivers/watchdog/dw_wdt.c
index 1cd877b..daab90b 100644
--- a/drivers/watchdog/dw_wdt.c
+++ b/drivers/watchdog/dw_wdt.c
@@ -387,6 +387,8 @@ out_unmark_probed:
 
 static int dw_wdt_drv_remove(struct platform_device *pdev)
 {
+	del_timer_sync(&dw_wdt.timer);
+
 	if (test_and_clear_bit(DW_WDT_RESTART_HANDLER, &dw_wdt.state))
 		unregister_restart_handler(&dw_wdt.restart_handler);
 
-- 
2.1.4

[PATCH 5/5] watchdog: dw_wdt: unqueue timer synchronously on unexpected close

[Ian Abbott]

The driver normally calls the timer function `dw_wdt_ping()` every half
a second to pat the dog while it is not in use, and while waiting for
the next heartbeat from the user that opened the device.  If the
watchdog device's "release" file operation determines that the watchdog
device is being closed unexpectedly, it calls `del_timer()` to remove
the timer from the queue and thus stop patting the dog.  However, since
the timer function re-queues the timer, there is a race condition on SMP
systems which could leave the timer re-queueing itself when it
shouldn't.  To prevent that, call `del_timer_sync()` instead of
`del_timer()`.

Signed-off-by: Ian Abbott <abbotti@mev.co.uk>
---
 drivers/watchdog/dw_wdt.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/watchdog/dw_wdt.c b/drivers/watchdog/dw_wdt.c
index daab90b..bf617e3 100644
--- a/drivers/watchdog/dw_wdt.c
+++ b/drivers/watchdog/dw_wdt.c
@@ -281,7 +281,7 @@ static int dw_wdt_release(struct inode *inode, struct file *filp)
 	clear_bit(DW_WDT_IN_USE, &dw_wdt.state);
 
 	if (!dw_wdt.expect_close) {
-		del_timer(&dw_wdt.timer);
+		del_timer_sync(&dw_wdt.timer);
 
 		if (!nowayout)
 			pr_crit("unexpected close, system will reboot soon\n");
-- 
2.1.4

Re: [PATCH 0/5] watchdog: dw_wdt: allow only one probe + various fixes

[Guenter Roeck]

On 06/26/2015 06:21 AM, Ian Abbott wrote:
> The dw_wdt watchdog driver supports a single DesignWare watchdog device
> using a statically allocated variable 'dw_wdt' to manage it, but there
> are some bits of hardware containing two such devices.  If the "probe"
> is called for more than one device, the 'dw_wdt' variable gets
> clobbered, generally resulting in a kernel crash.  Change it to reject
> additional probed devices.
>
> We probably don't expect the "remove" function to get called, but if it
> does, make sure the Linux timer used by device gets unqueued.
>
> The timer also gets unqueued if the watchdog device is closed by the
> user process unexpectedly.  However, this is not currently done
> asynchronously and the timer function normally requeues the timer.  If
> it manages to requeue the timer, the next call of the timer function
> will behave as though the close was expected and continue patting the
> dog every half a second when it should have stopped doing so.  Unqueue
> the timer synchronously to avoid that.
>
> NOTE: There is another potential problem that I haven't bothered to fix.
> If the "remove" function is called while the watchdog device is still
> open, any in-progress or future file operations are likely to screw
> things up.
>
> 1) watchdog: dw_wdt: prepare for more atomic bits
> 2) watchdog: dw_wdt: reject additional device probes
> 3) watchdog: dw_wdt: only unregister restart handler if registered
> 4) watchdog: dw_wdt: unqueue timer on device removal
> 5) watchdog: dw_wdt: unqueue timer synchronously on unexpected close
>
>   drivers/watchdog/dw_wdt.c | 46 +++++++++++++++++++++++++++++++++++-----------
>   1 file changed, 35 insertions(+), 11 deletions(-)

Hi Ian,

any chance you can convert the driver to use the watchdog APIs instead ?
This way it could support more than one watchdog, and the code would be
much simpler.

Thanks,
Guenter