Skip to contentSkip to conversation

au1xmmc: dev_pm_ops conversion

PATCH New Albin Tonnerre 7 participants 25 messages Aug 3, 2009 – Aug 6, 2009

Subsystems (3)
Suspend to RAM Hibernation Power management core
lore
Share & tools
lore

Albin Tonnerre

author
#0 lore raw
Quoted 3 lines · Frans Pop

Yes, I see that in drivers/base/platform.c (legacy) .suspend resp. .resume also got called for those cases?
Ouch :-(

This really looks like it's error-prone (I made this mistake for atmel_serial and wouldn't have noticed if Frans hadn't told me), and AFAICS, for a number of drivers we'll have suspend = freeze = poweroff and resume = thaw = restore. Maybe putting something like this in pm.h would help ?

#define PM_OPS(name, suspend, resume) \

struct dev_pm_ops name = { \
	.suspend = suspend, \
	.resume = resume, \
	.freeze = suspend, \
	.thaw = resume, \
	.poweroff = suspend, \
	.restore = resume, \
};

Cheers,

Signature · 4 lines
Albin Tonnerre, Free Electrons
Kernel, drivers and embedded Linux development,
consulting, training and support.
http://free-electrons.com
#1 lore raw
Quoted 11 lines · Albin Tonnerre
Quoted 3 lines · Frans Pop

Yes, I see that in drivers/base/platform.c (legacy) .suspend resp. .resume also got called for those cases?
Ouch :-(

This really looks like it's error-prone (I made this mistake for atmel_serial and wouldn't have noticed if Frans hadn't told me), and AFAICS, for a number of drivers we'll have suspend = freeze = poweroff and resume = thaw = restore. Maybe putting something like this in pm.h would help ?

#define PM_OPS(name, suspend, resume) \

Quoted 8 lines
struct dev_pm_ops name = { \
	.suspend = suspend, \
	.resume = resume, \
	.freeze = suspend, \
	.thaw = resume, \
	.poweroff = suspend, \
	.restore = resume, \
};

That looks like a good idea, but please call that macro SIMPLE_PM_OPS (or something similar) and submit a patch.

Best,
Rafael

author
#2 lore raw

PATCH 1/1 Add SIMPLE_PM_OPS: make switching to dev_pm_ops less error-prone

In a number of cases, the .suspend, .freeze, .poweroff and .resume, .thaw, .restore functions are identical. However, they all need to be assigned to avoid regressionsm as the previous code called .suspend resp. .resume in all those cases. SIMPLE_PM_OPS allows to deal with this case.

Signed-off-by Albin Tonnerre <[email protected]>
---
 include/linux/pm.h |   10 ++++++++++
 1 files changed, 10 insertions(+), 0 deletions(-)
include/linux/pm.h +10 -0
index b3f7476..994a62f 100644
@@ -184,6 +184,16 @@ struct dev_pm_ops {
 	int (*restore_noirq)(struct device *dev);
 };
 
+#define SIMPLE_PM_OPS(name, suspend_fn, resume_fn) \
+struct dev_pm_ops name = { \
+	.suspend = suspend_fn, \
+	.resume = resume_fn, \
+	.freeze = suspend_fn, \
+	.thaw = resume_fn, \
+	.poweroff = suspend_fn, \
+	.resume = resume_fn, \
+}
+
 /**
  * PM_EVENT_ messages
  *
Signature · 4 lines
Albin Tonnerre, Free Electrons
Kernel, drivers and embedded Linux development,
consulting, training and support.
http://free-electrons.com
author
#3 lore raw

PATCH 1/1 Add SIMPLE_PM_OPS: make switching to dev_pm_ops less error-prone

In reply to Rafael J. Wysocki

In a number of cases, the .suspend, .freeze, .poweroff and .resume, .thaw, .restore functions are identical. However, they all need to be assigned to avoid regressionsm as the previous code called .suspend resp. .resume in all those cases. SIMPLE_PM_OPS allows to deal with this case.

Signed-off-by Albin Tonnerre <[email protected]>
---
 include/linux/pm.h |   10 ++++++++++
 1 files changed, 10 insertions(+), 0 deletions(-)
include/linux/pm.h +10 -0
index b3f7476..994a62f 100644
@@ -184,6 +184,16 @@ struct dev_pm_ops {
 	int (*restore_noirq)(struct device *dev);
 };
 
+#define SIMPLE_PM_OPS(name, suspend_fn, resume_fn) \
+struct dev_pm_ops name = { \
+	.suspend = suspend_fn, \
+	.resume = resume_fn, \
+	.freeze = suspend_fn, \
+	.thaw = resume_fn, \
+	.poweroff = suspend_fn, \
+	.resume = resume_fn, \
+}
+
 /**
  * PM_EVENT_ messages
  *
Signature · 4 lines
Albin Tonnerre, Free Electrons
Kernel, drivers and embedded Linux development,
consulting, training and support.
http://free-electrons.com
#4 lore raw

Re: [PATCH] Add SIMPLE_PM_OPS: make switching to dev_pm_ops less error-prone

Quoted 6 lines · Albin Tonnerre

In a number of cases, the .suspend, .freeze, .poweroff and .resume, .thaw, .restore functions are identical. However, they all need to be assigned to avoid regressionsm as the previous code called .suspend resp. .resume in all those cases. SIMPLE_PM_OPS allows to deal with this case.

I'd much rather have conversions done with a bit more analysis now that our framework is more flexible and we can have specialized routines for hibernation and suspend.

Maybe we should try changing from run-time to build time warning so that users are not overly concerned with it?

Thanks.

Signature · 1 line
Dmitry
#5 lore raw

Re: [PATCH] Add SIMPLE_PM_OPS: make switching to dev_pm_ops less error-prone

In reply to Albin Tonnerre
Quoted 6 lines · Albin Tonnerre

In a number of cases, the .suspend, .freeze, .poweroff and .resume, .thaw, .restore functions are identical. However, they all need to be assigned to avoid regressionsm as the previous code called .suspend resp. .resume in all those cases. SIMPLE_PM_OPS allows to deal with this case.

I'd much rather have conversions done with a bit more analysis now that our framework is more flexible and we can have specialized routines for hibernation and suspend.

Maybe we should try changing from run-time to build time warning so that users are not overly concerned with it?

Thanks.

Signature · 1 line
Dmitry
author
#6 lore raw

Re: [PATCH] Add SIMPLE_PM_OPS: make switching to dev_pm_ops less error-prone

Quoted 11 lines · Dmitry Torokhov
Quoted 5 lines · Albin Tonnerre

In a number of cases, the .suspend, .freeze, .poweroff and .resume, .thaw, .restore functions are identical. However, they all need to be assigned to avoid regressionsm as the previous code called .suspend resp. .resume in all those cases. SIMPLE_PM_OPS allows to deal with this case.

I'd much rather have conversions done with a bit more analysis now that our framework is more flexible and we can have specialized routines for hibernation and suspend.

I still think that even though they can, quite a number of drivers won't /need/ to have different functions for this, but maybe I'm mistaken.

Quoted 2 lines

Maybe we should try changing from run-time to build time warning so that users are not overly concerned with it?

I'm not sure that solves the problem. The fact is that even for developers, it's easy to overlook that assiging only the .suspend and .resume fields is probably a mistake.

Regards,

Signature · 4 lines
Albin Tonnerre, Free Electrons
Kernel, drivers and embedded Linux development,
consulting, training and support.
http://free-electrons.com
#7 lore raw

Re: [PATCH] Add SIMPLE_PM_OPS: make switching to dev_pm_ops less error-prone

Quoted 16 lines · Albin Tonnerre
Quoted 11 lines · Dmitry Torokhov
Quoted 5 lines · Albin Tonnerre

In a number of cases, the .suspend, .freeze, .poweroff and .resume, .thaw, .restore functions are identical. However, they all need to be assigned to avoid regressionsm as the previous code called .suspend resp. .resume in all those cases. SIMPLE_PM_OPS allows to deal with this case.

I'd much rather have conversions done with a bit more analysis now that our framework is more flexible and we can have specialized routines for hibernation and suspend.

I still think that even though they can, quite a number of drivers won't /need/ to have different functions for this, but maybe I'm mistaken.

Quoted 6 lines
Quoted 2 lines

Maybe we should try changing from run-time to build time warning so that users are not overly concerned with it?

I'm not sure that solves the problem. The fact is that even for developers, it's easy to overlook that assiging only the .suspend and .resume fields is probably a mistake.

I agree, so I'm going to take the patch.

I'll add a comment describing what the macro is for, though.

Thanks,
Rafael

#8 lore raw

PATCH 1/1 PM: Add convenience macro to make switching to dev_pm_ops less error-prone

Quoted 20 lines · Rafael J. Wysocki
Quoted 19 lines · Albin Tonnerre
Quoted 11 lines · Dmitry Torokhov
Quoted 5 lines · Albin Tonnerre

In a number of cases, the .suspend, .freeze, .poweroff and .resume, .thaw, .restore functions are identical. However, they all need to be assigned to avoid regressionsm as the previous code called .suspend resp. .resume in all those cases. SIMPLE_PM_OPS allows to deal with this case.

I'd much rather have conversions done with a bit more analysis now that our framework is more flexible and we can have specialized routines for hibernation and suspend.

I still think that even though they can, quite a number of drivers won't /need/ to have different functions for this, but maybe I'm mistaken.

Quoted 2 lines

Maybe we should try changing from run-time to build time warning so that users are not overly concerned with it?

Quoted 7 lines
Quoted 3 lines

I'm not sure that solves the problem. The fact is that even for developers, it's easy to overlook that assiging only the .suspend and .resume fields is probably a mistake.

I agree, so I'm going to take the patch.

I'll add a comment describing what the macro is for, though.

Strictly speaking, I'm going to add the appended patch to the linux-next branch of the suspend-2.6 tree.

Thanks,
Rafael

---
From: Albin Tonnerre <[email protected]>
Subject: PM: Add convenience macro to make switching to dev_pm_ops less error-prone

In a number of cases, the .suspend, .freeze, .poweroff and .resume, .thaw, .restore functions are identical. However, they all need to be assigned to avoid regressionsm as the previous code called .suspend resp. .resume in all those cases. SIMPLE_DEV_PM_OPS helps to deal with this case.

[rjw: Changed the name of the macro and added the comment explaining its
purpose.]

Signed-off-by Albin Tonnerre <[email protected]>
Signed-off-by Rafael J. Wysocki <[email protected]>
---
 include/linux/pm.h |   14 ++++++++++++++
 1 file changed, 14 insertions(+)
include/linux/pm.h +14 -0
Index: linux-2.6/include/linux/pm.h
===================================================================
@@ -184,6 +184,20 @@ struct dev_pm_ops {
 	int (*restore_noirq)(struct device *dev);
 };
 
+/*
+ * Use this if you want to use the same suspend and resume callbacks for suspend
+ * to RAM and hibernation.
+ */
+#define SIMPLE_DEV_PM_OPS(name, suspend_fn, resume_fn) \
+struct dev_pm_ops name = { \
+	.suspend = suspend_fn, \
+	.resume = resume_fn, \
+	.freeze = suspend_fn, \
+	.thaw = resume_fn, \
+	.poweroff = suspend_fn, \
+	.resume = resume_fn, \
+}
+
 /**
  * PM_EVENT_ messages
  *
#9 lore raw

Re: [PATCH] PM: Add convenience macro to make switching to dev_pm_ops less error-prone

Quoted 64 lines · Rafael J. Wysocki
Quoted 20 lines · Rafael J. Wysocki
Quoted 19 lines · Albin Tonnerre
Quoted 11 lines · Dmitry Torokhov
Quoted 5 lines · Albin Tonnerre

In a number of cases, the .suspend, .freeze, .poweroff and .resume, .thaw, .restore functions are identical. However, they all need to be assigned to avoid regressionsm as the previous code called .suspend resp. .resume in all those cases. SIMPLE_PM_OPS allows to deal with this case.

I'd much rather have conversions done with a bit more analysis now that our framework is more flexible and we can have specialized routines for hibernation and suspend.

I still think that even though they can, quite a number of drivers won't /need/ to have different functions for this, but maybe I'm mistaken.

Quoted 2 lines

Maybe we should try changing from run-time to build time warning so that users are not overly concerned with it?

Quoted 7 lines
Quoted 3 lines

I'm not sure that solves the problem. The fact is that even for developers, it's easy to overlook that assiging only the .suspend and .resume fields is probably a mistake.

I agree, so I'm going to take the patch.

I'll add a comment describing what the macro is for, though.

Strictly speaking, I'm going to add the appended patch to the linux-next branch of the suspend-2.6 tree.

Thanks,
Rafael

---
From: Albin Tonnerre <[email protected]>
Subject: PM: Add convenience macro to make switching to dev_pm_ops less error-prone

In a number of cases, the .suspend, .freeze, .poweroff and .resume, .thaw, .restore functions are identical. However, they all need to be assigned to avoid regressionsm as the previous code called .suspend resp. .resume in all those cases. SIMPLE_DEV_PM_OPS helps to deal with this case.

[rjw: Changed the name of the macro and added the comment explaining its purpose.]

Signed-off-by Albin Tonnerre <[email protected]>
Signed-off-by Rafael J. Wysocki <[email protected]>

---
 include/linux/pm.h |   14 ++++++++++++++

 1 file changed, 14 insertions(+)

Index: linux-2.6/include/linux/pm.h
===================================================================

include/linux/pm.h +2 -0
@@ -184,6 +184,20 @@ struct dev_pm_ops {
        int (*restore_noirq)(struct device *dev);
  };
 
+/*
+ * Use this if you want to use the same suspend and resume callbacks for suspend

include/linux/pm.h struct dev_pm_ops { · line 198

Quoted 10 lines
include/linux/pm.h +10 -0
+ * to RAM and hibernation.
+ */
+#define SIMPLE_DEV_PM_OPS(name, suspend_fn, resume_fn) \
+struct dev_pm_ops name = { \
+       .suspend = suspend_fn, \
+       .resume = resume_fn, \
+       .freeze = suspend_fn, \
+       .thaw = resume_fn, \
+       .poweroff = suspend_fn, \
+       .resume = resume_fn, \

I think that second .resume was intended to be .restore instead.

regards
Philipp

#10 lore raw

Re: [PATCH] PM: Add convenience macro to make switching to dev_pm_ops less error-prone

In reply to Rafael J. Wysocki
Quoted 64 lines · Rafael J. Wysocki
Quoted 20 lines · Rafael J. Wysocki
Quoted 19 lines · Albin Tonnerre
Quoted 11 lines · Dmitry Torokhov
Quoted 5 lines · Albin Tonnerre

In a number of cases, the .suspend, .freeze, .poweroff and .resume, .thaw, .restore functions are identical. However, they all need to be assigned to avoid regressionsm as the previous code called .suspend resp. .resume in all those cases. SIMPLE_PM_OPS allows to deal with this case.

I'd much rather have conversions done with a bit more analysis now that our framework is more flexible and we can have specialized routines for hibernation and suspend.

I still think that even though they can, quite a number of drivers won't /need/ to have different functions for this, but maybe I'm mistaken.

Quoted 2 lines

Maybe we should try changing from run-time to build time warning so that users are not overly concerned with it?

Quoted 7 lines
Quoted 3 lines

I'm not sure that solves the problem. The fact is that even for developers, it's easy to overlook that assiging only the .suspend and .resume fields is probably a mistake.

I agree, so I'm going to take the patch.

I'll add a comment describing what the macro is for, though.

Strictly speaking, I'm going to add the appended patch to the linux-next branch of the suspend-2.6 tree.

Thanks,
Rafael

---
From: Albin Tonnerre <[email protected]>
Subject: PM: Add convenience macro to make switching to dev_pm_ops less error-prone

In a number of cases, the .suspend, .freeze, .poweroff and .resume, .thaw, .restore functions are identical. However, they all need to be assigned to avoid regressionsm as the previous code called .suspend resp. .resume in all those cases. SIMPLE_DEV_PM_OPS helps to deal with this case.

[rjw: Changed the name of the macro and added the comment explaining its purpose.]

Signed-off-by Albin Tonnerre <[email protected]>
Signed-off-by Rafael J. Wysocki <[email protected]>

---
 include/linux/pm.h |   14 ++++++++++++++

 1 file changed, 14 insertions(+)

Index: linux-2.6/include/linux/pm.h
===================================================================

include/linux/pm.h +2 -0
@@ -184,6 +184,20 @@ struct dev_pm_ops {
        int (*restore_noirq)(struct device *dev);
  };
 
+/*
+ * Use this if you want to use the same suspend and resume callbacks for suspend

include/linux/pm.h struct dev_pm_ops { · line 198

Quoted 10 lines
include/linux/pm.h +10 -0
+ * to RAM and hibernation.
+ */
+#define SIMPLE_DEV_PM_OPS(name, suspend_fn, resume_fn) \
+struct dev_pm_ops name = { \
+       .suspend = suspend_fn, \
+       .resume = resume_fn, \
+       .freeze = suspend_fn, \
+       .thaw = resume_fn, \
+       .poweroff = suspend_fn, \
+       .resume = resume_fn, \

I think that second .resume was intended to be .restore instead.

regards
Philipp

#11 lore raw

Re: [PATCH] PM: Add convenience macro to make switching to dev_pm_ops less error-prone

In reply to Rafael J. Wysocki

include/linux/pm.h struct dev_pm_ops { · line 198

Quoted 8 lines · Rafael J. Wysocki
include/linux/pm.h +8 -0
+#define SIMPLE_DEV_PM_OPS(name, suspend_fn, resume_fn) \
+struct dev_pm_ops name = { \
+       .suspend = suspend_fn, \
+       .resume = resume_fn, \
+       .freeze = suspend_fn, \
+       .thaw = resume_fn, \
+       .poweroff = suspend_fn, \
+       .resume = resume_fn, \

That defines .resume twice. Guess the last should be restore.

Cheers,
FJP

#12 lore raw

Re: [PATCH] PM: Add convenience macro to make switching to dev_pm_ops less error-prone

In reply to Rafael J. Wysocki

include/linux/pm.h struct dev_pm_ops { · line 198

Quoted 8 lines · Rafael J. Wysocki
include/linux/pm.h +8 -0
+#define SIMPLE_DEV_PM_OPS(name, suspend_fn, resume_fn) \
+struct dev_pm_ops name = { \
+       .suspend = suspend_fn, \
+       .resume = resume_fn, \
+       .freeze = suspend_fn, \
+       .thaw = resume_fn, \
+       .poweroff = suspend_fn, \
+       .resume = resume_fn, \

That defines .resume twice. Guess the last should be restore.

Cheers,
FJP

#13 lore raw

Re: [PATCH] PM: Add convenience macro to make switching to dev_pm_ops less error-prone

Quoted 12 lines · Frans Pop

include/linux/pm.h struct dev_pm_ops { · line 198

Quoted 8 lines · Rafael J. Wysocki
include/linux/pm.h +8 -0
+#define SIMPLE_DEV_PM_OPS(name, suspend_fn, resume_fn) \
+struct dev_pm_ops name = { \
+       .suspend = suspend_fn, \
+       .resume = resume_fn, \
+       .freeze = suspend_fn, \
+       .thaw = resume_fn, \
+       .poweroff = suspend_fn, \
+       .resume = resume_fn, \

That defines .resume twice. Guess the last should be restore.

Yes, thanks to you and Philipp for noticing that and sorry for the mistake.

Rafael

#14 lore raw

Re: [PATCH] PM: Add convenience macro to make switching to dev_pm_ops less error-prone

Quoted 14 lines · Rafael J. Wysocki
Quoted 12 lines · Frans Pop

include/linux/pm.h struct dev_pm_ops { · line 198

Quoted 8 lines · Rafael J. Wysocki
include/linux/pm.h +8 -0
+#define SIMPLE_DEV_PM_OPS(name, suspend_fn, resume_fn) \
+struct dev_pm_ops name = { \
+       .suspend = suspend_fn, \
+       .resume = resume_fn, \
+       .freeze = suspend_fn, \
+       .thaw = resume_fn, \
+       .poweroff = suspend_fn, \
+       .resume = resume_fn, \

That defines .resume twice. Guess the last should be restore.

Quoted 1 line

Yes, thanks to you and Philipp for noticing that and sorry for the mistake.

When is a good point to resend patches that use this macro? I guess they won't make it to .31 anyway, right? So I just wait for the next merge window?

Daniel

#15 lore raw

Re: [PATCH] PM: Add convenience macro to make switching to dev_pm_ops less error-prone

In reply to Rafael J. Wysocki
Quoted 14 lines · Rafael J. Wysocki
Quoted 12 lines · Frans Pop

include/linux/pm.h struct dev_pm_ops { · line 198

Quoted 8 lines · Rafael J. Wysocki
include/linux/pm.h +8 -0
+#define SIMPLE_DEV_PM_OPS(name, suspend_fn, resume_fn) \
+struct dev_pm_ops name = { \
+       .suspend = suspend_fn, \
+       .resume = resume_fn, \
+       .freeze = suspend_fn, \
+       .thaw = resume_fn, \
+       .poweroff = suspend_fn, \
+       .resume = resume_fn, \

That defines .resume twice. Guess the last should be restore.

Quoted 1 line

Yes, thanks to you and Philipp for noticing that and sorry for the mistake.

When is a good point to resend patches that use this macro? I guess they won't make it to .31 anyway, right? So I just wait for the next merge window?

Daniel

#16 lore raw

Re: [PATCH] PM: Add convenience macro to make switching to dev_pm_ops less error-prone

Quoted 15 lines · Daniel Mack
Quoted 14 lines · Rafael J. Wysocki
Quoted 12 lines · Frans Pop

include/linux/pm.h struct dev_pm_ops { · line 198

Quoted 8 lines · Rafael J. Wysocki
include/linux/pm.h +8 -0
+#define SIMPLE_DEV_PM_OPS(name, suspend_fn, resume_fn) \
+struct dev_pm_ops name = { \
+       .suspend = suspend_fn, \
+       .resume = resume_fn, \
+       .freeze = suspend_fn, \
+       .thaw = resume_fn, \
+       .poweroff = suspend_fn, \
+       .resume = resume_fn, \

That defines .resume twice. Guess the last should be restore.

Quoted 5 lines
Quoted 1 line

Yes, thanks to you and Philipp for noticing that and sorry for the mistake.

When is a good point to resend patches that use this macro? I guess they won't make it to .31 anyway, right? So I just wait for the next merge window?

Yes, I think that would be the right thing to do.

Best,
Rafael

#17 lore raw

Re: [PATCH] PM: Add convenience macro to make switching to dev_pm_ops less error-prone

Quoted 6 lines · Rafael J. Wysocki
Quoted 3 lines · Daniel Mack

When is a good point to resend patches that use this macro? I guess they won't make it to .31 anyway, right? So I just wait for the next merge window?

Yes, I think that would be the right thing to do.

Is it a good idea to disable the warning before the final 2.6.31 release?

Cheers,

/ magnus

#18 lore raw

Re: [PATCH] PM: Add convenience macro to make switching to dev_pm_ops less error-prone

Quoted 9 lines · Magnus Damm
Quoted 6 lines · Rafael J. Wysocki
Quoted 3 lines · Daniel Mack

When is a good point to resend patches that use this macro? I guess they won't make it to .31 anyway, right? So I just wait for the next merge window?

Yes, I think that would be the right thing to do.

Is it a good idea to disable the warning before the final 2.6.31 release?

I guess so. Let's disable it.

Thanks,
Rafael

#19 lore raw

Re: [PATCH] PM: Add convenience macro to make switching to dev_pm_ops less error-prone

In reply to Magnus Damm
Quoted 9 lines · Magnus Damm
Quoted 6 lines · Rafael J. Wysocki
Quoted 3 lines · Daniel Mack

When is a good point to resend patches that use this macro? I guess they won't make it to .31 anyway, right? So I just wait for the next merge window?

Yes, I think that would be the right thing to do.

Is it a good idea to disable the warning before the final 2.6.31 release?

I guess so. Let's disable it.

Thanks,
Rafael

#20 lore raw

Re: [PATCH] PM: Add convenience macro to make switching to dev_pm_ops less error-prone

In reply to Daniel Mack
Quoted 15 lines · Daniel Mack
Quoted 14 lines · Rafael J. Wysocki
Quoted 12 lines · Frans Pop

include/linux/pm.h struct dev_pm_ops { · line 198

Quoted 8 lines · Rafael J. Wysocki
include/linux/pm.h +8 -0
+#define SIMPLE_DEV_PM_OPS(name, suspend_fn, resume_fn) \
+struct dev_pm_ops name = { \
+       .suspend = suspend_fn, \
+       .resume = resume_fn, \
+       .freeze = suspend_fn, \
+       .thaw = resume_fn, \
+       .poweroff = suspend_fn, \
+       .resume = resume_fn, \

That defines .resume twice. Guess the last should be restore.

Quoted 5 lines
Quoted 1 line

Yes, thanks to you and Philipp for noticing that and sorry for the mistake.

When is a good point to resend patches that use this macro? I guess they won't make it to .31 anyway, right? So I just wait for the next merge window?

Yes, I think that would be the right thing to do.

Best,
Rafael

#21 lore raw

Re: [PATCH] PM: Add convenience macro to make switching to dev_pm_ops less error-prone

In reply to Frans Pop
Quoted 12 lines · Frans Pop

include/linux/pm.h struct dev_pm_ops { · line 198

Quoted 8 lines · Rafael J. Wysocki
include/linux/pm.h +8 -0
+#define SIMPLE_DEV_PM_OPS(name, suspend_fn, resume_fn) \
+struct dev_pm_ops name = { \
+       .suspend = suspend_fn, \
+       .resume = resume_fn, \
+       .freeze = suspend_fn, \
+       .thaw = resume_fn, \
+       .poweroff = suspend_fn, \
+       .resume = resume_fn, \

That defines .resume twice. Guess the last should be restore.

Yes, thanks to you and Philipp for noticing that and sorry for the mistake.

Rafael

#22 lore raw

PATCH 1/1 PM: Add convenience macro to make switching to dev_pm_ops less error-prone

In reply to Rafael J. Wysocki
Quoted 20 lines · Rafael J. Wysocki
Quoted 19 lines · Albin Tonnerre
Quoted 11 lines · Dmitry Torokhov
Quoted 5 lines · Albin Tonnerre

In a number of cases, the .suspend, .freeze, .poweroff and .resume, .thaw, .restore functions are identical. However, they all need to be assigned to avoid regressionsm as the previous code called .suspend resp. .resume in all those cases. SIMPLE_PM_OPS allows to deal with this case.

I'd much rather have conversions done with a bit more analysis now that our framework is more flexible and we can have specialized routines for hibernation and suspend.

I still think that even though they can, quite a number of drivers won't /need/ to have different functions for this, but maybe I'm mistaken.

Quoted 2 lines

Maybe we should try changing from run-time to build time warning so that users are not overly concerned with it?

Quoted 7 lines
Quoted 3 lines

I'm not sure that solves the problem. The fact is that even for developers, it's easy to overlook that assiging only the .suspend and .resume fields is probably a mistake.

I agree, so I'm going to take the patch.

I'll add a comment describing what the macro is for, though.

Strictly speaking, I'm going to add the appended patch to the linux-next branch of the suspend-2.6 tree.

Thanks,
Rafael

---
From: Albin Tonnerre <[email protected]>
Subject: PM: Add convenience macro to make switching to dev_pm_ops less error-prone

In a number of cases, the .suspend, .freeze, .poweroff and .resume, .thaw, .restore functions are identical. However, they all need to be assigned to avoid regressionsm as the previous code called .suspend resp. .resume in all those cases. SIMPLE_DEV_PM_OPS helps to deal with this case.

[rjw: Changed the name of the macro and added the comment explaining its
purpose.]

Signed-off-by Albin Tonnerre <[email protected]>
Signed-off-by Rafael J. Wysocki <[email protected]>
---
 include/linux/pm.h |   14 ++++++++++++++
 1 file changed, 14 insertions(+)
include/linux/pm.h +14 -0
Index: linux-2.6/include/linux/pm.h
===================================================================
@@ -184,6 +184,20 @@ struct dev_pm_ops {
 	int (*restore_noirq)(struct device *dev);
 };
 
+/*
+ * Use this if you want to use the same suspend and resume callbacks for suspend
+ * to RAM and hibernation.
+ */
+#define SIMPLE_DEV_PM_OPS(name, suspend_fn, resume_fn) \
+struct dev_pm_ops name = { \
+	.suspend = suspend_fn, \
+	.resume = resume_fn, \
+	.freeze = suspend_fn, \
+	.thaw = resume_fn, \
+	.poweroff = suspend_fn, \
+	.resume = resume_fn, \
+}
+
 /**
  * PM_EVENT_ messages
  *
#23 lore raw

Re: [PATCH] Add SIMPLE_PM_OPS: make switching to dev_pm_ops less error-prone

In reply to Albin Tonnerre
Quoted 16 lines · Albin Tonnerre
Quoted 11 lines · Dmitry Torokhov
Quoted 5 lines · Albin Tonnerre

In a number of cases, the .suspend, .freeze, .poweroff and .resume, .thaw, .restore functions are identical. However, they all need to be assigned to avoid regressionsm as the previous code called .suspend resp. .resume in all those cases. SIMPLE_PM_OPS allows to deal with this case.

I'd much rather have conversions done with a bit more analysis now that our framework is more flexible and we can have specialized routines for hibernation and suspend.

I still think that even though they can, quite a number of drivers won't /need/ to have different functions for this, but maybe I'm mistaken.

Quoted 6 lines
Quoted 2 lines

Maybe we should try changing from run-time to build time warning so that users are not overly concerned with it?

I'm not sure that solves the problem. The fact is that even for developers, it's easy to overlook that assiging only the .suspend and .resume fields is probably a mistake.

I agree, so I'm going to take the patch.

I'll add a comment describing what the macro is for, though.

Thanks,
Rafael

#24 lore raw
In reply to Albin Tonnerre
Quoted 11 lines · Albin Tonnerre
Quoted 3 lines · Frans Pop

Yes, I see that in drivers/base/platform.c (legacy) .suspend resp. .resume also got called for those cases?
Ouch :-(

This really looks like it's error-prone (I made this mistake for atmel_serial and wouldn't have noticed if Frans hadn't told me), and AFAICS, for a number of drivers we'll have suspend = freeze = poweroff and resume = thaw = restore. Maybe putting something like this in pm.h would help ?

#define PM_OPS(name, suspend, resume) \

Quoted 8 lines
struct dev_pm_ops name = { \
	.suspend = suspend, \
	.resume = resume, \
	.freeze = suspend, \
	.thaw = resume, \
	.poweroff = suspend, \
	.restore = resume, \
};

That looks like a good idea, but please call that macro SIMPLE_PM_OPS (or something similar) and submit a patch.

Best,
Rafael