[PATCH] backlight: add CONFIG_PM_SLEEP to suspend/resume functions

[PATCH] backlight: add CONFIG_PM_SLEEP to suspend/resume functions

[Jingoo Han]

Add CONFIG_PM_SLEEP to suspend/resume functions to fix the following
build warning when CONFIG_PM_SLEEP is not selected. This is because
sleep PM callbacks defined by SIMPLE_DEV_PM_OPS are only used when
the CONFIG_PM_SLEEP is enabled.

drivers/video/backlight/backlight.c:211:12: warning: 'backlight_suspend' defined but not used [-Wunused-function]
drivers/video/backlight/backlight.c:225:12: warning: 'backlight_resume' defined but not used [-Wunused-function]

Signed-off-by: Jingoo Han <jg1.han@samsung.com>
---
 drivers/video/backlight/backlight.c |    2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/video/backlight/backlight.c b/drivers/video/backlight/backlight.c
index cedbcbe..750807c 100644
--- a/drivers/video/backlight/backlight.c
+++ b/drivers/video/backlight/backlight.c
@@ -208,6 +208,7 @@ static ssize_t backlight_show_actual_brightness(struct device *dev,
 
 static struct class *backlight_class;
 
+#ifdef CONFIG_PM_SLEEP
 static int backlight_suspend(struct device *dev)
 {
 	struct backlight_device *bd = to_backlight_device(dev);
@@ -235,6 +236,7 @@ static int backlight_resume(struct device *dev)
 
 	return 0;
 }
+#endif
 
 static SIMPLE_DEV_PM_OPS(backlight_class_dev_pm_ops, backlight_suspend,
 			 backlight_resume);
-- 
1.7.10.4

Re: [PATCH] backlight: add CONFIG_PM_SLEEP to suspend/resume functions

[Arnd Bergmann]

On Friday 07 June 2013 10:39:20 Jingoo Han wrote:
> Add CONFIG_PM_SLEEP to suspend/resume functions to fix the following
> build warning when CONFIG_PM_SLEEP is not selected. This is because
> sleep PM callbacks defined by SIMPLE_DEV_PM_OPS are only used when
> the CONFIG_PM_SLEEP is enabled.
> 
> drivers/video/backlight/backlight.c:211:12: warning: 'backlight_suspend' defined but not used [-Wunused-function]
> drivers/video/backlight/backlight.c:225:12: warning: 'backlight_resume' defined but not used [-Wunused-function]
> 
> Signed-off-by: Jingoo Han <jg1.han@samsung.com>
> ---
>  drivers/video/backlight/backlight.c |    2 ++
>  1 file changed, 2 insertions(+)

Your patch looks ok, but I find it extremely annoying to have new warnings
like this one come up every single day in linux-next. It really shouldn't
be this hard to use a macro called SIMPLE_DEV_PM_OPS() correctly.

Below is an implementation of SIMPLE_DEV_PM_OPS and UNIVERSAL_DEV_PM_OPS
that avoids this issue by introducing an unused reference to the suspend
and resume functions. gcc is smart enough to leave out that unused code
by itself, and it would actually improve compile-time coverage to have
something like this, besides being harder to misuse.

This would be a better approach if we didn't already have all the "#ifdef
CONFIG_PM_SLEEP" in place that hide the functions now. Unfortunately we
already have over 300 uses of SIMPLE_DEV_PM_OPS/UNIVERSAL_DEV_PM_OPS
in the kernel today, so removing all the #ifdef atomically without
creating more build errors is rather hard to do.

Maybe someone has an idea how to extend my approach so it works with
and without the #ifdef, to let us transition to a situation that no
longer needs them.

	Arnd

diff --git a/include/linux/pm.h b/include/linux/pm.h
index a224c7f..5a801bd 100644
--- a/include/linux/pm.h
+++ b/include/linux/pm.h
@@ -320,6 +320,9 @@ struct dev_pm_ops {
 #define SET_RUNTIME_PM_OPS(suspend_fn, resume_fn, idle_fn)
 #endif
 
+#define UNUSED_DEV_PM_OPS(name, func...) \
+static const int (*__unused_ ## name[])(struct device*) __maybe_unused = { func };
+
 /*
  * Use this if you want to use the same suspend and resume callbacks for suspend
  * to RAM and hibernation.
@@ -327,7 +332,8 @@ struct dev_pm_ops {
 #define SIMPLE_DEV_PM_OPS(name, suspend_fn, resume_fn) \
 const struct dev_pm_ops name = { \
 	SET_SYSTEM_SLEEP_PM_OPS(suspend_fn, resume_fn) \
-}
+}; \
+UNUSED_DEV_PM_OPS(name, suspend_fn, resume_fn)
 
 /*
  * Use this for defining a set of PM operations to be used in all situations
@@ -346,7 +352,8 @@ const struct dev_pm_ops name = { \
 const struct dev_pm_ops name = { \
 	SET_SYSTEM_SLEEP_PM_OPS(suspend_fn, resume_fn) \
 	SET_RUNTIME_PM_OPS(suspend_fn, resume_fn, idle_fn) \
-}
+}; \
+UNUSED_DEV_PM_OPS(name, suspend_fn, resume_fn, idle_fn)
 
 /**
  * PM_EVENT_ messages

Re: [PATCH] backlight: add CONFIG_PM_SLEEP to suspend/resume functions

[Andrew Morton]

On Fri, 07 Jun 2013 12:02:31 +0200 Arnd Bergmann <arnd@arndb.de> wrote:

> On Friday 07 June 2013 10:39:20 Jingoo Han wrote:
> > Add CONFIG_PM_SLEEP to suspend/resume functions to fix the following
> > build warning when CONFIG_PM_SLEEP is not selected. This is because
> > sleep PM callbacks defined by SIMPLE_DEV_PM_OPS are only used when
> > the CONFIG_PM_SLEEP is enabled.
> > 
> > drivers/video/backlight/backlight.c:211:12: warning: 'backlight_suspend' defined but not used [-Wunused-function]
> > drivers/video/backlight/backlight.c:225:12: warning: 'backlight_resume' defined but not used [-Wunused-function]
> > 
> > Signed-off-by: Jingoo Han <jg1.han@samsung.com>
> > ---
> >  drivers/video/backlight/backlight.c |    2 ++
> >  1 file changed, 2 insertions(+)
> 
> Your patch looks ok, but I find it extremely annoying to have new warnings
> like this one come up every single day in linux-next. It really shouldn't
> be this hard to use a macro called SIMPLE_DEV_PM_OPS() correctly.
> 
> Below is an implementation of SIMPLE_DEV_PM_OPS and UNIVERSAL_DEV_PM_OPS
> that avoids this issue by introducing an unused reference to the suspend
> and resume functions. gcc is smart enough to leave out that unused code
> by itself, and it would actually improve compile-time coverage to have
> something like this, besides being harder to misuse.
> 
> This would be a better approach if we didn't already have all the "#ifdef
> CONFIG_PM_SLEEP" in place that hide the functions now. Unfortunately we
> already have over 300 uses of SIMPLE_DEV_PM_OPS/UNIVERSAL_DEV_PM_OPS
> in the kernel today, so removing all the #ifdef atomically without
> creating more build errors is rather hard to do.
> 
> Maybe someone has an idea how to extend my approach so it works with
> and without the #ifdef, to let us transition to a situation that no
> longer needs them.

You could create new macros, and add a checkpatch rule to remind people
to not use the old ones.  Then people can migrate over from the old
macros at a leisurely pace.

The problem will be in thinking up decent names for the new macros.

Re: [PATCH] backlight: add CONFIG_PM_SLEEP to suspend/resume functions

[Jean-Christophe PLAGNIOL-VILLARD]

On 16:31 Mon 10 Jun     , Andrew Morton wrote:
> On Fri, 07 Jun 2013 12:02:31 +0200 Arnd Bergmann <arnd@arndb.de> wrote:
> 
> > On Friday 07 June 2013 10:39:20 Jingoo Han wrote:
> > > Add CONFIG_PM_SLEEP to suspend/resume functions to fix the following
> > > build warning when CONFIG_PM_SLEEP is not selected. This is because
> > > sleep PM callbacks defined by SIMPLE_DEV_PM_OPS are only used when
> > > the CONFIG_PM_SLEEP is enabled.
> > > 
> > > drivers/video/backlight/backlight.c:211:12: warning: 'backlight_suspend' defined but not used [-Wunused-function]
> > > drivers/video/backlight/backlight.c:225:12: warning: 'backlight_resume' defined but not used [-Wunused-function]
> > > 
> > > Signed-off-by: Jingoo Han <jg1.han@samsung.com>
> > > ---
> > >  drivers/video/backlight/backlight.c |    2 ++
> > >  1 file changed, 2 insertions(+)
> > 
> > Your patch looks ok, but I find it extremely annoying to have new warnings
> > like this one come up every single day in linux-next. It really shouldn't
> > be this hard to use a macro called SIMPLE_DEV_PM_OPS() correctly.
> > 
> > Below is an implementation of SIMPLE_DEV_PM_OPS and UNIVERSAL_DEV_PM_OPS
> > that avoids this issue by introducing an unused reference to the suspend
> > and resume functions. gcc is smart enough to leave out that unused code
> > by itself, and it would actually improve compile-time coverage to have
> > something like this, besides being harder to misuse.
> > 
> > This would be a better approach if we didn't already have all the "#ifdef
> > CONFIG_PM_SLEEP" in place that hide the functions now. Unfortunately we
> > already have over 300 uses of SIMPLE_DEV_PM_OPS/UNIVERSAL_DEV_PM_OPS
> > in the kernel today, so removing all the #ifdef atomically without
> > creating more build errors is rather hard to do.
> > 
> > Maybe someone has an idea how to extend my approach so it works with
> > and without the #ifdef, to let us transition to a situation that no
> > longer needs them.
> 
> You could create new macros, and add a checkpatch rule to remind people
> to not use the old ones.  Then people can migrate over from the old
> macros at a leisurely pace.
> 
> The problem will be in thinking up decent names for the new macros.

Agreed

Best Regards,
J.

Re: [PATCH] backlight: add CONFIG_PM_SLEEP to suspend/resume functions

[Arnd Bergmann]

On Friday 07 June 2013 10:39:20 Jingoo Han wrote:
> Add CONFIG_PM_SLEEP to suspend/resume functions to fix the following
> build warning when CONFIG_PM_SLEEP is not selected. This is because
> sleep PM callbacks defined by SIMPLE_DEV_PM_OPS are only used when
> the CONFIG_PM_SLEEP is enabled.
> 
> drivers/video/backlight/backlight.c:211:12: warning: 'backlight_suspend' defined but not used [-Wunused-function]
> drivers/video/backlight/backlight.c:225:12: warning: 'backlight_resume' defined but not used [-Wunused-function]
> 
> Signed-off-by: Jingoo Han <jg1.han@samsung.com>

I forgot:

Acked-by: Arnd Bergmann <arnd@arndb.de>

Re: [PATCH] backlight: add CONFIG_PM_SLEEP to suspend/resume functions

[Shuah Khan]

On 06/07/2013 04:25 AM, Arnd Bergmann wrote:
> On Friday 07 June 2013 10:39:20 Jingoo Han wrote:
>> Add CONFIG_PM_SLEEP to suspend/resume functions to fix the following
>> build warning when CONFIG_PM_SLEEP is not selected. This is because
>> sleep PM callbacks defined by SIMPLE_DEV_PM_OPS are only used when
>> the CONFIG_PM_SLEEP is enabled.
>>
>> drivers/video/backlight/backlight.c:211:12: warning: 'backlight_suspend' defined but not used [-Wunused-function]
>> drivers/video/backlight/backlight.c:225:12: warning: 'backlight_resume' defined but not used [-Wunused-function]
>>
>> Signed-off-by: Jingoo Han <jg1.han@samsung.com>
>
> I forgot:
>
> Acked-by: Arnd Bergmann <arnd@arndb.de>
>

Jingoo Han, and Arnd,

Sorry I was on vacation all of last week. Thanks for fixing the problem. 
My apologies for introducing the warnings. I had CONFIG_PM and 
CONFIG_PM_SLEEP both enabled and didn't see this warning. I will be 
careful with subsequent patches in this area.

-- Shuah

Shuah Khan, Linux Kernel Developer - Open Source Group Samsung Research 
America (Silicon Valley) shuah.kh@samsung.com | (970) 672-0658