[PATCH 1/4] watchdog: imgpdc: Fix NULL pointer dereference on probe

[PATCH 1/4] watchdog: imgpdc: Fix NULL pointer dereference on probe

[Andrew Bresticker]

From: Naidu Tellapati <naidu.tellapati@imgtec.com>

pdc_wdt_probe() called pdc_wdt_stop() before watchdog_set_drvdata(),
resulting in the following NULL pointer dereference:

  CPU 0 Unable to handle kernel paging request at virtual address 0000008c, epc == 8082a2b8, ra == 8082a914
  Oops[#1]:
  CPU: 0 PID: 1 Comm: swapper/0 Not tainted 4.0.0-rc5+ #102
  task: 8f8c0000 ti: 8f89e000 task.ti: 8f89e000
  $ 0   : 00000000 8082a914 00000000 dc8ba200
  $ 4   : 8faf9418 80ac0000 8f9cdc10 00008012
  $ 8   : 00000000 806e2350 00000001 8f001880
  $12   : 8f89fd14 00000000 00000000 65646f4d
  $16   : 8faf9418 8f9cdc10 8f001880 8f9cdc00
  $20   : 00000000 80c20000 00000000 80bc0000
  $24   : 00000002 00000000
  $28   : 8f89e000 8f89fd00 00000000 8082a914
  Hi    : 0000006b
  Lo    : 00008013
  epc   : 8082a2b8 pdc_wdt_stop+0x1c/0x44
      Not tainted
  ra    : 8082a914 pdc_wdt_probe+0x404/0x550
  Status: 11000403	KERNEL EXL IE
  Cause : 00800008
  BadVA : 0000008c
  PrId  : 0001a120 (MIPS interAptiv (multi))

Fix it by moving the call to pdc_wdt_stop() after we've set drvdata.

Signed-off-by: Naidu Tellapati <naidu.tellapati@imgtec.com>
Signed-off-by: Andrew Bresticker <abrestic@chromium.org>
Cc: Ezequiel Garcia <ezequiel.garcia@imgtec.com>
---
 drivers/watchdog/imgpdc_wdt.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/watchdog/imgpdc_wdt.c b/drivers/watchdog/imgpdc_wdt.c
index c8def68..c4151cd 100644
--- a/drivers/watchdog/imgpdc_wdt.c
+++ b/drivers/watchdog/imgpdc_wdt.c
@@ -199,8 +199,6 @@ static int pdc_wdt_probe(struct platform_device *pdev)
 			 "Initial timeout out of range! setting max timeout\n");
 	}
 
-	pdc_wdt_stop(&pdc_wdt->wdt_dev);
-
 	/* Find what caused the last reset */
 	val = readl(pdc_wdt->base + PDC_WDT_TICKLE1);
 	val = (val & PDC_WDT_TICKLE_STATUS_MASK) >> PDC_WDT_TICKLE_STATUS_SHIFT;
@@ -234,6 +232,8 @@ static int pdc_wdt_probe(struct platform_device *pdev)
 	platform_set_drvdata(pdev, pdc_wdt);
 	watchdog_set_drvdata(&pdc_wdt->wdt_dev, pdc_wdt);
 
+	pdc_wdt_stop(&pdc_wdt->wdt_dev);
+
 	ret = watchdog_register_device(&pdc_wdt->wdt_dev);
 	if (ret)
 		goto disable_wdt_clk;
-- 
2.2.0.rc0.207.ga3a616c

[PATCH 2/4] watchdog: imgpdc: Intialize timeout to default value

[Andrew Bresticker]

From: Naidu Tellapati <naidu.tellapati@imgtec.com>

Currently the watchdog timeout is initialized to 0.  Initialize it to
its default value instead.

Signed-off-by: Naidu Tellapati <naidu.tellapati@imgtec.com>
Signed-off-by: Andrew Bresticker <abrestic@chromium.org>
Cc: Ezequiel Garcia <ezequiel.garcia@imgtec.com>
---
 drivers/watchdog/imgpdc_wdt.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/watchdog/imgpdc_wdt.c b/drivers/watchdog/imgpdc_wdt.c
index c4151cd..f3f65ac 100644
--- a/drivers/watchdog/imgpdc_wdt.c
+++ b/drivers/watchdog/imgpdc_wdt.c
@@ -42,7 +42,7 @@
 #define PDC_WDT_MIN_TIMEOUT		1
 #define PDC_WDT_DEF_TIMEOUT		64
 
-static int heartbeat;
+static int heartbeat = PDC_WDT_DEF_TIMEOUT;
 module_param(heartbeat, int, 0);
 MODULE_PARM_DESC(heartbeat, "Watchdog heartbeats in seconds. "
 	"(default = " __MODULE_STRING(PDC_WDT_DEF_TIMEOUT) ")");
-- 
2.2.0.rc0.207.ga3a616c

[PATCH 3/4] watchdog: imgpdc: Set timeout before starting watchdog

[Andrew Bresticker]

From: Naidu Tellapati <naidu.tellapati@imgtec.com>

Set up the watchdog for the specified timeout before attempting to start it.

Signed-off-by: Naidu Tellapati <naidu.tellapati@imgtec.com>
Signed-off-by: Andrew Bresticker <abrestic@chromium.org>
Cc: Ezequiel Garcia <ezequiel.garcia@imgtec.com>
---
 drivers/watchdog/imgpdc_wdt.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/watchdog/imgpdc_wdt.c b/drivers/watchdog/imgpdc_wdt.c
index f3f65ac..aef3596 100644
--- a/drivers/watchdog/imgpdc_wdt.c
+++ b/drivers/watchdog/imgpdc_wdt.c
@@ -106,6 +106,8 @@ static int pdc_wdt_start(struct watchdog_device *wdt_dev)
 	unsigned int val;
 	struct pdc_wdt_dev *wdt = watchdog_get_drvdata(wdt_dev);
 
+	pdc_wdt_set_timeout(&wdt->wdt_dev, wdt->wdt_dev.timeout);
+
 	val = readl(wdt->base + PDC_WDT_CONFIG);
 	val |= PDC_WDT_CONFIG_ENABLE;
 	writel(val, wdt->base + PDC_WDT_CONFIG);
-- 
2.2.0.rc0.207.ga3a616c

[PATCH 4/4] watchdog: imgpdc: Add reboot support

[Andrew Bresticker]

Register a restart handler that will restart the system by writing
to the watchdog's SOFT_RESET register.

Signed-off-by: Andrew Bresticker <abrestic@chromium.org>
Cc: Ezequiel Garcia <ezequiel.garcia@imgtec.com>
---
 drivers/watchdog/imgpdc_wdt.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/drivers/watchdog/imgpdc_wdt.c b/drivers/watchdog/imgpdc_wdt.c
index aef3596..d9eaee3 100644
--- a/drivers/watchdog/imgpdc_wdt.c
+++ b/drivers/watchdog/imgpdc_wdt.c
@@ -16,6 +16,7 @@
 #include <linux/log2.h>
 #include <linux/module.h>
 #include <linux/platform_device.h>
+#include <linux/reboot.h>
 #include <linux/slab.h>
 #include <linux/watchdog.h>
 
@@ -57,6 +58,7 @@ struct pdc_wdt_dev {
 	struct clk *wdt_clk;
 	struct clk *sys_clk;
 	void __iomem *base;
+	struct notifier_block restart_handler;
 };
 
 static int pdc_wdt_keepalive(struct watchdog_device *wdt_dev)
@@ -130,6 +132,18 @@ static const struct watchdog_ops pdc_wdt_ops = {
 	.set_timeout	= pdc_wdt_set_timeout,
 };
 
+static int pdc_wdt_restart(struct notifier_block *this, unsigned long mode,
+			   void *cmd)
+{
+	struct pdc_wdt_dev *wdt = container_of(this, struct pdc_wdt_dev,
+					       restart_handler);
+
+	/* Assert SOFT_RESET */
+	writel(0x1, wdt->base + PDC_WDT_SOFT_RESET);
+
+	return NOTIFY_OK;
+}
+
 static int pdc_wdt_probe(struct platform_device *pdev)
 {
 	int ret, val;
@@ -240,6 +254,13 @@ static int pdc_wdt_probe(struct platform_device *pdev)
 	if (ret)
 		goto disable_wdt_clk;
 
+	pdc_wdt->restart_handler.notifier_call = pdc_wdt_restart;
+	pdc_wdt->restart_handler.priority = 128;
+	ret = register_restart_handler(&pdc_wdt->restart_handler);
+	if (ret)
+		dev_warn(&pdev->dev, "failed to register restart handler: %d\n",
+			 ret);
+
 	return 0;
 
 disable_wdt_clk:
-- 
2.2.0.rc0.207.ga3a616c

Re: [PATCH 2/4] watchdog: imgpdc: Intialize timeout to default value

[Guenter Roeck]

On 03/31/2015 11:49 AM, Andrew Bresticker wrote:
> From: Naidu Tellapati <naidu.tellapati@imgtec.com>
>
> Currently the watchdog timeout is initialized to 0.  Initialize it to
> its default value instead.
>
> Signed-off-by: Naidu Tellapati <naidu.tellapati@imgtec.com>
> Signed-off-by: Andrew Bresticker <abrestic@chromium.org>
> Cc: Ezequiel Garcia <ezequiel.garcia@imgtec.com>
> ---
>   drivers/watchdog/imgpdc_wdt.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/watchdog/imgpdc_wdt.c b/drivers/watchdog/imgpdc_wdt.c
> index c4151cd..f3f65ac 100644
> --- a/drivers/watchdog/imgpdc_wdt.c
> +++ b/drivers/watchdog/imgpdc_wdt.c
> @@ -42,7 +42,7 @@
>   #define PDC_WDT_MIN_TIMEOUT		1
>   #define PDC_WDT_DEF_TIMEOUT		64
>
> -static int heartbeat;
> +static int heartbeat = PDC_WDT_DEF_TIMEOUT;
>   module_param(heartbeat, int, 0);
>   MODULE_PARM_DESC(heartbeat, "Watchdog heartbeats in seconds. "
>   	"(default = " __MODULE_STRING(PDC_WDT_DEF_TIMEOUT) ")");
>

The idea with watchdog_init_timeout() is that it can take the
timeout from devicetree unless the module parameter is provided.
By pre-initializing the module parameter, you defeat that and
make watchdog_init_timeout more or less a no-op. You might as well
set pdc_wdt->wdt_dev.timeout directly and not call watchdog_init_timeout
at all if that is what you want.

The "expected" solution would be to pre-initialize pdc_wdt->wdt_dev.timeout
but leave the module parameter alone.

Guenter

Re: [PATCH 2/4] watchdog: imgpdc: Intialize timeout to default value

[Andrew Bresticker]

On Tue, Mar 31, 2015 at 12:02 PM, Guenter Roeck <linux@roeck-us.net> wrote:
> On 03/31/2015 11:49 AM, Andrew Bresticker wrote:
>>
>> From: Naidu Tellapati <naidu.tellapati@imgtec.com>
>>
>> Currently the watchdog timeout is initialized to 0.  Initialize it to
>> its default value instead.
>>
>> Signed-off-by: Naidu Tellapati <naidu.tellapati@imgtec.com>
>> Signed-off-by: Andrew Bresticker <abrestic@chromium.org>
>> Cc: Ezequiel Garcia <ezequiel.garcia@imgtec.com>
>> ---
>>   drivers/watchdog/imgpdc_wdt.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/watchdog/imgpdc_wdt.c b/drivers/watchdog/imgpdc_wdt.c
>> index c4151cd..f3f65ac 100644
>> --- a/drivers/watchdog/imgpdc_wdt.c
>> +++ b/drivers/watchdog/imgpdc_wdt.c
>> @@ -42,7 +42,7 @@
>>   #define PDC_WDT_MIN_TIMEOUT           1
>>   #define PDC_WDT_DEF_TIMEOUT           64
>>
>> -static int heartbeat;
>> +static int heartbeat = PDC_WDT_DEF_TIMEOUT;
>>   module_param(heartbeat, int, 0);
>>   MODULE_PARM_DESC(heartbeat, "Watchdog heartbeats in seconds. "
>>         "(default = " __MODULE_STRING(PDC_WDT_DEF_TIMEOUT) ")");
>>
>
> The idea with watchdog_init_timeout() is that it can take the
> timeout from devicetree unless the module parameter is provided.
> By pre-initializing the module parameter, you defeat that and
> make watchdog_init_timeout more or less a no-op. You might as well
> set pdc_wdt->wdt_dev.timeout directly and not call watchdog_init_timeout
> at all if that is what you want.
>
> The "expected" solution would be to pre-initialize pdc_wdt->wdt_dev.timeout
> but leave the module parameter alone.

Ah, right - that would be better.  It looks like a patch that is
almost identical to mine has already been accepted - see linux-next
commit c774d71 ["watchdog: imgpdc: Fix default heartbeat"] - but I can
follow up with a patch that uses the approach you suggested.

Thanks,
Andrew

Re: [PATCH 1/4] watchdog: imgpdc: Fix NULL pointer dereference on probe

[Andrew Bresticker]

On Tue, Mar 31, 2015 at 11:49 AM, Andrew Bresticker
<abrestic@chromium.org> wrote:
> From: Naidu Tellapati <naidu.tellapati@imgtec.com>
>
> pdc_wdt_probe() called pdc_wdt_stop() before watchdog_set_drvdata(),
> resulting in the following NULL pointer dereference:
>
>   CPU 0 Unable to handle kernel paging request at virtual address 0000008c, epc == 8082a2b8, ra == 8082a914
>   Oops[#1]:
>   CPU: 0 PID: 1 Comm: swapper/0 Not tainted 4.0.0-rc5+ #102
>   task: 8f8c0000 ti: 8f89e000 task.ti: 8f89e000
>   $ 0   : 00000000 8082a914 00000000 dc8ba200
>   $ 4   : 8faf9418 80ac0000 8f9cdc10 00008012
>   $ 8   : 00000000 806e2350 00000001 8f001880
>   $12   : 8f89fd14 00000000 00000000 65646f4d
>   $16   : 8faf9418 8f9cdc10 8f001880 8f9cdc00
>   $20   : 00000000 80c20000 00000000 80bc0000
>   $24   : 00000002 00000000
>   $28   : 8f89e000 8f89fd00 00000000 8082a914
>   Hi    : 0000006b
>   Lo    : 00008013
>   epc   : 8082a2b8 pdc_wdt_stop+0x1c/0x44
>       Not tainted
>   ra    : 8082a914 pdc_wdt_probe+0x404/0x550
>   Status: 11000403      KERNEL EXL IE
>   Cause : 00800008
>   BadVA : 0000008c
>   PrId  : 0001a120 (MIPS interAptiv (multi))
>
> Fix it by moving the call to pdc_wdt_stop() after we've set drvdata.

Disregard this patch and patch 2 - it looks like James Hogan beat me
to it :).  Patches 3 and 4 still apply I think.

Re: [PATCH 3/4] watchdog: imgpdc: Set timeout before starting watchdog

[Guenter Roeck]

On 03/31/2015 11:49 AM, Andrew Bresticker wrote:
> From: Naidu Tellapati <naidu.tellapati@imgtec.com>
>
> Set up the watchdog for the specified timeout before attempting to start it.
>
> Signed-off-by: Naidu Tellapati <naidu.tellapati@imgtec.com>
> Signed-off-by: Andrew Bresticker <abrestic@chromium.org>
> Cc: Ezequiel Garcia <ezequiel.garcia@imgtec.com>
> ---
>   drivers/watchdog/imgpdc_wdt.c | 2 ++
>   1 file changed, 2 insertions(+)
>
> diff --git a/drivers/watchdog/imgpdc_wdt.c b/drivers/watchdog/imgpdc_wdt.c
> index f3f65ac..aef3596 100644
> --- a/drivers/watchdog/imgpdc_wdt.c
> +++ b/drivers/watchdog/imgpdc_wdt.c
> @@ -106,6 +106,8 @@ static int pdc_wdt_start(struct watchdog_device *wdt_dev)
>   	unsigned int val;
>   	struct pdc_wdt_dev *wdt = watchdog_get_drvdata(wdt_dev);
>
> +	pdc_wdt_set_timeout(&wdt->wdt_dev, wdt->wdt_dev.timeout);
> +
>   	val = readl(wdt->base + PDC_WDT_CONFIG);
>   	val |= PDC_WDT_CONFIG_ENABLE;
>   	writel(val, wdt->base + PDC_WDT_CONFIG);
>
It might be better to provide a helper function that doesn't set wdt->wdt_dev.timeout
again, or to just set the timeout with the write to PDC_WDT_CONFIG when starting
the watchdog.

Guenter

Re: [PATCH 4/4] watchdog: imgpdc: Add reboot support

[Guenter Roeck]

On 03/31/2015 11:49 AM, Andrew Bresticker wrote:
> Register a restart handler that will restart the system by writing
> to the watchdog's SOFT_RESET register.
>
> Signed-off-by: Andrew Bresticker <abrestic@chromium.org>
> Cc: Ezequiel Garcia <ezequiel.garcia@imgtec.com>

Reviewed-by: Guenter Roeck <linux@roeck-us.net>