[PATCH 0/8] DEV_PM_OPS macros rework

[PATCH 0/8] DEV_PM_OPS macros rework

[Paul Cercueil]

Hi,

This set of commits rework a bit the *_DEV_PM_OPS() macros that were
introduced recently.

- Remove the DEFINE_UNIVERSAL_DEV_PM_OPS() macro, since I highly doubt
  anything is going to use it. The macro it replaces
  (UNIVERSAL_DEV_PM_OPS) seems to only be used incorrectly in code that
  hasn't been updated in ages.

- Remove the static qualifier in DEFINE_SIMPLE_DEV_PM_OPS, so that the
  macro is more in line with what's done elsewhere in the kernel.

- Add a DEFINE_RUNTIME_DEV_PM_OPS() macro, for use with drivers that use
  runtime PM, and use runtime_pm_force_suspend/runtime_pm_force_resume
  as their system sleep callbacks.

- Add EXPORT_*_DEV_PM_OPS macros, which can be used for when the
  underlying dev_pm_ops is to be exported. With CONFIG_PM set, the
  symbol is exported as you would expect. With CONFIG_PM disabled, the
  dev_pm_ops is garbage-collected along with the suspend/resume
  callbacks.

- Update the two places which used DEFINE_SIMPLE_DEV_PM_OPS, to add back
  the "static" qualifier that was stripped from the macro.

- Update one driver to use EXPORT_RUNTIME_DEV_PM_OPS(), just to showcase
  how to use this macro in the case where a dev_pm_ops is to be
  exported.
  Note that the driver itself is GPL, and the symbol is only used within
  a GPL driver, so I would assume the symbol would be exported as GPL.
  But it was not the case in the original code, so I did not change the
  behaviour.

Feedback welcome.

Cheers,
-Paul


Paul Cercueil (8):
  PM: core: Remove DEFINE_UNIVERSAL_DEV_PM_OPS() macro
  PM: core: Remove static qualifier in DEFINE_SIMPLE_DEV_PM_OPS macro
  PM: core: Add EXPORT[_GPL]_SIMPLE_DEV_PM_OPS macros
  PM: runtime: Add DEFINE_RUNTIME_DEV_PM_OPS() macro
  PM: runtime: Add EXPORT[_GPL]_RUNTIME_DEV_PM_OPS macros
  mmc: mxc: Make dev_pm_ops struct static
  mmc: jz4740: Make dev_pm_ops struct static
  iio: gyro: mpu3050: Use new PM macros

 drivers/iio/gyro/mpu3050-core.c | 13 +++-----
 drivers/iio/gyro/mpu3050-i2c.c  |  2 +-
 drivers/mmc/host/jz4740_mmc.c   |  4 +--
 drivers/mmc/host/mxcmmc.c       |  2 +-
 include/linux/pm.h              | 53 +++++++++++++++++++++++----------
 include/linux/pm_runtime.h      | 21 +++++++++++++
 6 files changed, 67 insertions(+), 28 deletions(-)

-- 
2.34.1

[PATCH 1/8] PM: core: Remove DEFINE_UNIVERSAL_DEV_PM_OPS() macro

[Paul Cercueil]

The deprecated UNIVERSAL_DEV_PM_OPS() macro uses the provided callbacks
for both runtime PM and system sleep, which is very likely to be a
mistake, as a system sleep can be triggered while a given device is
already PM-suspended, which would cause the suspend callback to be
called twice.

The amount of users of UNIVERSAL_DEV_PM_OPS() is also tiny (16
occurences) compared to the number of places where
SET_SYSTEM_SLEEP_PM_OPS() is used with pm_runtime_force_suspend() and
pm_runtime_force_resume(), which makes me think that none of these cases
are actually valid.

As this macro is currently unused, remove it before someone starts to
use it in yet another invalid case.

Signed-off-by: Paul Cercueil <paul@crapouillou.net>
---
 include/linux/pm.h | 19 ++++++-------------
 1 file changed, 6 insertions(+), 13 deletions(-)

diff --git a/include/linux/pm.h b/include/linux/pm.h
index e1e9402180b9..31bbaafb06d2 100644
--- a/include/linux/pm.h
+++ b/include/linux/pm.h
@@ -366,6 +366,12 @@ static const struct dev_pm_ops name = { \
 	SYSTEM_SLEEP_PM_OPS(suspend_fn, resume_fn) \
 }
 
+/* Deprecated. Use DEFINE_SIMPLE_DEV_PM_OPS() instead. */
+#define SIMPLE_DEV_PM_OPS(name, suspend_fn, resume_fn) \
+const struct dev_pm_ops __maybe_unused name = { \
+	SET_SYSTEM_SLEEP_PM_OPS(suspend_fn, resume_fn) \
+}
+
 /*
  * Use this for defining a set of PM operations to be used in all situations
  * (system suspend, hibernation or runtime PM).
@@ -379,19 +385,6 @@ static const struct dev_pm_ops name = { \
  * .resume_early(), to the same routines as .runtime_suspend() and
  * .runtime_resume(), respectively (and analogously for hibernation).
  */
-#define DEFINE_UNIVERSAL_DEV_PM_OPS(name, suspend_fn, resume_fn, idle_fn) \
-static const struct dev_pm_ops name = { \
-	SYSTEM_SLEEP_PM_OPS(suspend_fn, resume_fn) \
-	RUNTIME_PM_OPS(suspend_fn, resume_fn, idle_fn) \
-}
-
-/* Deprecated. Use DEFINE_SIMPLE_DEV_PM_OPS() instead. */
-#define SIMPLE_DEV_PM_OPS(name, suspend_fn, resume_fn) \
-const struct dev_pm_ops __maybe_unused name = { \
-	SET_SYSTEM_SLEEP_PM_OPS(suspend_fn, resume_fn) \
-}
-
-/* Deprecated. Use DEFINE_UNIVERSAL_DEV_PM_OPS() instead. */
 #define UNIVERSAL_DEV_PM_OPS(name, suspend_fn, resume_fn, idle_fn) \
 const struct dev_pm_ops __maybe_unused name = { \
 	SET_SYSTEM_SLEEP_PM_OPS(suspend_fn, resume_fn) \
-- 
2.34.1

[PATCH 2/8] PM: core: Remove static qualifier in DEFINE_SIMPLE_DEV_PM_OPS macro

[Paul Cercueil]

Keep this macro in line with the other ones. This makes it possible to
use them in the cases where the underlying dev_pm_ops structure is
exported.

Signed-off-by: Paul Cercueil <paul@crapouillou.net>
---
 include/linux/pm.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/pm.h b/include/linux/pm.h
index 31bbaafb06d2..389e600df233 100644
--- a/include/linux/pm.h
+++ b/include/linux/pm.h
@@ -362,7 +362,7 @@ struct dev_pm_ops {
  * to RAM and hibernation.
  */
 #define DEFINE_SIMPLE_DEV_PM_OPS(name, suspend_fn, resume_fn) \
-static const struct dev_pm_ops name = { \
+const struct dev_pm_ops name = { \
 	SYSTEM_SLEEP_PM_OPS(suspend_fn, resume_fn) \
 }
 
-- 
2.34.1

[PATCH 3/8] PM: core: Add EXPORT[_GPL]_SIMPLE_DEV_PM_OPS macros

[Paul Cercueil]

These macros are defined conditionally, according to CONFIG_PM:
- if CONFIG_PM is enabled, these macros resolve to
  DEFINE_SIMPLE_DEV_PM_OPS(), and the dev_pm_ops symbol will be
  exported.

- if CONFIG_PM is disabled, these macros will result in a dummy static
  dev_pm_ops to be created with the __maybe_unused flag. The dev_pm_ops
  will then be discarded by the compiler, along with the provided
  callback functions if they are not used anywhere else.

In the second case, the symbol is not exported, which should be
perfectly fine - users of the symbol should all use the pm_ptr() or
pm_sleep_ptr() macro, so the dev_pm_ops marked as "extern" in the
client's code will never be accessed.

Signed-off-by: Paul Cercueil <paul@crapouillou.net>
---
 include/linux/pm.h | 33 ++++++++++++++++++++++++++++++---
 1 file changed, 30 insertions(+), 3 deletions(-)

diff --git a/include/linux/pm.h b/include/linux/pm.h
index 389e600df233..a1ce29566aea 100644
--- a/include/linux/pm.h
+++ b/include/linux/pm.h
@@ -8,6 +8,7 @@
 #ifndef _LINUX_PM_H
 #define _LINUX_PM_H
 
+#include <linux/export.h>
 #include <linux/list.h>
 #include <linux/workqueue.h>
 #include <linux/spinlock.h>
@@ -357,14 +358,40 @@ struct dev_pm_ops {
 #define SET_RUNTIME_PM_OPS(suspend_fn, resume_fn, idle_fn)
 #endif
 
+#define _DEFINE_DEV_PM_OPS(name, \
+			   suspend_fn, resume_fn, \
+			   runtime_suspend_fn, runtime_resume_fn, idle_fn) \
+const struct dev_pm_ops name = { \
+	SYSTEM_SLEEP_PM_OPS(suspend_fn, resume_fn) \
+	RUNTIME_PM_OPS(runtime_suspend_fn, runtime_resume_fn, idle_fn) \
+}
+
+
 /*
  * Use this if you want to use the same suspend and resume callbacks for suspend
  * to RAM and hibernation.
  */
 #define DEFINE_SIMPLE_DEV_PM_OPS(name, suspend_fn, resume_fn) \
-const struct dev_pm_ops name = { \
-	SYSTEM_SLEEP_PM_OPS(suspend_fn, resume_fn) \
-}
+	_DEFINE_DEV_PM_OPS(name, suspend_fn, resume_fn, NULL, NULL, NULL)
+
+#ifdef CONFIG_PM
+#define _EXPORT_DEV_PM_OPS(name, suspend_fn, resume_fn, runtime_suspend_fn, \
+			   runtime_resume_fn, idle_fn, sec) \
+	_DEFINE_DEV_PM_OPS(name, suspend_fn, resume_fn, runtime_suspend_fn, \
+			   runtime_resume_fn, idle_fn); \
+	_EXPORT_SYMBOL(name, sec)
+#else
+#define _EXPORT_DEV_PM_OPS(name, suspend_fn, resume_fn, runtime_suspend_fn, \
+			   runtime_resume_fn, idle_fn, sec) \
+static __maybe_unused _DEFINE_DEV_PM_OPS(__static_##name, suspend_fn, \
+					 resume_fn, runtime_suspend_fn, \
+					 runtime_resume_fn, idle_fn)
+#endif
+
+#define EXPORT_SIMPLE_DEV_PM_OPS(name, suspend_fn, resume_fn) \
+	_EXPORT_DEV_PM_OPS(name, suspend_fn, resume_fn, NULL, NULL, NULL, "")
+#define EXPORT_GPL_SIMPLE_DEV_PM_OPS(name, suspend_fn, resume_fn) \
+	_EXPORT_DEV_PM_OPS(name, suspend_fn, resume_fn, NULL, NULL, NULL, "_gpl")
 
 /* Deprecated. Use DEFINE_SIMPLE_DEV_PM_OPS() instead. */
 #define SIMPLE_DEV_PM_OPS(name, suspend_fn, resume_fn) \
-- 
2.34.1

[PATCH 4/8] PM: runtime: Add DEFINE_RUNTIME_DEV_PM_OPS() macro

[Paul Cercueil]

A lot of drivers create a dev_pm_ops struct with the system sleep
suspend/resume callbacks set to pm_runtime_force_suspend() and
pm_runtime_force_resume().

These drivers can now use the DEFINE_RUNTIME_DEV_PM_OPS() macro, which
will use pm_runtime_force_{suspend,resume}() as the system sleep
callbacks, while having the same dead code removal characteristic that
is already provided by DEFINE_SIMPLE_DEV_PM_OPS().

Signed-off-by: Paul Cercueil <paul@crapouillou.net>
---
 include/linux/pm.h         |  3 +++
 include/linux/pm_runtime.h | 14 ++++++++++++++
 2 files changed, 17 insertions(+)

diff --git a/include/linux/pm.h b/include/linux/pm.h
index a1ce29566aea..01c4fe495b7a 100644
--- a/include/linux/pm.h
+++ b/include/linux/pm.h
@@ -411,6 +411,9 @@ const struct dev_pm_ops __maybe_unused name = { \
  * suspend and "early" resume callback pointers, .suspend_late() and
  * .resume_early(), to the same routines as .runtime_suspend() and
  * .runtime_resume(), respectively (and analogously for hibernation).
+ *
+ * Deprecated. You most likely don't want this macro. Use
+ * DEFINE_RUNTIME_DEV_PM_OPS() instead.
  */
 #define UNIVERSAL_DEV_PM_OPS(name, suspend_fn, resume_fn, idle_fn) \
 const struct dev_pm_ops __maybe_unused name = { \
diff --git a/include/linux/pm_runtime.h b/include/linux/pm_runtime.h
index 016de5776b6d..4af454d29281 100644
--- a/include/linux/pm_runtime.h
+++ b/include/linux/pm_runtime.h
@@ -22,6 +22,20 @@
 					    usage_count */
 #define RPM_AUTO		0x08	/* Use autosuspend_delay */
 
+/*
+ * Use this for defining a set of PM operations to be used in all situations
+ * (system suspend, hibernation or runtime PM).
+ *
+ * Note that the behaviour differs from the deprecated UNIVERSAL_DEV_PM_OPS()
+ * macro, which uses the provided callbacks for both runtime PM and system
+ * sleep, while DEFINE_RUNTIME_DEV_PM_OPS() uses pm_runtime_force_suspend()
+ * and pm_runtime_force_resume() for its system sleep callbacks.
+ */
+#define DEFINE_RUNTIME_DEV_PM_OPS(name, suspend_fn, resume_fn, idle_fn) \
+	_DEFINE_DEV_PM_OPS(name, pm_runtime_force_suspend, \
+			   pm_runtime_force_resume, suspend_fn, \
+			   resume_fn, idle_fn)
+
 #ifdef CONFIG_PM
 extern struct workqueue_struct *pm_wq;
 
-- 
2.34.1

[PATCH 5/8] PM: runtime: Add EXPORT[_GPL]_RUNTIME_DEV_PM_OPS macros

[Paul Cercueil]

Similar to EXPORT[_GPL]_SIMPLE_DEV_PM_OPS, but for users with runtime-PM
suspend/resume callbacks.

Signed-off-by: Paul Cercueil <paul@crapouillou.net>
---
 include/linux/pm_runtime.h | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/include/linux/pm_runtime.h b/include/linux/pm_runtime.h
index 4af454d29281..a7f862a26c03 100644
--- a/include/linux/pm_runtime.h
+++ b/include/linux/pm_runtime.h
@@ -36,6 +36,13 @@
 			   pm_runtime_force_resume, suspend_fn, \
 			   resume_fn, idle_fn)
 
+#define EXPORT_RUNTIME_DEV_PM_OPS(name, suspend_fn, resume_fn, idle_fn) \
+	_EXPORT_DEV_PM_OPS(name, pm_runtime_force_suspend, pm_runtime_force_resume, \
+			   suspend_fn, resume_fn, idle_fn, "")
+#define EXPORT_GPL_RUNTIME_DEV_PM_OPS(name, suspend_fn, resume_fn, idle_fn) \
+	_EXPORT_DEV_PM_OPS(name, pm_runtime_force_suspend, pm_runtime_force_resume, \
+			   suspend_fn, resume_fn, idle_fn, "_gpl")
+
 #ifdef CONFIG_PM
 extern struct workqueue_struct *pm_wq;
 
-- 
2.34.1

[PATCH 6/8] mmc: mxc: Make dev_pm_ops struct static

[Paul Cercueil]

The new DEFINE_SIMPLE_DEV_PM_OPS() macro does not set the "static"
qualifier anymore, so we can add it here, as the underlying dev_pm_ops
struct is only used in this file.

Signed-off-by: Paul Cercueil <paul@crapouillou.net>
---
 drivers/mmc/host/mxcmmc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/mmc/host/mxcmmc.c b/drivers/mmc/host/mxcmmc.c
index 98c218bd6669..40b6878bea6c 100644
--- a/drivers/mmc/host/mxcmmc.c
+++ b/drivers/mmc/host/mxcmmc.c
@@ -1210,7 +1210,7 @@ static int mxcmci_resume(struct device *dev)
 	return ret;
 }
 
-DEFINE_SIMPLE_DEV_PM_OPS(mxcmci_pm_ops, mxcmci_suspend, mxcmci_resume);
+static DEFINE_SIMPLE_DEV_PM_OPS(mxcmci_pm_ops, mxcmci_suspend, mxcmci_resume);
 
 static struct platform_driver mxcmci_driver = {
 	.probe		= mxcmci_probe,
-- 
2.34.1

[PATCH 7/8] mmc: jz4740: Make dev_pm_ops struct static

[Paul Cercueil]

The new DEFINE_SIMPLE_DEV_PM_OPS() macro does not set the "static"
qualifier anymore, so we can add it here, as the underlying dev_pm_ops
struct is only used in this file.

Signed-off-by: Paul Cercueil <paul@crapouillou.net>
---
 drivers/mmc/host/jz4740_mmc.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/mmc/host/jz4740_mmc.c b/drivers/mmc/host/jz4740_mmc.c
index 7693236c946f..7ab1b38a7be5 100644
--- a/drivers/mmc/host/jz4740_mmc.c
+++ b/drivers/mmc/host/jz4740_mmc.c
@@ -1128,8 +1128,8 @@ static int jz4740_mmc_resume(struct device *dev)
 	return pinctrl_select_default_state(dev);
 }
 
-DEFINE_SIMPLE_DEV_PM_OPS(jz4740_mmc_pm_ops, jz4740_mmc_suspend,
-	jz4740_mmc_resume);
+static DEFINE_SIMPLE_DEV_PM_OPS(jz4740_mmc_pm_ops, jz4740_mmc_suspend,
+				jz4740_mmc_resume);
 
 static struct platform_driver jz4740_mmc_driver = {
 	.probe = jz4740_mmc_probe,
-- 
2.34.1

[PATCH 8/8] iio: gyro: mpu3050: Use new PM macros

[Paul Cercueil]

Use the new EXPORT_RUNTIME_DEV_PM_OPS() macro. It allows the underlying
dev_pm_ops struct as well as the suspend/resume callbacks to be detected
as dead code in the case where CONFIG_PM is disabled, without having to
wrap everything inside #ifdef CONFIG_PM guards.

Signed-off-by: Paul Cercueil <paul@crapouillou.net>
---
 drivers/iio/gyro/mpu3050-core.c | 13 ++++---------
 drivers/iio/gyro/mpu3050-i2c.c  |  2 +-
 2 files changed, 5 insertions(+), 10 deletions(-)

diff --git a/drivers/iio/gyro/mpu3050-core.c b/drivers/iio/gyro/mpu3050-core.c
index ea387efab62d..7d6721e268fe 100644
--- a/drivers/iio/gyro/mpu3050-core.c
+++ b/drivers/iio/gyro/mpu3050-core.c
@@ -1281,7 +1281,6 @@ int mpu3050_common_remove(struct device *dev)
 }
 EXPORT_SYMBOL(mpu3050_common_remove);
 
-#ifdef CONFIG_PM
 static int mpu3050_runtime_suspend(struct device *dev)
 {
 	return mpu3050_power_down(iio_priv(dev_get_drvdata(dev)));
@@ -1291,15 +1290,11 @@ static int mpu3050_runtime_resume(struct device *dev)
 {
 	return mpu3050_power_up(iio_priv(dev_get_drvdata(dev)));
 }
-#endif /* CONFIG_PM */
 
-const struct dev_pm_ops mpu3050_dev_pm_ops = {
-	SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
-				pm_runtime_force_resume)
-	SET_RUNTIME_PM_OPS(mpu3050_runtime_suspend,
-			   mpu3050_runtime_resume, NULL)
-};
-EXPORT_SYMBOL(mpu3050_dev_pm_ops);
+EXPORT_RUNTIME_DEV_PM_OPS(mpu3050_dev_pm_ops,
+			  mpu3050_runtime_suspend,
+			  mpu3050_runtime_resume,
+			  NULL);
 
 MODULE_AUTHOR("Linus Walleij");
 MODULE_DESCRIPTION("MPU3050 gyroscope driver");
diff --git a/drivers/iio/gyro/mpu3050-i2c.c b/drivers/iio/gyro/mpu3050-i2c.c
index ef5bcbc4b45b..820133cad601 100644
--- a/drivers/iio/gyro/mpu3050-i2c.c
+++ b/drivers/iio/gyro/mpu3050-i2c.c
@@ -114,7 +114,7 @@ static struct i2c_driver mpu3050_i2c_driver = {
 	.driver = {
 		.of_match_table = mpu3050_i2c_of_match,
 		.name = "mpu3050-i2c",
-		.pm = &mpu3050_dev_pm_ops,
+		.pm = pm_ptr(&mpu3050_dev_pm_ops),
 	},
 };
 module_i2c_driver(mpu3050_i2c_driver);
-- 
2.34.1

Re: [PATCH 1/8] PM: core: Remove DEFINE_UNIVERSAL_DEV_PM_OPS() macro

[Jonathan Cameron]

On Tue, 4 Jan 2022 21:42:07 +0000
Paul Cercueil <paul@crapouillou.net> wrote:

> The deprecated UNIVERSAL_DEV_PM_OPS() macro uses the provided callbacks
> for both runtime PM and system sleep, which is very likely to be a
> mistake, as a system sleep can be triggered while a given device is
> already PM-suspended, which would cause the suspend callback to be
> called twice.
> 
> The amount of users of UNIVERSAL_DEV_PM_OPS() is also tiny (16
> occurences) compared to the number of places where
> SET_SYSTEM_SLEEP_PM_OPS() is used with pm_runtime_force_suspend() and
> pm_runtime_force_resume(), which makes me think that none of these cases
> are actually valid.
> 
> As this macro is currently unused, remove it before someone starts to
> use it in yet another invalid case.
> 
> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
I suspect there are cases where calling suspend twice doesn't matter, but
it does seem unlikely to be particularly helpful.

So, makes sense to drop this unless there is some subtlety I'm missing.
Acked-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> ---
>  include/linux/pm.h | 19 ++++++-------------
>  1 file changed, 6 insertions(+), 13 deletions(-)
> 
> diff --git a/include/linux/pm.h b/include/linux/pm.h
> index e1e9402180b9..31bbaafb06d2 100644
> --- a/include/linux/pm.h
> +++ b/include/linux/pm.h
> @@ -366,6 +366,12 @@ static const struct dev_pm_ops name = { \
>  	SYSTEM_SLEEP_PM_OPS(suspend_fn, resume_fn) \
>  }
>  
> +/* Deprecated. Use DEFINE_SIMPLE_DEV_PM_OPS() instead. */
> +#define SIMPLE_DEV_PM_OPS(name, suspend_fn, resume_fn) \
> +const struct dev_pm_ops __maybe_unused name = { \
> +	SET_SYSTEM_SLEEP_PM_OPS(suspend_fn, resume_fn) \
> +}
> +
>  /*
>   * Use this for defining a set of PM operations to be used in all situations
>   * (system suspend, hibernation or runtime PM).
> @@ -379,19 +385,6 @@ static const struct dev_pm_ops name = { \
>   * .resume_early(), to the same routines as .runtime_suspend() and
>   * .runtime_resume(), respectively (and analogously for hibernation).
>   */
> -#define DEFINE_UNIVERSAL_DEV_PM_OPS(name, suspend_fn, resume_fn, idle_fn) \
> -static const struct dev_pm_ops name = { \
> -	SYSTEM_SLEEP_PM_OPS(suspend_fn, resume_fn) \
> -	RUNTIME_PM_OPS(suspend_fn, resume_fn, idle_fn) \
> -}
> -
> -/* Deprecated. Use DEFINE_SIMPLE_DEV_PM_OPS() instead. */
> -#define SIMPLE_DEV_PM_OPS(name, suspend_fn, resume_fn) \
> -const struct dev_pm_ops __maybe_unused name = { \
> -	SET_SYSTEM_SLEEP_PM_OPS(suspend_fn, resume_fn) \
> -}
> -
> -/* Deprecated. Use DEFINE_UNIVERSAL_DEV_PM_OPS() instead. */
>  #define UNIVERSAL_DEV_PM_OPS(name, suspend_fn, resume_fn, idle_fn) \
>  const struct dev_pm_ops __maybe_unused name = { \
>  	SET_SYSTEM_SLEEP_PM_OPS(suspend_fn, resume_fn) \

Re: [PATCH 2/8] PM: core: Remove static qualifier in DEFINE_SIMPLE_DEV_PM_OPS macro

[Jonathan Cameron]

On Tue, 4 Jan 2022 21:42:08 +0000
Paul Cercueil <paul@crapouillou.net> wrote:

> Keep this macro in line with the other ones. This makes it possible to
> use them in the cases where the underlying dev_pm_ops structure is
> exported.
> 
> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
Acked-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>


> ---
>  include/linux/pm.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/include/linux/pm.h b/include/linux/pm.h
> index 31bbaafb06d2..389e600df233 100644
> --- a/include/linux/pm.h
> +++ b/include/linux/pm.h
> @@ -362,7 +362,7 @@ struct dev_pm_ops {
>   * to RAM and hibernation.
>   */
>  #define DEFINE_SIMPLE_DEV_PM_OPS(name, suspend_fn, resume_fn) \
> -static const struct dev_pm_ops name = { \
> +const struct dev_pm_ops name = { \
>  	SYSTEM_SLEEP_PM_OPS(suspend_fn, resume_fn) \
>  }
>  

Re: [PATCH 3/8] PM: core: Add EXPORT[_GPL]_SIMPLE_DEV_PM_OPS macros

[Jonathan Cameron]

On Tue, 4 Jan 2022 21:42:09 +0000
Paul Cercueil <paul@crapouillou.net> wrote:

> These macros are defined conditionally, according to CONFIG_PM:
> - if CONFIG_PM is enabled, these macros resolve to
>   DEFINE_SIMPLE_DEV_PM_OPS(), and the dev_pm_ops symbol will be
>   exported.
> 
> - if CONFIG_PM is disabled, these macros will result in a dummy static
>   dev_pm_ops to be created with the __maybe_unused flag. The dev_pm_ops
>   will then be discarded by the compiler, along with the provided
>   callback functions if they are not used anywhere else.
> 
> In the second case, the symbol is not exported, which should be
> perfectly fine - users of the symbol should all use the pm_ptr() or
> pm_sleep_ptr() macro, so the dev_pm_ops marked as "extern" in the
> client's code will never be accessed.
> 
> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> ---
>  include/linux/pm.h | 33 ++++++++++++++++++++++++++++++---
>  1 file changed, 30 insertions(+), 3 deletions(-)
> 
> diff --git a/include/linux/pm.h b/include/linux/pm.h
> index 389e600df233..a1ce29566aea 100644
> --- a/include/linux/pm.h
> +++ b/include/linux/pm.h
> @@ -8,6 +8,7 @@
>  #ifndef _LINUX_PM_H
>  #define _LINUX_PM_H
>  
> +#include <linux/export.h>
>  #include <linux/list.h>
>  #include <linux/workqueue.h>
>  #include <linux/spinlock.h>
> @@ -357,14 +358,40 @@ struct dev_pm_ops {
>  #define SET_RUNTIME_PM_OPS(suspend_fn, resume_fn, idle_fn)
>  #endif
>  
> +#define _DEFINE_DEV_PM_OPS(name, \
> +			   suspend_fn, resume_fn, \
> +			   runtime_suspend_fn, runtime_resume_fn, idle_fn) \
> +const struct dev_pm_ops name = { \
> +	SYSTEM_SLEEP_PM_OPS(suspend_fn, resume_fn) \
> +	RUNTIME_PM_OPS(runtime_suspend_fn, runtime_resume_fn, idle_fn) \
> +}
> +

one blank line probably enough.

> +
>  /*
>   * Use this if you want to use the same suspend and resume callbacks for suspend
>   * to RAM and hibernation.
>   */
>  #define DEFINE_SIMPLE_DEV_PM_OPS(name, suspend_fn, resume_fn) \
> -const struct dev_pm_ops name = { \
> -	SYSTEM_SLEEP_PM_OPS(suspend_fn, resume_fn) \
> -}
> +	_DEFINE_DEV_PM_OPS(name, suspend_fn, resume_fn, NULL, NULL, NULL)
> +
> +#ifdef CONFIG_PM
> +#define _EXPORT_DEV_PM_OPS(name, suspend_fn, resume_fn, runtime_suspend_fn, \
> +			   runtime_resume_fn, idle_fn, sec) \
> +	_DEFINE_DEV_PM_OPS(name, suspend_fn, resume_fn, runtime_suspend_fn, \
> +			   runtime_resume_fn, idle_fn); \
> +	_EXPORT_SYMBOL(name, sec)
> +#else
> +#define _EXPORT_DEV_PM_OPS(name, suspend_fn, resume_fn, runtime_suspend_fn, \
> +			   runtime_resume_fn, idle_fn, sec) \
> +static __maybe_unused _DEFINE_DEV_PM_OPS(__static_##name, suspend_fn, \
> +					 resume_fn, runtime_suspend_fn, \
> +					 runtime_resume_fn, idle_fn)
> +#endif
> +
> +#define EXPORT_SIMPLE_DEV_PM_OPS(name, suspend_fn, resume_fn) \
> +	_EXPORT_DEV_PM_OPS(name, suspend_fn, resume_fn, NULL, NULL, NULL, "")
> +#define EXPORT_GPL_SIMPLE_DEV_PM_OPS(name, suspend_fn, resume_fn) \
> +	_EXPORT_DEV_PM_OPS(name, suspend_fn, resume_fn, NULL, NULL, NULL, "_gpl")

So you can get away with these two cases because the SYSTEM_SLEEP_PM_OPS() all have
pm_sleep_ptr() wrappers.  However, _EXPORT_DEV_PM_OPS() could be used directly and
would require __maybe_unused for the RUNTIME_PM_OPS() parameters which isn't ideal.

Maybe I'm missing some reason that isn't a problem though as easy to get lost in
these macros. :)

You could argue that the _ is meant to indicate that macro shouldn't be used directly
but I'm not that optimistic.

Jonathan



>  
>  /* Deprecated. Use DEFINE_SIMPLE_DEV_PM_OPS() instead. */
>  #define SIMPLE_DEV_PM_OPS(name, suspend_fn, resume_fn) \

Re: [PATCH 4/8] PM: runtime: Add DEFINE_RUNTIME_DEV_PM_OPS() macro

[Jonathan Cameron]

On Tue, 4 Jan 2022 21:42:10 +0000
Paul Cercueil <paul@crapouillou.net> wrote:

> A lot of drivers create a dev_pm_ops struct with the system sleep
> suspend/resume callbacks set to pm_runtime_force_suspend() and
> pm_runtime_force_resume().
> 
> These drivers can now use the DEFINE_RUNTIME_DEV_PM_OPS() macro, which
> will use pm_runtime_force_{suspend,resume}() as the system sleep
> callbacks, while having the same dead code removal characteristic that
> is already provided by DEFINE_SIMPLE_DEV_PM_OPS().
> 
> Signed-off-by: Paul Cercueil <paul@crapouillou.net>

I guess this is common enough to bother with a macro.

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> ---
>  include/linux/pm.h         |  3 +++
>  include/linux/pm_runtime.h | 14 ++++++++++++++
>  2 files changed, 17 insertions(+)
> 
> diff --git a/include/linux/pm.h b/include/linux/pm.h
> index a1ce29566aea..01c4fe495b7a 100644
> --- a/include/linux/pm.h
> +++ b/include/linux/pm.h
> @@ -411,6 +411,9 @@ const struct dev_pm_ops __maybe_unused name = { \
>   * suspend and "early" resume callback pointers, .suspend_late() and
>   * .resume_early(), to the same routines as .runtime_suspend() and
>   * .runtime_resume(), respectively (and analogously for hibernation).
> + *
> + * Deprecated. You most likely don't want this macro. Use
> + * DEFINE_RUNTIME_DEV_PM_OPS() instead.
>   */
>  #define UNIVERSAL_DEV_PM_OPS(name, suspend_fn, resume_fn, idle_fn) \
>  const struct dev_pm_ops __maybe_unused name = { \
> diff --git a/include/linux/pm_runtime.h b/include/linux/pm_runtime.h
> index 016de5776b6d..4af454d29281 100644
> --- a/include/linux/pm_runtime.h
> +++ b/include/linux/pm_runtime.h
> @@ -22,6 +22,20 @@
>  					    usage_count */
>  #define RPM_AUTO		0x08	/* Use autosuspend_delay */
>  
> +/*
> + * Use this for defining a set of PM operations to be used in all situations
> + * (system suspend, hibernation or runtime PM).
> + *
> + * Note that the behaviour differs from the deprecated UNIVERSAL_DEV_PM_OPS()
> + * macro, which uses the provided callbacks for both runtime PM and system
> + * sleep, while DEFINE_RUNTIME_DEV_PM_OPS() uses pm_runtime_force_suspend()
> + * and pm_runtime_force_resume() for its system sleep callbacks.
> + */
> +#define DEFINE_RUNTIME_DEV_PM_OPS(name, suspend_fn, resume_fn, idle_fn) \
> +	_DEFINE_DEV_PM_OPS(name, pm_runtime_force_suspend, \
> +			   pm_runtime_force_resume, suspend_fn, \
> +			   resume_fn, idle_fn)
> +
>  #ifdef CONFIG_PM
>  extern struct workqueue_struct *pm_wq;
>  

Re: [PATCH 5/8] PM: runtime: Add EXPORT[_GPL]_RUNTIME_DEV_PM_OPS macros

[Jonathan Cameron]

On Tue, 4 Jan 2022 21:42:11 +0000
Paul Cercueil <paul@crapouillou.net> wrote:

> Similar to EXPORT[_GPL]_SIMPLE_DEV_PM_OPS, but for users with runtime-PM
> suspend/resume callbacks.
> 
> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
Follow up earlier comment.  I think you want pm_ptr() around all the
entries for RUNTIME_PM_OPS

Jonathan

> ---
>  include/linux/pm_runtime.h | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/include/linux/pm_runtime.h b/include/linux/pm_runtime.h
> index 4af454d29281..a7f862a26c03 100644
> --- a/include/linux/pm_runtime.h
> +++ b/include/linux/pm_runtime.h
> @@ -36,6 +36,13 @@
>  			   pm_runtime_force_resume, suspend_fn, \
>  			   resume_fn, idle_fn)
>  
> +#define EXPORT_RUNTIME_DEV_PM_OPS(name, suspend_fn, resume_fn, idle_fn) \
> +	_EXPORT_DEV_PM_OPS(name, pm_runtime_force_suspend, pm_runtime_force_resume, \
> +			   suspend_fn, resume_fn, idle_fn, "")
> +#define EXPORT_GPL_RUNTIME_DEV_PM_OPS(name, suspend_fn, resume_fn, idle_fn) \
> +	_EXPORT_DEV_PM_OPS(name, pm_runtime_force_suspend, pm_runtime_force_resume, \
> +			   suspend_fn, resume_fn, idle_fn, "_gpl")
> +
>  #ifdef CONFIG_PM
>  extern struct workqueue_struct *pm_wq;
>  

Re: [PATCH 8/8] iio: gyro: mpu3050: Use new PM macros

[Jonathan Cameron]

On Tue, 4 Jan 2022 21:42:14 +0000
Paul Cercueil <paul@crapouillou.net> wrote:

> Use the new EXPORT_RUNTIME_DEV_PM_OPS() macro. It allows the underlying
> dev_pm_ops struct as well as the suspend/resume callbacks to be detected
> as dead code in the case where CONFIG_PM is disabled, without having to
> wrap everything inside #ifdef CONFIG_PM guards.
> 
> Signed-off-by: Paul Cercueil <paul@crapouillou.net>

Hohum - bad choice of example. These shouldn't be exported as only used within
the same module ;)  No one ever wrote the other bus interface (and the part is
ancient so I can't see it happening now) hence whilst there are two files, they
are built into a single module.  There is a comment about this in the Makefile.

Jonathan


> ---
>  drivers/iio/gyro/mpu3050-core.c | 13 ++++---------
>  drivers/iio/gyro/mpu3050-i2c.c  |  2 +-
>  2 files changed, 5 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/iio/gyro/mpu3050-core.c b/drivers/iio/gyro/mpu3050-core.c
> index ea387efab62d..7d6721e268fe 100644
> --- a/drivers/iio/gyro/mpu3050-core.c
> +++ b/drivers/iio/gyro/mpu3050-core.c
> @@ -1281,7 +1281,6 @@ int mpu3050_common_remove(struct device *dev)
>  }
>  EXPORT_SYMBOL(mpu3050_common_remove);
>  
> -#ifdef CONFIG_PM
>  static int mpu3050_runtime_suspend(struct device *dev)
>  {
>  	return mpu3050_power_down(iio_priv(dev_get_drvdata(dev)));
> @@ -1291,15 +1290,11 @@ static int mpu3050_runtime_resume(struct device *dev)
>  {
>  	return mpu3050_power_up(iio_priv(dev_get_drvdata(dev)));
>  }
> -#endif /* CONFIG_PM */
>  
> -const struct dev_pm_ops mpu3050_dev_pm_ops = {
> -	SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
> -				pm_runtime_force_resume)
> -	SET_RUNTIME_PM_OPS(mpu3050_runtime_suspend,
> -			   mpu3050_runtime_resume, NULL)
> -};
> -EXPORT_SYMBOL(mpu3050_dev_pm_ops);
> +EXPORT_RUNTIME_DEV_PM_OPS(mpu3050_dev_pm_ops,
> +			  mpu3050_runtime_suspend,
> +			  mpu3050_runtime_resume,
> +			  NULL);
>  
>  MODULE_AUTHOR("Linus Walleij");
>  MODULE_DESCRIPTION("MPU3050 gyroscope driver");
> diff --git a/drivers/iio/gyro/mpu3050-i2c.c b/drivers/iio/gyro/mpu3050-i2c.c
> index ef5bcbc4b45b..820133cad601 100644
> --- a/drivers/iio/gyro/mpu3050-i2c.c
> +++ b/drivers/iio/gyro/mpu3050-i2c.c
> @@ -114,7 +114,7 @@ static struct i2c_driver mpu3050_i2c_driver = {
>  	.driver = {
>  		.of_match_table = mpu3050_i2c_of_match,
>  		.name = "mpu3050-i2c",
> -		.pm = &mpu3050_dev_pm_ops,
> +		.pm = pm_ptr(&mpu3050_dev_pm_ops),
>  	},
>  };
>  module_i2c_driver(mpu3050_i2c_driver);

Re: [PATCH 6/8] mmc: mxc: Make dev_pm_ops struct static

[Jonathan Cameron]

On Tue, 4 Jan 2022 21:42:12 +0000
Paul Cercueil <paul@crapouillou.net> wrote:

> The new DEFINE_SIMPLE_DEV_PM_OPS() macro does not set the "static"
> qualifier anymore, so we can add it here, as the underlying dev_pm_ops
> struct is only used in this file.
> 
> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
FWIW on this trivial patch

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> ---
>  drivers/mmc/host/mxcmmc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/mmc/host/mxcmmc.c b/drivers/mmc/host/mxcmmc.c
> index 98c218bd6669..40b6878bea6c 100644
> --- a/drivers/mmc/host/mxcmmc.c
> +++ b/drivers/mmc/host/mxcmmc.c
> @@ -1210,7 +1210,7 @@ static int mxcmci_resume(struct device *dev)
>  	return ret;
>  }
>  
> -DEFINE_SIMPLE_DEV_PM_OPS(mxcmci_pm_ops, mxcmci_suspend, mxcmci_resume);
> +static DEFINE_SIMPLE_DEV_PM_OPS(mxcmci_pm_ops, mxcmci_suspend, mxcmci_resume);
>  
>  static struct platform_driver mxcmci_driver = {
>  	.probe		= mxcmci_probe,

Re: [PATCH 7/8] mmc: jz4740: Make dev_pm_ops struct static

[Jonathan Cameron]

On Tue, 4 Jan 2022 21:42:13 +0000
Paul Cercueil <paul@crapouillou.net> wrote:

> The new DEFINE_SIMPLE_DEV_PM_OPS() macro does not set the "static"
> qualifier anymore, so we can add it here, as the underlying dev_pm_ops
> struct is only used in this file.
> 
> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> ---
>  drivers/mmc/host/jz4740_mmc.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/mmc/host/jz4740_mmc.c b/drivers/mmc/host/jz4740_mmc.c
> index 7693236c946f..7ab1b38a7be5 100644
> --- a/drivers/mmc/host/jz4740_mmc.c
> +++ b/drivers/mmc/host/jz4740_mmc.c
> @@ -1128,8 +1128,8 @@ static int jz4740_mmc_resume(struct device *dev)
>  	return pinctrl_select_default_state(dev);
>  }
>  
> -DEFINE_SIMPLE_DEV_PM_OPS(jz4740_mmc_pm_ops, jz4740_mmc_suspend,
> -	jz4740_mmc_resume);
> +static DEFINE_SIMPLE_DEV_PM_OPS(jz4740_mmc_pm_ops, jz4740_mmc_suspend,
> +				jz4740_mmc_resume);
>  
>  static struct platform_driver jz4740_mmc_driver = {
>  	.probe = jz4740_mmc_probe,

Re: [PATCH 3/8] PM: core: Add EXPORT[_GPL]_SIMPLE_DEV_PM_OPS macros

[Paul Cercueil]

Hi Jonathan,

Le mer., janv. 5 2022 at 10:03:32 +0000, Jonathan Cameron 
<Jonathan.Cameron@Huawei.com> a écrit :
> On Tue, 4 Jan 2022 21:42:09 +0000
> Paul Cercueil <paul@crapouillou.net> wrote:
> 
>>  These macros are defined conditionally, according to CONFIG_PM:
>>  - if CONFIG_PM is enabled, these macros resolve to
>>    DEFINE_SIMPLE_DEV_PM_OPS(), and the dev_pm_ops symbol will be
>>    exported.
>> 
>>  - if CONFIG_PM is disabled, these macros will result in a dummy 
>> static
>>    dev_pm_ops to be created with the __maybe_unused flag. The 
>> dev_pm_ops
>>    will then be discarded by the compiler, along with the provided
>>    callback functions if they are not used anywhere else.
>> 
>>  In the second case, the symbol is not exported, which should be
>>  perfectly fine - users of the symbol should all use the pm_ptr() or
>>  pm_sleep_ptr() macro, so the dev_pm_ops marked as "extern" in the
>>  client's code will never be accessed.
>> 
>>  Signed-off-by: Paul Cercueil <paul@crapouillou.net>
>>  ---
>>   include/linux/pm.h | 33 ++++++++++++++++++++++++++++++---
>>   1 file changed, 30 insertions(+), 3 deletions(-)
>> 
>>  diff --git a/include/linux/pm.h b/include/linux/pm.h
>>  index 389e600df233..a1ce29566aea 100644
>>  --- a/include/linux/pm.h
>>  +++ b/include/linux/pm.h
>>  @@ -8,6 +8,7 @@
>>   #ifndef _LINUX_PM_H
>>   #define _LINUX_PM_H
>> 
>>  +#include <linux/export.h>
>>   #include <linux/list.h>
>>   #include <linux/workqueue.h>
>>   #include <linux/spinlock.h>
>>  @@ -357,14 +358,40 @@ struct dev_pm_ops {
>>   #define SET_RUNTIME_PM_OPS(suspend_fn, resume_fn, idle_fn)
>>   #endif
>> 
>>  +#define _DEFINE_DEV_PM_OPS(name, \
>>  +			   suspend_fn, resume_fn, \
>>  +			   runtime_suspend_fn, runtime_resume_fn, idle_fn) \
>>  +const struct dev_pm_ops name = { \
>>  +	SYSTEM_SLEEP_PM_OPS(suspend_fn, resume_fn) \
>>  +	RUNTIME_PM_OPS(runtime_suspend_fn, runtime_resume_fn, idle_fn) \
>>  +}
>>  +
> 
> one blank line probably enough.
> 
>>  +
>>   /*
>>    * Use this if you want to use the same suspend and resume 
>> callbacks for suspend
>>    * to RAM and hibernation.
>>    */
>>   #define DEFINE_SIMPLE_DEV_PM_OPS(name, suspend_fn, resume_fn) \
>>  -const struct dev_pm_ops name = { \
>>  -	SYSTEM_SLEEP_PM_OPS(suspend_fn, resume_fn) \
>>  -}
>>  +	_DEFINE_DEV_PM_OPS(name, suspend_fn, resume_fn, NULL, NULL, NULL)
>>  +
>>  +#ifdef CONFIG_PM
>>  +#define _EXPORT_DEV_PM_OPS(name, suspend_fn, resume_fn, 
>> runtime_suspend_fn, \
>>  +			   runtime_resume_fn, idle_fn, sec) \
>>  +	_DEFINE_DEV_PM_OPS(name, suspend_fn, resume_fn, 
>> runtime_suspend_fn, \
>>  +			   runtime_resume_fn, idle_fn); \
>>  +	_EXPORT_SYMBOL(name, sec)
>>  +#else
>>  +#define _EXPORT_DEV_PM_OPS(name, suspend_fn, resume_fn, 
>> runtime_suspend_fn, \
>>  +			   runtime_resume_fn, idle_fn, sec) \
>>  +static __maybe_unused _DEFINE_DEV_PM_OPS(__static_##name, 
>> suspend_fn, \
>>  +					 resume_fn, runtime_suspend_fn, \
>>  +					 runtime_resume_fn, idle_fn)
>>  +#endif
>>  +
>>  +#define EXPORT_SIMPLE_DEV_PM_OPS(name, suspend_fn, resume_fn) \
>>  +	_EXPORT_DEV_PM_OPS(name, suspend_fn, resume_fn, NULL, NULL, NULL, 
>> "")
>>  +#define EXPORT_GPL_SIMPLE_DEV_PM_OPS(name, suspend_fn, resume_fn) \
>>  +	_EXPORT_DEV_PM_OPS(name, suspend_fn, resume_fn, NULL, NULL, NULL, 
>> "_gpl")
> 
> So you can get away with these two cases because the 
> SYSTEM_SLEEP_PM_OPS() all have
> pm_sleep_ptr() wrappers.  However, _EXPORT_DEV_PM_OPS() could be used 
> directly and
> would require __maybe_unused for the RUNTIME_PM_OPS() parameters 
> which isn't ideal.

I don't see why. On both cases (CONFIG_PM enabled/disabled) the 
runtime-PM callbacks are referenced directly, so at no point do they 
appear as unused; therefore __maybe_unused is not needed.

Cheers,
-Paul

> Maybe I'm missing some reason that isn't a problem though as easy to 
> get lost in
> these macros. :)
> 
> You could argue that the _ is meant to indicate that macro shouldn't 
> be used directly
> but I'm not that optimistic.
> 
> Jonathan
> 
> 
> 
>> 
>>   /* Deprecated. Use DEFINE_SIMPLE_DEV_PM_OPS() instead. */
>>   #define SIMPLE_DEV_PM_OPS(name, suspend_fn, resume_fn) \
> 

Re: [PATCH 0/8] DEV_PM_OPS macros rework

[Jonathan Cameron]

On Tue, 4 Jan 2022 21:42:06 +0000
Paul Cercueil <paul@crapouillou.net> wrote:

> Hi,
> 
> This set of commits rework a bit the *_DEV_PM_OPS() macros that were
> introduced recently.
> 
> - Remove the DEFINE_UNIVERSAL_DEV_PM_OPS() macro, since I highly doubt
>   anything is going to use it. The macro it replaces
>   (UNIVERSAL_DEV_PM_OPS) seems to only be used incorrectly in code that
>   hasn't been updated in ages.
> 
> - Remove the static qualifier in DEFINE_SIMPLE_DEV_PM_OPS, so that the
>   macro is more in line with what's done elsewhere in the kernel.
> 
> - Add a DEFINE_RUNTIME_DEV_PM_OPS() macro, for use with drivers that use
>   runtime PM, and use runtime_pm_force_suspend/runtime_pm_force_resume
>   as their system sleep callbacks.
> 
> - Add EXPORT_*_DEV_PM_OPS macros, which can be used for when the
>   underlying dev_pm_ops is to be exported. With CONFIG_PM set, the
>   symbol is exported as you would expect. With CONFIG_PM disabled, the
>   dev_pm_ops is garbage-collected along with the suspend/resume
>   callbacks.
> 
> - Update the two places which used DEFINE_SIMPLE_DEV_PM_OPS, to add back
>   the "static" qualifier that was stripped from the macro.
> 
> - Update one driver to use EXPORT_RUNTIME_DEV_PM_OPS(), just to showcase
>   how to use this macro in the case where a dev_pm_ops is to be
>   exported.
>   Note that the driver itself is GPL, and the symbol is only used within
>   a GPL driver, so I would assume the symbol would be exported as GPL.
>   But it was not the case in the original code, so I did not change the
>   behaviour.
> 
> Feedback welcome.

Comments on individual patches (in particular bad pick for that final example ;)

Given how late we are in the cycle, I'd argue we 'need' patches 2 (+ 5,6 which
should probably be all one patch to avoid introducing then fixing a warning in
different patches).  The others could wait for the following cycle if needed.

It would slow down a few patches I have queued up behind this, but most of them
would be unaffected so it wouldn't annoy me too much. Can't speak for others
however!

Jonathan

> 
> Cheers,
> -Paul
> 
> 
> Paul Cercueil (8):
>   PM: core: Remove DEFINE_UNIVERSAL_DEV_PM_OPS() macro
>   PM: core: Remove static qualifier in DEFINE_SIMPLE_DEV_PM_OPS macro
>   PM: core: Add EXPORT[_GPL]_SIMPLE_DEV_PM_OPS macros
>   PM: runtime: Add DEFINE_RUNTIME_DEV_PM_OPS() macro
>   PM: runtime: Add EXPORT[_GPL]_RUNTIME_DEV_PM_OPS macros
>   mmc: mxc: Make dev_pm_ops struct static
>   mmc: jz4740: Make dev_pm_ops struct static
>   iio: gyro: mpu3050: Use new PM macros
> 
>  drivers/iio/gyro/mpu3050-core.c | 13 +++-----
>  drivers/iio/gyro/mpu3050-i2c.c  |  2 +-
>  drivers/mmc/host/jz4740_mmc.c   |  4 +--
>  drivers/mmc/host/mxcmmc.c       |  2 +-
>  include/linux/pm.h              | 53 +++++++++++++++++++++++----------
>  include/linux/pm_runtime.h      | 21 +++++++++++++
>  6 files changed, 67 insertions(+), 28 deletions(-)
> 

Re: [PATCH 8/8] iio: gyro: mpu3050: Use new PM macros

[Paul Cercueil]

Le mer., janv. 5 2022 at 10:11:06 +0000, Jonathan Cameron 
<Jonathan.Cameron@Huawei.com> a écrit :
> On Tue, 4 Jan 2022 21:42:14 +0000
> Paul Cercueil <paul@crapouillou.net> wrote:
> 
>>  Use the new EXPORT_RUNTIME_DEV_PM_OPS() macro. It allows the 
>> underlying
>>  dev_pm_ops struct as well as the suspend/resume callbacks to be 
>> detected
>>  as dead code in the case where CONFIG_PM is disabled, without 
>> having to
>>  wrap everything inside #ifdef CONFIG_PM guards.
>> 
>>  Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> 
> Hohum - bad choice of example. These shouldn't be exported as only 
> used within
> the same module ;)  No one ever wrote the other bus interface (and 
> the part is
> ancient so I can't see it happening now) hence whilst there are two 
> files, they
> are built into a single module.  There is a comment about this in the 
> Makefile.

Ok - then I'll drop this patch and try to find a better driver to 
showcase this.

Cheers,
-Paul

>>  ---
>>   drivers/iio/gyro/mpu3050-core.c | 13 ++++---------
>>   drivers/iio/gyro/mpu3050-i2c.c  |  2 +-
>>   2 files changed, 5 insertions(+), 10 deletions(-)
>> 
>>  diff --git a/drivers/iio/gyro/mpu3050-core.c 
>> b/drivers/iio/gyro/mpu3050-core.c
>>  index ea387efab62d..7d6721e268fe 100644
>>  --- a/drivers/iio/gyro/mpu3050-core.c
>>  +++ b/drivers/iio/gyro/mpu3050-core.c
>>  @@ -1281,7 +1281,6 @@ int mpu3050_common_remove(struct device *dev)
>>   }
>>   EXPORT_SYMBOL(mpu3050_common_remove);
>> 
>>  -#ifdef CONFIG_PM
>>   static int mpu3050_runtime_suspend(struct device *dev)
>>   {
>>   	return mpu3050_power_down(iio_priv(dev_get_drvdata(dev)));
>>  @@ -1291,15 +1290,11 @@ static int mpu3050_runtime_resume(struct 
>> device *dev)
>>   {
>>   	return mpu3050_power_up(iio_priv(dev_get_drvdata(dev)));
>>   }
>>  -#endif /* CONFIG_PM */
>> 
>>  -const struct dev_pm_ops mpu3050_dev_pm_ops = {
>>  -	SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
>>  -				pm_runtime_force_resume)
>>  -	SET_RUNTIME_PM_OPS(mpu3050_runtime_suspend,
>>  -			   mpu3050_runtime_resume, NULL)
>>  -};
>>  -EXPORT_SYMBOL(mpu3050_dev_pm_ops);
>>  +EXPORT_RUNTIME_DEV_PM_OPS(mpu3050_dev_pm_ops,
>>  +			  mpu3050_runtime_suspend,
>>  +			  mpu3050_runtime_resume,
>>  +			  NULL);
>> 
>>   MODULE_AUTHOR("Linus Walleij");
>>   MODULE_DESCRIPTION("MPU3050 gyroscope driver");
>>  diff --git a/drivers/iio/gyro/mpu3050-i2c.c 
>> b/drivers/iio/gyro/mpu3050-i2c.c
>>  index ef5bcbc4b45b..820133cad601 100644
>>  --- a/drivers/iio/gyro/mpu3050-i2c.c
>>  +++ b/drivers/iio/gyro/mpu3050-i2c.c
>>  @@ -114,7 +114,7 @@ static struct i2c_driver mpu3050_i2c_driver = {
>>   	.driver = {
>>   		.of_match_table = mpu3050_i2c_of_match,
>>   		.name = "mpu3050-i2c",
>>  -		.pm = &mpu3050_dev_pm_ops,
>>  +		.pm = pm_ptr(&mpu3050_dev_pm_ops),
>>   	},
>>   };
>>   module_i2c_driver(mpu3050_i2c_driver);
> 

Re: [PATCH 3/8] PM: core: Add EXPORT[_GPL]_SIMPLE_DEV_PM_OPS macros

[Jonathan Cameron]

On Wed, 5 Jan 2022 10:15:36 +0000
Paul Cercueil <paul@crapouillou.net> wrote:

> Hi Jonathan,
> 
> Le mer., janv. 5 2022 at 10:03:32 +0000, Jonathan Cameron 
> <Jonathan.Cameron@Huawei.com> a écrit :
> > On Tue, 4 Jan 2022 21:42:09 +0000
> > Paul Cercueil <paul@crapouillou.net> wrote:
> >   
> >>  These macros are defined conditionally, according to CONFIG_PM:
> >>  - if CONFIG_PM is enabled, these macros resolve to
> >>    DEFINE_SIMPLE_DEV_PM_OPS(), and the dev_pm_ops symbol will be
> >>    exported.
> >> 
> >>  - if CONFIG_PM is disabled, these macros will result in a dummy 
> >> static
> >>    dev_pm_ops to be created with the __maybe_unused flag. The 
> >> dev_pm_ops
> >>    will then be discarded by the compiler, along with the provided
> >>    callback functions if they are not used anywhere else.
> >> 
> >>  In the second case, the symbol is not exported, which should be
> >>  perfectly fine - users of the symbol should all use the pm_ptr() or
> >>  pm_sleep_ptr() macro, so the dev_pm_ops marked as "extern" in the
> >>  client's code will never be accessed.
> >> 
> >>  Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> >>  ---
> >>   include/linux/pm.h | 33 ++++++++++++++++++++++++++++++---
> >>   1 file changed, 30 insertions(+), 3 deletions(-)
> >> 
> >>  diff --git a/include/linux/pm.h b/include/linux/pm.h
> >>  index 389e600df233..a1ce29566aea 100644
> >>  --- a/include/linux/pm.h
> >>  +++ b/include/linux/pm.h
> >>  @@ -8,6 +8,7 @@
> >>   #ifndef _LINUX_PM_H
> >>   #define _LINUX_PM_H
> >> 
> >>  +#include <linux/export.h>
> >>   #include <linux/list.h>
> >>   #include <linux/workqueue.h>
> >>   #include <linux/spinlock.h>
> >>  @@ -357,14 +358,40 @@ struct dev_pm_ops {
> >>   #define SET_RUNTIME_PM_OPS(suspend_fn, resume_fn, idle_fn)
> >>   #endif
> >> 
> >>  +#define _DEFINE_DEV_PM_OPS(name, \
> >>  +			   suspend_fn, resume_fn, \
> >>  +			   runtime_suspend_fn, runtime_resume_fn, idle_fn) \
> >>  +const struct dev_pm_ops name = { \
> >>  +	SYSTEM_SLEEP_PM_OPS(suspend_fn, resume_fn) \
> >>  +	RUNTIME_PM_OPS(runtime_suspend_fn, runtime_resume_fn, idle_fn) \
> >>  +}
> >>  +  
> > 
> > one blank line probably enough.
> >   
> >>  +
> >>   /*
> >>    * Use this if you want to use the same suspend and resume 
> >> callbacks for suspend
> >>    * to RAM and hibernation.
> >>    */
> >>   #define DEFINE_SIMPLE_DEV_PM_OPS(name, suspend_fn, resume_fn) \
> >>  -const struct dev_pm_ops name = { \
> >>  -	SYSTEM_SLEEP_PM_OPS(suspend_fn, resume_fn) \
> >>  -}
> >>  +	_DEFINE_DEV_PM_OPS(name, suspend_fn, resume_fn, NULL, NULL, NULL)
> >>  +
> >>  +#ifdef CONFIG_PM
> >>  +#define _EXPORT_DEV_PM_OPS(name, suspend_fn, resume_fn, 
> >> runtime_suspend_fn, \
> >>  +			   runtime_resume_fn, idle_fn, sec) \
> >>  +	_DEFINE_DEV_PM_OPS(name, suspend_fn, resume_fn, 
> >> runtime_suspend_fn, \
> >>  +			   runtime_resume_fn, idle_fn); \
> >>  +	_EXPORT_SYMBOL(name, sec)
> >>  +#else
> >>  +#define _EXPORT_DEV_PM_OPS(name, suspend_fn, resume_fn, 
> >> runtime_suspend_fn, \
> >>  +			   runtime_resume_fn, idle_fn, sec) \
> >>  +static __maybe_unused _DEFINE_DEV_PM_OPS(__static_##name, 
> >> suspend_fn, \
> >>  +					 resume_fn, runtime_suspend_fn, \
> >>  +					 runtime_resume_fn, idle_fn)
> >>  +#endif
> >>  +
> >>  +#define EXPORT_SIMPLE_DEV_PM_OPS(name, suspend_fn, resume_fn) \
> >>  +	_EXPORT_DEV_PM_OPS(name, suspend_fn, resume_fn, NULL, NULL, NULL, 
> >> "")
> >>  +#define EXPORT_GPL_SIMPLE_DEV_PM_OPS(name, suspend_fn, resume_fn) \
> >>  +	_EXPORT_DEV_PM_OPS(name, suspend_fn, resume_fn, NULL, NULL, NULL, 
> >> "_gpl")  
> > 
> > So you can get away with these two cases because the 
> > SYSTEM_SLEEP_PM_OPS() all have
> > pm_sleep_ptr() wrappers.  However, _EXPORT_DEV_PM_OPS() could be used 
> > directly and
> > would require __maybe_unused for the RUNTIME_PM_OPS() parameters 
> > which isn't ideal.  
> 
> I don't see why. On both cases (CONFIG_PM enabled/disabled) the 
> runtime-PM callbacks are referenced directly, so at no point do they 
> appear as unused; therefore __maybe_unused is not needed.

Ah. I'd miss followed things through. Indeed the 'magic' __static_xxx_pm_ops
structure maintains a reference that the compiler can then remove.
On the plus side, turned out I'd not done a full set of tests with my
own patch set and found one bug in that :)

Acked-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> 
> Cheers,
> -Paul
> 
> > Maybe I'm missing some reason that isn't a problem though as easy to 
> > get lost in
> > these macros. :)
> > 
> > You could argue that the _ is meant to indicate that macro shouldn't 
> > be used directly
> > but I'm not that optimistic.
> > 
> > Jonathan
> > 
> > 
> >   
> >> 
> >>   /* Deprecated. Use DEFINE_SIMPLE_DEV_PM_OPS() instead. */
> >>   #define SIMPLE_DEV_PM_OPS(name, suspend_fn, resume_fn) \  
> >   
> 
> 

Re: [PATCH 5/8] PM: runtime: Add EXPORT[_GPL]_RUNTIME_DEV_PM_OPS macros

[Jonathan Cameron]

On Wed, 5 Jan 2022 10:07:23 +0000
Jonathan Cameron <Jonathan.Cameron@Huawei.com> wrote:

> On Tue, 4 Jan 2022 21:42:11 +0000
> Paul Cercueil <paul@crapouillou.net> wrote:
> 
> > Similar to EXPORT[_GPL]_SIMPLE_DEV_PM_OPS, but for users with runtime-PM
> > suspend/resume callbacks.
> > 
> > Signed-off-by: Paul Cercueil <paul@crapouillou.net>  
> Follow up earlier comment.  I think you want pm_ptr() around all the
> entries for RUNTIME_PM_OPS
> 

That issue didn't exist... 
Acked-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> Jonathan
> 
> > ---
> >  include/linux/pm_runtime.h | 7 +++++++
> >  1 file changed, 7 insertions(+)
> > 
> > diff --git a/include/linux/pm_runtime.h b/include/linux/pm_runtime.h
> > index 4af454d29281..a7f862a26c03 100644
> > --- a/include/linux/pm_runtime.h
> > +++ b/include/linux/pm_runtime.h
> > @@ -36,6 +36,13 @@
> >  			   pm_runtime_force_resume, suspend_fn, \
> >  			   resume_fn, idle_fn)
> >  
> > +#define EXPORT_RUNTIME_DEV_PM_OPS(name, suspend_fn, resume_fn, idle_fn) \
> > +	_EXPORT_DEV_PM_OPS(name, pm_runtime_force_suspend, pm_runtime_force_resume, \
> > +			   suspend_fn, resume_fn, idle_fn, "")
> > +#define EXPORT_GPL_RUNTIME_DEV_PM_OPS(name, suspend_fn, resume_fn, idle_fn) \
> > +	_EXPORT_DEV_PM_OPS(name, pm_runtime_force_suspend, pm_runtime_force_resume, \
> > +			   suspend_fn, resume_fn, idle_fn, "_gpl")
> > +
> >  #ifdef CONFIG_PM
> >  extern struct workqueue_struct *pm_wq;
> >    
> 

Re: [PATCH 0/8] DEV_PM_OPS macros rework

[Paul Cercueil]

Le mer., janv. 5 2022 at 10:17:37 +0000, Jonathan Cameron 
<Jonathan.Cameron@Huawei.com> a écrit :
> On Tue, 4 Jan 2022 21:42:06 +0000
> Paul Cercueil <paul@crapouillou.net> wrote:
> 
>>  Hi,
>> 
>>  This set of commits rework a bit the *_DEV_PM_OPS() macros that were
>>  introduced recently.
>> 
>>  - Remove the DEFINE_UNIVERSAL_DEV_PM_OPS() macro, since I highly 
>> doubt
>>    anything is going to use it. The macro it replaces
>>    (UNIVERSAL_DEV_PM_OPS) seems to only be used incorrectly in code 
>> that
>>    hasn't been updated in ages.
>> 
>>  - Remove the static qualifier in DEFINE_SIMPLE_DEV_PM_OPS, so that 
>> the
>>    macro is more in line with what's done elsewhere in the kernel.
>> 
>>  - Add a DEFINE_RUNTIME_DEV_PM_OPS() macro, for use with drivers 
>> that use
>>    runtime PM, and use 
>> runtime_pm_force_suspend/runtime_pm_force_resume
>>    as their system sleep callbacks.
>> 
>>  - Add EXPORT_*_DEV_PM_OPS macros, which can be used for when the
>>    underlying dev_pm_ops is to be exported. With CONFIG_PM set, the
>>    symbol is exported as you would expect. With CONFIG_PM disabled, 
>> the
>>    dev_pm_ops is garbage-collected along with the suspend/resume
>>    callbacks.
>> 
>>  - Update the two places which used DEFINE_SIMPLE_DEV_PM_OPS, to add 
>> back
>>    the "static" qualifier that was stripped from the macro.
>> 
>>  - Update one driver to use EXPORT_RUNTIME_DEV_PM_OPS(), just to 
>> showcase
>>    how to use this macro in the case where a dev_pm_ops is to be
>>    exported.
>>    Note that the driver itself is GPL, and the symbol is only used 
>> within
>>    a GPL driver, so I would assume the symbol would be exported as 
>> GPL.
>>    But it was not the case in the original code, so I did not change 
>> the
>>    behaviour.
>> 
>>  Feedback welcome.
> 
> Comments on individual patches (in particular bad pick for that final 
> example ;)
> 
> Given how late we are in the cycle, I'd argue we 'need' patches 2 (+ 
> 5,6 which
> should probably be all one patch to avoid introducing then fixing a 
> warning in
> different patches).  The others could wait for the following cycle if 
> needed.

Ok, should I V2 with patches 2/5/6 merged together?

-Paul

> It would slow down a few patches I have queued up behind this, but 
> most of them
> would be unaffected so it wouldn't annoy me too much. Can't speak for 
> others
> however!
> 
> Jonathan
> 
>> 
>>  Cheers,
>>  -Paul
>> 
>> 
>>  Paul Cercueil (8):
>>    PM: core: Remove DEFINE_UNIVERSAL_DEV_PM_OPS() macro
>>    PM: core: Remove static qualifier in DEFINE_SIMPLE_DEV_PM_OPS 
>> macro
>>    PM: core: Add EXPORT[_GPL]_SIMPLE_DEV_PM_OPS macros
>>    PM: runtime: Add DEFINE_RUNTIME_DEV_PM_OPS() macro
>>    PM: runtime: Add EXPORT[_GPL]_RUNTIME_DEV_PM_OPS macros
>>    mmc: mxc: Make dev_pm_ops struct static
>>    mmc: jz4740: Make dev_pm_ops struct static
>>    iio: gyro: mpu3050: Use new PM macros
>> 
>>   drivers/iio/gyro/mpu3050-core.c | 13 +++-----
>>   drivers/iio/gyro/mpu3050-i2c.c  |  2 +-
>>   drivers/mmc/host/jz4740_mmc.c   |  4 +--
>>   drivers/mmc/host/mxcmmc.c       |  2 +-
>>   include/linux/pm.h              | 53 
>> +++++++++++++++++++++++----------
>>   include/linux/pm_runtime.h      | 21 +++++++++++++
>>   6 files changed, 67 insertions(+), 28 deletions(-)
>> 
> 

Re: [PATCH 0/8] DEV_PM_OPS macros rework

[Rafael J. Wysocki]

On Wed, Jan 5, 2022 at 5:32 PM Paul Cercueil <paul@crapouillou.net> wrote:
>
>
>
> Le mer., janv. 5 2022 at 10:17:37 +0000, Jonathan Cameron
> <Jonathan.Cameron@Huawei.com> a écrit :
> > On Tue, 4 Jan 2022 21:42:06 +0000
> > Paul Cercueil <paul@crapouillou.net> wrote:
> >
> >>  Hi,
> >>
> >>  This set of commits rework a bit the *_DEV_PM_OPS() macros that were
> >>  introduced recently.
> >>
> >>  - Remove the DEFINE_UNIVERSAL_DEV_PM_OPS() macro, since I highly
> >> doubt
> >>    anything is going to use it. The macro it replaces
> >>    (UNIVERSAL_DEV_PM_OPS) seems to only be used incorrectly in code
> >> that
> >>    hasn't been updated in ages.
> >>
> >>  - Remove the static qualifier in DEFINE_SIMPLE_DEV_PM_OPS, so that
> >> the
> >>    macro is more in line with what's done elsewhere in the kernel.
> >>
> >>  - Add a DEFINE_RUNTIME_DEV_PM_OPS() macro, for use with drivers
> >> that use
> >>    runtime PM, and use
> >> runtime_pm_force_suspend/runtime_pm_force_resume
> >>    as their system sleep callbacks.
> >>
> >>  - Add EXPORT_*_DEV_PM_OPS macros, which can be used for when the
> >>    underlying dev_pm_ops is to be exported. With CONFIG_PM set, the
> >>    symbol is exported as you would expect. With CONFIG_PM disabled,
> >> the
> >>    dev_pm_ops is garbage-collected along with the suspend/resume
> >>    callbacks.
> >>
> >>  - Update the two places which used DEFINE_SIMPLE_DEV_PM_OPS, to add
> >> back
> >>    the "static" qualifier that was stripped from the macro.
> >>
> >>  - Update one driver to use EXPORT_RUNTIME_DEV_PM_OPS(), just to
> >> showcase
> >>    how to use this macro in the case where a dev_pm_ops is to be
> >>    exported.
> >>    Note that the driver itself is GPL, and the symbol is only used
> >> within
> >>    a GPL driver, so I would assume the symbol would be exported as
> >> GPL.
> >>    But it was not the case in the original code, so I did not change
> >> the
> >>    behaviour.
> >>
> >>  Feedback welcome.
> >
> > Comments on individual patches (in particular bad pick for that final
> > example ;)
> >
> > Given how late we are in the cycle, I'd argue we 'need' patches 2 (+
> > 5,6 which
> > should probably be all one patch to avoid introducing then fixing a
> > warning in
> > different patches).  The others could wait for the following cycle if
> > needed.
>
> Ok, should I V2 with patches 2/5/6 merged together?

Yes, please!