diff mbox series

[v4] watchdog: qcom_wdt: set WDOG_HW_RUNNING bit when appropriate

Message ID 20201031121115.542752-1-robert.marko@sartura.hr (mailing list archive)
State Accepted
Commit 8650d0f9e9334f2e1c209f1e2ac8341f91e30d75
Headers show
Series [v4] watchdog: qcom_wdt: set WDOG_HW_RUNNING bit when appropriate | expand

Commit Message

Robert Marko Oct. 31, 2020, 12:11 p.m. UTC
If the watchdog hardware is enabled/running during boot, e.g.
due to a boot loader configuring it, we must tell the
watchdog framework about this fact so that it can ping the
watchdog until userspace opens the device and takes over
control.

Do so using the WDOG_HW_RUNNING flag that exists for exactly
that use-case.

Signed-off-by: Robert Marko <robert.marko@sartura.hr>
Cc: Luka Perkov <luka.perkov@sartura.hr>
---
Changes in v4:
* Use QCOM_WDT_ENABLE macro

Changes in v3:
* Drop call to stop as start already does it
* Update commit message

Changes in v2:
* Correct authorship

 drivers/watchdog/qcom-wdt.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

Comments

Guenter Roeck Oct. 31, 2020, 2:08 p.m. UTC | #1
On 10/31/20 5:11 AM, Robert Marko wrote:
> If the watchdog hardware is enabled/running during boot, e.g.
> due to a boot loader configuring it, we must tell the
> watchdog framework about this fact so that it can ping the
> watchdog until userspace opens the device and takes over
> control.
> 
> Do so using the WDOG_HW_RUNNING flag that exists for exactly
> that use-case.
> 
> Signed-off-by: Robert Marko <robert.marko@sartura.hr>

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

> Cc: Luka Perkov <luka.perkov@sartura.hr>
> ---
> Changes in v4:
> * Use QCOM_WDT_ENABLE macro
> 
> Changes in v3:
> * Drop call to stop as start already does it
> * Update commit message
> 
> Changes in v2:
> * Correct authorship
> 
>  drivers/watchdog/qcom-wdt.c | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
> 
> diff --git a/drivers/watchdog/qcom-wdt.c b/drivers/watchdog/qcom-wdt.c
> index ab7465d186fd..07d399c4edc4 100644
> --- a/drivers/watchdog/qcom-wdt.c
> +++ b/drivers/watchdog/qcom-wdt.c
> @@ -152,6 +152,13 @@ static int qcom_wdt_restart(struct watchdog_device *wdd, unsigned long action,
>  	return 0;
>  }
>  
> +static int qcom_wdt_is_running(struct watchdog_device *wdd)
> +{
> +	struct qcom_wdt *wdt = to_qcom_wdt(wdd);
> +
> +	return (readl(wdt_addr(wdt, WDT_EN)) & QCOM_WDT_ENABLE);
> +}
> +
>  static const struct watchdog_ops qcom_wdt_ops = {
>  	.start		= qcom_wdt_start,
>  	.stop		= qcom_wdt_stop,
> @@ -294,6 +301,17 @@ static int qcom_wdt_probe(struct platform_device *pdev)
>  	wdt->wdd.timeout = min(wdt->wdd.max_timeout, 30U);
>  	watchdog_init_timeout(&wdt->wdd, 0, dev);
>  
> +	/*
> +	 * If WDT is already running, call WDT start which
> +	 * will stop the WDT, set timeouts as bootloader
> +	 * might use different ones and set running bit
> +	 * to inform the WDT subsystem to ping the WDT
> +	 */
> +	if (qcom_wdt_is_running(&wdt->wdd)) {
> +		qcom_wdt_start(&wdt->wdd);
> +		set_bit(WDOG_HW_RUNNING, &wdt->wdd.status);
> +	}
> +
>  	ret = devm_watchdog_register_device(dev, &wdt->wdd);
>  	if (ret)
>  		return ret;
>
Kathiravan T Nov. 2, 2020, 3:58 a.m. UTC | #2
On 10/31/2020 7:38 PM, Guenter Roeck wrote:
> On 10/31/20 5:11 AM, Robert Marko wrote:
>> If the watchdog hardware is enabled/running during boot, e.g.
>> due to a boot loader configuring it, we must tell the
>> watchdog framework about this fact so that it can ping the
>> watchdog until userspace opens the device and takes over
>> control.
>>
>> Do so using the WDOG_HW_RUNNING flag that exists for exactly
>> that use-case.
>>
>> Signed-off-by: Robert Marko <robert.marko@sartura.hr>
> Reviewed-by: Guenter Roeck <linux@roeck-us.net>

Thanks for addressing the comments and now the patch looks good to me. 
One more suggestion, can we make the initcall level of the driver to 
subsys_initcall_sync so that the driver gets registered immediately 
after the watchdog_core is registered and watchdog_core starts pinging 
the WDT?

>> Cc: Luka Perkov <luka.perkov@sartura.hr>
>> ---
>> Changes in v4:
>> * Use QCOM_WDT_ENABLE macro
>>
>> Changes in v3:
>> * Drop call to stop as start already does it
>> * Update commit message
>>
>> Changes in v2:
>> * Correct authorship
>>
>>   drivers/watchdog/qcom-wdt.c | 18 ++++++++++++++++++
>>   1 file changed, 18 insertions(+)
>>
>> diff --git a/drivers/watchdog/qcom-wdt.c b/drivers/watchdog/qcom-wdt.c
>> index ab7465d186fd..07d399c4edc4 100644
>> --- a/drivers/watchdog/qcom-wdt.c
>> +++ b/drivers/watchdog/qcom-wdt.c
>> @@ -152,6 +152,13 @@ static int qcom_wdt_restart(struct watchdog_device *wdd, unsigned long action,
>>   	return 0;
>>   }
>>   
>> +static int qcom_wdt_is_running(struct watchdog_device *wdd)
>> +{
>> +	struct qcom_wdt *wdt = to_qcom_wdt(wdd);
>> +
>> +	return (readl(wdt_addr(wdt, WDT_EN)) & QCOM_WDT_ENABLE);
>> +}
>> +
>>   static const struct watchdog_ops qcom_wdt_ops = {
>>   	.start		= qcom_wdt_start,
>>   	.stop		= qcom_wdt_stop,
>> @@ -294,6 +301,17 @@ static int qcom_wdt_probe(struct platform_device *pdev)
>>   	wdt->wdd.timeout = min(wdt->wdd.max_timeout, 30U);
>>   	watchdog_init_timeout(&wdt->wdd, 0, dev);
>>   
>> +	/*
>> +	 * If WDT is already running, call WDT start which
>> +	 * will stop the WDT, set timeouts as bootloader
>> +	 * might use different ones and set running bit
>> +	 * to inform the WDT subsystem to ping the WDT
>> +	 */
>> +	if (qcom_wdt_is_running(&wdt->wdd)) {
>> +		qcom_wdt_start(&wdt->wdd);
>> +		set_bit(WDOG_HW_RUNNING, &wdt->wdd.status);
>> +	}
>> +
>>   	ret = devm_watchdog_register_device(dev, &wdt->wdd);
>>   	if (ret)
>>   		return ret;
>>
Guenter Roeck Nov. 2, 2020, 5:03 a.m. UTC | #3
On 11/1/20 7:58 PM, Kathiravan T wrote:
> 
> On 10/31/2020 7:38 PM, Guenter Roeck wrote:
>> On 10/31/20 5:11 AM, Robert Marko wrote:
>>> If the watchdog hardware is enabled/running during boot, e.g.
>>> due to a boot loader configuring it, we must tell the
>>> watchdog framework about this fact so that it can ping the
>>> watchdog until userspace opens the device and takes over
>>> control.
>>>
>>> Do so using the WDOG_HW_RUNNING flag that exists for exactly
>>> that use-case.
>>>
>>> Signed-off-by: Robert Marko <robert.marko@sartura.hr>
>> Reviewed-by: Guenter Roeck <linux@roeck-us.net>
> 
> Thanks for addressing the comments and now the patch looks good to me. One more suggestion, can we make the initcall level of the driver to subsys_initcall_sync so that the driver gets registered immediately after the watchdog_core is registered and watchdog_core starts pinging the WDT?
> 

That would mean to replace module_platform_driver(), which would be a whole
different discussion, is not widely needed, and would potentially interfere
with the subsys_initcall_sync() in the watchdog core. This will require
specific evidence that a problem is seen in the field, and that it is truly
needed. Plus, it would have to be a different patch (which you could submit
yourself, with evidence). Let's stick with one logical change per patch,
please.

Guenter
Kathiravan T Nov. 2, 2020, 5:19 a.m. UTC | #4
On 11/2/2020 10:33 AM, Guenter Roeck wrote:
> On 11/1/20 7:58 PM, Kathiravan T wrote:
>> On 10/31/2020 7:38 PM, Guenter Roeck wrote:
>>> On 10/31/20 5:11 AM, Robert Marko wrote:
>>>> If the watchdog hardware is enabled/running during boot, e.g.
>>>> due to a boot loader configuring it, we must tell the
>>>> watchdog framework about this fact so that it can ping the
>>>> watchdog until userspace opens the device and takes over
>>>> control.
>>>>
>>>> Do so using the WDOG_HW_RUNNING flag that exists for exactly
>>>> that use-case.
>>>>
>>>> Signed-off-by: Robert Marko <robert.marko@sartura.hr>
>>> Reviewed-by: Guenter Roeck <linux@roeck-us.net>
>> Thanks for addressing the comments and now the patch looks good to me. One more suggestion, can we make the initcall level of the driver to subsys_initcall_sync so that the driver gets registered immediately after the watchdog_core is registered and watchdog_core starts pinging the WDT?
>>
> That would mean to replace module_platform_driver(), which would be a whole
> different discussion, is not widely needed, and would potentially interfere
> with the subsys_initcall_sync() in the watchdog core. This will require
> specific evidence that a problem is seen in the field, and that it is truly
> needed. Plus, it would have to be a different patch (which you could submit
> yourself, with evidence). Let's stick with one logical change per patch,
> please.
>
> Guenter
Yeah, of course I don't want to squash the initcall level change with 
this one. Just made a suggestion to consider it. Anyway I will try to 
collect some data and post the patch by own on that suggestion. Thanks 
Guenter.
Robert Marko Nov. 30, 2020, 6:27 p.m. UTC | #5
On Mon, Nov 2, 2020 at 6:19 AM Kathiravan T <kathirav@codeaurora.org> wrote:
>
>
> On 11/2/2020 10:33 AM, Guenter Roeck wrote:
> > On 11/1/20 7:58 PM, Kathiravan T wrote:
> >> On 10/31/2020 7:38 PM, Guenter Roeck wrote:
> >>> On 10/31/20 5:11 AM, Robert Marko wrote:
> >>>> If the watchdog hardware is enabled/running during boot, e.g.
> >>>> due to a boot loader configuring it, we must tell the
> >>>> watchdog framework about this fact so that it can ping the
> >>>> watchdog until userspace opens the device and takes over
> >>>> control.
> >>>>
> >>>> Do so using the WDOG_HW_RUNNING flag that exists for exactly
> >>>> that use-case.
> >>>>
> >>>> Signed-off-by: Robert Marko <robert.marko@sartura.hr>
> >>> Reviewed-by: Guenter Roeck <linux@roeck-us.net>
> >> Thanks for addressing the comments and now the patch looks good to me. One more suggestion, can we make the initcall level of the driver to subsys_initcall_sync so that the driver gets registered immediately after the watchdog_core is registered and watchdog_core starts pinging the WDT?
> >>
> > That would mean to replace module_platform_driver(), which would be a whole
> > different discussion, is not widely needed, and would potentially interfere
> > with the subsys_initcall_sync() in the watchdog core. This will require
> > specific evidence that a problem is seen in the field, and that it is truly
> > needed. Plus, it would have to be a different patch (which you could submit
> > yourself, with evidence). Let's stick with one logical change per patch,
> > please.
> >
> > Guenter
> Yeah, of course I don't want to squash the initcall level change with
> this one. Just made a suggestion to consider it. Anyway I will try to
> collect some data and post the patch by own on that suggestion. Thanks
> Guenter.
>
> --
> QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
>

Any chance this could be picked for 5.11?
I have some boards depending on it for normal boot.

Regards,
Robert
Bjorn Andersson Nov. 30, 2020, 11:23 p.m. UTC | #6
On Sat 31 Oct 07:11 CDT 2020, Robert Marko wrote:

> If the watchdog hardware is enabled/running during boot, e.g.
> due to a boot loader configuring it, we must tell the
> watchdog framework about this fact so that it can ping the
> watchdog until userspace opens the device and takes over
> control.
> 
> Do so using the WDOG_HW_RUNNING flag that exists for exactly
> that use-case.
> 
> Signed-off-by: Robert Marko <robert.marko@sartura.hr>
> Cc: Luka Perkov <luka.perkov@sartura.hr>

Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>

> ---
> Changes in v4:
> * Use QCOM_WDT_ENABLE macro
> 
> Changes in v3:
> * Drop call to stop as start already does it
> * Update commit message
> 
> Changes in v2:
> * Correct authorship
> 
>  drivers/watchdog/qcom-wdt.c | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
> 
> diff --git a/drivers/watchdog/qcom-wdt.c b/drivers/watchdog/qcom-wdt.c
> index ab7465d186fd..07d399c4edc4 100644
> --- a/drivers/watchdog/qcom-wdt.c
> +++ b/drivers/watchdog/qcom-wdt.c
> @@ -152,6 +152,13 @@ static int qcom_wdt_restart(struct watchdog_device *wdd, unsigned long action,
>  	return 0;
>  }
>  
> +static int qcom_wdt_is_running(struct watchdog_device *wdd)
> +{
> +	struct qcom_wdt *wdt = to_qcom_wdt(wdd);
> +
> +	return (readl(wdt_addr(wdt, WDT_EN)) & QCOM_WDT_ENABLE);

Although, you don't need the outer () here.

Regards,
Bjorn

> +}
> +
>  static const struct watchdog_ops qcom_wdt_ops = {
>  	.start		= qcom_wdt_start,
>  	.stop		= qcom_wdt_stop,
> @@ -294,6 +301,17 @@ static int qcom_wdt_probe(struct platform_device *pdev)
>  	wdt->wdd.timeout = min(wdt->wdd.max_timeout, 30U);
>  	watchdog_init_timeout(&wdt->wdd, 0, dev);
>  
> +	/*
> +	 * If WDT is already running, call WDT start which
> +	 * will stop the WDT, set timeouts as bootloader
> +	 * might use different ones and set running bit
> +	 * to inform the WDT subsystem to ping the WDT
> +	 */
> +	if (qcom_wdt_is_running(&wdt->wdd)) {
> +		qcom_wdt_start(&wdt->wdd);
> +		set_bit(WDOG_HW_RUNNING, &wdt->wdd.status);
> +	}
> +
>  	ret = devm_watchdog_register_device(dev, &wdt->wdd);
>  	if (ret)
>  		return ret;
> -- 
> 2.28.0
>
patchwork-bot+linux-arm-msm@kernel.org Dec. 29, 2020, 8:15 p.m. UTC | #7
Hello:

This patch was applied to qcom/linux.git (refs/heads/for-next):

On Sat, 31 Oct 2020 13:11:15 +0100 you wrote:
> If the watchdog hardware is enabled/running during boot, e.g.
> due to a boot loader configuring it, we must tell the
> watchdog framework about this fact so that it can ping the
> watchdog until userspace opens the device and takes over
> control.
> 
> Do so using the WDOG_HW_RUNNING flag that exists for exactly
> that use-case.
> 
> [...]

Here is the summary with links:
  - [v4] watchdog: qcom_wdt: set WDOG_HW_RUNNING bit when appropriate
    https://git.kernel.org/qcom/c/8650d0f9e933

You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
diff mbox series

Patch

diff --git a/drivers/watchdog/qcom-wdt.c b/drivers/watchdog/qcom-wdt.c
index ab7465d186fd..07d399c4edc4 100644
--- a/drivers/watchdog/qcom-wdt.c
+++ b/drivers/watchdog/qcom-wdt.c
@@ -152,6 +152,13 @@  static int qcom_wdt_restart(struct watchdog_device *wdd, unsigned long action,
 	return 0;
 }
 
+static int qcom_wdt_is_running(struct watchdog_device *wdd)
+{
+	struct qcom_wdt *wdt = to_qcom_wdt(wdd);
+
+	return (readl(wdt_addr(wdt, WDT_EN)) & QCOM_WDT_ENABLE);
+}
+
 static const struct watchdog_ops qcom_wdt_ops = {
 	.start		= qcom_wdt_start,
 	.stop		= qcom_wdt_stop,
@@ -294,6 +301,17 @@  static int qcom_wdt_probe(struct platform_device *pdev)
 	wdt->wdd.timeout = min(wdt->wdd.max_timeout, 30U);
 	watchdog_init_timeout(&wdt->wdd, 0, dev);
 
+	/*
+	 * If WDT is already running, call WDT start which
+	 * will stop the WDT, set timeouts as bootloader
+	 * might use different ones and set running bit
+	 * to inform the WDT subsystem to ping the WDT
+	 */
+	if (qcom_wdt_is_running(&wdt->wdd)) {
+		qcom_wdt_start(&wdt->wdd);
+		set_bit(WDOG_HW_RUNNING, &wdt->wdd.status);
+	}
+
 	ret = devm_watchdog_register_device(dev, &wdt->wdd);
 	if (ret)
 		return ret;