Re: [PATCH V2] au1xmmc: dev_pm_ops conversion

Re: [PATCH V2] au1xmmc: dev_pm_ops conversion

[Albin Tonnerre]

On Sat, Jul 25, 2009, Frans Pop wrote:
> 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,
-- 
Albin Tonnerre, Free Electrons
Kernel, drivers and embedded Linux development,
consulting, training and support.
http://free-electrons.com

Re: [PATCH V2] au1xmmc: dev_pm_ops conversion

[Rafael J. Wysocki]

On Monday 03 August 2009, Albin Tonnerre wrote:
> On Sat, Jul 25, 2009, Frans Pop wrote:
> > 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, \
> };

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

Best,
Rafael

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

[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.

Signed-off-by: Albin Tonnerre <albin.tonnerre@free-electrons.com>
---
 include/linux/pm.h |   10 ++++++++++
 1 files changed, 10 insertions(+), 0 deletions(-)

diff --git a/include/linux/pm.h b/include/linux/pm.h
index b3f7476..994a62f 100644
--- a/include/linux/pm.h
+++ b/include/linux/pm.h
@@ -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
  *
-- 
Albin Tonnerre, Free Electrons
Kernel, drivers and embedded Linux development,
consulting, training and support.
http://free-electrons.com

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

[Dmitry Torokhov]

On Tue, Aug 04, 2009 at 11:36:12AM +0200, Albin Tonnerre wrote:
> 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.

-- 
Dmitry

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

[Albin Tonnerre]

On Tue, Aug 04, 2009 at 09:55:33PM -0700, Dmitry Torokhov wrote :
> On Tue, Aug 04, 2009 at 11:36:12AM +0200, Albin Tonnerre wrote:
> > 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.

> 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,
-- 
Albin Tonnerre, Free Electrons
Kernel, drivers and embedded Linux development,
consulting, training and support.
http://free-electrons.com

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

[Rafael J. Wysocki]

On Wednesday 05 August 2009, Albin Tonnerre wrote:
> On Tue, Aug 04, 2009 at 09:55:33PM -0700, Dmitry Torokhov wrote :
> > On Tue, Aug 04, 2009 at 11:36:12AM +0200, Albin Tonnerre wrote:
> > > 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.
> 
> > 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

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

[Rafael J. Wysocki]

On Wednesday 05 August 2009, Rafael J. Wysocki wrote:
> On Wednesday 05 August 2009, Albin Tonnerre wrote:
> > On Tue, Aug 04, 2009 at 09:55:33PM -0700, Dmitry Torokhov wrote :
> > > On Tue, Aug 04, 2009 at 11:36:12AM +0200, Albin Tonnerre wrote:
> > > > 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.
> > 
> > > 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.

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 <albin.tonnerre@free-electrons.com>
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 <albin.tonnerre@free-electrons.com>
Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
 include/linux/pm.h |   14 ++++++++++++++
 1 file changed, 14 insertions(+)

Index: linux-2.6/include/linux/pm.h
===================================================================
--- linux-2.6.orig/include/linux/pm.h
+++ 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
  *

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

[pHilipp Zabel]

On Wed, Aug 5, 2009 at 10:05 PM, Rafael J. Wysocki<rjw@sisk.pl> wrote:
> On Wednesday 05 August 2009, Rafael J. Wysocki wrote:
>> On Wednesday 05 August 2009, Albin Tonnerre wrote:
>> > On Tue, Aug 04, 2009 at 09:55:33PM -0700, Dmitry Torokhov wrote :
>> > > On Tue, Aug 04, 2009 at 11:36:12AM +0200, Albin Tonnerre wrote:
>> > > > 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.
>> >
>> > > 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.
>
> 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 <albin.tonnerre@free-electrons.com>
> 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 <albin.tonnerre@free-electrons.com>
> Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> ---
>  include/linux/pm.h |   14 ++++++++++++++
>  1 file changed, 14 insertions(+)
>
> Index: linux-2.6/include/linux/pm.h
> ===================================================================
> --- linux-2.6.orig/include/linux/pm.h
> +++ 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, \

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

regards
Philipp

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

[Frans Pop]

Rafael J. Wysocki wrote:

> +#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

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

[Rafael J. Wysocki]

On Wednesday 05 August 2009, Frans Pop wrote:
> Rafael J. Wysocki wrote:
> 
> > +#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

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

[Daniel Mack]

On Wed, Aug 05, 2009 at 11:30:07PM +0200, Rafael J. Wysocki wrote:
> On Wednesday 05 August 2009, Frans Pop wrote:
> > Rafael J. Wysocki wrote:
> > 
> > > +#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.

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

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

[Rafael J. Wysocki]

On Thursday 06 August 2009, Daniel Mack wrote:
> On Wed, Aug 05, 2009 at 11:30:07PM +0200, Rafael J. Wysocki wrote:
> > On Wednesday 05 August 2009, Frans Pop wrote:
> > > Rafael J. Wysocki wrote:
> > > 
> > > > +#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.
> 
> 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

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

[Magnus Damm]

On Thu, Aug 6, 2009 at 9:16 PM, Rafael J. Wysocki<rjw@sisk.pl> wrote:
> On Thursday 06 August 2009, Daniel Mack wrote:
>> 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

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

[Rafael J. Wysocki]

On Thursday 06 August 2009, Magnus Damm wrote:
> On Thu, Aug 6, 2009 at 9:16 PM, Rafael J. Wysocki<rjw@sisk.pl> wrote:
> > On Thursday 06 August 2009, Daniel Mack wrote:
> >> 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