[PATCH] watchdog: bcm47xx_wdt.c: add restart handler support

[PATCH] watchdog: bcm47xx_wdt.c: add restart handler support

[Rafał Miłecki]

Just like in case of other watchdog drivers, use the new kernel core
API to provide restart support.

Signed-off-by: Rafał Miłecki <zajec5@gmail.com>
---
 drivers/watchdog/bcm47xx_wdt.c | 22 ++++++++++++++++++++--
 1 file changed, 20 insertions(+), 2 deletions(-)

diff --git a/drivers/watchdog/bcm47xx_wdt.c b/drivers/watchdog/bcm47xx_wdt.c
index 9816485..dac3c5d 100644
--- a/drivers/watchdog/bcm47xx_wdt.c
+++ b/drivers/watchdog/bcm47xx_wdt.c
@@ -169,6 +169,17 @@ static int bcm47xx_wdt_notify_sys(struct notifier_block *this,
 	return NOTIFY_DONE;
 }
 
+static int bcm47xx_wdt_restart(struct notifier_block *this, unsigned long mode,
+			       void *cmd)
+{
+	struct bcm47xx_wdt *wdt;
+
+	wdt = container_of(this, struct bcm47xx_wdt, restart_handler);
+	wdt->timer_set(wdt, 1);
+
+	return NOTIFY_DONE;
+}
+
 static struct watchdog_ops bcm47xx_wdt_soft_ops = {
 	.owner		= THIS_MODULE,
 	.start		= bcm47xx_wdt_soft_start,
@@ -204,20 +215,27 @@ static int bcm47xx_wdt_probe(struct platform_device *pdev)
 	watchdog_set_nowayout(&wdt->wdd, nowayout);
 
 	wdt->notifier.notifier_call = &bcm47xx_wdt_notify_sys;
-
 	ret = register_reboot_notifier(&wdt->notifier);
 	if (ret)
 		goto err_timer;
 
-	ret = watchdog_register_device(&wdt->wdd);
+	wdt->restart_handler.notifier_call = &bcm47xx_wdt_restart;
+	wdt->restart_handler.priority = 128;
+	ret = register_restart_handler(&wdt->restart_handler);
 	if (ret)
 		goto err_notifier;
 
+	ret = watchdog_register_device(&wdt->wdd);
+	if (ret)
+		goto err_handler;
+
 	dev_info(&pdev->dev, "BCM47xx Watchdog Timer enabled (%d seconds%s%s)\n",
 		timeout, nowayout ? ", nowayout" : "",
 		soft ? ", Software Timer" : "");
 	return 0;
 
+err_handler:
+	unregister_restart_handler(&wdt->restart_handler);
 err_notifier:
 	unregister_reboot_notifier(&wdt->notifier);
 err_timer:
-- 
1.8.4.5

[PATCH V2] watchdog: bcm47xx_wdt.c: add restart handler support

[Rafał Miłecki]

Just like in case of other watchdog drivers, use the new kernel core
API to provide restart support.

Signed-off-by: Rafał Miłecki <zajec5@gmail.com>
---
V2: Include changes to include/linux/bcm47xx_wdt.h
---
 drivers/watchdog/bcm47xx_wdt.c | 22 ++++++++++++++++++++--
 include/linux/bcm47xx_wdt.h    |  1 +
 2 files changed, 21 insertions(+), 2 deletions(-)

diff --git a/drivers/watchdog/bcm47xx_wdt.c b/drivers/watchdog/bcm47xx_wdt.c
index 9816485..dac3c5d 100644
--- a/drivers/watchdog/bcm47xx_wdt.c
+++ b/drivers/watchdog/bcm47xx_wdt.c
@@ -169,6 +169,17 @@ static int bcm47xx_wdt_notify_sys(struct notifier_block *this,
 	return NOTIFY_DONE;
 }
 
+static int bcm47xx_wdt_restart(struct notifier_block *this, unsigned long mode,
+			       void *cmd)
+{
+	struct bcm47xx_wdt *wdt;
+
+	wdt = container_of(this, struct bcm47xx_wdt, restart_handler);
+	wdt->timer_set(wdt, 1);
+
+	return NOTIFY_DONE;
+}
+
 static struct watchdog_ops bcm47xx_wdt_soft_ops = {
 	.owner		= THIS_MODULE,
 	.start		= bcm47xx_wdt_soft_start,
@@ -204,20 +215,27 @@ static int bcm47xx_wdt_probe(struct platform_device *pdev)
 	watchdog_set_nowayout(&wdt->wdd, nowayout);
 
 	wdt->notifier.notifier_call = &bcm47xx_wdt_notify_sys;
-
 	ret = register_reboot_notifier(&wdt->notifier);
 	if (ret)
 		goto err_timer;
 
-	ret = watchdog_register_device(&wdt->wdd);
+	wdt->restart_handler.notifier_call = &bcm47xx_wdt_restart;
+	wdt->restart_handler.priority = 128;
+	ret = register_restart_handler(&wdt->restart_handler);
 	if (ret)
 		goto err_notifier;
 
+	ret = watchdog_register_device(&wdt->wdd);
+	if (ret)
+		goto err_handler;
+
 	dev_info(&pdev->dev, "BCM47xx Watchdog Timer enabled (%d seconds%s%s)\n",
 		timeout, nowayout ? ", nowayout" : "",
 		soft ? ", Software Timer" : "");
 	return 0;
 
+err_handler:
+	unregister_restart_handler(&wdt->restart_handler);
 err_notifier:
 	unregister_reboot_notifier(&wdt->notifier);
 err_timer:
diff --git a/include/linux/bcm47xx_wdt.h b/include/linux/bcm47xx_wdt.h
index b708786..5582c21 100644
--- a/include/linux/bcm47xx_wdt.h
+++ b/include/linux/bcm47xx_wdt.h
@@ -16,6 +16,7 @@ struct bcm47xx_wdt {
 
 	struct watchdog_device wdd;
 	struct notifier_block notifier;
+	struct notifier_block restart_handler;
 
 	struct timer_list soft_timer;
 	atomic_t soft_ticks;
-- 
1.8.4.5

Re: [PATCH V2] watchdog: bcm47xx_wdt.c: add restart handler support

[Guenter Roeck]

On 01/24/2015 05:58 AM, Rafał Miłecki wrote:
> Just like in case of other watchdog drivers, use the new kernel core
> API to provide restart support.
>
> Signed-off-by: Rafał Miłecki <zajec5@gmail.com>
> ---
> V2: Include changes to include/linux/bcm47xx_wdt.h
> ---
>   drivers/watchdog/bcm47xx_wdt.c | 22 ++++++++++++++++++++--
>   include/linux/bcm47xx_wdt.h    |  1 +
>   2 files changed, 21 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/watchdog/bcm47xx_wdt.c b/drivers/watchdog/bcm47xx_wdt.c
> index 9816485..dac3c5d 100644
> --- a/drivers/watchdog/bcm47xx_wdt.c
> +++ b/drivers/watchdog/bcm47xx_wdt.c
> @@ -169,6 +169,17 @@ static int bcm47xx_wdt_notify_sys(struct notifier_block *this,
>   	return NOTIFY_DONE;
>   }
>
> +static int bcm47xx_wdt_restart(struct notifier_block *this, unsigned long mode,
> +			       void *cmd)
> +{
> +	struct bcm47xx_wdt *wdt;
> +
> +	wdt = container_of(this, struct bcm47xx_wdt, restart_handler);
> +	wdt->timer_set(wdt, 1);
> +
> +	return NOTIFY_DONE;
> +}
> +
>   static struct watchdog_ops bcm47xx_wdt_soft_ops = {
>   	.owner		= THIS_MODULE,
>   	.start		= bcm47xx_wdt_soft_start,
> @@ -204,20 +215,27 @@ static int bcm47xx_wdt_probe(struct platform_device *pdev)
>   	watchdog_set_nowayout(&wdt->wdd, nowayout);
>
>   	wdt->notifier.notifier_call = &bcm47xx_wdt_notify_sys;
> -

Unnecessary whitespace change.

Guenter

>   	ret = register_reboot_notifier(&wdt->notifier);
>   	if (ret)
>   		goto err_timer;
>
> -	ret = watchdog_register_device(&wdt->wdd);
> +	wdt->restart_handler.notifier_call = &bcm47xx_wdt_restart;
> +	wdt->restart_handler.priority = 128;
> +	ret = register_restart_handler(&wdt->restart_handler);
>   	if (ret)
>   		goto err_notifier;
>
> +	ret = watchdog_register_device(&wdt->wdd);
> +	if (ret)
> +		goto err_handler;
> +
>   	dev_info(&pdev->dev, "BCM47xx Watchdog Timer enabled (%d seconds%s%s)\n",
>   		timeout, nowayout ? ", nowayout" : "",
>   		soft ? ", Software Timer" : "");
>   	return 0;
>
> +err_handler:
> +	unregister_restart_handler(&wdt->restart_handler);
>   err_notifier:
>   	unregister_reboot_notifier(&wdt->notifier);
>   err_timer:
> diff --git a/include/linux/bcm47xx_wdt.h b/include/linux/bcm47xx_wdt.h
> index b708786..5582c21 100644
> --- a/include/linux/bcm47xx_wdt.h
> +++ b/include/linux/bcm47xx_wdt.h
> @@ -16,6 +16,7 @@ struct bcm47xx_wdt {
>
>   	struct watchdog_device wdd;
>   	struct notifier_block notifier;
> +	struct notifier_block restart_handler;
>
>   	struct timer_list soft_timer;
>   	atomic_t soft_ticks;
>

Re: [PATCH V2] watchdog: bcm47xx_wdt.c: add restart handler support

[Rafał Miłecki]

On 24 January 2015 at 17:33, Guenter Roeck <linux@roeck-us.net> wrote:
> On 01/24/2015 05:58 AM, Rafał Miłecki wrote:
>>
>> Just like in case of other watchdog drivers, use the new kernel core
>> API to provide restart support.
>>
>> Signed-off-by: Rafał Miłecki <zajec5@gmail.com>
>> ---
>> V2: Include changes to include/linux/bcm47xx_wdt.h
>> ---
>>   drivers/watchdog/bcm47xx_wdt.c | 22 ++++++++++++++++++++--
>>   include/linux/bcm47xx_wdt.h    |  1 +
>>   2 files changed, 21 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/watchdog/bcm47xx_wdt.c
>> b/drivers/watchdog/bcm47xx_wdt.c
>> index 9816485..dac3c5d 100644
>> --- a/drivers/watchdog/bcm47xx_wdt.c
>> +++ b/drivers/watchdog/bcm47xx_wdt.c
>> @@ -169,6 +169,17 @@ static int bcm47xx_wdt_notify_sys(struct
>> notifier_block *this,
>>         return NOTIFY_DONE;
>>   }
>>
>> +static int bcm47xx_wdt_restart(struct notifier_block *this, unsigned long
>> mode,
>> +                              void *cmd)
>> +{
>> +       struct bcm47xx_wdt *wdt;
>> +
>> +       wdt = container_of(this, struct bcm47xx_wdt, restart_handler);
>> +       wdt->timer_set(wdt, 1);
>> +
>> +       return NOTIFY_DONE;
>> +}
>> +
>>   static struct watchdog_ops bcm47xx_wdt_soft_ops = {
>>         .owner          = THIS_MODULE,
>>         .start          = bcm47xx_wdt_soft_start,
>> @@ -204,20 +215,27 @@ static int bcm47xx_wdt_probe(struct platform_device
>> *pdev)
>>         watchdog_set_nowayout(&wdt->wdd, nowayout);
>>
>>         wdt->notifier.notifier_call = &bcm47xx_wdt_notify_sys;
>> -
>
>
> Unnecessary whitespace change.

I changed it to improve code readability. Now you have two seprated
code blocks. First one setups notification and registers it. Second
setups handler and registers it.

Re: [PATCH V2] watchdog: bcm47xx_wdt.c: add restart handler support

[Guenter Roeck]

On 01/24/2015 08:48 AM, Rafał Miłecki wrote:
> On 24 January 2015 at 17:33, Guenter Roeck <linux@roeck-us.net> wrote:
>> On 01/24/2015 05:58 AM, Rafał Miłecki wrote:
>>>
>>> Just like in case of other watchdog drivers, use the new kernel core
>>> API to provide restart support.
>>>
>>> Signed-off-by: Rafał Miłecki <zajec5@gmail.com>
>>> ---
>>> V2: Include changes to include/linux/bcm47xx_wdt.h
>>> ---
>>>    drivers/watchdog/bcm47xx_wdt.c | 22 ++++++++++++++++++++--
>>>    include/linux/bcm47xx_wdt.h    |  1 +
>>>    2 files changed, 21 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/watchdog/bcm47xx_wdt.c
>>> b/drivers/watchdog/bcm47xx_wdt.c
>>> index 9816485..dac3c5d 100644
>>> --- a/drivers/watchdog/bcm47xx_wdt.c
>>> +++ b/drivers/watchdog/bcm47xx_wdt.c
>>> @@ -169,6 +169,17 @@ static int bcm47xx_wdt_notify_sys(struct
>>> notifier_block *this,
>>>          return NOTIFY_DONE;
>>>    }
>>>
>>> +static int bcm47xx_wdt_restart(struct notifier_block *this, unsigned long
>>> mode,
>>> +                              void *cmd)
>>> +{
>>> +       struct bcm47xx_wdt *wdt;
>>> +
>>> +       wdt = container_of(this, struct bcm47xx_wdt, restart_handler);
>>> +       wdt->timer_set(wdt, 1);
>>> +
>>> +       return NOTIFY_DONE;
>>> +}
>>> +
>>>    static struct watchdog_ops bcm47xx_wdt_soft_ops = {
>>>          .owner          = THIS_MODULE,
>>>          .start          = bcm47xx_wdt_soft_start,
>>> @@ -204,20 +215,27 @@ static int bcm47xx_wdt_probe(struct platform_device
>>> *pdev)
>>>          watchdog_set_nowayout(&wdt->wdd, nowayout);
>>>
>>>          wdt->notifier.notifier_call = &bcm47xx_wdt_notify_sys;
>>> -
>>
>>
>> Unnecessary whitespace change.
>
> I changed it to improve code readability. Now you have two seprated
> code blocks. First one setups notification and registers it. Second
> setups handler and registers it.
>

That is not how it works; this is a logically separate change,
which should be submitted as separate patch if at all.

Keep in mind though that this is a personal preference; if we accept
this change, someone else may come along tomorrow asking to change it
back "to improve readability by removing unnecessary empty lines".
Normally I would leave that alone unless you get a checkpatch
warning or error, and even then it is advisable to know the maintainer's
position on accepting coding style changes.

Thanks,
Guenter

Re: [PATCH V2] watchdog: bcm47xx_wdt.c: add restart handler support

[Hauke Mehrtens]

On 01/24/2015 02:58 PM, Rafał Miłecki wrote:
> Just like in case of other watchdog drivers, use the new kernel core
> API to provide restart support.
> 
> Signed-off-by: Rafał Miłecki <zajec5@gmail.com>
> ---
> V2: Include changes to include/linux/bcm47xx_wdt.h
> ---
>  drivers/watchdog/bcm47xx_wdt.c | 22 ++++++++++++++++++++--
>  include/linux/bcm47xx_wdt.h    |  1 +
>  2 files changed, 21 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/watchdog/bcm47xx_wdt.c b/drivers/watchdog/bcm47xx_wdt.c
> index 9816485..dac3c5d 100644
> --- a/drivers/watchdog/bcm47xx_wdt.c
> +++ b/drivers/watchdog/bcm47xx_wdt.c
> @@ -169,6 +169,17 @@ static int bcm47xx_wdt_notify_sys(struct notifier_block *this,
>  	return NOTIFY_DONE;
>  }
>  
> +static int bcm47xx_wdt_restart(struct notifier_block *this, unsigned long mode,
> +			       void *cmd)
> +{
> +	struct bcm47xx_wdt *wdt;
> +
> +	wdt = container_of(this, struct bcm47xx_wdt, restart_handler);
> +	wdt->timer_set(wdt, 1);

I assume you want to replace the restart code used in
arch/mips/bcm47xx/setup.c and use this for restart on the arm SoCs.

On BCM4705 some mips asm is needed to make the restart operation
reliable, where do you plan to put it?

> +
> +	return NOTIFY_DONE;
> +}
> +
>  static struct watchdog_ops bcm47xx_wdt_soft_ops = {
>  	.owner		= THIS_MODULE,
>  	.start		= bcm47xx_wdt_soft_start,
> @@ -204,20 +215,27 @@ static int bcm47xx_wdt_probe(struct platform_device *pdev)
>  	watchdog_set_nowayout(&wdt->wdd, nowayout);
>  
>  	wdt->notifier.notifier_call = &bcm47xx_wdt_notify_sys;
> -
>  	ret = register_reboot_notifier(&wdt->notifier);
>  	if (ret)
>  		goto err_timer;
>  
> -	ret = watchdog_register_device(&wdt->wdd);
> +	wdt->restart_handler.notifier_call = &bcm47xx_wdt_restart;
> +	wdt->restart_handler.priority = 128;
> +	ret = register_restart_handler(&wdt->restart_handler);
>  	if (ret)
>  		goto err_notifier;
>  
> +	ret = watchdog_register_device(&wdt->wdd);
> +	if (ret)
> +		goto err_handler;
> +
>  	dev_info(&pdev->dev, "BCM47xx Watchdog Timer enabled (%d seconds%s%s)\n",
>  		timeout, nowayout ? ", nowayout" : "",
>  		soft ? ", Software Timer" : "");
>  	return 0;
>  
> +err_handler:
> +	unregister_restart_handler(&wdt->restart_handler);
>  err_notifier:
>  	unregister_reboot_notifier(&wdt->notifier);
>  err_timer:
> diff --git a/include/linux/bcm47xx_wdt.h b/include/linux/bcm47xx_wdt.h
> index b708786..5582c21 100644
> --- a/include/linux/bcm47xx_wdt.h
> +++ b/include/linux/bcm47xx_wdt.h
> @@ -16,6 +16,7 @@ struct bcm47xx_wdt {
>  
>  	struct watchdog_device wdd;
>  	struct notifier_block notifier;
> +	struct notifier_block restart_handler;
>  
>  	struct timer_list soft_timer;
>  	atomic_t soft_ticks;
> 

Re: [PATCH V2] watchdog: bcm47xx_wdt.c: add restart handler support

[Rafał Miłecki]

On 24 January 2015 at 18:17, Hauke Mehrtens <hauke@hauke-m.de> wrote:
> On 01/24/2015 02:58 PM, Rafał Miłecki wrote:
>> Just like in case of other watchdog drivers, use the new kernel core
>> API to provide restart support.
>>
>> Signed-off-by: Rafał Miłecki <zajec5@gmail.com>
>> ---
>> V2: Include changes to include/linux/bcm47xx_wdt.h
>> ---
>>  drivers/watchdog/bcm47xx_wdt.c | 22 ++++++++++++++++++++--
>>  include/linux/bcm47xx_wdt.h    |  1 +
>>  2 files changed, 21 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/watchdog/bcm47xx_wdt.c b/drivers/watchdog/bcm47xx_wdt.c
>> index 9816485..dac3c5d 100644
>> --- a/drivers/watchdog/bcm47xx_wdt.c
>> +++ b/drivers/watchdog/bcm47xx_wdt.c
>> @@ -169,6 +169,17 @@ static int bcm47xx_wdt_notify_sys(struct notifier_block *this,
>>       return NOTIFY_DONE;
>>  }
>>
>> +static int bcm47xx_wdt_restart(struct notifier_block *this, unsigned long mode,
>> +                            void *cmd)
>> +{
>> +     struct bcm47xx_wdt *wdt;
>> +
>> +     wdt = container_of(this, struct bcm47xx_wdt, restart_handler);
>> +     wdt->timer_set(wdt, 1);
>
> I assume you want to replace the restart code used in
> arch/mips/bcm47xx/setup.c and use this for restart on the arm SoCs.
>
> On BCM4705 some mips asm is needed to make the restart operation
> reliable, where do you plan to put it?

I think BCM4705 restart it so arch/hw specific, it'll have to stay in
arch/mips/ code. For all other hardware we should be able to use
do_kernel_restart (and the watchdog driver in the result).

Standard do_kernel_restart already works nicely for me when using
Broadcom ARM hardware.

[PATCH V3] watchdog: bcm47xx_wdt.c: add restart handler support

[Rafał Miłecki]

Just like in case of other watchdog drivers, use the new kernel core
API to provide restart support.

Signed-off-by: Rafał Miłecki <zajec5@gmail.com>
---
V2: Include changes to include/linux/bcm47xx_wdt.h
V3: Don't drop some empty line
    Use lower priority (64 instead of 128)
---
 drivers/watchdog/bcm47xx_wdt.c | 21 ++++++++++++++++++++-
 include/linux/bcm47xx_wdt.h    |  1 +
 2 files changed, 21 insertions(+), 1 deletion(-)

diff --git a/drivers/watchdog/bcm47xx_wdt.c b/drivers/watchdog/bcm47xx_wdt.c
index 9816485..b28a072 100644
--- a/drivers/watchdog/bcm47xx_wdt.c
+++ b/drivers/watchdog/bcm47xx_wdt.c
@@ -169,6 +169,17 @@ static int bcm47xx_wdt_notify_sys(struct notifier_block *this,
 	return NOTIFY_DONE;
 }
 
+static int bcm47xx_wdt_restart(struct notifier_block *this, unsigned long mode,
+			       void *cmd)
+{
+	struct bcm47xx_wdt *wdt;
+
+	wdt = container_of(this, struct bcm47xx_wdt, restart_handler);
+	wdt->timer_set(wdt, 1);
+
+	return NOTIFY_DONE;
+}
+
 static struct watchdog_ops bcm47xx_wdt_soft_ops = {
 	.owner		= THIS_MODULE,
 	.start		= bcm47xx_wdt_soft_start,
@@ -209,15 +220,23 @@ static int bcm47xx_wdt_probe(struct platform_device *pdev)
 	if (ret)
 		goto err_timer;
 
-	ret = watchdog_register_device(&wdt->wdd);
+	wdt->restart_handler.notifier_call = &bcm47xx_wdt_restart;
+	wdt->restart_handler.priority = 64;
+	ret = register_restart_handler(&wdt->restart_handler);
 	if (ret)
 		goto err_notifier;
 
+	ret = watchdog_register_device(&wdt->wdd);
+	if (ret)
+		goto err_handler;
+
 	dev_info(&pdev->dev, "BCM47xx Watchdog Timer enabled (%d seconds%s%s)\n",
 		timeout, nowayout ? ", nowayout" : "",
 		soft ? ", Software Timer" : "");
 	return 0;
 
+err_handler:
+	unregister_restart_handler(&wdt->restart_handler);
 err_notifier:
 	unregister_reboot_notifier(&wdt->notifier);
 err_timer:
diff --git a/include/linux/bcm47xx_wdt.h b/include/linux/bcm47xx_wdt.h
index b708786..5582c21 100644
--- a/include/linux/bcm47xx_wdt.h
+++ b/include/linux/bcm47xx_wdt.h
@@ -16,6 +16,7 @@ struct bcm47xx_wdt {
 
 	struct watchdog_device wdd;
 	struct notifier_block notifier;
+	struct notifier_block restart_handler;
 
 	struct timer_list soft_timer;
 	atomic_t soft_ticks;
-- 
1.8.4.5

Re: [PATCH V3] watchdog: bcm47xx_wdt.c: add restart handler support

[Guenter Roeck]

On 01/25/2015 02:40 AM, Rafał Miłecki wrote:
> Just like in case of other watchdog drivers, use the new kernel core
> API to provide restart support.
>
> Signed-off-by: Rafał Miłecki <zajec5@gmail.com>
> ---

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

Re: [PATCH] watchdog: bcm47xx_wdt.c: add restart handler support

[Guenter Roeck]

On 01/24/2015 05:47 AM, Rafał Miłecki wrote:
> Just like in case of other watchdog drivers, use the new kernel core
> API to provide restart support.
>
> Signed-off-by: Rafał Miłecki <zajec5@gmail.com>
> ---
>   drivers/watchdog/bcm47xx_wdt.c | 22 ++++++++++++++++++++--
>   1 file changed, 20 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/watchdog/bcm47xx_wdt.c b/drivers/watchdog/bcm47xx_wdt.c
> index 9816485..dac3c5d 100644
> --- a/drivers/watchdog/bcm47xx_wdt.c
> +++ b/drivers/watchdog/bcm47xx_wdt.c
> @@ -169,6 +169,17 @@ static int bcm47xx_wdt_notify_sys(struct notifier_block *this,
>   	return NOTIFY_DONE;
>   }
>
> +static int bcm47xx_wdt_restart(struct notifier_block *this, unsigned long mode,
> +			       void *cmd)
> +{
> +	struct bcm47xx_wdt *wdt;
> +
> +	wdt = container_of(this, struct bcm47xx_wdt, restart_handler);
> +	wdt->timer_set(wdt, 1);
> +
> +	return NOTIFY_DONE;
> +}
> +
>   static struct watchdog_ops bcm47xx_wdt_soft_ops = {
>   	.owner		= THIS_MODULE,
>   	.start		= bcm47xx_wdt_soft_start,
> @@ -204,20 +215,27 @@ static int bcm47xx_wdt_probe(struct platform_device *pdev)
>   	watchdog_set_nowayout(&wdt->wdd, nowayout);
>
>   	wdt->notifier.notifier_call = &bcm47xx_wdt_notify_sys;
> -
>   	ret = register_reboot_notifier(&wdt->notifier);
>   	if (ret)
>   		goto err_timer;
>
> -	ret = watchdog_register_device(&wdt->wdd);
> +	wdt->restart_handler.notifier_call = &bcm47xx_wdt_restart;
> +	wdt->restart_handler.priority = 128;

This should be low priority since it doesn't immediately restart the system
but uses a watchdog timeout.

Thanks,
Guenter

Re: [PATCH] watchdog: bcm47xx_wdt.c: add restart handler support

[Rafał Miłecki]

On 24 January 2015 at 17:32, Guenter Roeck <linux@roeck-us.net> wrote:
> On 01/24/2015 05:47 AM, Rafał Miłecki wrote:
>>
>> Just like in case of other watchdog drivers, use the new kernel core
>> API to provide restart support.
>>
>> Signed-off-by: Rafał Miłecki <zajec5@gmail.com>
>> ---
>>   drivers/watchdog/bcm47xx_wdt.c | 22 ++++++++++++++++++++--
>>   1 file changed, 20 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/watchdog/bcm47xx_wdt.c
>> b/drivers/watchdog/bcm47xx_wdt.c
>> index 9816485..dac3c5d 100644
>> --- a/drivers/watchdog/bcm47xx_wdt.c
>> +++ b/drivers/watchdog/bcm47xx_wdt.c
>> @@ -169,6 +169,17 @@ static int bcm47xx_wdt_notify_sys(struct
>> notifier_block *this,
>>         return NOTIFY_DONE;
>>   }
>>
>> +static int bcm47xx_wdt_restart(struct notifier_block *this, unsigned long
>> mode,
>> +                              void *cmd)
>> +{
>> +       struct bcm47xx_wdt *wdt;
>> +
>> +       wdt = container_of(this, struct bcm47xx_wdt, restart_handler);
>> +       wdt->timer_set(wdt, 1);
>> +
>> +       return NOTIFY_DONE;
>> +}
>> +
>>   static struct watchdog_ops bcm47xx_wdt_soft_ops = {
>>         .owner          = THIS_MODULE,
>>         .start          = bcm47xx_wdt_soft_start,
>> @@ -204,20 +215,27 @@ static int bcm47xx_wdt_probe(struct platform_device
>> *pdev)
>>         watchdog_set_nowayout(&wdt->wdd, nowayout);
>>
>>         wdt->notifier.notifier_call = &bcm47xx_wdt_notify_sys;
>> -
>>         ret = register_reboot_notifier(&wdt->notifier);
>>         if (ret)
>>                 goto err_timer;
>>
>> -       ret = watchdog_register_device(&wdt->wdd);
>> +       wdt->restart_handler.notifier_call = &bcm47xx_wdt_restart;
>> +       wdt->restart_handler.priority = 128;
>
>
> This should be low priority since it doesn't immediately restart the system
> but uses a watchdog timeout.

I think this is also the only way to reboot Broadcom device. Is there
some howto on values I should use? Should I change it to sth like 64?
Or what number?

-- 
Rafał

Re: [PATCH] watchdog: bcm47xx_wdt.c: add restart handler support

[Guenter Roeck]

On 01/24/2015 08:45 AM, Rafał Miłecki wrote:
> On 24 January 2015 at 17:32, Guenter Roeck <linux@roeck-us.net> wrote:
>> On 01/24/2015 05:47 AM, Rafał Miłecki wrote:
>>>
>>> Just like in case of other watchdog drivers, use the new kernel core
>>> API to provide restart support.
>>>
>>> Signed-off-by: Rafał Miłecki <zajec5@gmail.com>
>>> ---
>>>    drivers/watchdog/bcm47xx_wdt.c | 22 ++++++++++++++++++++--
>>>    1 file changed, 20 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/watchdog/bcm47xx_wdt.c
>>> b/drivers/watchdog/bcm47xx_wdt.c
>>> index 9816485..dac3c5d 100644
>>> --- a/drivers/watchdog/bcm47xx_wdt.c
>>> +++ b/drivers/watchdog/bcm47xx_wdt.c
>>> @@ -169,6 +169,17 @@ static int bcm47xx_wdt_notify_sys(struct
>>> notifier_block *this,
>>>          return NOTIFY_DONE;
>>>    }
>>>
>>> +static int bcm47xx_wdt_restart(struct notifier_block *this, unsigned long
>>> mode,
>>> +                              void *cmd)
>>> +{
>>> +       struct bcm47xx_wdt *wdt;
>>> +
>>> +       wdt = container_of(this, struct bcm47xx_wdt, restart_handler);
>>> +       wdt->timer_set(wdt, 1);
>>> +
>>> +       return NOTIFY_DONE;
>>> +}
>>> +
>>>    static struct watchdog_ops bcm47xx_wdt_soft_ops = {
>>>          .owner          = THIS_MODULE,
>>>          .start          = bcm47xx_wdt_soft_start,
>>> @@ -204,20 +215,27 @@ static int bcm47xx_wdt_probe(struct platform_device
>>> *pdev)
>>>          watchdog_set_nowayout(&wdt->wdd, nowayout);
>>>
>>>          wdt->notifier.notifier_call = &bcm47xx_wdt_notify_sys;
>>> -
>>>          ret = register_reboot_notifier(&wdt->notifier);
>>>          if (ret)
>>>                  goto err_timer;
>>>
>>> -       ret = watchdog_register_device(&wdt->wdd);
>>> +       wdt->restart_handler.notifier_call = &bcm47xx_wdt_restart;
>>> +       wdt->restart_handler.priority = 128;
>>
>>
>> This should be low priority since it doesn't immediately restart the system
>> but uses a watchdog timeout.
>
> I think this is also the only way to reboot Broadcom device. Is there
> some howto on values I should use? Should I change it to sth like 64?
> Or what number?
>

64 is fine.

You never know if someone using that chip implements, for example, reset
with a gpio pin.

Guenter

Re: [PATCH] watchdog: bcm47xx_wdt.c: add restart handler support

[Rafał Miłecki]

On 24 January 2015 at 17:54, Guenter Roeck <linux@roeck-us.net> wrote:
> On 01/24/2015 08:45 AM, Rafał Miłecki wrote:
>>
>> On 24 January 2015 at 17:32, Guenter Roeck <linux@roeck-us.net> wrote:
>>>
>>> On 01/24/2015 05:47 AM, Rafał Miłecki wrote:
>>>>
>>>>
>>>> Just like in case of other watchdog drivers, use the new kernel core
>>>> API to provide restart support.
>>>>
>>>> Signed-off-by: Rafał Miłecki <zajec5@gmail.com>
>>>> ---
>>>>    drivers/watchdog/bcm47xx_wdt.c | 22 ++++++++++++++++++++--
>>>>    1 file changed, 20 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/watchdog/bcm47xx_wdt.c
>>>> b/drivers/watchdog/bcm47xx_wdt.c
>>>> index 9816485..dac3c5d 100644
>>>> --- a/drivers/watchdog/bcm47xx_wdt.c
>>>> +++ b/drivers/watchdog/bcm47xx_wdt.c
>>>> @@ -169,6 +169,17 @@ static int bcm47xx_wdt_notify_sys(struct
>>>> notifier_block *this,
>>>>          return NOTIFY_DONE;
>>>>    }
>>>>
>>>> +static int bcm47xx_wdt_restart(struct notifier_block *this, unsigned
>>>> long
>>>> mode,
>>>> +                              void *cmd)
>>>> +{
>>>> +       struct bcm47xx_wdt *wdt;
>>>> +
>>>> +       wdt = container_of(this, struct bcm47xx_wdt, restart_handler);
>>>> +       wdt->timer_set(wdt, 1);
>>>> +
>>>> +       return NOTIFY_DONE;
>>>> +}
>>>> +
>>>>    static struct watchdog_ops bcm47xx_wdt_soft_ops = {
>>>>          .owner          = THIS_MODULE,
>>>>          .start          = bcm47xx_wdt_soft_start,
>>>> @@ -204,20 +215,27 @@ static int bcm47xx_wdt_probe(struct
>>>> platform_device
>>>> *pdev)
>>>>          watchdog_set_nowayout(&wdt->wdd, nowayout);
>>>>
>>>>          wdt->notifier.notifier_call = &bcm47xx_wdt_notify_sys;
>>>> -
>>>>          ret = register_reboot_notifier(&wdt->notifier);
>>>>          if (ret)
>>>>                  goto err_timer;
>>>>
>>>> -       ret = watchdog_register_device(&wdt->wdd);
>>>> +       wdt->restart_handler.notifier_call = &bcm47xx_wdt_restart;
>>>> +       wdt->restart_handler.priority = 128;
>>>
>>>
>>>
>>> This should be low priority since it doesn't immediately restart the
>>> system
>>> but uses a watchdog timeout.
>>
>>
>> I think this is also the only way to reboot Broadcom device. Is there
>> some howto on values I should use? Should I change it to sth like 64?
>> Or what number?
>>
>
> 64 is fine.
>
> You never know if someone using that chip implements, for example, reset
> with a gpio pin.

Sure, that's why I wanted to left 129-255 values for other situations.
As watchdog reset is used in 99% of cases, I wanted to use the middle
priority for it.

Actually it seems that currently every watchdog driver with restart
handler uses priority 128. This includes: alim7101_wdt.c dw_wdt.c
imx2_wdt.c meson_wdt.c moxart_wdt.c s3c2410_wdt.c sunxi_wdt.c

Are you sure this value 128 is wrong in case of bcm47xx_wdt? What
makes it alright for all other drivers?

-- 
Rafał

Re: [PATCH] watchdog: bcm47xx_wdt.c: add restart handler support

[Guenter Roeck]

On 01/24/2015 09:31 AM, Rafał Miłecki wrote:
> On 24 January 2015 at 17:54, Guenter Roeck <linux@roeck-us.net> wrote:
>> On 01/24/2015 08:45 AM, Rafał Miłecki wrote:
>>>
>>> On 24 January 2015 at 17:32, Guenter Roeck <linux@roeck-us.net> wrote:
>>>>
>>>> On 01/24/2015 05:47 AM, Rafał Miłecki wrote:
>>>>>
>>>>>
>>>>> Just like in case of other watchdog drivers, use the new kernel core
>>>>> API to provide restart support.
>>>>>
>>>>> Signed-off-by: Rafał Miłecki <zajec5@gmail.com>
>>>>> ---
>>>>>     drivers/watchdog/bcm47xx_wdt.c | 22 ++++++++++++++++++++--
>>>>>     1 file changed, 20 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/drivers/watchdog/bcm47xx_wdt.c
>>>>> b/drivers/watchdog/bcm47xx_wdt.c
>>>>> index 9816485..dac3c5d 100644
>>>>> --- a/drivers/watchdog/bcm47xx_wdt.c
>>>>> +++ b/drivers/watchdog/bcm47xx_wdt.c
>>>>> @@ -169,6 +169,17 @@ static int bcm47xx_wdt_notify_sys(struct
>>>>> notifier_block *this,
>>>>>           return NOTIFY_DONE;
>>>>>     }
>>>>>
>>>>> +static int bcm47xx_wdt_restart(struct notifier_block *this, unsigned
>>>>> long
>>>>> mode,
>>>>> +                              void *cmd)
>>>>> +{
>>>>> +       struct bcm47xx_wdt *wdt;
>>>>> +
>>>>> +       wdt = container_of(this, struct bcm47xx_wdt, restart_handler);
>>>>> +       wdt->timer_set(wdt, 1);
>>>>> +
>>>>> +       return NOTIFY_DONE;
>>>>> +}
>>>>> +
>>>>>     static struct watchdog_ops bcm47xx_wdt_soft_ops = {
>>>>>           .owner          = THIS_MODULE,
>>>>>           .start          = bcm47xx_wdt_soft_start,
>>>>> @@ -204,20 +215,27 @@ static int bcm47xx_wdt_probe(struct
>>>>> platform_device
>>>>> *pdev)
>>>>>           watchdog_set_nowayout(&wdt->wdd, nowayout);
>>>>>
>>>>>           wdt->notifier.notifier_call = &bcm47xx_wdt_notify_sys;
>>>>> -
>>>>>           ret = register_reboot_notifier(&wdt->notifier);
>>>>>           if (ret)
>>>>>                   goto err_timer;
>>>>>
>>>>> -       ret = watchdog_register_device(&wdt->wdd);
>>>>> +       wdt->restart_handler.notifier_call = &bcm47xx_wdt_restart;
>>>>> +       wdt->restart_handler.priority = 128;
>>>>
>>>>
>>>>
>>>> This should be low priority since it doesn't immediately restart the
>>>> system
>>>> but uses a watchdog timeout.
>>>
>>>
>>> I think this is also the only way to reboot Broadcom device. Is there
>>> some howto on values I should use? Should I change it to sth like 64?
>>> Or what number?
>>>
>>
>> 64 is fine.
>>
>> You never know if someone using that chip implements, for example, reset
>> with a gpio pin.
>
> Sure, that's why I wanted to left 129-255 values for other situations.
> As watchdog reset is used in 99% of cases, I wanted to use the middle
> priority for it.
>
... and then forcing everyone else to use a higher priority than 128,
resulting in a priority war. Guess those who predicted that this would
happen were right after all :-(.

> Actually it seems that currently every watchdog driver with restart
> handler uses priority 128. This includes: alim7101_wdt.c dw_wdt.c
> imx2_wdt.c meson_wdt.c moxart_wdt.c s3c2410_wdt.c sunxi_wdt.c
>
> Are you sure this value 128 is wrong in case of bcm47xx_wdt? What
> makes it alright for all other drivers?
>
The other drivers do not use a watchdog timeout to accomplish an indirect
restart, but use a register from the set of registers used by the watchdog
driver to implement the restart. Or at least that is how it should be.

Guenter

Re: [PATCH] watchdog: bcm47xx_wdt.c: add restart handler support

[Rafał Miłecki]

On 24 January 2015 at 19:09, Guenter Roeck <linux@roeck-us.net> wrote:
> On 01/24/2015 09:31 AM, Rafał Miłecki wrote:
>>
>> On 24 January 2015 at 17:54, Guenter Roeck <linux@roeck-us.net> wrote:
>>>
>>> On 01/24/2015 08:45 AM, Rafał Miłecki wrote:
>>>>
>>>>
>>>> On 24 January 2015 at 17:32, Guenter Roeck <linux@roeck-us.net> wrote:
>>>>>
>>>>>
>>>>> On 01/24/2015 05:47 AM, Rafał Miłecki wrote:
>>>>>>
>>>>>>
>>>>>>
>>>>>> Just like in case of other watchdog drivers, use the new kernel core
>>>>>> API to provide restart support.
>>>>>>
>>>>>> Signed-off-by: Rafał Miłecki <zajec5@gmail.com>
>>>>>> ---
>>>>>>     drivers/watchdog/bcm47xx_wdt.c | 22 ++++++++++++++++++++--
>>>>>>     1 file changed, 20 insertions(+), 2 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/watchdog/bcm47xx_wdt.c
>>>>>> b/drivers/watchdog/bcm47xx_wdt.c
>>>>>> index 9816485..dac3c5d 100644
>>>>>> --- a/drivers/watchdog/bcm47xx_wdt.c
>>>>>> +++ b/drivers/watchdog/bcm47xx_wdt.c
>>>>>> @@ -169,6 +169,17 @@ static int bcm47xx_wdt_notify_sys(struct
>>>>>> notifier_block *this,
>>>>>>           return NOTIFY_DONE;
>>>>>>     }
>>>>>>
>>>>>> +static int bcm47xx_wdt_restart(struct notifier_block *this, unsigned
>>>>>> long
>>>>>> mode,
>>>>>> +                              void *cmd)
>>>>>> +{
>>>>>> +       struct bcm47xx_wdt *wdt;
>>>>>> +
>>>>>> +       wdt = container_of(this, struct bcm47xx_wdt, restart_handler);
>>>>>> +       wdt->timer_set(wdt, 1);
>>>>>> +
>>>>>> +       return NOTIFY_DONE;
>>>>>> +}
>>>>>> +
>>>>>>     static struct watchdog_ops bcm47xx_wdt_soft_ops = {
>>>>>>           .owner          = THIS_MODULE,
>>>>>>           .start          = bcm47xx_wdt_soft_start,
>>>>>> @@ -204,20 +215,27 @@ static int bcm47xx_wdt_probe(struct
>>>>>> platform_device
>>>>>> *pdev)
>>>>>>           watchdog_set_nowayout(&wdt->wdd, nowayout);
>>>>>>
>>>>>>           wdt->notifier.notifier_call = &bcm47xx_wdt_notify_sys;
>>>>>> -
>>>>>>           ret = register_reboot_notifier(&wdt->notifier);
>>>>>>           if (ret)
>>>>>>                   goto err_timer;
>>>>>>
>>>>>> -       ret = watchdog_register_device(&wdt->wdd);
>>>>>> +       wdt->restart_handler.notifier_call = &bcm47xx_wdt_restart;
>>>>>> +       wdt->restart_handler.priority = 128;
>>>>>
>>>>>
>>>>>
>>>>>
>>>>> This should be low priority since it doesn't immediately restart the
>>>>> system
>>>>> but uses a watchdog timeout.
>>>>
>>>>
>>>>
>>>> I think this is also the only way to reboot Broadcom device. Is there
>>>> some howto on values I should use? Should I change it to sth like 64?
>>>> Or what number?
>>>>
>>>
>>> 64 is fine.
>>>
>>> You never know if someone using that chip implements, for example, reset
>>> with a gpio pin.
>>
>>
>> Sure, that's why I wanted to left 129-255 values for other situations.
>> As watchdog reset is used in 99% of cases, I wanted to use the middle
>> priority for it.
>>
> ... and then forcing everyone else to use a higher priority than 128,
> resulting in a priority war. Guess those who predicted that this would
> happen were right after all :-(.

At least you're there to stop non-aware developers ;)

But honestly maybe it'd be worth to document this priority a bit
better. Documentation from kernel/reboot.c:
> 128: Default restart handler; use if no other restart handler is expected to be available, and/or if restart functionality is sufficient to restart the entire system
made me believe I'm doing well. Apparently I didn't understand it well enough :|


>> Actually it seems that currently every watchdog driver with restart
>> handler uses priority 128. This includes: alim7101_wdt.c dw_wdt.c
>> imx2_wdt.c meson_wdt.c moxart_wdt.c s3c2410_wdt.c sunxi_wdt.c
>>
>> Are you sure this value 128 is wrong in case of bcm47xx_wdt? What
>> makes it alright for all other drivers?
>>
> The other drivers do not use a watchdog timeout to accomplish an indirect
> restart, but use a register from the set of registers used by the watchdog
> driver to implement the restart. Or at least that is how it should be.

Uh, that "register from the set of registers used by the watchdog
driver to implement the restart" make it really complex to follow for
someone non-involved into restart/priorities/watchdog.

I can't really see the difference, I'll just trust your comment.

P.S.
I just checked few watchdog drivers and:
1) meson_wdt.c uses
while (1) {
        (..., no break)
        mdelay(5);
}
2) sunxi_wdt.c uses
while (1) {
        mdelay(5);
        (..., no break)
}

Since they loop forever in restart handler it seems they also force
all other handlers to use priority higher than 128.

-- 
Rafał
--
To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html