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

From: Sai Prakash Ranjan <saiprakash.ranjan@codeaurora.org>
To: Brian Masney <masneyb@onstation.org>
Cc: Guenter Roeck <linux@roeck-us.net>,
	Wim Van Sebroeck <wim@linux-watchdog.org>,
	linux-watchdog@vger.kernel.org,
	Guenter Roeck <groeck@chromium.org>,
	Rajendra Nayak <rnayak@codeaurora.org>,
	Vivek Gautam <vivek.gautam@codeaurora.org>,
	Sibi Sankar <sibis@codeaurora.org>,
	Stephen Boyd <sboyd@kernel.org>,
	Doug Anderson <dianders@chromium.org>,
	Bjorn Andersson <bjorn.andersson@linaro.org>,
	linux-arm-kernel@lists.infradead.org,
	linux-kernel@vger.kernel.org, linux-arm-msm@vger.kernel.org
Subject: Re: [PATCH] watchdog: qcom: Add suspend/resume support
Date: Thu, 17 Jan 2019 18:30:41 +0530	[thread overview]
Message-ID: <b9f573ec-3787-2b05-52e3-60f11c061dd7@codeaurora.org> (raw)
In-Reply-To: <20190117112730.GA25198@basecamp>

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