diff mbox

PM / core: refactor PM_OPS initializers

Message ID 20180514082146.4318-1-a.hajda@samsung.com (mailing list archive)
State Not Applicable, archived
Headers show

Commit Message

Andrzej Hajda May 14, 2018, 8:21 a.m. UTC 
With current implementation of PM_OPS initializers users should annotate
all PM callbacks with __maybe_unused attribute to prevent compiler from
complaining in case respective option is not enabled. Using ternary
operator with IS_ENABLED(symbol) as a condition allows to avoid marking
these functions with __maybe_unused.
Solution was tested successfully with multiple versions of gcc since
4.9.4 up to 7.2.1. No functional changes has been observed and callbacks
were compiled out if not used, as before.

Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
---
 include/linux/pm.h | 61 ++++++++++++++++++----------------------------
 1 file changed, 24 insertions(+), 37 deletions(-)

Comments

kernel test robot May 14, 2018, 11:51 a.m. UTC | #1 
Hi Andrzej,

I love your patch! Perhaps something to improve:

[auto build test WARNING on pm/linux-next]
[also build test WARNING on v4.17-rc5 next-20180511]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Andrzej-Hajda/PM-core-refactor-PM_OPS-initializers/20180514-172201
base:   https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git linux-next
config: openrisc-or1ksim_defconfig (attached as .config)
compiler: or1k-linux-gcc (GCC) 6.0.0 20160327 (experimental)
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=openrisc 

All warnings (new ones prefixed by >>):

   In file included from include/linux/device.h:23:0,
                    from include/linux/cdev.h:8,
                    from include/linux/tty_driver.h:245,
                    from include/linux/tty.h:9,
                    from include/linux/serial_core.h:29,
                    from drivers/tty/serial/8250/8250_of.c:11:
   drivers/tty/serial/8250/8250_of.c:293:44: error: 'of_serial_suspend' undeclared here (not in a function)
    static SIMPLE_DEV_PM_OPS(of_serial_pm_ops, of_serial_suspend, of_serial_resume);
                                               ^
   include/linux/pm.h:316:59: note: in definition of macro 'PM_SLEEP_PTR'
    #define PM_SLEEP_PTR(ptr) (IS_ENABLED(CONFIG_PM_SLEEP) ? (ptr) : NULL)
                                                              ^~~
   include/linux/pm.h:354:2: note: in expansion of macro 'SET_SYSTEM_SLEEP_PM_OPS'
     SET_SYSTEM_SLEEP_PM_OPS(suspend_fn, resume_fn) \
     ^~~~~~~~~~~~~~~~~~~~~~~
>> drivers/tty/serial/8250/8250_of.c:293:8: note: in expansion of macro 'SIMPLE_DEV_PM_OPS'
    static SIMPLE_DEV_PM_OPS(of_serial_pm_ops, of_serial_suspend, of_serial_resume);
           ^~~~~~~~~~~~~~~~~
   drivers/tty/serial/8250/8250_of.c:293:63: error: 'of_serial_resume' undeclared here (not in a function)
    static SIMPLE_DEV_PM_OPS(of_serial_pm_ops, of_serial_suspend, of_serial_resume);
                                                                  ^
   include/linux/pm.h:316:59: note: in definition of macro 'PM_SLEEP_PTR'
    #define PM_SLEEP_PTR(ptr) (IS_ENABLED(CONFIG_PM_SLEEP) ? (ptr) : NULL)
                                                              ^~~
   include/linux/pm.h:354:2: note: in expansion of macro 'SET_SYSTEM_SLEEP_PM_OPS'
     SET_SYSTEM_SLEEP_PM_OPS(suspend_fn, resume_fn) \
     ^~~~~~~~~~~~~~~~~~~~~~~
>> drivers/tty/serial/8250/8250_of.c:293:8: note: in expansion of macro 'SIMPLE_DEV_PM_OPS'
    static SIMPLE_DEV_PM_OPS(of_serial_pm_ops, of_serial_suspend, of_serial_resume);
           ^~~~~~~~~~~~~~~~~

vim +/SIMPLE_DEV_PM_OPS +293 drivers/tty/serial/8250/8250_of.c

8ad3b1352 drivers/tty/serial/of_serial.c    Jingchang Lu         2014-11-11  276  
aa42db44f drivers/tty/serial/8250/8250_of.c Arnd Bergmann        2017-01-25  277  static int of_serial_resume(struct device *dev)
8ad3b1352 drivers/tty/serial/of_serial.c    Jingchang Lu         2014-11-11  278  {
aa42db44f drivers/tty/serial/8250/8250_of.c Arnd Bergmann        2017-01-25  279  	struct of_serial_info *info = dev_get_drvdata(dev);
8ad3b1352 drivers/tty/serial/of_serial.c    Jingchang Lu         2014-11-11  280  	struct uart_8250_port *port8250 = serial8250_get_port(info->line);
8ad3b1352 drivers/tty/serial/of_serial.c    Jingchang Lu         2014-11-11  281  	struct uart_port *port = &port8250->port;
8ad3b1352 drivers/tty/serial/of_serial.c    Jingchang Lu         2014-11-11  282  
a2d23edae drivers/tty/serial/8250/8250_of.c Franklin S Cooper Jr 2017-08-16  283  	if (!uart_console(port) || console_suspend_enabled) {
a2d23edae drivers/tty/serial/8250/8250_of.c Franklin S Cooper Jr 2017-08-16  284  		pm_runtime_get_sync(dev);
8ad3b1352 drivers/tty/serial/of_serial.c    Jingchang Lu         2014-11-11  285  		clk_prepare_enable(info->clk);
a2d23edae drivers/tty/serial/8250/8250_of.c Franklin S Cooper Jr 2017-08-16  286  	}
8ad3b1352 drivers/tty/serial/of_serial.c    Jingchang Lu         2014-11-11  287  
8ad3b1352 drivers/tty/serial/of_serial.c    Jingchang Lu         2014-11-11  288  	serial8250_resume_port(info->line);
8ad3b1352 drivers/tty/serial/of_serial.c    Jingchang Lu         2014-11-11  289  
8ad3b1352 drivers/tty/serial/of_serial.c    Jingchang Lu         2014-11-11  290  	return 0;
8ad3b1352 drivers/tty/serial/of_serial.c    Jingchang Lu         2014-11-11  291  }
8ad3b1352 drivers/tty/serial/of_serial.c    Jingchang Lu         2014-11-11  292  #endif
8ad3b1352 drivers/tty/serial/of_serial.c    Jingchang Lu         2014-11-11 @293  static SIMPLE_DEV_PM_OPS(of_serial_pm_ops, of_serial_suspend, of_serial_resume);
8ad3b1352 drivers/tty/serial/of_serial.c    Jingchang Lu         2014-11-11  294  

:::::: The code at line 293 was first introduced by commit
:::::: 8ad3b1352693972b737607c7b9c89b56d45fea9b serial: of-serial: add PM suspend/resume support

:::::: TO: Jingchang Lu <jingchang.lu@freescale.com>
:::::: CC: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation
kernel test robot May 14, 2018, 11:51 a.m. UTC | #2 
Hi Andrzej,

I love your patch! Yet something to improve:

[auto build test ERROR on pm/linux-next]
[also build test ERROR on v4.17-rc5 next-20180511]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Andrzej-Hajda/PM-core-refactor-PM_OPS-initializers/20180514-172201
base:   https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git linux-next
config: ia64-defconfig (attached as .config)
compiler: ia64-linux-gcc (GCC) 7.2.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=ia64 

All error/warnings (new ones prefixed by >>):

   In file included from include/linux/device.h:23:0,
                    from include/linux/pci.h:31,
                    from drivers/net//ethernet/broadcom/tg3.c:33:
>> drivers/net//ethernet/broadcom/tg3.c:18150:38: error: 'tg3_suspend' undeclared here (not in a function); did you mean 'phy_suspend'?
    static SIMPLE_DEV_PM_OPS(tg3_pm_ops, tg3_suspend, tg3_resume);
                                         ^
   include/linux/pm.h:316:59: note: in definition of macro 'PM_SLEEP_PTR'
    #define PM_SLEEP_PTR(ptr) (IS_ENABLED(CONFIG_PM_SLEEP) ? (ptr) : NULL)
                                                              ^~~
   include/linux/pm.h:354:2: note: in expansion of macro 'SET_SYSTEM_SLEEP_PM_OPS'
     SET_SYSTEM_SLEEP_PM_OPS(suspend_fn, resume_fn) \
     ^~~~~~~~~~~~~~~~~~~~~~~
>> drivers/net//ethernet/broadcom/tg3.c:18150:8: note: in expansion of macro 'SIMPLE_DEV_PM_OPS'
    static SIMPLE_DEV_PM_OPS(tg3_pm_ops, tg3_suspend, tg3_resume);
           ^~~~~~~~~~~~~~~~~
>> drivers/net//ethernet/broadcom/tg3.c:18150:51: error: 'tg3_resume' undeclared here (not in a function); did you mean 'phy_resume'?
    static SIMPLE_DEV_PM_OPS(tg3_pm_ops, tg3_suspend, tg3_resume);
                                                      ^
   include/linux/pm.h:316:59: note: in definition of macro 'PM_SLEEP_PTR'
    #define PM_SLEEP_PTR(ptr) (IS_ENABLED(CONFIG_PM_SLEEP) ? (ptr) : NULL)
                                                              ^~~
   include/linux/pm.h:354:2: note: in expansion of macro 'SET_SYSTEM_SLEEP_PM_OPS'
     SET_SYSTEM_SLEEP_PM_OPS(suspend_fn, resume_fn) \
     ^~~~~~~~~~~~~~~~~~~~~~~
>> drivers/net//ethernet/broadcom/tg3.c:18150:8: note: in expansion of macro 'SIMPLE_DEV_PM_OPS'
    static SIMPLE_DEV_PM_OPS(tg3_pm_ops, tg3_suspend, tg3_resume);
           ^~~~~~~~~~~~~~~~~
--
   In file included from include/linux/device.h:23:0,
                    from drivers/video//backlight/backlight.c:12:
>> drivers/video//backlight/backlight.c:280:54: error: 'backlight_suspend' undeclared here (not in a function); did you mean 'backlight_types'?
    static SIMPLE_DEV_PM_OPS(backlight_class_dev_pm_ops, backlight_suspend,
                                                         ^
   include/linux/pm.h:316:59: note: in definition of macro 'PM_SLEEP_PTR'
    #define PM_SLEEP_PTR(ptr) (IS_ENABLED(CONFIG_PM_SLEEP) ? (ptr) : NULL)
                                                              ^~~
   include/linux/pm.h:354:2: note: in expansion of macro 'SET_SYSTEM_SLEEP_PM_OPS'
     SET_SYSTEM_SLEEP_PM_OPS(suspend_fn, resume_fn) \
     ^~~~~~~~~~~~~~~~~~~~~~~
   drivers/video//backlight/backlight.c:280:8: note: in expansion of macro 'SIMPLE_DEV_PM_OPS'
    static SIMPLE_DEV_PM_OPS(backlight_class_dev_pm_ops, backlight_suspend,
           ^~~~~~~~~~~~~~~~~
>> drivers/video//backlight/backlight.c:281:5: error: 'backlight_resume' undeclared here (not in a function); did you mean 'backlight_device'?
        backlight_resume);
        ^
   include/linux/pm.h:316:59: note: in definition of macro 'PM_SLEEP_PTR'
    #define PM_SLEEP_PTR(ptr) (IS_ENABLED(CONFIG_PM_SLEEP) ? (ptr) : NULL)
                                                              ^~~
   include/linux/pm.h:354:2: note: in expansion of macro 'SET_SYSTEM_SLEEP_PM_OPS'
     SET_SYSTEM_SLEEP_PM_OPS(suspend_fn, resume_fn) \
     ^~~~~~~~~~~~~~~~~~~~~~~
   drivers/video//backlight/backlight.c:280:8: note: in expansion of macro 'SIMPLE_DEV_PM_OPS'
    static SIMPLE_DEV_PM_OPS(backlight_class_dev_pm_ops, backlight_suspend,
           ^~~~~~~~~~~~~~~~~

vim +18150 drivers/net//ethernet/broadcom/tg3.c

^1da177e drivers/net/tg3.c Linus Torvalds    2005-04-16  18149  
c866b7ea drivers/net/tg3.c Rafael J. Wysocki 2010-12-25 @18150  static SIMPLE_DEV_PM_OPS(tg3_pm_ops, tg3_suspend, tg3_resume);
c866b7ea drivers/net/tg3.c Rafael J. Wysocki 2010-12-25  18151  

:::::: The code at line 18150 was first introduced by commit
:::::: c866b7eac073198cef03ea6bac2dc978635a9f5c tg3: Do not use legacy PCI power management

:::::: TO: Rafael J. Wysocki <rjw@sisk.pl>
:::::: CC: David S. Miller <davem@davemloft.net>

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation
Andrzej Hajda May 14, 2018, 12:07 p.m. UTC | #3 
On 14.05.2018 10:21, Andrzej Hajda wrote:
> With current implementation of PM_OPS initializers users should annotate
> all PM callbacks with __maybe_unused attribute to prevent compiler from
> complaining in case respective option is not enabled. Using ternary
> operator with IS_ENABLED(symbol) as a condition allows to avoid marking
> these functions with __maybe_unused.
> Solution was tested successfully with multiple versions of gcc since
> 4.9.4 up to 7.2.1. No functional changes has been observed and callbacks
> were compiled out if not used, as before.

The kernel does not compile with the patch applied if the driver defines
PM callback inside "#ifdef CONFIG_PM(_SLEEP)" - initializers assume that
callbacks are defined, even if they are not used.
So if the idea is OK I should figure it out how to make proper
transition, any ideas welcome :)

Regards
Andrzej

>
> Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
> ---
>  include/linux/pm.h | 61 ++++++++++++++++++----------------------------
>  1 file changed, 24 insertions(+), 37 deletions(-)
>
> diff --git a/include/linux/pm.h b/include/linux/pm.h
> index e723b78d8357..59f333922c15 100644
> --- a/include/linux/pm.h
> +++ b/include/linux/pm.h
> @@ -313,50 +313,37 @@ struct dev_pm_ops {
>  	int (*runtime_idle)(struct device *dev);
>  };
>  
> -#ifdef CONFIG_PM_SLEEP
> +#define PM_SLEEP_PTR(ptr) (IS_ENABLED(CONFIG_PM_SLEEP) ? (ptr) : NULL)
> +#define PM_PTR(ptr) (IS_ENABLED(CONFIG_PM) ? (ptr) : NULL)
> +
>  #define SET_SYSTEM_SLEEP_PM_OPS(suspend_fn, resume_fn) \
> -	.suspend = suspend_fn, \
> -	.resume = resume_fn, \
> -	.freeze = suspend_fn, \
> -	.thaw = resume_fn, \
> -	.poweroff = suspend_fn, \
> -	.restore = resume_fn,
> -#else
> -#define SET_SYSTEM_SLEEP_PM_OPS(suspend_fn, resume_fn)
> -#endif
> +	.suspend = PM_SLEEP_PTR(suspend_fn), \
> +	.resume = PM_SLEEP_PTR(resume_fn), \
> +	.freeze = PM_SLEEP_PTR(suspend_fn), \
> +	.thaw = PM_SLEEP_PTR(resume_fn), \
> +	.poweroff = PM_SLEEP_PTR(suspend_fn), \
> +	.restore = PM_SLEEP_PTR(resume_fn),
>  
> -#ifdef CONFIG_PM_SLEEP
>  #define SET_LATE_SYSTEM_SLEEP_PM_OPS(suspend_fn, resume_fn) \
> -	.suspend_late = suspend_fn, \
> -	.resume_early = resume_fn, \
> -	.freeze_late = suspend_fn, \
> -	.thaw_early = resume_fn, \
> -	.poweroff_late = suspend_fn, \
> -	.restore_early = resume_fn,
> -#else
> -#define SET_LATE_SYSTEM_SLEEP_PM_OPS(suspend_fn, resume_fn)
> -#endif
> +	.suspend_late = PM_SLEEP_PTR(suspend_fn), \
> +	.resume_early = PM_SLEEP_PTR(resume_fn), \
> +	.freeze_late = PM_SLEEP_PTR(suspend_fn), \
> +	.thaw_early = PM_SLEEP_PTR(resume_fn), \
> +	.poweroff_late = PM_SLEEP_PTR(suspend_fn), \
> +	.restore_early = PM_SLEEP_PTR(resume_fn),
>  
> -#ifdef CONFIG_PM_SLEEP
>  #define SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(suspend_fn, resume_fn) \
> -	.suspend_noirq = suspend_fn, \
> -	.resume_noirq = resume_fn, \
> -	.freeze_noirq = suspend_fn, \
> -	.thaw_noirq = resume_fn, \
> -	.poweroff_noirq = suspend_fn, \
> -	.restore_noirq = resume_fn,
> -#else
> -#define SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(suspend_fn, resume_fn)
> -#endif
> +	.suspend_noirq = PM_SLEEP_PTR(suspend_fn), \
> +	.resume_noirq = PM_SLEEP_PTR(resume_fn), \
> +	.freeze_noirq = PM_SLEEP_PTR(suspend_fn), \
> +	.thaw_noirq = PM_SLEEP_PTR(resume_fn), \
> +	.poweroff_noirq = PM_SLEEP_PTR(suspend_fn), \
> +	.restore_noirq = PM_SLEEP_PTR(resume_fn),
>  
> -#ifdef CONFIG_PM
>  #define SET_RUNTIME_PM_OPS(suspend_fn, resume_fn, idle_fn) \
> -	.runtime_suspend = suspend_fn, \
> -	.runtime_resume = resume_fn, \
> -	.runtime_idle = idle_fn,
> -#else
> -#define SET_RUNTIME_PM_OPS(suspend_fn, resume_fn, idle_fn)
> -#endif
> +	.runtime_suspend = PM_PTR(suspend_fn), \
> +	.runtime_resume = PM_PTR(resume_fn), \
> +	.runtime_idle = PM_PTR(idle_fn),
>  
>  /*
>   * Use this if you want to use the same suspend and resume callbacks for suspend
diff mbox

Patch

diff --git a/include/linux/pm.h b/include/linux/pm.h
index e723b78d8357..59f333922c15 100644
--- a/include/linux/pm.h
+++ b/include/linux/pm.h
@@ -313,50 +313,37 @@  struct dev_pm_ops {
 	int (*runtime_idle)(struct device *dev);
 };
 
-#ifdef CONFIG_PM_SLEEP
+#define PM_SLEEP_PTR(ptr) (IS_ENABLED(CONFIG_PM_SLEEP) ? (ptr) : NULL)
+#define PM_PTR(ptr) (IS_ENABLED(CONFIG_PM) ? (ptr) : NULL)
+
 #define SET_SYSTEM_SLEEP_PM_OPS(suspend_fn, resume_fn) \
-	.suspend = suspend_fn, \
-	.resume = resume_fn, \
-	.freeze = suspend_fn, \
-	.thaw = resume_fn, \
-	.poweroff = suspend_fn, \
-	.restore = resume_fn,
-#else
-#define SET_SYSTEM_SLEEP_PM_OPS(suspend_fn, resume_fn)
-#endif
+	.suspend = PM_SLEEP_PTR(suspend_fn), \
+	.resume = PM_SLEEP_PTR(resume_fn), \
+	.freeze = PM_SLEEP_PTR(suspend_fn), \
+	.thaw = PM_SLEEP_PTR(resume_fn), \
+	.poweroff = PM_SLEEP_PTR(suspend_fn), \
+	.restore = PM_SLEEP_PTR(resume_fn),
 
-#ifdef CONFIG_PM_SLEEP
 #define SET_LATE_SYSTEM_SLEEP_PM_OPS(suspend_fn, resume_fn) \
-	.suspend_late = suspend_fn, \
-	.resume_early = resume_fn, \
-	.freeze_late = suspend_fn, \
-	.thaw_early = resume_fn, \
-	.poweroff_late = suspend_fn, \
-	.restore_early = resume_fn,
-#else
-#define SET_LATE_SYSTEM_SLEEP_PM_OPS(suspend_fn, resume_fn)
-#endif
+	.suspend_late = PM_SLEEP_PTR(suspend_fn), \
+	.resume_early = PM_SLEEP_PTR(resume_fn), \
+	.freeze_late = PM_SLEEP_PTR(suspend_fn), \
+	.thaw_early = PM_SLEEP_PTR(resume_fn), \
+	.poweroff_late = PM_SLEEP_PTR(suspend_fn), \
+	.restore_early = PM_SLEEP_PTR(resume_fn),
 
-#ifdef CONFIG_PM_SLEEP
 #define SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(suspend_fn, resume_fn) \
-	.suspend_noirq = suspend_fn, \
-	.resume_noirq = resume_fn, \
-	.freeze_noirq = suspend_fn, \
-	.thaw_noirq = resume_fn, \
-	.poweroff_noirq = suspend_fn, \
-	.restore_noirq = resume_fn,
-#else
-#define SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(suspend_fn, resume_fn)
-#endif
+	.suspend_noirq = PM_SLEEP_PTR(suspend_fn), \
+	.resume_noirq = PM_SLEEP_PTR(resume_fn), \
+	.freeze_noirq = PM_SLEEP_PTR(suspend_fn), \
+	.thaw_noirq = PM_SLEEP_PTR(resume_fn), \
+	.poweroff_noirq = PM_SLEEP_PTR(suspend_fn), \
+	.restore_noirq = PM_SLEEP_PTR(resume_fn),
 
-#ifdef CONFIG_PM
 #define SET_RUNTIME_PM_OPS(suspend_fn, resume_fn, idle_fn) \
-	.runtime_suspend = suspend_fn, \
-	.runtime_resume = resume_fn, \
-	.runtime_idle = idle_fn,
-#else
-#define SET_RUNTIME_PM_OPS(suspend_fn, resume_fn, idle_fn)
-#endif
+	.runtime_suspend = PM_PTR(suspend_fn), \
+	.runtime_resume = PM_PTR(resume_fn), \
+	.runtime_idle = PM_PTR(idle_fn),
 
 /*
  * Use this if you want to use the same suspend and resume callbacks for suspend