ahci: st: Switch from CONFIG_PM_SLEEP guards to pm_sleep_ptr()

PATCH Applied Raphael Gallais-Pou 3 participants 10 messages Jan 9, 2025 – Jan 15, 2025

Subsystems (2)

libata Arm/STI architecture

lore

Share & tools

lore

Raphael Gallais-Pou

author

2025-01-09 17:54 UTC #0 lore raw

PATCH v2 1/1 ahci: st: Switch from CONFIG_PM_SLEEP guards to pm_sleep_ptr()

Letting the compiler remove these functions when the kernel is built without CONFIG_PM_SLEEP support is simpler and less error prone than the use of #ifdef based kernel configuration guards.

Signed-off-by Raphael Gallais-Pou <[email protected]>

---
Changes in v2:
- Split serie in single patches
- Remove irrelevant 'Link:' from commit log
- Link to v1: https://lore.kernel.org/r/[email protected]

---
 drivers/ata/ahci_st.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

drivers/ata/ahci_st.c +2 -4

index 6b9b4a1dfa15..4336c8a6e208 100644
@@ -176,7 +176,6 @@ static int st_ahci_probe(struct platform_device *pdev)
 	return 0;
 }
 
-#ifdef CONFIG_PM_SLEEP
 static int st_ahci_suspend(struct device *dev)
 {
 	struct ata_host *host = dev_get_drvdata(dev);

@@ -221,9 +220,8 @@ static int st_ahci_resume(struct device *dev)
 
 	return ahci_platform_resume_host(dev);
 }
-#endif
 
-static SIMPLE_DEV_PM_OPS(st_ahci_pm_ops, st_ahci_suspend, st_ahci_resume);
+static DEFINE_SIMPLE_DEV_PM_OPS(st_ahci_pm_ops, st_ahci_suspend, st_ahci_resume);
 
 static const struct of_device_id st_ahci_match[] = {
 	{ .compatible = "st,ahci", },

@@ -234,7 +232,7 @@ MODULE_DEVICE_TABLE(of, st_ahci_match);
 static struct platform_driver st_ahci_driver = {
 	.driver = {
 		.name = DRV_NAME,
-		.pm = &st_ahci_pm_ops,
+		.pm = pm_sleep_ptr(&st_ahci_pm_ops),
 		.of_match_table = st_ahci_match,
 	},
 	.probe = st_ahci_probe,

Signature · 1 line

2.47.1

Damien Le Moal

maintainer

2025-01-10 05:19 UTC #1 lore raw

Quoted 27 lines · Raphael Gallais-Pou

Letting the compiler remove these functions when the kernel is built without CONFIG_PM_SLEEP support is simpler and less error prone than the use of #ifdef based kernel configuration guards.
Signed-off-by Raphael Gallais-Pou <[email protected]>
---
Changes in v2:
- Split serie in single patches
- Remove irrelevant 'Link:' from commit log
- Link to v1: https://lore.kernel.org/r/[email protected]
---
 drivers/ata/ahci_st.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)
drivers/ata/ahci_st.c +0 -1
index 6b9b4a1dfa15..4336c8a6e208 100644
@@ -176,7 +176,6 @@ static int st_ahci_probe(struct platform_device *pdev)
 	return 0;
 }
 
-#ifdef CONFIG_PM_SLEEP
 static int st_ahci_suspend(struct device *dev)
 {
 	struct ata_host *host = dev_get_drvdata(dev);

drivers/ata/ahci_st.c static int st_ahci_resume(struct device *dev) · line 224

Quoted 4 lines

drivers/ata/ahci_st.c +0 -1

@@ -221,9 +220,8 @@ static int st_ahci_resume(struct device *dev)
 
 	return ahci_platform_resume_host(dev);
 }
-#endif

I do not think you can remove the ifdef here. Otherwise, there is going to be a compilation warning when CONFIG_PM_SLEEP is not enabled. No ?

Quoted 15 lines

drivers/ata/ahci_st.c +2 -2

-static SIMPLE_DEV_PM_OPS(st_ahci_pm_ops, st_ahci_suspend, st_ahci_resume);
+static DEFINE_SIMPLE_DEV_PM_OPS(st_ahci_pm_ops, st_ahci_suspend, st_ahci_resume);
 
 static const struct of_device_id st_ahci_match[] = {
 	{ .compatible = "st,ahci", },

@@ -234,7 +232,7 @@ MODULE_DEVICE_TABLE(of, st_ahci_match);
 static struct platform_driver st_ahci_driver = {
 	.driver = {
 		.name = DRV_NAME,
-		.pm = &st_ahci_pm_ops,
+		.pm = pm_sleep_ptr(&st_ahci_pm_ops),
 		.of_match_table = st_ahci_match,
 	},
 	.probe = st_ahci_probe,

Signature · 2 lines

Damien Le Moal
Western Digital Research

Niklas Cassel

maintainer

2025-01-10 11:23 UTC #2 lore raw

Quoted 29 lines · Damien Le Moal

Quoted 28 lines · Raphael Gallais-Pou
Letting the compiler remove these functions when the kernel is built without CONFIG_PM_SLEEP support is simpler and less error prone than the use of #ifdef based kernel configuration guards.
Signed-off-by Raphael Gallais-Pou <[email protected]>
---
Changes in v2:
- Split serie in single patches
- Remove irrelevant 'Link:' from commit log
- Link to v1: https://lore.kernel.org/r/[email protected]
---
 drivers/ata/ahci_st.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)
drivers/ata/ahci_st.c +0 -1
index 6b9b4a1dfa15..4336c8a6e208 100644
@@ -176,7 +176,6 @@ static int st_ahci_probe(struct platform_device *pdev)
 	return 0;
 }
 
-#ifdef CONFIG_PM_SLEEP
 static int st_ahci_suspend(struct device *dev)
 {
 	struct ata_host *host = dev_get_drvdata(dev);
@@ -221,9 +220,8 @@ static int st_ahci_resume(struct device *dev)
 

Quoted 6 lines

drivers/ata/ahci_st.c static int st_ahci_resume(struct device *dev) · line 224
Quoted 3 lines
drivers/ata/ahci_st.c +0 -1
 	return ahci_platform_resume_host(dev);
 }
-#endif
I do not think you can remove the ifdef here. Otherwise, there is going to be a compilation warning when CONFIG_PM_SLEEP is not enabled. No ?

Look at the pm_sleep_ptr macro:
include/linux/pm.h:#define pm_sleep_ptr(_ptr) PTR_IF(IS_ENABLED(CONFIG_PM_SLEEP), (_ptr))

I would expect the function should be optimized out by the compiler using dead code elimination.

Raphael, perhaps you could show the before and after output using ./scripts/bloat-o-meter ?
(When the config is not enabled: before and after your patch.)

Quoted 21 lines

Quoted 16 lines

drivers/ata/ahci_st.c +2 -2

-static SIMPLE_DEV_PM_OPS(st_ahci_pm_ops, st_ahci_suspend, st_ahci_resume);
+static DEFINE_SIMPLE_DEV_PM_OPS(st_ahci_pm_ops, st_ahci_suspend, st_ahci_resume);
 
 static const struct of_device_id st_ahci_match[] = {
 	{ .compatible = "st,ahci", },

@@ -234,7 +232,7 @@ MODULE_DEVICE_TABLE(of, st_ahci_match);
 static struct platform_driver st_ahci_driver = {
 	.driver = {
 		.name = DRV_NAME,
-		.pm = &st_ahci_pm_ops,
+		.pm = pm_sleep_ptr(&st_ahci_pm_ops),
 		.of_match_table = st_ahci_match,
 	},
 	.probe = st_ahci_probe,

Signature · 2 lines

Damien Le Moal
Western Digital Research

Damien Le Moal

maintainer

2025-01-10 12:02 UTC #3 lore raw

Quoted 13 lines · Niklas Cassel

Quoted 6 lines
Quoted 6 lines
drivers/ata/ahci_st.c +0 -1
-#ifdef CONFIG_PM_SLEEP
 static int st_ahci_suspend(struct device *dev)
 {
 	struct ata_host *host = dev_get_drvdata(dev);
@@ -221,9 +220,8 @@ static int st_ahci_resume(struct device *dev)
 
Quoted 6 lines
drivers/ata/ahci_st.c static int st_ahci_resume(struct device *dev) · line 224
Quoted 3 lines
drivers/ata/ahci_st.c +0 -1
 	return ahci_platform_resume_host(dev);
 }
-#endif
I do not think you can remove the ifdef here. Otherwise, there is going to be a compilation warning when CONFIG_PM_SLEEP is not enabled. No ?

Quoted 5 lines

Look at the pm_sleep_ptr macro:
include/linux/pm.h:#define pm_sleep_ptr(_ptr) PTR_IF(IS_ENABLED(CONFIG_PM_SLEEP), (_ptr))
I would expect the function should be optimized out by the compiler using dead code elimination.

Indeed. Just tried and no warning. I was expecting a "defined but not used" warning, but none showed up. So all good.

Quoted 3 lines

Raphael, perhaps you could show the before and after output using ./scripts/bloat-o-meter ?
(When the config is not enabled: before and after your patch.)

No need to do that I guess. But there are 17 other ata driver that set .pm operations. What about these ? Don't they need the same treatment as ahci_st ? 15 of these also use SIMPLE_DEV_PM_OPS() which can be replaced with DEFINE_SIMPLE_DEV_PM_OPS() also, no ?

Do you want us to do that cleanup ? (fine with me).

Signature · 2 lines

Damien Le Moal
Western Digital Research

Raphaël Gallais-Pou

author

2025-01-13 20:28 UTC #4 lore raw

Le 10/01/2025 à 13:02, Damien Le Moal a écrit :

Quoted 14 lines · Niklas Cassel

Quoted 13 lines
Quoted 12 lines
drivers/ata/ahci_st.c static int st_ahci_resume(struct device *dev) · line 224
Quoted 9 lines
drivers/ata/ahci_st.c +0 -2
-#ifdef CONFIG_PM_SLEEP
  static int st_ahci_suspend(struct device *dev)
  {
  	struct ata_host *host = dev_get_drvdata(dev);
@@ -221,9 +220,8 @@ static int st_ahci_resume(struct device *dev)
 
  	return ahci_platform_resume_host(dev);
  }
-#endif
I do not think you can remove the ifdef here. Otherwise, there is going to be a compilation warning when CONFIG_PM_SLEEP is not enabled. No ?

Quoted 12 lines

Quoted 5 lines
Look at the pm_sleep_ptr macro:
include/linux/pm.h:#define pm_sleep_ptr(_ptr) PTR_IF(IS_ENABLED(CONFIG_PM_SLEEP), (_ptr))
I would expect the function should be optimized out by the compiler using dead code elimination.
Indeed. Just tried and no warning. I was expecting a "defined but not used" warning, but none showed up. So all good.
Quoted 3 lines
Raphael, perhaps you could show the before and after output using ./scripts/bloat-o-meter ?
(When the config is not enabled: before and after your patch.)

Hi,

I have not used the bloat-o-meter until now, thanks ! :) Here are my results:

* with the configuration

$ ./scripts/bloat-o-meter ahci_st_no_patch_pm.o ahci_st_patch_pm.o
add/remove: 1/1 grow/shrink: 0/0 up/down: 4/-4 (0)
Function                                     old     new   delta
__initcall__kmod_ahci_st__384_241_st_ahci_driver_init6       -       4 
   +4
__initcall__kmod_ahci_st__384_243_st_ahci_driver_init6       4       - 
   -4
Total: Before=2200, After=2200, chg +0.00%

* without the configuration

$ ./scripts/bloat-o-meter ahci_st_no_patch_no_pm.o ahci_st_patch_no_pm.o
add/remove: 1/2 grow/shrink: 0/0 up/down: 4/-96 (-92)
Function                                     old     new   delta
__initcall__kmod_ahci_st__383_241_st_ahci_driver_init6       -       4 
   +4
__initcall__kmod_ahci_st__383_243_st_ahci_driver_init6       4       - 
   -4
st_ahci_pm_ops                                92       -     -92
Total: Before=1904, After=1812, chg -4.83%

Looks like the patch shrinks a bit more the driver. I also tested, so we should be fine I think.

Quoted 7 lines

No need to do that I guess. But there are 17 other ata driver that set .pm operations. What about these ? Don't they need the same treatment as ahci_st ? 15 of these also use SIMPLE_DEV_PM_OPS() which can be replaced with DEFINE_SIMPLE_DEV_PM_OPS() also, no ?
Do you want us to do that cleanup ? (fine with me).

Regarding the other ata drivers, if you have the patience I can do this in a few weeks. There is other things on the stove I would like to do.

Regards,
Raphaël

Quoted 1 line

Damien Le Moal

maintainer

2025-01-14 00:59 UTC #5 lore raw

Quoted 4 lines · Raphaël Gallais-Pou

Quoted 1 line
Do you want us to do that cleanup ? (fine with me).
Regarding the other ata drivers, if you have the patience I can do this in a few weeks. There is other things on the stove I would like to do.

OK. We will work on this.

Signature · 2 lines

Damien Le Moal
Western Digital Research

Raphaël Gallais-Pou

author

2025-01-15 08:28 UTC #6 lore raw

Le 14/01/2025 à 01:59, Damien Le Moal a écrit :

Quoted 7 lines · Raphaël Gallais-Pou

Quoted 4 lines
Quoted 1 line
Do you want us to do that cleanup ? (fine with me).
Regarding the other ata drivers, if you have the patience I can do this in a few weeks. There is other things on the stove I would like to do.
OK. We will work on this.

Sorry to change my plans, I actually started working on this. Is this okay to let me handle this ? I will try to submit something promptly.

Thanks,
Raphaël

Quoted 2 lines

Niklas Cassel

maintainer

2025-01-15 10:44 UTC #7 lore raw

Quoted 11 lines · Raphaël Gallais-Pou

Le 14/01/2025 à 01:59, Damien Le Moal a écrit :
Quoted 7 lines · Raphaël Gallais-Pou
Quoted 4 lines
Quoted 1 line
Do you want us to do that cleanup ? (fine with me).
Regarding the other ata drivers, if you have the patience I can do this in a few weeks. There is other things on the stove I would like to do.
OK. We will work on this.
Sorry to change my plans, I actually started working on this. Is this okay to let me handle this ? I will try to submit something promptly.

Of course, I know that both me and Damien are quite busy at the moment.

Looking forward to your patches!

Kind regards,
Niklas

Niklas Cassel

maintainer Reviewed-by on v2 1/1

2025-01-10 11:20 UTC #8 lore raw

In reply to Raphael Gallais-Pou

Quoted 49 lines · Raphael Gallais-Pou

Letting the compiler remove these functions when the kernel is built without CONFIG_PM_SLEEP support is simpler and less error prone than the use of #ifdef based kernel configuration guards.

Signed-off-by Raphael Gallais-Pou <[email protected]>

---
Changes in v2:
- Split serie in single patches
- Remove irrelevant 'Link:' from commit log
- Link to v1: https://lore.kernel.org/r/[email protected]

---
 drivers/ata/ahci_st.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

drivers/ata/ahci_st.c +2 -4

index 6b9b4a1dfa15..4336c8a6e208 100644
@@ -176,7 +176,6 @@ static int st_ahci_probe(struct platform_device *pdev)
 	return 0;
 }
 
-#ifdef CONFIG_PM_SLEEP
 static int st_ahci_suspend(struct device *dev)
 {
 	struct ata_host *host = dev_get_drvdata(dev);

@@ -221,9 +220,8 @@ static int st_ahci_resume(struct device *dev)
 
 	return ahci_platform_resume_host(dev);
 }
-#endif
 
-static SIMPLE_DEV_PM_OPS(st_ahci_pm_ops, st_ahci_suspend, st_ahci_resume);
+static DEFINE_SIMPLE_DEV_PM_OPS(st_ahci_pm_ops, st_ahci_suspend, st_ahci_resume);
 
 static const struct of_device_id st_ahci_match[] = {
 	{ .compatible = "st,ahci", },

@@ -234,7 +232,7 @@ MODULE_DEVICE_TABLE(of, st_ahci_match);
 static struct platform_driver st_ahci_driver = {
 	.driver = {
 		.name = DRV_NAME,
-		.pm = &st_ahci_pm_ops,
+		.pm = pm_sleep_ptr(&st_ahci_pm_ops),
 		.of_match_table = st_ahci_match,
 	},
 	.probe = st_ahci_probe,

Signature · 1 line

2.47.1

Reviewed-by Niklas Cassel <[email protected]>

Niklas Cassel

maintainer Applied

2025-01-15 14:23 UTC #9 lore raw

In reply to Raphael Gallais-Pou

Quoted 5 lines · Raphael Gallais-Pou

Letting the compiler remove these functions when the kernel is built without CONFIG_PM_SLEEP support is simpler and less error prone than the use of #ifdef based kernel configuration guards.

Applied to libata/linux.git (for-6.14), thanks!

[1/1] ahci: st: Switch from CONFIG_PM_SLEEP guards to pm_sleep_ptr()

      https://git.kernel.org/libata/linux/c/f2809aa4

Kind regards,
Niklas