[PATCH] watchdog: qcom: Add suspend/resume support

[PATCH] watchdog: qcom: Add suspend/resume support

[Sai Prakash Ranjan]

This adds the support for qcom watchdog suspend and resume
when entering and exiting deep sleep states. Otherwise
having watchdog active after suspend would result in unwanted
crashes/resets if resume happens after a long time.

Signed-off-by: Sai Prakash Ranjan <saiprakash.ranjan@codeaurora.org>
---
 drivers/watchdog/qcom-wdt.c | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/drivers/watchdog/qcom-wdt.c b/drivers/watchdog/qcom-wdt.c
index 780971318810..4f58185e89eb 100644
--- a/drivers/watchdog/qcom-wdt.c
+++ b/drivers/watchdog/qcom-wdt.c
@@ -245,6 +245,30 @@ static int qcom_wdt_remove(struct platform_device *pdev)
 	return 0;
 }
 
+#ifdef CONFIG_PM_SLEEP
+static int qcom_wdt_suspend(struct device *dev)
+{
+	struct qcom_wdt *wdt = dev_get_drvdata(dev);
+
+	if (watchdog_active(&wdt->wdd))
+		qcom_wdt_stop(&wdt->wdd);
+
+	return 0;
+}
+
+static int qcom_wdt_resume(struct device *dev)
+{
+	struct qcom_wdt *wdt = dev_get_drvdata(dev);
+
+	if (watchdog_active(&wdt->wdd))
+		qcom_wdt_start(&wdt->wdd);
+
+	return 0;
+}
+#endif /* CONFIG_PM_SLEEP */
+
+static SIMPLE_DEV_PM_OPS(qcom_wdt_pm_ops, qcom_wdt_suspend, qcom_wdt_resume);
+
 static const struct of_device_id qcom_wdt_of_table[] = {
 	{ .compatible = "qcom,kpss-timer", .data = reg_offset_data_apcs_tmr },
 	{ .compatible = "qcom,scss-timer", .data = reg_offset_data_apcs_tmr },
@@ -259,6 +283,7 @@ static struct platform_driver qcom_watchdog_driver = {
 	.driver	= {
 		.name		= KBUILD_MODNAME,
 		.of_match_table	= qcom_wdt_of_table,
+		.pm		= &qcom_wdt_pm_ops,
 	},
 };
 module_platform_driver(qcom_watchdog_driver);
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

Re: [PATCH] watchdog: qcom: Add suspend/resume support

[Brian Masney]

On Thu, Jan 17, 2019 at 02:45:55PM +0530, Sai Prakash Ranjan wrote:
> +#ifdef CONFIG_PM_SLEEP
> +static int qcom_wdt_suspend(struct device *dev)
> +{
> +	struct qcom_wdt *wdt = dev_get_drvdata(dev);
> +
> +	if (watchdog_active(&wdt->wdd))
> +		qcom_wdt_stop(&wdt->wdd);
> +
> +	return 0;
> +}
> +
> +static int qcom_wdt_resume(struct device *dev)
> +{
> +	struct qcom_wdt *wdt = dev_get_drvdata(dev);
> +
> +	if (watchdog_active(&wdt->wdd))
> +		qcom_wdt_start(&wdt->wdd);
> +
> +	return 0;
> +}
> +#endif /* CONFIG_PM_SLEEP */

You can use the __maybe_unused attribute to remove the #ifdef:

static int __maybe_unused qcom_wdt_suspend(struct device *dev)

Brian

Re: [PATCH] watchdog: qcom: Add suspend/resume support

[Sai Prakash Ranjan]

Hi Brian,

On 1/17/2019 3:01 PM, Brian Masney wrote:
> 
> You can use the __maybe_unused attribute to remove the #ifdef:
> 
> static int __maybe_unused qcom_wdt_suspend(struct device *dev)
> 

Thanks for looking into this.

As for __maybe_unused, I think it's better to keep #ifdef rather than
this attribute which seems to be meaning unused when actually its 
possible that it's used often(PM_SLEEP is def y). It's like saying 
unused when you are actually using it. The attribute seems like a
hack to avoid compilation error. Please correct me if I am wrong.

  - Sai
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

Re: [PATCH] watchdog: qcom: Add suspend/resume support

[Brian Masney]

On Thu, Jan 17, 2019 at 04:08:52PM +0530, Sai Prakash Ranjan wrote:
> On 1/17/2019 3:01 PM, Brian Masney wrote:
> > 
> > You can use the __maybe_unused attribute to remove the #ifdef:
> > 
> > static int __maybe_unused qcom_wdt_suspend(struct device *dev)
> > 
> 
> Thanks for looking into this.
> 
> As for __maybe_unused, I think it's better to keep #ifdef rather than
> this attribute which seems to be meaning unused when actually its possible
> that it's used often(PM_SLEEP is def y). It's like saying unused when you
> are actually using it. The attribute seems like a
> hack to avoid compilation error. Please correct me if I am wrong.

That attribute suppresses a warning from the compiler if the function is
unused when PM_SLEEP is disabled.  I don't consider it hackish since the
function name no longer appears outside the #ifdef. For example:

	#ifdef CONFIG_PM_SLEEP
	static int qcom_wdt_suspend(struct device *dev)
	{
		...
	}
	#endif /* CONFIG_PM_SLEEP */

	static SIMPLE_DEV_PM_OPS(..., qcom_wdt_suspend, ...);

SIMPLE_DEV_PM_OPS (actually SET_SYSTEM_SLEEP_PM_OP) includes the check
for PM_SLEEP and its a noop if PM_SLEEP is disabled so this works.

Now here's the code with __maybe_unused:

	static int __maybe_unused qcom_wdt_suspend(struct device *dev)
	{
		...
	}

	static SIMPLE_DEV_PM_OPS(..., qcom_wdt_suspend, ...);

This will still be a NOOP when power management is disabled, but have
the benefit of increased compile-time test coverage in that situation.
The symbols won't be included in the final executable. I personally
think the code a is cleaner with __maybe_unused.

This pattern is already in use across various subsystems in the kernel
for suspend and resume functions:

$ git grep __maybe_unused | egrep "_suspend|_resume"  | wc -l
767

Brian

Re: [PATCH] watchdog: qcom: Add suspend/resume support

[Sai Prakash Ranjan]

On 1/17/2019 4:57 PM, Brian Masney wrote:
> 
> That attribute suppresses a warning from the compiler if the function is
> unused when PM_SLEEP is disabled.  I don't consider it hackish since the
> function name no longer appears outside the #ifdef. For example:
> 
> 	#ifdef CONFIG_PM_SLEEP
> 	static int qcom_wdt_suspend(struct device *dev)
> 	{
> 		...
> 	}
> 	#endif /* CONFIG_PM_SLEEP */
> 
> 	static SIMPLE_DEV_PM_OPS(..., qcom_wdt_suspend, ...);
> 
> SIMPLE_DEV_PM_OPS (actually SET_SYSTEM_SLEEP_PM_OP) includes the check
> for PM_SLEEP and its a noop if PM_SLEEP is disabled so this works.
> 
> Now here's the code with __maybe_unused:
> 
> 	static int __maybe_unused qcom_wdt_suspend(struct device *dev)
> 	{
> 		...
> 	}
> 
> 	static SIMPLE_DEV_PM_OPS(..., qcom_wdt_suspend, ...);
> 
> This will still be a NOOP when power management is disabled, but have
> the benefit of increased compile-time test coverage in that situation.
> The symbols won't be included in the final executable. I personally
> think the code a is cleaner with __maybe_unused.
> 
> This pattern is already in use across various subsystems in the kernel
> for suspend and resume functions:
> 
> $ git grep __maybe_unused | egrep "_suspend|_resume"  | wc -l
> 767
> 

Thanks for the explanation Brian.

But I did see the maybe_unused attribute usage in other suspend and 
resume functions before posting and decided to go with ifdef because
I think this attribute wastes CPU time by building and later discarding 
if the function is unused (it may be negligible).

I did not understand the increased compile-time test coverage
you mentioned when PM_SLEEP=n because why would we need to compile
when the config is disabled? We could just discard it. We would just
increase the build time with this attribute (although for this case it 
would be negligible but say we compile with PM_SLEEP disabled for all 
those suspend/resume functions with maybe_unused attribute).

Looking at previous discussions in LKML[1] as to why the pm 
suspend/resume functions used __maybe_unused seemes to be because of
wrong ifdef usage. For ex: Using #ifdef CONFIG_PM instead of
#ifdef CONFIG_PM_SLEEP would result in a warning when
CONFIG_PM_SLEEP=n but CONFIG_PM=y.

Anyways, *I am OK with either of them*, after some more review on the
patch I can make the change in next version of the patch.

[1] https://lore.kernel.org/patchwork/patch/732981/

- Sai
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

Re: [PATCH] watchdog: qcom: Add suspend/resume support

[Guenter Roeck]

On Thu, Jan 17, 2019 at 06:30:41PM +0530, Sai Prakash Ranjan wrote:
> On 1/17/2019 4:57 PM, Brian Masney wrote:
> >
> >That attribute suppresses a warning from the compiler if the function is
> >unused when PM_SLEEP is disabled.  I don't consider it hackish since the
> >function name no longer appears outside the #ifdef. For example:
> >
> >	#ifdef CONFIG_PM_SLEEP
> >	static int qcom_wdt_suspend(struct device *dev)
> >	{
> >		...
> >	}
> >	#endif /* CONFIG_PM_SLEEP */
> >
> >	static SIMPLE_DEV_PM_OPS(..., qcom_wdt_suspend, ...);
> >
> >SIMPLE_DEV_PM_OPS (actually SET_SYSTEM_SLEEP_PM_OP) includes the check
> >for PM_SLEEP and its a noop if PM_SLEEP is disabled so this works.
> >
> >Now here's the code with __maybe_unused:
> >
> >	static int __maybe_unused qcom_wdt_suspend(struct device *dev)
> >	{
> >		...
> >	}
> >
> >	static SIMPLE_DEV_PM_OPS(..., qcom_wdt_suspend, ...);
> >
> >This will still be a NOOP when power management is disabled, but have
> >the benefit of increased compile-time test coverage in that situation.
> >The symbols won't be included in the final executable. I personally
> >think the code a is cleaner with __maybe_unused.
> >
> >This pattern is already in use across various subsystems in the kernel
> >for suspend and resume functions:
> >
> >$ git grep __maybe_unused | egrep "_suspend|_resume"  | wc -l
> >767
> >
> 
> Thanks for the explanation Brian.
> 
> But I did see the maybe_unused attribute usage in other suspend and resume
> functions before posting and decided to go with ifdef because
> I think this attribute wastes CPU time by building and later discarding if
> the function is unused (it may be negligible).
> 
Your argument is that the increased build time test coverage, the improved
code readability, and the reduced risk of dependency errors are not worth
the additional compile time. You might want to take that up with the upstream
kernel community.

> I did not understand the increased compile-time test coverage
> you mentioned when PM_SLEEP=n because why would we need to compile
> when the config is disabled? We could just discard it. We would just

It is beneficial to know that a future change in the conditional code,
or an infrastructure change affecting it, doesn't cause a build error,
even if the person submitting the change didn't bother compiling with
PM_SLEEP=y.

> increase the build time with this attribute (although for this case it would
> be negligible but say we compile with PM_SLEEP disabled for all those
> suspend/resume functions with maybe_unused attribute).
> 
> Looking at previous discussions in LKML[1] as to why the pm suspend/resume
> functions used __maybe_unused seemes to be because of
> wrong ifdef usage. For ex: Using #ifdef CONFIG_PM instead of
> #ifdef CONFIG_PM_SLEEP would result in a warning when
> CONFIG_PM_SLEEP=n but CONFIG_PM=y.
> 

Do you realize that you are proving a point here, and that it isn't
your point ?

I personally very much prefer __maybe_unused over #ifdef, for the reasons
stated above. Other maintainers may think differently.

Guenter

> Anyways, *I am OK with either of them*, after some more review on the
> patch I can make the change in next version of the patch.
> 
> [1] https://lore.kernel.org/patchwork/patch/732981/
> 
> - Sai
> -- 
> QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
> of Code Aurora Forum, hosted by The Linux Foundation

Re: [PATCH] watchdog: qcom: Add suspend/resume support

[Sai Prakash Ranjan]

Hi Guenter,

On 1/17/2019 8:00 PM, Guenter Roeck wrote:
> 
> I personally very much prefer __maybe_unused over #ifdef, for the reasons
> stated above. Other maintainers may think differently.
> 

Thanks for making your preference clear. I will post the second version
soon with the changes.

- Sai
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation