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

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

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel