* [PATCH 1/4] watchdog: imgpdc: Fix NULL pointer dereference on probe
@ 2015-03-31 18:49 Andrew Bresticker
2015-03-31 18:49 ` [PATCH 2/4] watchdog: imgpdc: Intialize timeout to default value Andrew Bresticker
` (3 more replies)
0 siblings, 4 replies; 9+ messages in thread
From: Andrew Bresticker @ 2015-03-31 18:49 UTC (permalink / raw)
To: Wim Van Sebroeck, Guenter Roeck
Cc: linux-watchdog, linux-kernel, Naidu Tellapati, Andrew Bresticker,
Ezequiel Garcia
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
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH 2/4] watchdog: imgpdc: Intialize timeout to default value
2015-03-31 18:49 [PATCH 1/4] watchdog: imgpdc: Fix NULL pointer dereference on probe Andrew Bresticker
@ 2015-03-31 18:49 ` Andrew Bresticker
2015-03-31 19:02 ` Guenter Roeck
2015-03-31 18:49 ` [PATCH 3/4] watchdog: imgpdc: Set timeout before starting watchdog Andrew Bresticker
` (2 subsequent siblings)
3 siblings, 1 reply; 9+ messages in thread
From: Andrew Bresticker @ 2015-03-31 18:49 UTC (permalink / raw)
To: Wim Van Sebroeck, Guenter Roeck
Cc: linux-watchdog, linux-kernel, Naidu Tellapati, Andrew Bresticker,
Ezequiel Garcia
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
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH 3/4] watchdog: imgpdc: Set timeout before starting watchdog
2015-03-31 18:49 [PATCH 1/4] watchdog: imgpdc: Fix NULL pointer dereference on probe Andrew Bresticker
2015-03-31 18:49 ` [PATCH 2/4] watchdog: imgpdc: Intialize timeout to default value Andrew Bresticker
@ 2015-03-31 18:49 ` Andrew Bresticker
2015-04-01 3:50 ` Guenter Roeck
2015-03-31 18:49 ` [PATCH 4/4] watchdog: imgpdc: Add reboot support Andrew Bresticker
2015-03-31 19:37 ` [PATCH 1/4] watchdog: imgpdc: Fix NULL pointer dereference on probe Andrew Bresticker
3 siblings, 1 reply; 9+ messages in thread
From: Andrew Bresticker @ 2015-03-31 18:49 UTC (permalink / raw)
To: Wim Van Sebroeck, Guenter Roeck
Cc: linux-watchdog, linux-kernel, Naidu Tellapati, Andrew Bresticker,
Ezequiel Garcia
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
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH 4/4] watchdog: imgpdc: Add reboot support
2015-03-31 18:49 [PATCH 1/4] watchdog: imgpdc: Fix NULL pointer dereference on probe Andrew Bresticker
2015-03-31 18:49 ` [PATCH 2/4] watchdog: imgpdc: Intialize timeout to default value Andrew Bresticker
2015-03-31 18:49 ` [PATCH 3/4] watchdog: imgpdc: Set timeout before starting watchdog Andrew Bresticker
@ 2015-03-31 18:49 ` Andrew Bresticker
2015-04-01 3:51 ` Guenter Roeck
2015-03-31 19:37 ` [PATCH 1/4] watchdog: imgpdc: Fix NULL pointer dereference on probe Andrew Bresticker
3 siblings, 1 reply; 9+ messages in thread
From: Andrew Bresticker @ 2015-03-31 18:49 UTC (permalink / raw)
To: Wim Van Sebroeck, Guenter Roeck
Cc: linux-watchdog, linux-kernel, Andrew Bresticker, Ezequiel Garcia
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
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 2/4] watchdog: imgpdc: Intialize timeout to default value
2015-03-31 18:49 ` [PATCH 2/4] watchdog: imgpdc: Intialize timeout to default value Andrew Bresticker
@ 2015-03-31 19:02 ` Guenter Roeck
2015-03-31 19:34 ` Andrew Bresticker
0 siblings, 1 reply; 9+ messages in thread
From: Guenter Roeck @ 2015-03-31 19:02 UTC (permalink / raw)
To: Andrew Bresticker, Wim Van Sebroeck
Cc: linux-watchdog, linux-kernel, Naidu Tellapati, Ezequiel Garcia
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/4] watchdog: imgpdc: Intialize timeout to default value
2015-03-31 19:02 ` Guenter Roeck
@ 2015-03-31 19:34 ` Andrew Bresticker
0 siblings, 0 replies; 9+ messages in thread
From: Andrew Bresticker @ 2015-03-31 19:34 UTC (permalink / raw)
To: Guenter Roeck
Cc: Wim Van Sebroeck, linux-watchdog, linux-kernel, Naidu Tellapati,
Ezequiel Garcia
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/4] watchdog: imgpdc: Fix NULL pointer dereference on probe
2015-03-31 18:49 [PATCH 1/4] watchdog: imgpdc: Fix NULL pointer dereference on probe Andrew Bresticker
` (2 preceding siblings ...)
2015-03-31 18:49 ` [PATCH 4/4] watchdog: imgpdc: Add reboot support Andrew Bresticker
@ 2015-03-31 19:37 ` Andrew Bresticker
3 siblings, 0 replies; 9+ messages in thread
From: Andrew Bresticker @ 2015-03-31 19:37 UTC (permalink / raw)
To: Wim Van Sebroeck, Guenter Roeck
Cc: linux-watchdog, linux-kernel, Naidu Tellapati, Andrew Bresticker,
Ezequiel Garcia
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.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 3/4] watchdog: imgpdc: Set timeout before starting watchdog
2015-03-31 18:49 ` [PATCH 3/4] watchdog: imgpdc: Set timeout before starting watchdog Andrew Bresticker
@ 2015-04-01 3:50 ` Guenter Roeck
0 siblings, 0 replies; 9+ messages in thread
From: Guenter Roeck @ 2015-04-01 3:50 UTC (permalink / raw)
To: Andrew Bresticker, Wim Van Sebroeck
Cc: linux-watchdog, linux-kernel, Naidu Tellapati, Ezequiel Garcia
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 4/4] watchdog: imgpdc: Add reboot support
2015-03-31 18:49 ` [PATCH 4/4] watchdog: imgpdc: Add reboot support Andrew Bresticker
@ 2015-04-01 3:51 ` Guenter Roeck
0 siblings, 0 replies; 9+ messages in thread
From: Guenter Roeck @ 2015-04-01 3:51 UTC (permalink / raw)
To: Andrew Bresticker, Wim Van Sebroeck
Cc: linux-watchdog, linux-kernel, Ezequiel Garcia
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>
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2015-04-01 3:51 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-03-31 18:49 [PATCH 1/4] watchdog: imgpdc: Fix NULL pointer dereference on probe Andrew Bresticker
2015-03-31 18:49 ` [PATCH 2/4] watchdog: imgpdc: Intialize timeout to default value Andrew Bresticker
2015-03-31 19:02 ` Guenter Roeck
2015-03-31 19:34 ` Andrew Bresticker
2015-03-31 18:49 ` [PATCH 3/4] watchdog: imgpdc: Set timeout before starting watchdog Andrew Bresticker
2015-04-01 3:50 ` Guenter Roeck
2015-03-31 18:49 ` [PATCH 4/4] watchdog: imgpdc: Add reboot support Andrew Bresticker
2015-04-01 3:51 ` Guenter Roeck
2015-03-31 19:37 ` [PATCH 1/4] watchdog: imgpdc: Fix NULL pointer dereference on probe Andrew Bresticker
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).