LinuxLists.cc - [PATCH 1/1] acer-wmi: switch driver to dev_pm

[permalink] [raw]

Subject: Re: [PATCH 1/1] acer-wmi: switch driver to dev_pm_ops

Hi Arnaud,

On Sat, Jul 25, 2009 at 09:04:51AM -0400, Arnaud Faucher wrote:
> Gets rid of the following warning:
> Platform driver 'acer-wmi' needs updating - please use dev_pm_ops
>

Have you tested it with Suspend to disk? You are [potentially] breaking
it since the new suspend and resume methods are not used by it, it calls
freeze() and thaw() instead.

Rafael,

I wonder if PM core should automatically use suspend()/resume() in place of
freeze()/thaw() when the latter pair is missing.

> Signed-off-by: Arnaud Faucher <[email protected]>
> ---
> drivers/platform/x86/acer-wmi.c | 13 ++++++++-----
> 1 files changed, 8 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/platform/x86/acer-wmi.c b/drivers/platform/x86/acer-wmi.c
> index be2fd6f..b2b6aef 100644
> --- a/drivers/platform/x86/acer-wmi.c
> +++ b/drivers/platform/x86/acer-wmi.c
> @@ -1152,8 +1152,7 @@ static int acer_platform_remove(struct platform_device *device)
> return 0;
> }
>
> -static int acer_platform_suspend(struct platform_device *dev,
> -pm_message_t state)
> +static int acer_platform_suspend(struct device *dev)
> {
> u32 value;
> struct acer_data *data = &interface->data;
> @@ -1174,7 +1173,7 @@ pm_message_t state)
> return 0;
> }
>
> -static int acer_platform_resume(struct platform_device *device)
> +static int acer_platform_resume(struct device *dev)
> {
> struct acer_data *data = &interface->data;
>
> @@ -1190,15 +1189,19 @@ static int acer_platform_resume(struct platform_device *device)
> return 0;
> }
>
> +static struct dev_pm_ops acer_platform_pm_ops = {
> + .suspend = acer_platform_suspend,
> + .resume = acer_platform_resume,
> +};
> +
> static struct platform_driver acer_platform_driver = {
> .driver = {
> .name = "acer-wmi",
> .owner = THIS_MODULE,
> + .pm = &acer_platform_pm_ops,
> },
> .probe = acer_platform_probe,
> .remove = acer_platform_remove,
> - .suspend = acer_platform_suspend,
> - .resume = acer_platform_resume,
> };
>
> static struct platform_device *acer_platform_device;
> --
> 1.6.3.3
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/

--
Dmitry

2009-07-25 20:04:37

[permalink] [raw]

Subject: Re: [PATCH 1/1] acer-wmi: switch driver to dev_pm_ops

On Saturday 25 July 2009, Dmitry Torokhov wrote:
> Hi Arnaud,
>
> On Sat, Jul 25, 2009 at 09:04:51AM -0400, Arnaud Faucher wrote:
> > Gets rid of the following warning:
> > Platform driver 'acer-wmi' needs updating - please use dev_pm_ops
> >
>
> Have you tested it with Suspend to disk? You are [potentially] breaking
> it since the new suspend and resume methods are not used by it, it calls
> freeze() and thaw() instead.
>
> Rafael,
>
> I wonder if PM core should automatically use suspend()/resume() in place of
> freeze()/thaw() when the latter pair is missing.

Well, in fact it often is not necessary to do .thaw() at all, because the
state of the device doesn't change in .freeze(). Also, PCI drivers should
not need .thaw(), unless they manipulate the standard PM registers of the
device themselves.

That said, if .suspend() is defined, then most probably .freeze() is necessary
as well. Also, if .resume() is defined, .restore() is most probably necessary
and it should be safe to use .suspend() as .freeze() and .resume() as
.restore().

OTOH, it's really easy to point .restore() to the same routine as .resume()
etc., so I'm not sure.

Best,
Rafael

2009-07-25 20:10:35

[permalink] [raw]

Subject: Re: [PATCH 1/1] acer-wmi: switch driver to dev_pm_ops

On Sat, 2009-07-25 at 10:43 -0700, Dmitry Torokhov wrote:
> Hi Arnaud,
>
> On Sat, Jul 25, 2009 at 09:04:51AM -0400, Arnaud Faucher wrote:
> > Gets rid of the following warning:
> > Platform driver 'acer-wmi' needs updating - please use dev_pm_ops
> >
>
> Have you tested it with Suspend to disk? You are [potentially] breaking
> it since the new suspend and resume methods are not used by it, it calls
> freeze() and thaw() instead.
>
> Rafael,
>
> I wonder if PM core should automatically use suspend()/resume() in place of
> freeze()/thaw() when the latter pair is missing.
>

By studying drivers/base/platform.c, suspend()/resume() are not called
when freeze()/thaw() are missing. So you're right, this patch breaks
something.

I am testing right now a new patch against hibernation.

2009-07-26 13:53:38

[permalink] [raw]

Subject: Re: [PATCH 1/1] acer-wmi: switch driver to dev_pm_ops

Gets rid of the following warning:
Platform driver 'acer-wmi' needs updating - please use dev_pm_ops

Take 2, thanks to Dmitry, Rafael and Frans for pointing out PM issue on
hibernation when using dev_pm_ops blindly.

This patch was tested against suspendand hibernation (Acer mail led
status).

Signed-off-by: Arnaud Faucher <[email protected]>
---
drivers/platform/x86/acer-wmi.c | 17 ++++++++++++-----
1 files changed, 12 insertions(+), 5 deletions(-)

diff --git a/drivers/platform/x86/acer-wmi.c
b/drivers/platform/x86/acer-wmi.c
index be2fd6f..29374bc 100644
--- a/drivers/platform/x86/acer-wmi.c
+++ b/drivers/platform/x86/acer-wmi.c
@@ -1152,8 +1152,7 @@ static int acer_platform_remove(struct
platform_device *device)
return 0;
}

-static int acer_platform_suspend(struct platform_device *dev,
-pm_message_t state)
+static int acer_platform_suspend(struct device *dev)
{
u32 value;
struct acer_data *data = &interface->data;
@@ -1174,7 +1173,7 @@ pm_message_t state)
return 0;
}

-static int acer_platform_resume(struct platform_device *device)
+static int acer_platform_resume(struct device *dev)
{
struct acer_data *data = &interface->data;

@@ -1190,15 +1189,23 @@ static int acer_platform_resume(struct
platform_device *device)
return 0;
}

+static struct dev_pm_ops acer_platform_pm_ops = {
+ .suspend = acer_platform_suspend,
+ .resume = acer_platform_resume,
+ .freeze = acer_platform_suspend,
+ .thaw = acer_platform_resume,
+ .poweroff = acer_platform_suspend,
+ .restore = acer_platform_resume,
+};
+
static struct platform_driver acer_platform_driver = {
.driver = {
.name = "acer-wmi",
.owner = THIS_MODULE,
+ .pm = &acer_platform_pm_ops,
},
.probe = acer_platform_probe,
.remove = acer_platform_remove,
- .suspend = acer_platform_suspend,
- .resume = acer_platform_resume,
};

static struct platform_device *acer_platform_device;
--
1.6.3.3

2009-07-26 14:37:49

by Carlos Corbacho

[permalink] [raw]

Subject: Re: [PATCH 1/1] acer-wmi: switch driver to dev_pm_ops

[Removing linux-mips from CC - I don't know why they'd be interested in an x86
only platform driver...]

On Sunday 26 July 2009 14:53:33 Arnaud Faucher wrote:
> Gets rid of the following warning:
> Platform driver 'acer-wmi' needs updating - please use dev_pm_ops
>
> Take 2, thanks to Dmitry, Rafael and Frans for pointing out PM issue on
> hibernation when using dev_pm_ops blindly.
>
> This patch was tested against suspendand hibernation (Acer mail led
> status).
>
> Signed-off-by: Arnaud Faucher <[email protected]>
> ---
> drivers/platform/x86/acer-wmi.c | 17 ++++++++++++-----
> 1 files changed, 12 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/platform/x86/acer-wmi.c
> b/drivers/platform/x86/acer-wmi.c
> index be2fd6f..29374bc 100644
> --- a/drivers/platform/x86/acer-wmi.c
> +++ b/drivers/platform/x86/acer-wmi.c
> @@ -1152,8 +1152,7 @@ static int acer_platform_remove(struct
> platform_device *device)
> return 0;
> }
>
> -static int acer_platform_suspend(struct platform_device *dev,
> -pm_message_t state)
> +static int acer_platform_suspend(struct device *dev)
> {
> u32 value;
> struct acer_data *data = &interface->data;
> @@ -1174,7 +1173,7 @@ pm_message_t state)
> return 0;
> }
>
> -static int acer_platform_resume(struct platform_device *device)
> +static int acer_platform_resume(struct device *dev)
> {
> struct acer_data *data = &interface->data;
>
> @@ -1190,15 +1189,23 @@ static int acer_platform_resume(struct
> platform_device *device)
> return 0;
> }
>
> +static struct dev_pm_ops acer_platform_pm_ops = {
> + .suspend = acer_platform_suspend,
> + .resume = acer_platform_resume,

Are these necessary? For suspend-to-RAM, I've never needed these. The old
callbacks here were just for suspend-to-disk.

> + .freeze = acer_platform_suspend,
> + .thaw = acer_platform_resume,

If we only need these callbacks for freeze & thaw, they should be rebamed.

> + .poweroff = acer_platform_suspend,
> + .restore = acer_platform_resume,

What do poweroff and restore mean in this context. Do my comments above apply
again (i.e. are the callbacks necessary here)?

> +};
> +
> static struct platform_driver acer_platform_driver = {
> .driver = {
> .name = "acer-wmi",
> .owner = THIS_MODULE,
> + .pm = &acer_platform_pm_ops,
> },
> .probe = acer_platform_probe,
> .remove = acer_platform_remove,
> - .suspend = acer_platform_suspend,
> - .resume = acer_platform_resume,
> };
>
> static struct platform_device *acer_platform_device;

-Carlos
--
E-Mail: [email protected]
Web: strangeworlds.co.uk
GPG Key ID: 0x23EE722D

2009-07-26 18:08:19

[permalink] [raw]

Subject: Re: [PATCH 1/1] acer-wmi: switch driver to dev_pm_ops

On Sun, Jul 26, 2009 at 03:23:29PM +0100, Carlos Corbacho wrote:
> [Removing linux-mips from CC - I don't know why they'd be interested in an x86
> only platform driver...]
>
> On Sunday 26 July 2009 14:53:33 Arnaud Faucher wrote:
> > Gets rid of the following warning:
> > Platform driver 'acer-wmi' needs updating - please use dev_pm_ops
> >
> > Take 2, thanks to Dmitry, Rafael and Frans for pointing out PM issue on
> > hibernation when using dev_pm_ops blindly.
> >
> > This patch was tested against suspendand hibernation (Acer mail led
> > status).
> >
> > Signed-off-by: Arnaud Faucher <[email protected]>
> > ---
> > drivers/platform/x86/acer-wmi.c | 17 ++++++++++++-----
> > 1 files changed, 12 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/platform/x86/acer-wmi.c
> > b/drivers/platform/x86/acer-wmi.c
> > index be2fd6f..29374bc 100644
> > --- a/drivers/platform/x86/acer-wmi.c
> > +++ b/drivers/platform/x86/acer-wmi.c
> > @@ -1152,8 +1152,7 @@ static int acer_platform_remove(struct
> > platform_device *device)
> > return 0;
> > }
> >
> > -static int acer_platform_suspend(struct platform_device *dev,
> > -pm_message_t state)
> > +static int acer_platform_suspend(struct device *dev)
> > {
> > u32 value;
> > struct acer_data *data = &interface->data;
> > @@ -1174,7 +1173,7 @@ pm_message_t state)
> > return 0;
> > }
> >
> > -static int acer_platform_resume(struct platform_device *device)
> > +static int acer_platform_resume(struct device *dev)
> > {
> > struct acer_data *data = &interface->data;
> >
> > @@ -1190,15 +1189,23 @@ static int acer_platform_resume(struct
> > platform_device *device)
> > return 0;
> > }
> >
> > +static struct dev_pm_ops acer_platform_pm_ops = {
> > + .suspend = acer_platform_suspend,
> > + .resume = acer_platform_resume,
>
> Are these necessary? For suspend-to-RAM, I've never needed these. The old
> callbacks here were just for suspend-to-disk.
>

That is not correct. Old suspend and resume callbacks were called for
both S2R and S2D. Whether it is actually needed for S2R I don't know but
looking at the code they should not hurt.

> > + .freeze = acer_platform_suspend,
> > + .thaw = acer_platform_resume,
>
> If we only need these callbacks for freeze & thaw, they should be rebamed.
>
> > + .poweroff = acer_platform_suspend,
> > + .restore = acer_platform_resume,
>
> What do poweroff and restore mean in this context. Do my comments above apply
> again (i.e. are the callbacks necessary here)?
>

I don't think poweroff handler is needed.

BTW, why so we retuen -ENOMEM from these methods if interface->data is
missing? I'd say we should not fail suspend resume in that case or if we
fail then with somethig like -EINVAL - we did not have mempry allocation
failure.

--
Dmitry

2009-07-26 18:35:20

by Carlos Corbacho

[permalink] [raw]

Subject: Re: [PATCH 1/1] acer-wmi: switch driver to dev_pm_ops

On Sunday 26 July 2009 19:08:09 Dmitry Torokhov wrote:
> On Sun, Jul 26, 2009 at 03:23:29PM +0100, Carlos Corbacho wrote:
> > [Removing linux-mips from CC - I don't know why they'd be interested in
> > an x86 only platform driver...]
> >
> > On Sunday 26 July 2009 14:53:33 Arnaud Faucher wrote:
> > > Gets rid of the following warning:
> > > Platform driver 'acer-wmi' needs updating - please use dev_pm_ops
> > >
> > > Take 2, thanks to Dmitry, Rafael and Frans for pointing out PM issue on
> > > hibernation when using dev_pm_ops blindly.
> > >
> > > This patch was tested against suspendand hibernation (Acer mail led
> > > status).
> > >
> > > Signed-off-by: Arnaud Faucher <[email protected]>
> > > ---
> > > drivers/platform/x86/acer-wmi.c | 17 ++++++++++++-----
> > > 1 files changed, 12 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/drivers/platform/x86/acer-wmi.c
> > > b/drivers/platform/x86/acer-wmi.c
> > > index be2fd6f..29374bc 100644
> > > --- a/drivers/platform/x86/acer-wmi.c
> > > +++ b/drivers/platform/x86/acer-wmi.c
> > > @@ -1152,8 +1152,7 @@ static int acer_platform_remove(struct
> > > platform_device *device)
> > > return 0;
> > > }
> > >
> > > -static int acer_platform_suspend(struct platform_device *dev,
> > > -pm_message_t state)
> > > +static int acer_platform_suspend(struct device *dev)
> > > {
> > > u32 value;
> > > struct acer_data *data = &interface->data;
> > > @@ -1174,7 +1173,7 @@ pm_message_t state)
> > > return 0;
> > > }
> > >
> > > -static int acer_platform_resume(struct platform_device *device)
> > > +static int acer_platform_resume(struct device *dev)
> > > {
> > > struct acer_data *data = &interface->data;
> > >
> > > @@ -1190,15 +1189,23 @@ static int acer_platform_resume(struct
> > > platform_device *device)
> > > return 0;
> > > }
> > >
> > > +static struct dev_pm_ops acer_platform_pm_ops = {
> > > + .suspend = acer_platform_suspend,
> > > + .resume = acer_platform_resume,
> >
> > Are these necessary? For suspend-to-RAM, I've never needed these. The old
> > callbacks here were just for suspend-to-disk.
>
> That is not correct. Old suspend and resume callbacks were called for
> both S2R and S2D. Whether it is actually needed for S2R I don't know but
> looking at the code they should not hurt.

I'm aware they were called for S2RAM as well, but that was just a limitation
of the old calls - as I say, they're not needed for it (at least on my
hardware anyway).

> > > + .freeze = acer_platform_suspend,
> > > + .thaw = acer_platform_resume,
> >
> > If we only need these callbacks for freeze & thaw, they should be
> > rebamed.
> >
> > > + .poweroff = acer_platform_suspend,
> > > + .restore = acer_platform_resume,
> >
> > What do poweroff and restore mean in this context. Do my comments above
> > apply again (i.e. are the callbacks necessary here)?
>
> I don't think poweroff handler is needed.
>
> BTW, why so we retuen -ENOMEM from these methods if interface->data is
> missing? I'd say we should not fail suspend resume in that case or if we
> fail then with somethig like -EINVAL - we did not have mempry allocation
> failure.

Ok.

-Carlos
--
E-Mail: [email protected]
Web: strangeworlds.co.uk
GPG Key ID: 0x23EE722D

2009-07-26 21:00:06

[permalink] [raw]

Subject: Re: [PATCH 1/1] acer-wmi: switch driver to dev_pm_ops

On dim, 2009-07-26 at 19:35 +0100, Carlos Corbacho wrote:
> On Sunday 26 July 2009 19:08:09 Dmitry Torokhov wrote:
> > On Sun, Jul 26, 2009 at 03:23:29PM +0100, Carlos Corbacho wrote:
> > > [Removing linux-mips from CC - I don't know why they'd be interested in
> > > an x86 only platform driver...]
> > >
> > > On Sunday 26 July 2009 14:53:33 Arnaud Faucher wrote:
> > > > Gets rid of the following warning:
> > > > Platform driver 'acer-wmi' needs updating - please use dev_pm_ops
> > > >
> > > > Take 2, thanks to Dmitry, Rafael and Frans for pointing out PM issue on
> > > > hibernation when using dev_pm_ops blindly.
> > > >
> > > > This patch was tested against suspendand hibernation (Acer mail led
> > > > status).
> > > >
> > > > Signed-off-by: Arnaud Faucher <[email protected]>
> > > > ---
> > > > drivers/platform/x86/acer-wmi.c | 17 ++++++++++++-----
> > > > 1 files changed, 12 insertions(+), 5 deletions(-)
> > > >
> > > > diff --git a/drivers/platform/x86/acer-wmi.c
> > > > b/drivers/platform/x86/acer-wmi.c
> > > > index be2fd6f..29374bc 100644
> > > > --- a/drivers/platform/x86/acer-wmi.c
> > > > +++ b/drivers/platform/x86/acer-wmi.c
> > > > @@ -1152,8 +1152,7 @@ static int acer_platform_remove(struct
> > > > platform_device *device)
> > > > return 0;
> > > > }
> > > >
> > > > -static int acer_platform_suspend(struct platform_device *dev,
> > > > -pm_message_t state)
> > > > +static int acer_platform_suspend(struct device *dev)
> > > > {
> > > > u32 value;
> > > > struct acer_data *data = &interface->data;
> > > > @@ -1174,7 +1173,7 @@ pm_message_t state)
> > > > return 0;
> > > > }
> > > >
> > > > -static int acer_platform_resume(struct platform_device *device)
> > > > +static int acer_platform_resume(struct device *dev)
> > > > {
> > > > struct acer_data *data = &interface->data;
> > > >
> > > > @@ -1190,15 +1189,23 @@ static int acer_platform_resume(struct
> > > > platform_device *device)
> > > > return 0;
> > > > }
> > > >
> > > > +static struct dev_pm_ops acer_platform_pm_ops = {
> > > > + .suspend = acer_platform_suspend,
> > > > + .resume = acer_platform_resume,
> > >
> > > Are these necessary? For suspend-to-RAM, I've never needed these. The old
> > > callbacks here were just for suspend-to-disk.
> >
> > That is not correct. Old suspend and resume callbacks were called for
> > both S2R and S2D. Whether it is actually needed for S2R I don't know but
> > looking at the code they should not hurt.
>
> I'm aware they were called for S2RAM as well, but that was just a limitation
> of the old calls - as I say, they're not needed for it (at least on my
> hardware anyway).
>

I was looking for similar functionality.

> > > > + .freeze = acer_platform_suspend,
> > > > + .thaw = acer_platform_resume,
> > >
> > > If we only need these callbacks for freeze & thaw, they should be
> > > rebamed.
> > >
> > > > + .poweroff = acer_platform_suspend,
> > > > + .restore = acer_platform_resume,
> > >
> > > What do poweroff and restore mean in this context. Do my comments above
> > > apply again (i.e. are the callbacks necessary here)?
> >
> > I don't think poweroff handler is needed.

After testing many combinations, I observed that I had to use that much
callbacks. For example, when omitting to wire .poweroff/.restore,
with .freeze/.thaw linked to suspend()/resume(), the state (of the mail
led) is not restored correctly after S2D.

> >
> > BTW, why so we retuen -ENOMEM from these methods if interface->data is
> > missing? I'd say we should not fail suspend resume in that case or if we
> > fail then with somethig like -EINVAL - we did not have mempry allocation
> > failure.
>
> Ok.
>
> -Carlos

2009-07-26 21:33:26

[permalink] [raw]

Subject: Re: [PATCH 1/1] acer-wmi: switch driver to dev_pm_ops

On Jul 26, 2009, at 1:28 PM, Arnaud Faucher <[email protected]>
wrote:

> On dim, 2009-07-26 at 19:35 +0100, Carlos Corbacho wrote:
>> On Sunday 26 July 2009 19:08:09 Dmitry Torokhov wrote:
>>> On Sun, Jul 26, 2009 at 03:23:29PM +0100, Carlos Corbacho wrote:
>>>> [Removing linux-mips from CC - I don't know why they'd be
>>>> interested in
>>>> an x86 only platform driver...]
>>>>
>>>> On Sunday 26 July 2009 14:53:33 Arnaud Faucher wrote:
>>>>> Gets rid of the following warning:
>>>>> Platform driver 'acer-wmi' needs updating - please use dev_pm_ops
>>>>>
>>>>> Take 2, thanks to Dmitry, Rafael and Frans for pointing out PM
>>>>> issue on
>>>>> hibernation when using dev_pm_ops blindly.
>>>>>
>>>>> This patch was tested against suspendand hibernation (Acer mail
>>>>> led
>>>>> status).
>>>>>
>>>>> Signed-off-by: Arnaud Faucher <[email protected]>
>>>>> ---
>>>>> drivers/platform/x86/acer-wmi.c | 17 ++++++++++++-----
>>>>> 1 files changed, 12 insertions(+), 5 deletions(-)
>>>>>
>>>>> diff --git a/drivers/platform/x86/acer-wmi.c
>>>>> b/drivers/platform/x86/acer-wmi.c
>>>>> index be2fd6f..29374bc 100644
>>>>> --- a/drivers/platform/x86/acer-wmi.c
>>>>> +++ b/drivers/platform/x86/acer-wmi.c
>>>>> @@ -1152,8 +1152,7 @@ static int acer_platform_remove(struct
>>>>> platform_device *device)
>>>>> return 0;
>>>>> }
>>>>>
>>>>> -static int acer_platform_suspend(struct platform_device *dev,
>>>>> -pm_message_t state)
>>>>> +static int acer_platform_suspend(struct device *dev)
>>>>> {
>>>>> u32 value;
>>>>> struct acer_data *data = &interface->data;
>>>>> @@ -1174,7 +1173,7 @@ pm_message_t state)
>>>>> return 0;
>>>>> }
>>>>>
>>>>> -static int acer_platform_resume(struct platform_device *device)
>>>>> +static int acer_platform_resume(struct device *dev)
>>>>> {
>>>>> struct acer_data *data = &interface->data;
>>>>>
>>>>> @@ -1190,15 +1189,23 @@ static int acer_platform_resume(struct
>>>>> platform_device *device)
>>>>> return 0;
>>>>> }
>>>>>
>>>>> +static struct dev_pm_ops acer_platform_pm_ops = {
>>>>> + .suspend = acer_platform_suspend,
>>>>> + .resume = acer_platform_resume,
>>>>
>>>> Are these necessary? For suspend-to-RAM, I've never needed these.
>>>> The old
>>>> callbacks here were just for suspend-to-disk.
>>>
>>> That is not correct. Old suspend and resume callbacks were called
>>> for
>>> both S2R and S2D. Whether it is actually needed for S2R I don't
>>> know but
>>> looking at the code they should not hurt.
>>
>> I'm aware they were called for S2RAM as well, but that was just a
>> limitation
>> of the old calls - as I say, they're not needed for it (at least on
>> my
>> hardware anyway).
>>
>
> I was looking for similar functionality.
>
>>>>> + .freeze = acer_platform_suspend,
>>>>> + .thaw = acer_platform_resume,
>>>>
>>>> If we only need these callbacks for freeze & thaw, they should be
>>>> rebamed.
>>>>
>>>>> + .poweroff = acer_platform_suspend,
>>>>> + .restore = acer_platform_resume,
>>>>
>>>> What do poweroff and restore mean in this context. Do my comments
>>>> above
>>>> apply again (i.e. are the callbacks necessary here)?
>>>
>>> I don't think poweroff handler is needed.
>
> After testing many combinations, I observed that I had to use that
> much
> callbacks. For example, when omitting to wire .poweroff/.restore,
> with .freeze/.thaw linked to suspend()/resume(), the state (of the
> mail
> led) is not restored correctly after S2D.

Have you tried with just 3 - freeze, thaw and restore?

>

--
Dmitry

2009-07-26 22:51:09

[permalink] [raw]

Subject: Re: [PATCH 1/1] acer-wmi: switch driver to dev_pm_ops

On dim, 2009-07-26 at 14:33 -0700, Dmitry Torokhov wrote:
> On Jul 26, 2009, at 1:28 PM, Arnaud Faucher <[email protected]>
> wrote:
>
> > On dim, 2009-07-26 at 19:35 +0100, Carlos Corbacho wrote:
> >> On Sunday 26 July 2009 19:08:09 Dmitry Torokhov wrote:
> >>> On Sun, Jul 26, 2009 at 03:23:29PM +0100, Carlos Corbacho wrote:
> >>>> [Removing linux-mips from CC - I don't know why they'd be
> >>>> interested in
> >>>> an x86 only platform driver...]
> >>>>
> >>>> On Sunday 26 July 2009 14:53:33 Arnaud Faucher wrote:
> >>>>> Gets rid of the following warning:
> >>>>> Platform driver 'acer-wmi' needs updating - please use dev_pm_ops
> >>>>>
> >>>>> Take 2, thanks to Dmitry, Rafael and Frans for pointing out PM
> >>>>> issue on
> >>>>> hibernation when using dev_pm_ops blindly.
> >>>>>
> >>>>> This patch was tested against suspendand hibernation (Acer mail
> >>>>> led
> >>>>> status).
> >>>>>
> >>>>> Signed-off-by: Arnaud Faucher <[email protected]>
> >>>>> ---
> >>>>> drivers/platform/x86/acer-wmi.c | 17 ++++++++++++-----
> >>>>> 1 files changed, 12 insertions(+), 5 deletions(-)
> >>>>>
> >>>>> diff --git a/drivers/platform/x86/acer-wmi.c
> >>>>> b/drivers/platform/x86/acer-wmi.c
> >>>>> index be2fd6f..29374bc 100644
> >>>>> --- a/drivers/platform/x86/acer-wmi.c
> >>>>> +++ b/drivers/platform/x86/acer-wmi.c
> >>>>> @@ -1152,8 +1152,7 @@ static int acer_platform_remove(struct
> >>>>> platform_device *device)
> >>>>> return 0;
> >>>>> }
> >>>>>
> >>>>> -static int acer_platform_suspend(struct platform_device *dev,
> >>>>> -pm_message_t state)
> >>>>> +static int acer_platform_suspend(struct device *dev)
> >>>>> {
> >>>>> u32 value;
> >>>>> struct acer_data *data = &interface->data;
> >>>>> @@ -1174,7 +1173,7 @@ pm_message_t state)
> >>>>> return 0;
> >>>>> }
> >>>>>
> >>>>> -static int acer_platform_resume(struct platform_device *device)
> >>>>> +static int acer_platform_resume(struct device *dev)
> >>>>> {
> >>>>> struct acer_data *data = &interface->data;
> >>>>>
> >>>>> @@ -1190,15 +1189,23 @@ static int acer_platform_resume(struct
> >>>>> platform_device *device)
> >>>>> return 0;
> >>>>> }
> >>>>>
> >>>>> +static struct dev_pm_ops acer_platform_pm_ops = {
> >>>>> + .suspend = acer_platform_suspend,
> >>>>> + .resume = acer_platform_resume,
> >>>>
> >>>> Are these necessary? For suspend-to-RAM, I've never needed these.
> >>>> The old
> >>>> callbacks here were just for suspend-to-disk.
> >>>
> >>> That is not correct. Old suspend and resume callbacks were called
> >>> for
> >>> both S2R and S2D. Whether it is actually needed for S2R I don't
> >>> know but
> >>> looking at the code they should not hurt.
> >>
> >> I'm aware they were called for S2RAM as well, but that was just a
> >> limitation
> >> of the old calls - as I say, they're not needed for it (at least on
> >> my
> >> hardware anyway).
> >>
> >
> > I was looking for similar functionality.
> >
> >>>>> + .freeze = acer_platform_suspend,
> >>>>> + .thaw = acer_platform_resume,
> >>>>
> >>>> If we only need these callbacks for freeze & thaw, they should be
> >>>> rebamed.
> >>>>
> >>>>> + .poweroff = acer_platform_suspend,
> >>>>> + .restore = acer_platform_resume,
> >>>>
> >>>> What do poweroff and restore mean in this context. Do my comments
> >>>> above
> >>>> apply again (i.e. are the callbacks necessary here)?
> >>>
> >>> I don't think poweroff handler is needed.
> >
> > After testing many combinations, I observed that I had to use that
> > much
> > callbacks. For example, when omitting to wire .poweroff/.restore,
> > with .freeze/.thaw linked to suspend()/resume(), the state (of the
> > mail
> > led) is not restored correctly after S2D.
>
>
> Have you tried with just 3 - freeze, thaw and restore?
>

State restoration seems to be OK with only those three ones (tested
against both S2RAM and S2D on my Acer Aspire 5680).

BTW, in the "struct dev_pm_ops" documentation, it would be interesting
to know which callback sequence occurs in case of S2RAM and S2D events.

2009-07-28 23:39:35

[permalink] [raw]

Subject: Re: [PATCH 1/1] acer-wmi: switch driver to dev_pm_ops

On Sun, Jul 26, 2009 at 18:51 -0400, Arnaud Faucher wrote :
> On dim, 2009-07-26 at 14:33 -0700, Dmitry Torokhov wrote:
> > On Jul 26, 2009, at 1:28 PM, Arnaud Faucher <[email protected]>
> > wrote:
> >
> > > On dim, 2009-07-26 at 19:35 +0100, Carlos Corbacho wrote:
> > >> On Sunday 26 July 2009 19:08:09 Dmitry Torokhov wrote:
> > >>> On Sun, Jul 26, 2009 at 03:23:29PM +0100, Carlos Corbacho wrote:
> > >>>> [Removing linux-mips from CC - I don't know why they'd be
> > >>>> interested in
> > >>>> an x86 only platform driver...]
> > >>>>
> > >>>> On Sunday 26 July 2009 14:53:33 Arnaud Faucher wrote:
> > >>>>> Gets rid of the following warning:
> > >>>>> Platform driver 'acer-wmi' needs updating - please use dev_pm_ops
> > >>>>>
> > >>>>> Take 2, thanks to Dmitry, Rafael and Frans for pointing out PM
> > >>>>> issue on
> > >>>>> hibernation when using dev_pm_ops blindly.
> > >>>>>
> > >>>>> This patch was tested against suspendand hibernation (Acer mail
> > >>>>> led
> > >>>>> status).
> > >>>>>
> > >>>>> Signed-off-by: Arnaud Faucher <[email protected]>
> > >>>>> ---
> > >>>>> drivers/platform/x86/acer-wmi.c | 17 ++++++++++++-----
> > >>>>> 1 files changed, 12 insertions(+), 5 deletions(-)
> > >>>>>
> > >>>>> diff --git a/drivers/platform/x86/acer-wmi.c
> > >>>>> b/drivers/platform/x86/acer-wmi.c
> > >>>>> index be2fd6f..29374bc 100644
> > >>>>> --- a/drivers/platform/x86/acer-wmi.c
> > >>>>> +++ b/drivers/platform/x86/acer-wmi.c
> > >>>>> @@ -1152,8 +1152,7 @@ static int acer_platform_remove(struct
> > >>>>> platform_device *device)
> > >>>>> return 0;
> > >>>>> }
> > >>>>>
> > >>>>> -static int acer_platform_suspend(struct platform_device *dev,
> > >>>>> -pm_message_t state)
> > >>>>> +static int acer_platform_suspend(struct device *dev)
> > >>>>> {
> > >>>>> u32 value;
> > >>>>> struct acer_data *data = &interface->data;
> > >>>>> @@ -1174,7 +1173,7 @@ pm_message_t state)
> > >>>>> return 0;
> > >>>>> }
> > >>>>>
> > >>>>> -static int acer_platform_resume(struct platform_device *device)
> > >>>>> +static int acer_platform_resume(struct device *dev)
> > >>>>> {
> > >>>>> struct acer_data *data = &interface->data;
> > >>>>>
> > >>>>> @@ -1190,15 +1189,23 @@ static int acer_platform_resume(struct
> > >>>>> platform_device *device)
> > >>>>> return 0;
> > >>>>> }
> > >>>>>
> > >>>>> +static struct dev_pm_ops acer_platform_pm_ops = {
> > >>>>> + .suspend = acer_platform_suspend,
> > >>>>> + .resume = acer_platform_resume,
> > >>>>
> > >>>> Are these necessary? For suspend-to-RAM, I've never needed these.
> > >>>> The old
> > >>>> callbacks here were just for suspend-to-disk.
> > >>>
> > >>> That is not correct. Old suspend and resume callbacks were called
> > >>> for
> > >>> both S2R and S2D. Whether it is actually needed for S2R I don't
> > >>> know but
> > >>> looking at the code they should not hurt.
> > >>
> > >> I'm aware they were called for S2RAM as well, but that was just a
> > >> limitation
> > >> of the old calls - as I say, they're not needed for it (at least on
> > >> my
> > >> hardware anyway).
> > >>
> > >
> > > I was looking for similar functionality.
> > >
> > >>>>> + .freeze = acer_platform_suspend,
> > >>>>> + .thaw = acer_platform_resume,
> > >>>>
> > >>>> If we only need these callbacks for freeze & thaw, they should be
> > >>>> rebamed.
> > >>>>
> > >>>>> + .poweroff = acer_platform_suspend,
> > >>>>> + .restore = acer_platform_resume,
> > >>>>
> > >>>> What do poweroff and restore mean in this context. Do my comments
> > >>>> above
> > >>>> apply again (i.e. are the callbacks necessary here)?
> > >>>
> > >>> I don't think poweroff handler is needed.
> > >
> > > After testing many combinations, I observed that I had to use that
> > > much
> > > callbacks. For example, when omitting to wire .poweroff/.restore,
> > > with .freeze/.thaw linked to suspend()/resume(), the state (of the
> > > mail
> > > led) is not restored correctly after S2D.
> >
> >
> > Have you tried with just 3 - freeze, thaw and restore?
> >
>
> State restoration seems to be OK with only those three ones (tested
> against both S2RAM and S2D on my Acer Aspire 5680).
>
> BTW, in the "struct dev_pm_ops" documentation, it would be interesting
> to know which callback sequence occurs in case of S2RAM and S2D events.
>
>

Here are my observations regarding PM events and callbacks:

S2RAM ("Suspend"):
event trigged -> .suspend gets called
recover from S2RAM -> .resume gets called
For *this module*, these callbacks seem not necessary, because the state
of hardware seems to be automatically restored on resume from S2RAM (by
BIOS ???).

S2DISK ("Hibernation"):
event trigged -> .freeze, then .thaw gets called
recover from S2DISK -> .restore gets called
As per the pm.h documentation .thaw is called after RAM image has been
created, in order to restore hardware state in case RAM image failed and
the system cannot power off. So this callback should be linked to
something similar to the .restore callback. For *this module*, though it
may not be necessary because the state of both LCD backlight and mail
LED persist after RAM image creation, I have linked this callback
anyway.

After around 2 days of testing, here is a third attempt for this patch.
Please note that I renamed the functions to better reflect their use.
Can you please stress it ?

Signed-off-by: Arnaud Faucher <[email protected]>
---
drivers/platform/x86/acer-wmi.c | 14 +++++++++-----
1 files changed, 9 insertions(+), 5 deletions(-)

diff --git a/drivers/platform/x86/acer-wmi.c
b/drivers/platform/x86/acer-wmi.c
index fb45f5e..331771d 100644
--- a/drivers/platform/x86/acer-wmi.c
+++ b/drivers/platform/x86/acer-wmi.c
@@ -1152,8 +1152,7 @@ static int acer_platform_remove(struct
platform_device *device)
return 0;
}

-static int acer_platform_suspend(struct platform_device *dev,
-pm_message_t state)
+static int acer_platform_freeze(struct device *dev)
{
u32 value;
struct acer_data *data = &interface->data;
@@ -1174,7 +1173,7 @@ pm_message_t state)
return 0;
}

-static int acer_platform_resume(struct platform_device *device)
+static int acer_platform_restore(struct device *dev)
{
struct acer_data *data = &interface->data;

@@ -1190,15 +1189,20 @@ static int acer_platform_resume(struct
platform_device *device)
return 0;
}

+static struct dev_pm_ops acer_platform_pm_ops = {
+ .freeze = acer_platform_freeze,
+ .thaw = acer_platform_restore,
+ .restore = acer_platform_restore,
+};
+
static struct platform_driver acer_platform_driver = {
.driver = {
.name = "acer-wmi",
.owner = THIS_MODULE,
+ .pm = &acer_platform_pm_ops,
},
.probe = acer_platform_probe,
.remove = acer_platform_remove,
- .suspend = acer_platform_suspend,
- .resume = acer_platform_resume,
};

static struct platform_device *acer_platform_device;
--
1.6.3.3

2009-07-29 20:49:28

[permalink] [raw]

Subject: Re: [PATCH 1/1] acer-wmi: switch driver to dev_pm_ops

On Wednesday 29 July 2009, Arnaud Faucher wrote:
> On Sun, Jul 26, 2009 at 18:51 -0400, Arnaud Faucher wrote :
> > On dim, 2009-07-26 at 14:33 -0700, Dmitry Torokhov wrote:
> > > On Jul 26, 2009, at 1:28 PM, Arnaud Faucher <[email protected]>
> > > wrote:
> > >
> > > > On dim, 2009-07-26 at 19:35 +0100, Carlos Corbacho wrote:
> > > >> On Sunday 26 July 2009 19:08:09 Dmitry Torokhov wrote:
> > > >>> On Sun, Jul 26, 2009 at 03:23:29PM +0100, Carlos Corbacho wrote:
> > > >>>> [Removing linux-mips from CC - I don't know why they'd be
> > > >>>> interested in
> > > >>>> an x86 only platform driver...]
> > > >>>>
> > > >>>> On Sunday 26 July 2009 14:53:33 Arnaud Faucher wrote:
> > > >>>>> Gets rid of the following warning:
> > > >>>>> Platform driver 'acer-wmi' needs updating - please use dev_pm_ops
> > > >>>>>
> > > >>>>> Take 2, thanks to Dmitry, Rafael and Frans for pointing out PM
> > > >>>>> issue on
> > > >>>>> hibernation when using dev_pm_ops blindly.
> > > >>>>>
> > > >>>>> This patch was tested against suspendand hibernation (Acer mail
> > > >>>>> led
> > > >>>>> status).
> > > >>>>>
> > > >>>>> Signed-off-by: Arnaud Faucher <[email protected]>
> > > >>>>> ---
> > > >>>>> drivers/platform/x86/acer-wmi.c | 17 ++++++++++++-----
> > > >>>>> 1 files changed, 12 insertions(+), 5 deletions(-)
> > > >>>>>
> > > >>>>> diff --git a/drivers/platform/x86/acer-wmi.c
> > > >>>>> b/drivers/platform/x86/acer-wmi.c
> > > >>>>> index be2fd6f..29374bc 100644
> > > >>>>> --- a/drivers/platform/x86/acer-wmi.c
> > > >>>>> +++ b/drivers/platform/x86/acer-wmi.c
> > > >>>>> @@ -1152,8 +1152,7 @@ static int acer_platform_remove(struct
> > > >>>>> platform_device *device)
> > > >>>>> return 0;
> > > >>>>> }
> > > >>>>>
> > > >>>>> -static int acer_platform_suspend(struct platform_device *dev,
> > > >>>>> -pm_message_t state)
> > > >>>>> +static int acer_platform_suspend(struct device *dev)
> > > >>>>> {
> > > >>>>> u32 value;
> > > >>>>> struct acer_data *data = &interface->data;
> > > >>>>> @@ -1174,7 +1173,7 @@ pm_message_t state)
> > > >>>>> return 0;
> > > >>>>> }
> > > >>>>>
> > > >>>>> -static int acer_platform_resume(struct platform_device *device)
> > > >>>>> +static int acer_platform_resume(struct device *dev)
> > > >>>>> {
> > > >>>>> struct acer_data *data = &interface->data;
> > > >>>>>
> > > >>>>> @@ -1190,15 +1189,23 @@ static int acer_platform_resume(struct
> > > >>>>> platform_device *device)
> > > >>>>> return 0;
> > > >>>>> }
> > > >>>>>
> > > >>>>> +static struct dev_pm_ops acer_platform_pm_ops = {
> > > >>>>> + .suspend = acer_platform_suspend,
> > > >>>>> + .resume = acer_platform_resume,
> > > >>>>
> > > >>>> Are these necessary? For suspend-to-RAM, I've never needed these.
> > > >>>> The old
> > > >>>> callbacks here were just for suspend-to-disk.
> > > >>>
> > > >>> That is not correct. Old suspend and resume callbacks were called
> > > >>> for
> > > >>> both S2R and S2D. Whether it is actually needed for S2R I don't
> > > >>> know but
> > > >>> looking at the code they should not hurt.
> > > >>
> > > >> I'm aware they were called for S2RAM as well, but that was just a
> > > >> limitation
> > > >> of the old calls - as I say, they're not needed for it (at least on
> > > >> my
> > > >> hardware anyway).
> > > >>
> > > >
> > > > I was looking for similar functionality.
> > > >
> > > >>>>> + .freeze = acer_platform_suspend,
> > > >>>>> + .thaw = acer_platform_resume,
> > > >>>>
> > > >>>> If we only need these callbacks for freeze & thaw, they should be
> > > >>>> rebamed.
> > > >>>>
> > > >>>>> + .poweroff = acer_platform_suspend,
> > > >>>>> + .restore = acer_platform_resume,
> > > >>>>
> > > >>>> What do poweroff and restore mean in this context. Do my comments
> > > >>>> above
> > > >>>> apply again (i.e. are the callbacks necessary here)?
> > > >>>
> > > >>> I don't think poweroff handler is needed.
> > > >
> > > > After testing many combinations, I observed that I had to use that
> > > > much
> > > > callbacks. For example, when omitting to wire .poweroff/.restore,
> > > > with .freeze/.thaw linked to suspend()/resume(), the state (of the
> > > > mail
> > > > led) is not restored correctly after S2D.
> > >
> > >
> > > Have you tried with just 3 - freeze, thaw and restore?
> > >
> >
> > State restoration seems to be OK with only those three ones (tested
> > against both S2RAM and S2D on my Acer Aspire 5680).
> >
> > BTW, in the "struct dev_pm_ops" documentation, it would be interesting
> > to know which callback sequence occurs in case of S2RAM and S2D events.
> >
> >
>
> Here are my observations regarding PM events and callbacks:
>
> S2RAM ("Suspend"):
> event trigged -> .suspend gets called
> recover from S2RAM -> .resume gets called

That's correct, plus there are .suspend_noirq() which is called during suspend,
after suspend_device_irqs(), and .resume_noirq() which is called during resume,
before resume_device_irqs().

> For *this module*,

What's *this module*? acer-wmi?

> these callbacks seem not necessary, because the state
> of hardware seems to be automatically restored on resume from S2RAM (by
> BIOS ???).
>
> S2DISK ("Hibernation"):
> event trigged -> .freeze, then .thaw gets called

To be precise, the ordering is

.freeze()
.freeze_noirq()
create image
.thaw_noirq()
.thaw()
save image
.poweroff()
.poweroff_noirq()
enter sleep state

> recover from S2DISK -> .restore gets called

To be precise, the ordering is:

Boot kernel:
load image
.freeze()
.freeze_noirq()
pass control to the image kernel
Image kernel:
.restore_noirq()
.restore()

> As per the pm.h documentation .thaw is called after RAM image has been
> created, in order to restore hardware state in case RAM image failed and
> the system cannot power off.

That's not correct (please see above). .thaw() is called after creating the
image in case .freeze() has changed the state of the device. This often is not
necessary, though, so .thaw() may be skipped in many cases. Of course, you
should know exactly what you're doing.

> So this callback should be linked to
> something similar to the .restore callback. For *this module*, though it
> may not be necessary because the state of both LCD backlight and mail
> LED persist after RAM image creation, I have linked this callback
> anyway.
>
> After around 2 days of testing, here is a third attempt for this patch.
> Please note that I renamed the functions to better reflect their use.
> Can you please stress it ?
>
> Signed-off-by: Arnaud Faucher <[email protected]>
> ---
> drivers/platform/x86/acer-wmi.c | 14 +++++++++-----
> 1 files changed, 9 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/platform/x86/acer-wmi.c
> b/drivers/platform/x86/acer-wmi.c
> index fb45f5e..331771d 100644
> --- a/drivers/platform/x86/acer-wmi.c
> +++ b/drivers/platform/x86/acer-wmi.c
> @@ -1152,8 +1152,7 @@ static int acer_platform_remove(struct
> platform_device *device)
> return 0;
> }
>
> -static int acer_platform_suspend(struct platform_device *dev,
> -pm_message_t state)
> +static int acer_platform_freeze(struct device *dev)
> {
> u32 value;
> struct acer_data *data = &interface->data;
> @@ -1174,7 +1173,7 @@ pm_message_t state)
> return 0;
> }
>
> -static int acer_platform_resume(struct platform_device *device)
> +static int acer_platform_restore(struct device *dev)
> {
> struct acer_data *data = &interface->data;
>
> @@ -1190,15 +1189,20 @@ static int acer_platform_resume(struct
> platform_device *device)
> return 0;
> }
>
> +static struct dev_pm_ops acer_platform_pm_ops = {
> + .freeze = acer_platform_freeze,

+ .suspend = acer_platform_freeze,

> + .thaw = acer_platform_restore,

.thaw is not necessary AFAICS.

> + .restore = acer_platform_restore,

+ .resume = acer_platform_restore,

> +};
> +
> static struct platform_driver acer_platform_driver = {
> .driver = {
> .name = "acer-wmi",
> .owner = THIS_MODULE,
> + .pm = &acer_platform_pm_ops,
> },
> .probe = acer_platform_probe,
> .remove = acer_platform_remove,
> - .suspend = acer_platform_suspend,
> - .resume = acer_platform_resume,
> };
>
> static struct platform_device *acer_platform_device;

Thanks,
Rafael

2009-07-29 21:04:07

[permalink] [raw]

Subject: Re: [PATCH 1/1] acer-wmi: switch driver to dev_pm_ops

On Wed, Jul 29, 2009 at 10:49:46PM +0200, Rafael J. Wysocki wrote:
> On Wednesday 29 July 2009, Arnaud Faucher wrote:
>
> > As per the pm.h documentation .thaw is called after RAM image has been
> > created, in order to restore hardware state in case RAM image failed and
> > the system cannot power off.
>
> That's not correct (please see above). .thaw() is called after creating the
> image in case .freeze() has changed the state of the device. This often is not
> necessary, though, so .thaw() may be skipped in many cases. Of course, you
> should know exactly what you're doing.
>

Umm, but thaw() _is_ called in case of hibernate failure:

case PM_EVENT_THAW:
case PM_EVENT_RECOVER:
if (ops->thaw) {
error = ops->thaw(dev);
suspend_report_result(ops->thaw, error);
}
break;

so I don't believe you can easily skip thaw() if you have freeze() that
stops/resets device.

--
Dmitry

2009-07-29 22:53:55

[permalink] [raw]

Subject: Re: [PATCH 1/1] acer-wmi: switch driver to dev_pm_ops

On Wed, Jul 29, 2009 at 14:03 -0700, Dmitry Torokhov wrote:
> On Wed, Jul 29, 2009 at 10:49:46PM +0200, Rafael J. Wysocki wrote:
> > On Wednesday 29 July 2009, Arnaud Faucher wrote:
> >
> > > As per the pm.h documentation .thaw is called after RAM image has been
> > > created, in order to restore hardware state in case RAM image failed and
> > > the system cannot power off.
> >
> > That's not correct (please see above). .thaw() is called after creating the
> > image in case .freeze() has changed the state of the device. This often is not
> > necessary, though, so .thaw() may be skipped in many cases. Of course, you
> > should know exactly what you're doing.
> >
>
> Umm, but thaw() _is_ called in case of hibernate failure:
>
> case PM_EVENT_THAW:
> case PM_EVENT_RECOVER:
> if (ops->thaw) {
> error = ops->thaw(dev);
> suspend_report_result(ops->thaw, error);
> }
> break;
>
> so I don't believe you can easily skip thaw() if you have freeze() that
> stops/resets device.
>

As of today, acer_platform_freeze() does not stop/reset any device, so,
I agree with Rafael that for acer-wmi, .thaw() can be skipped. I had
also tested this configuration and it was working like the non-patched
code.

Carlos, do you think that any acer-specific hardware could be switched
off or reset inside acer_platform_freeze() ? If this was the case, we
would have to wire .thaw()...

2009-07-29 23:27:13

[permalink] [raw]

Subject: Re: [PATCH 1/1] acer-wmi: switch driver to dev_pm_ops

On Wednesday 29 July 2009, Dmitry Torokhov wrote:
> On Wed, Jul 29, 2009 at 10:49:46PM +0200, Rafael J. Wysocki wrote:
> > On Wednesday 29 July 2009, Arnaud Faucher wrote:
> >
> > > As per the pm.h documentation .thaw is called after RAM image has been
> > > created, in order to restore hardware state in case RAM image failed and
> > > the system cannot power off.
> >
> > That's not correct (please see above). .thaw() is called after creating the
> > image in case .freeze() has changed the state of the device. This often is not
> > necessary, though, so .thaw() may be skipped in many cases. Of course, you
> > should know exactly what you're doing.
> >
>
> Umm, but thaw() _is_ called in case of hibernate failure:
>
> case PM_EVENT_THAW:
> case PM_EVENT_RECOVER:
> if (ops->thaw) {
> error = ops->thaw(dev);
> suspend_report_result(ops->thaw, error);
> }
> break;
>
> so I don't believe you can easily skip thaw() if you have freeze() that
> stops/resets device.

_RECOVER means "image creation failed or unable to run the image kernel", so
from the devices' point of view it's the same as _THAW.

Still, if your .freeze() modifies the device's registers, then most likely
.thaw() _is_ necessary.

Best,
Rafael

2009-07-30 22:05:23

[permalink] [raw]

Subject: Re: [PATCH 1/1] acer-wmi: switch driver to dev_pm_ops

On Wed, Jul 29, 2009 at 18:53 -0400, Arnaud Faucher wrote :
> As of today, acer_platform_freeze() does not stop/reset any device, so,
> I agree with Rafael that for acer-wmi, .thaw() can be skipped. I had
> also tested this configuration and it was working like the non-patched
> code.
>
> Carlos, do you think that any acer-specific hardware could be switched
> off or reset inside acer_platform_freeze() ? If this was the case, we
> would have to wire .thaw()...
>

Should I send a new patch with only .freeze() and .restore() wired ?

---
Hope to see Cox and Torvalds reconcile...

2009-07-31 11:56:25