| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-Id: <1248824367.4112.31.camel@green>
Date: Tue, 28 Jul 2009 19:39:27 -0400
From: Arnaud Faucher <arnaud.faucher@...il.com>
To: Carlos Corbacho <carlos@...angeworlds.co.uk>,
Dmitry Torokhov <dmitry.torokhov@...il.com>
Cc: "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"Rafael J. Wysocki" <rjw@...k.pl>, Frans Pop <elendil@...net.nl>,
Manuel Lauss <manuel.lauss@...il.com>,
Erik Ekman <erik@...o.se>,
Mark Brown <broonie@...nsource.wolfsonmicro.com>
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 <arnaud.faucher@...il.com>
> > 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 <arnaud.faucher@...il.com>
> > >>>>> ---
> > >>>>> 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 <arnaud.faucher@...il.com>
---
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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists