Hi,
While Maxim is testing if the patch below helps with
http://bugzilla.kernel.org/show_bug.cgi?id=14668
I think it's necessary anyway.
The problem is that currently there's nothing to prevent us from suspending in
the middle of an EC transaction in progress, at least as far as I can see.
As a result, we can suspend with the ACPI global lock held or something like
this, which leads to problems especially for hibernation (if the resume kernel
passes control to the image kernel in the middle of an EC transaction, things
aren't nice). For this reason I think we should wait until there are no EC
transactions in progress before we suspend and we should prevent any new
EC transactions from starting after that point. The patch below does that.
However, it does that in the EC's suspend callback, which may be too early,
because there still is _PTS to run, so it might be necessary to do that later.
On the other hand, the mechanics behind the ACPI global lock, which is
acquired in acpi_ec_transaction(), requires that interrupts work, because
otherwise there may be a problem if the global lock is not actually acquired
after ACPI_ACQUIRE_GLOBAL_LOCK(), so the last place in which to
wait for EC transactions to complete seems to be the platform suspend
.prepare() callback. Unfortunately, it's not implemented at the moment for
ACPI and it doesn't have a hibernate counterpart and that's why I'd rather use
the patch below, unless it's known to break things for someone. So, if you
can, please test it and tell me if you have any problems with it.
Of course, comments are welcome as well.
Thanks,
Rafael
---
drivers/acpi/ec.c | 17 ++++++++++++++++-
1 file changed, 16 insertions(+), 1 deletion(-)
Index: linux-2.6/drivers/acpi/ec.c
===================================================================
--- linux-2.6.orig/drivers/acpi/ec.c
+++ linux-2.6/drivers/acpi/ec.c
@@ -76,8 +76,9 @@ enum ec_command {
enum {
EC_FLAGS_QUERY_PENDING, /* Query is pending */
EC_FLAGS_GPE_STORM, /* GPE storm detected */
- EC_FLAGS_HANDLERS_INSTALLED /* Handlers for GPE and
+ EC_FLAGS_HANDLERS_INSTALLED, /* Handlers for GPE and
* OpReg are installed */
+ EC_FLAGS_SUSPENDED, /* Driver is suspended */
};
/* If we find an EC via the ECDT, we need to keep a ptr to its context */
@@ -291,6 +292,10 @@ static int acpi_ec_transaction(struct ac
if (t->rdata)
memset(t->rdata, 0, t->rlen);
mutex_lock(&ec->lock);
+ if (test_bit(EC_FLAGS_SUSPENDED, &ec->flags)) {
+ status = -EBUSY;
+ goto unlock;
+ }
if (ec->global_lock) {
status = acpi_acquire_global_lock(ACPI_EC_UDELAY_GLK, &glk);
if (ACPI_FAILURE(status)) {
@@ -1059,16 +1064,26 @@ error:
static int acpi_ec_suspend(struct acpi_device *device, pm_message_t state)
{
struct acpi_ec *ec = acpi_driver_data(device);
+
+ mutex_lock(&ec->lock);
+ /* Prevent transactions from happening while suspended */
+ set_bit(EC_FLAGS_SUSPENDED, &ec->flags);
/* Stop using GPE */
acpi_disable_gpe(NULL, ec->gpe);
+ mutex_unlock(&ec->lock);
return 0;
}
static int acpi_ec_resume(struct acpi_device *device)
{
struct acpi_ec *ec = acpi_driver_data(device);
+
+ mutex_lock(&ec->lock);
/* Enable use of GPE back */
acpi_enable_gpe(NULL, ec->gpe);
+ /* Allow transactions to happen again */
+ set_bit(EC_FLAGS_SUSPENDED, &ec->flags);
+ mutex_unlock(&ec->lock);
return 0;
}
On 1/30/10, Rafael J. Wysocki <[email protected]> wrote:
> Hi,
>
> While Maxim is testing if the patch below helps with
> http://bugzilla.kernel.org/show_bug.cgi?id=14668
> I think it's necessary anyway.
>
> The problem is that currently there's nothing to prevent us from suspending
> in
> the middle of an EC transaction in progress, at least as far as I can see.
> As a result, we can suspend with the ACPI global lock held or something like
> this, which leads to problems especially for hibernation (if the resume
> kernel
> passes control to the image kernel in the middle of an EC transaction,
> things
> aren't nice). For this reason I think we should wait until there are no EC
> transactions in progress before we suspend and we should prevent any new
> EC transactions from starting after that point. The patch below does that.
I also believe this is a problem. I reported the following -
<http://bugzilla.kernel.org/show_bug.cgi?id=14112> "Pressing acpi
hotkeys duing hibernation may occasionally cause EC transaction
timeout"
and posted a patch, but I couldn't reproduce my specific issue in
order to confirm that the patch helped.
Regards
Alan
On Sun, 2010-01-31 at 14:11 +0000, Alan Jenkins wrote:
> On 1/30/10, Rafael J. Wysocki <[email protected]> wrote:
> > Hi,
> >
> > While Maxim is testing if the patch below helps with
> > http://bugzilla.kernel.org/show_bug.cgi?id=14668
> > I think it's necessary anyway.
> >
> > The problem is that currently there's nothing to prevent us from suspending
> > in
> > the middle of an EC transaction in progress, at least as far as I can see.
> > As a result, we can suspend with the ACPI global lock held or something like
> > this, which leads to problems especially for hibernation (if the resume
> > kernel
> > passes control to the image kernel in the middle of an EC transaction,
> > things
> > aren't nice). For this reason I think we should wait until there are no EC
> > transactions in progress before we suspend and we should prevent any new
> > EC transactions from starting after that point. The patch below does that.
Unfortunately, this patch even causes regressions on my notebook (it
survive 63 hibernate cycles), but now I battery driver reports 'battery
absent', backlight driver reports 0 brightness, but reload helped.
I think that not only _PTS ans _WAK are problematic. What about other
ACPI drivers that start accessing the EC before it is resumed?
I think that these cause the problems I observe.
Still, at least it does resume.
Best regards,
Maxim Levitsky
On Sun, 2010-01-31 at 00:29 +0100, Rafael J. Wysocki wrote:
> Hi,
>
> While Maxim is testing if the patch below helps with
> http://bugzilla.kernel.org/show_bug.cgi?id=14668
> I think it's necessary anyway.
>
> The problem is that currently there's nothing to prevent us from suspending in
> the middle of an EC transaction in progress, at least as far as I can see.
> As a result, we can suspend with the ACPI global lock held or something like
> this, which leads to problems especially for hibernation (if the resume kernel
> passes control to the image kernel in the middle of an EC transaction, things
> aren't nice). For this reason I think we should wait until there are no EC
> transactions in progress before we suspend and we should prevent any new
> EC transactions from starting after that point. The patch below does that.
>
> However, it does that in the EC's suspend callback, which may be too early,
> because there still is _PTS to run, so it might be necessary to do that later.
> On the other hand, the mechanics behind the ACPI global lock, which is
> acquired in acpi_ec_transaction(), requires that interrupts work, because
> otherwise there may be a problem if the global lock is not actually acquired
> after ACPI_ACQUIRE_GLOBAL_LOCK(), so the last place in which to
> wait for EC transactions to complete seems to be the platform suspend
> .prepare() callback. Unfortunately, it's not implemented at the moment for
> ACPI and it doesn't have a hibernate counterpart and that's why I'd rather use
> the patch below, unless it's known to break things for someone. So, if you
> can, please test it and tell me if you have any problems with it.
>
> Of course, comments are welcome as well.
>
> Thanks,
> Rafael
>
> ---
> drivers/acpi/ec.c | 17 ++++++++++++++++-
> 1 file changed, 16 insertions(+), 1 deletion(-)
>
> Index: linux-2.6/drivers/acpi/ec.c
> ===================================================================
> --- linux-2.6.orig/drivers/acpi/ec.c
> +++ linux-2.6/drivers/acpi/ec.c
> @@ -76,8 +76,9 @@ enum ec_command {
> enum {
> EC_FLAGS_QUERY_PENDING, /* Query is pending */
> EC_FLAGS_GPE_STORM, /* GPE storm detected */
> - EC_FLAGS_HANDLERS_INSTALLED /* Handlers for GPE and
> + EC_FLAGS_HANDLERS_INSTALLED, /* Handlers for GPE and
> * OpReg are installed */
> + EC_FLAGS_SUSPENDED, /* Driver is suspended */
> };
>
> /* If we find an EC via the ECDT, we need to keep a ptr to its context */
> @@ -291,6 +292,10 @@ static int acpi_ec_transaction(struct ac
> if (t->rdata)
> memset(t->rdata, 0, t->rlen);
> mutex_lock(&ec->lock);
> + if (test_bit(EC_FLAGS_SUSPENDED, &ec->flags)) {
> + status = -EBUSY;
> + goto unlock;
> + }
> if (ec->global_lock) {
> status = acpi_acquire_global_lock(ACPI_EC_UDELAY_GLK, &glk);
> if (ACPI_FAILURE(status)) {
> @@ -1059,16 +1064,26 @@ error:
> static int acpi_ec_suspend(struct acpi_device *device, pm_message_t state)
> {
> struct acpi_ec *ec = acpi_driver_data(device);
> +
> + mutex_lock(&ec->lock);
> + /* Prevent transactions from happening while suspended */
> + set_bit(EC_FLAGS_SUSPENDED, &ec->flags);
> /* Stop using GPE */
> acpi_disable_gpe(NULL, ec->gpe);
> + mutex_unlock(&ec->lock);
> return 0;
> }
>
> static int acpi_ec_resume(struct acpi_device *device)
> {
> struct acpi_ec *ec = acpi_driver_data(device);
> +
> + mutex_lock(&ec->lock);
> /* Enable use of GPE back */
> acpi_enable_gpe(NULL, ec->gpe);
> + /* Allow transactions to happen again */
> + set_bit(EC_FLAGS_SUSPENDED, &ec->flags);
^^^^^^^^^^^^
Thats why it doesn't work here....
Will retest now.
> + mutex_unlock(&ec->lock);
> return 0;
> }
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sunday 31 January 2010, Maxim Levitsky wrote:
> On Sun, 2010-01-31 at 00:29 +0100, Rafael J. Wysocki wrote:
...
> >
> > static int acpi_ec_resume(struct acpi_device *device)
> > {
> > struct acpi_ec *ec = acpi_driver_data(device);
> > +
> > + mutex_lock(&ec->lock);
> > /* Enable use of GPE back */
> > acpi_enable_gpe(NULL, ec->gpe);
> > + /* Allow transactions to happen again */
> > + set_bit(EC_FLAGS_SUSPENDED, &ec->flags);
> ^^^^^^^^^^^^
> Thats why it doesn't work here....
> Will retest now.
Ouch, sorry.
Rafael
On Sunday 31 January 2010, Rafael J. Wysocki wrote:
> On Sunday 31 January 2010, Maxim Levitsky wrote:
> > On Sun, 2010-01-31 at 00:29 +0100, Rafael J. Wysocki wrote:
> ...
> > >
> > > static int acpi_ec_resume(struct acpi_device *device)
> > > {
> > > struct acpi_ec *ec = acpi_driver_data(device);
> > > +
> > > + mutex_lock(&ec->lock);
> > > /* Enable use of GPE back */
> > > acpi_enable_gpe(NULL, ec->gpe);
> > > + /* Allow transactions to happen again */
> > > + set_bit(EC_FLAGS_SUSPENDED, &ec->flags);
> > ^^^^^^^^^^^^
> > Thats why it doesn't work here....
> > Will retest now.
>
> Ouch, sorry.
Fixed version is appended, for the record.
Rafael
---
From: Rafael J. Wysocki <[email protected]>
Subject: ACPI / EC: Add protection from suspending in the middle of EC transaction
There is a race between the suspend process and the EC driver that
may result in suspending in the middle of an EC transaction in
progress, which may lead to unpredictable behavior of the platform.
To remove that race condition, make acpi_ec_suspend() and
acpi_ec_resume() acquire ec->lock and introduce a new EC flag,
EC_FLAGS_SUSPENDED, set in acpi_ec_suspend() and cleared in
acpi_ec_resume(), that will cause acpi_ec_transaction() to return
error code (-EBUSY) if set.
Signed-off-by: Rafael J. Wysocki <[email protected]>
---
drivers/acpi/ec.c | 17 ++++++++++++++++-
1 file changed, 16 insertions(+), 1 deletion(-)
Index: linux-2.6/drivers/acpi/ec.c
===================================================================
--- linux-2.6.orig/drivers/acpi/ec.c
+++ linux-2.6/drivers/acpi/ec.c
@@ -76,8 +76,9 @@ enum ec_command {
enum {
EC_FLAGS_QUERY_PENDING, /* Query is pending */
EC_FLAGS_GPE_STORM, /* GPE storm detected */
- EC_FLAGS_HANDLERS_INSTALLED /* Handlers for GPE and
+ EC_FLAGS_HANDLERS_INSTALLED, /* Handlers for GPE and
* OpReg are installed */
+ EC_FLAGS_SUSPENDED, /* Driver is suspended */
};
/* If we find an EC via the ECDT, we need to keep a ptr to its context */
@@ -291,6 +292,10 @@ static int acpi_ec_transaction(struct ac
if (t->rdata)
memset(t->rdata, 0, t->rlen);
mutex_lock(&ec->lock);
+ if (test_bit(EC_FLAGS_SUSPENDED, &ec->flags)) {
+ status = -EBUSY;
+ goto unlock;
+ }
if (ec->global_lock) {
status = acpi_acquire_global_lock(ACPI_EC_UDELAY_GLK, &glk);
if (ACPI_FAILURE(status)) {
@@ -1059,16 +1064,26 @@ error:
static int acpi_ec_suspend(struct acpi_device *device, pm_message_t state)
{
struct acpi_ec *ec = acpi_driver_data(device);
+
+ mutex_lock(&ec->lock);
+ /* Prevent transactions from happening while suspended */
+ set_bit(EC_FLAGS_SUSPENDED, &ec->flags);
/* Stop using GPE */
acpi_disable_gpe(NULL, ec->gpe);
+ mutex_unlock(&ec->lock);
return 0;
}
static int acpi_ec_resume(struct acpi_device *device)
{
struct acpi_ec *ec = acpi_driver_data(device);
+
+ mutex_lock(&ec->lock);
/* Enable use of GPE back */
acpi_enable_gpe(NULL, ec->gpe);
+ /* Allow transactions to happen again */
+ clear_bit(EC_FLAGS_SUSPENDED, &ec->flags);
+ mutex_unlock(&ec->lock);
return 0;
}
On Sun, 31 Jan 2010, Maxim Levitsky wrote:
> Unfortunately, this patch even causes regressions on my notebook (it
> survive 63 hibernate cycles), but now I battery driver reports 'battery
> absent', backlight driver reports 0 brightness, but reload helped.
...
> I think that not only _PTS ans _WAK are problematic. What about other
> ACPI drivers that start accessing the EC before it is resumed?
> I think that these cause the problems I observe.
ACPI drivers might access the EC (even indirectly, through the DSDT). And
platform drivers do often access the EC both at suspend and resume time.
This needs some sort of strong ordering, the EC must suspend last, and
resume first (as seen by any ACPI and ACPI-aware drivers such as libata,
some platform drivers, etc). If EC interrupts are a problem, maybe it can
be kicked to poll mode for the suspend/resume transition?
Some platform drivers (like thinkpad-acpi) might need some changes to access
the EC syncronized with their ACPI devices (often they have all sort of
devices, e.g. backlight, ALSA, hwmon...) if the ordering is based on logic
in the ACPi bus driver.
--
"One disk to rule them all, One disk to find them. One disk to bring
them all and in the darkness grind them. In the Land of Redmond
where the shadows lie." -- The Silicon Valley Tarot
Henrique Holschuh
Henrique de Moraes Holschuh пишет:
> On Sun, 31 Jan 2010, Maxim Levitsky wrote:
>> Unfortunately, this patch even causes regressions on my notebook (it
>> survive 63 hibernate cycles), but now I battery driver reports 'battery
>> absent', backlight driver reports 0 brightness, but reload helped.
>
> ...
>
>> I think that not only _PTS ans _WAK are problematic. What about other
>> ACPI drivers that start accessing the EC before it is resumed?
>> I think that these cause the problems I observe.
>
> ACPI drivers might access the EC (even indirectly, through the DSDT). And
> platform drivers do often access the EC both at suspend and resume time.
Actually, only SBS and thinkpad-acpi access EC directly. All others go through
DSDT for access. Still, stopping EC in .suspend is too early, IMHO...
>
> This needs some sort of strong ordering, the EC must suspend last, and
> resume first (as seen by any ACPI and ACPI-aware drivers such as libata,
> some platform drivers, etc). If EC interrupts are a problem, maybe it can
> be kicked to poll mode for the suspend/resume transition?
It's this way already for about a year now... The problem is that EC driver might be
stopped in middle of the transaction, thus leaving EC in unknown state for BIOS or
switch-over kernel.
On Monday 01 February 2010, Alexey Starikovskiy wrote:
> Henrique de Moraes Holschuh пишет:
> > On Sun, 31 Jan 2010, Maxim Levitsky wrote:
> >> Unfortunately, this patch even causes regressions on my notebook (it
> >> survive 63 hibernate cycles), but now I battery driver reports 'battery
> >> absent', backlight driver reports 0 brightness, but reload helped.
> >
> > ...
> >
> >> I think that not only _PTS ans _WAK are problematic. What about other
> >> ACPI drivers that start accessing the EC before it is resumed?
> >> I think that these cause the problems I observe.
> >
> > ACPI drivers might access the EC (even indirectly, through the DSDT). And
> > platform drivers do often access the EC both at suspend and resume time.
> Actually, only SBS and thinkpad-acpi access EC directly. All others go through
> DSDT for access. Still, stopping EC in .suspend is too early, IMHO...
That's very likely the case, although that's kind of the latest we can do that
without major modifications.
Is there a possibility to have more than one EC in a system in practice?
> > This needs some sort of strong ordering, the EC must suspend last, and
> > resume first (as seen by any ACPI and ACPI-aware drivers such as libata,
> > some platform drivers, etc). If EC interrupts are a problem, maybe it can
> > be kicked to poll mode for the suspend/resume transition?
> It's this way already for about a year now... The problem is that EC driver might be
> stopped in middle of the transaction, thus leaving EC in unknown state for BIOS or
> switch-over kernel.
Exactly.
In theory I can try to disable EC transactions from the ACPI platform
suspend/resume callbacks, but then there still is a problem with the global
lock, since we shouldn't try to acquire it after disabling device interrupts,
because an SCI is waited for in case we don't acquire it immediately.
So, perhaps instead we should disable the GPEs and call _PTS before the late
suspend phase (and analogously, execute _WAK after the early resume phase)
and disable the EC transacations right after that?
This leaves the problem with PCI devices that may require ACPI support during
late suspend/early resume, but well.
Rafael
On Monday 01 February 2010, Rafael J. Wysocki wrote:
> On Monday 01 February 2010, Alexey Starikovskiy wrote:
> > Henrique de Moraes Holschuh пишет:
> > > On Sun, 31 Jan 2010, Maxim Levitsky wrote:
> > >> Unfortunately, this patch even causes regressions on my notebook (it
> > >> survive 63 hibernate cycles), but now I battery driver reports 'battery
> > >> absent', backlight driver reports 0 brightness, but reload helped.
> > >
> > > ...
> > >
> > >> I think that not only _PTS ans _WAK are problematic. What about other
> > >> ACPI drivers that start accessing the EC before it is resumed?
> > >> I think that these cause the problems I observe.
> > >
> > > ACPI drivers might access the EC (even indirectly, through the DSDT). And
> > > platform drivers do often access the EC both at suspend and resume time.
> > Actually, only SBS and thinkpad-acpi access EC directly. All others go through
> > DSDT for access. Still, stopping EC in .suspend is too early, IMHO...
>
> That's very likely the case, although that's kind of the latest we can do that
> without major modifications.
>
> Is there a possibility to have more than one EC in a system in practice?
>
> > > This needs some sort of strong ordering, the EC must suspend last, and
> > > resume first (as seen by any ACPI and ACPI-aware drivers such as libata,
> > > some platform drivers, etc). If EC interrupts are a problem, maybe it can
> > > be kicked to poll mode for the suspend/resume transition?
> > It's this way already for about a year now... The problem is that EC driver might be
> > stopped in middle of the transaction, thus leaving EC in unknown state for BIOS or
> > switch-over kernel.
>
> Exactly.
>
> In theory I can try to disable EC transactions from the ACPI platform
> suspend/resume callbacks, but then there still is a problem with the global
> lock, since we shouldn't try to acquire it after disabling device interrupts,
> because an SCI is waited for in case we don't acquire it immediately.
>
> So, perhaps instead we should disable the GPEs and call _PTS before the late
> suspend phase (and analogously, execute _WAK after the early resume phase)
> and disable the EC transacations right after that?
>
> This leaves the problem with PCI devices that may require ACPI support during
> late suspend/early resume, but well.
I'm going to send a more in-depth analysis of the ACPI vs suspend problems I
see in our current code later. For now, appended is an alternative to the
$subject patch that turns off the EC transactions after executing _PTS, but
assumes that there's only one EC in the system.
Rafael
---
From: Rafael J. Wysocki <[email protected]>
Subject: ACPI / EC: Remover race between EC driver and suspend process
There is a race between the suspend process and the EC driver that
may result in suspending in the middle of an EC transaction in
progress, which may lead to unpredictable behavior of the platform.
To remove that race condition, add functions for suspending and
resuming the EC transactions in a safe way to be executed by the
ACPI platform suspend/resume callbacks. Modify these callbacks so
that the EC transactions are suspended right after executing the _PTS
global control method and resumed right before executing the _WAK
global control method.
Signed-off-by: Rafael J. Wysocki <[email protected]>
---
drivers/acpi/ec.c | 33 ++++++++++++++++++++++++++++++++-
drivers/acpi/internal.h | 2 ++
drivers/acpi/sleep.c | 41 ++++++++++++++++++++++++-----------------
3 files changed, 58 insertions(+), 18 deletions(-)
Index: linux-2.6/drivers/acpi/ec.c
===================================================================
--- linux-2.6.orig/drivers/acpi/ec.c
+++ linux-2.6/drivers/acpi/ec.c
@@ -76,8 +76,9 @@ enum ec_command {
enum {
EC_FLAGS_QUERY_PENDING, /* Query is pending */
EC_FLAGS_GPE_STORM, /* GPE storm detected */
- EC_FLAGS_HANDLERS_INSTALLED /* Handlers for GPE and
+ EC_FLAGS_HANDLERS_INSTALLED, /* Handlers for GPE and
* OpReg are installed */
+ EC_FLAGS_SUSPENDED, /* Driver is suspended */
};
/* If we find an EC via the ECDT, we need to keep a ptr to its context */
@@ -291,6 +292,10 @@ static int acpi_ec_transaction(struct ac
if (t->rdata)
memset(t->rdata, 0, t->rlen);
mutex_lock(&ec->lock);
+ if (test_bit(EC_FLAGS_SUSPENDED, &ec->flags)) {
+ status = -EBUSY;
+ goto unlock;
+ }
if (ec->global_lock) {
status = acpi_acquire_global_lock(ACPI_EC_UDELAY_GLK, &glk);
if (ACPI_FAILURE(status)) {
@@ -445,6 +450,32 @@ int ec_transaction(u8 command,
EXPORT_SYMBOL(ec_transaction);
+void acpi_ec_suspend_transactions(void)
+{
+ struct acpi_ec *ec = first_ec;
+
+ if (!ec)
+ return;
+
+ mutex_lock(&ec->lock);
+ /* Prevent transactions from happening while suspended */
+ set_bit(EC_FLAGS_SUSPENDED, &ec->flags);
+ mutex_unlock(&ec->lock);
+}
+
+void acpi_ec_resume_transactions(void)
+{
+ struct acpi_ec *ec = first_ec;
+
+ if (!ec)
+ return;
+
+ mutex_lock(&ec->lock);
+ /* Allow transactions to happen again */
+ clear_bit(EC_FLAGS_SUSPENDED, &ec->flags);
+ mutex_unlock(&ec->lock);
+}
+
static int acpi_ec_query_unlocked(struct acpi_ec *ec, u8 * data)
{
int result;
Index: linux-2.6/drivers/acpi/internal.h
===================================================================
--- linux-2.6.orig/drivers/acpi/internal.h
+++ linux-2.6/drivers/acpi/internal.h
@@ -49,6 +49,8 @@ void acpi_early_processor_set_pdc(void);
int acpi_ec_init(void);
int acpi_ec_ecdt_probe(void);
int acpi_boot_ec_enable(void);
+void acpi_ec_suspend_transactions(void);
+void acpi_ec_resume_transactions(void);
/*--------------------------------------------------------------------------
Suspend/Resume
Index: linux-2.6/drivers/acpi/sleep.c
===================================================================
--- linux-2.6.orig/drivers/acpi/sleep.c
+++ linux-2.6/drivers/acpi/sleep.c
@@ -112,9 +112,10 @@ void __init acpi_old_suspend_ordering(vo
/**
* acpi_pm_disable_gpes - Disable the GPEs.
*/
-static int acpi_pm_disable_gpes(void)
+static int acpi_pm_disable_gpes_and_ec(void)
{
acpi_disable_all_gpes();
+ acpi_ec_suspend_transactions();
return 0;
}
@@ -142,7 +143,8 @@ static int acpi_pm_prepare(void)
int error = __acpi_pm_prepare();
if (!error)
- acpi_disable_all_gpes();
+ acpi_pm_disable_gpes_and_ec();
+
return error;
}
@@ -286,6 +288,12 @@ static int acpi_suspend_enter(suspend_st
return ACPI_SUCCESS(status) ? 0 : -EFAULT;
}
+static void acpi_suspend_finish(void)
+{
+ acpi_ec_resume_transactions();
+ acpi_pm_finish();
+}
+
static int acpi_suspend_state_valid(suspend_state_t pm_state)
{
u32 acpi_state;
@@ -307,7 +315,7 @@ static struct platform_suspend_ops acpi_
.begin = acpi_suspend_begin,
.prepare_late = acpi_pm_prepare,
.enter = acpi_suspend_enter,
- .wake = acpi_pm_finish,
+ .wake = acpi_suspend_finish,
.end = acpi_pm_end,
};
@@ -333,9 +341,9 @@ static int acpi_suspend_begin_old(suspen
static struct platform_suspend_ops acpi_suspend_ops_old = {
.valid = acpi_suspend_state_valid,
.begin = acpi_suspend_begin_old,
- .prepare_late = acpi_pm_disable_gpes,
+ .prepare_late = acpi_pm_disable_gpes_and_ec,
.enter = acpi_suspend_enter,
- .wake = acpi_pm_finish,
+ .wake = acpi_suspend_finish,
.end = acpi_pm_end,
.recover = acpi_pm_finish,
};
@@ -530,6 +538,7 @@ static int acpi_hibernation_enter(void)
static void acpi_hibernation_finish(void)
{
hibernate_nvs_free();
+ acpi_ec_resume_transactions();
acpi_pm_finish();
}
@@ -552,8 +561,9 @@ static void acpi_hibernation_leave(void)
hibernate_nvs_restore();
}
-static void acpi_pm_enable_gpes(void)
+static void acpi_pm_enable_gpes_and_ec(void)
{
+ acpi_ec_resume_transactions();
acpi_enable_all_runtime_gpes();
}
@@ -565,8 +575,8 @@ static struct platform_hibernation_ops a
.prepare = acpi_pm_prepare,
.enter = acpi_hibernation_enter,
.leave = acpi_hibernation_leave,
- .pre_restore = acpi_pm_disable_gpes,
- .restore_cleanup = acpi_pm_enable_gpes,
+ .pre_restore = acpi_pm_disable_gpes_and_ec,
+ .restore_cleanup = acpi_pm_enable_gpes_and_ec,
};
/**
@@ -598,12 +608,9 @@ static int acpi_hibernation_begin_old(vo
static int acpi_hibernation_pre_snapshot_old(void)
{
- int error = acpi_pm_disable_gpes();
-
- if (!error)
- hibernate_nvs_save();
-
- return error;
+ acpi_pm_disable_gpes_and_ec();
+ hibernate_nvs_save();
+ return 0;
}
/*
@@ -615,11 +622,11 @@ static struct platform_hibernation_ops a
.end = acpi_pm_end,
.pre_snapshot = acpi_hibernation_pre_snapshot_old,
.finish = acpi_hibernation_finish,
- .prepare = acpi_pm_disable_gpes,
+ .prepare = acpi_pm_disable_gpes_and_ec,
.enter = acpi_hibernation_enter,
.leave = acpi_hibernation_leave,
- .pre_restore = acpi_pm_disable_gpes,
- .restore_cleanup = acpi_pm_enable_gpes,
+ .pre_restore = acpi_pm_disable_gpes_and_ec,
+ .restore_cleanup = acpi_pm_enable_gpes_and_ec,
.recover = acpi_pm_finish,
};
#endif /* CONFIG_HIBERNATION */
On Sunday 31 January 2010, Rafael J. Wysocki wrote:
> Hi,
>
> While Maxim is testing if the patch below helps with
> http://bugzilla.kernel.org/show_bug.cgi?id=14668
> I think it's necessary anyway.
>
> The problem is that currently there's nothing to prevent us from suspending in
> the middle of an EC transaction in progress, at least as far as I can see.
> As a result, we can suspend with the ACPI global lock held or something like
> this, which leads to problems especially for hibernation (if the resume kernel
> passes control to the image kernel in the middle of an EC transaction, things
> aren't nice). For this reason I think we should wait until there are no EC
> transactions in progress before we suspend and we should prevent any new
> EC transactions from starting after that point. The patch below does that.
After discussing the patch with Alex I have a new version which is appended
below. This one causes EC transactions to be discarded after we've executed
_PTS (which is reasonable IMO) and allows them to happen again very early
during wake-up, so effectively the resume behavior should be unchanged.
Rafael
---
From: Rafael J. Wysocki <[email protected]>
Subject: ACPI / EC: Remove race between EC driver and suspend process (rev. 2)
There is a race between the suspend process and the EC driver that
may result in suspending in the middle of an EC transaction in
progress, which in turn may lead to unpredictable behavior of the
platform.
To remove that race condition, add a helper for suspending EC
transactions in a safe way to be executed by the ACPI platform
suspend/hibernate callbacks. Modify these callbacks so that the EC
transactions are suspended right after executing the _PTS global
control method and are allowed to happen right after the low-level
wake-up.
Signed-off-by: Rafael J. Wysocki <[email protected]>
Reported-by: Maxim Levitsky <[email protected]>
---
drivers/acpi/ec.c | 32 +++++++++++++++++++++++++++++++-
drivers/acpi/internal.h | 2 ++
drivers/acpi/sleep.c | 29 ++++++++++++++++-------------
3 files changed, 49 insertions(+), 14 deletions(-)
Index: linux-2.6/drivers/acpi/ec.c
===================================================================
--- linux-2.6.orig/drivers/acpi/ec.c
+++ linux-2.6/drivers/acpi/ec.c
@@ -76,8 +76,9 @@ enum ec_command {
enum {
EC_FLAGS_QUERY_PENDING, /* Query is pending */
EC_FLAGS_GPE_STORM, /* GPE storm detected */
- EC_FLAGS_HANDLERS_INSTALLED /* Handlers for GPE and
+ EC_FLAGS_HANDLERS_INSTALLED, /* Handlers for GPE and
* OpReg are installed */
+ EC_FLAGS_SUSPENDED, /* Driver is suspended */
};
/* If we find an EC via the ECDT, we need to keep a ptr to its context */
@@ -291,6 +292,12 @@ static int acpi_ec_transaction(struct ac
if (t->rdata)
memset(t->rdata, 0, t->rlen);
mutex_lock(&ec->lock);
+ if (test_bit(EC_FLAGS_SUSPENDED, &ec->flags)) {
+ pr_err(PREFIX
+ "EC transaction discarded due to power transition\n");
+ status = -EINVAL;
+ goto unlock;
+ }
if (ec->global_lock) {
status = acpi_acquire_global_lock(ACPI_EC_UDELAY_GLK, &glk);
if (ACPI_FAILURE(status)) {
@@ -445,6 +452,29 @@ int ec_transaction(u8 command,
EXPORT_SYMBOL(ec_transaction);
+void acpi_ec_suspend_transactions(void)
+{
+ struct acpi_ec *ec = first_ec;
+
+ if (!ec)
+ return;
+
+ mutex_lock(&ec->lock);
+ /* Prevent transactions from happening while suspended */
+ set_bit(EC_FLAGS_SUSPENDED, &ec->flags);
+ mutex_unlock(&ec->lock);
+}
+
+void acpi_ec_resume_transactions(void)
+{
+ /*
+ * Allow transactions to happen again (this function is called from
+ * atomic context during wake-up, so we don't need to acquire the mutex)
+ */
+ if (first_ec)
+ clear_bit(EC_FLAGS_SUSPENDED, &first_ec->flags);
+}
+
static int acpi_ec_query_unlocked(struct acpi_ec *ec, u8 * data)
{
int result;
Index: linux-2.6/drivers/acpi/internal.h
===================================================================
--- linux-2.6.orig/drivers/acpi/internal.h
+++ linux-2.6/drivers/acpi/internal.h
@@ -49,6 +49,8 @@ void acpi_early_processor_set_pdc(void);
int acpi_ec_init(void);
int acpi_ec_ecdt_probe(void);
int acpi_boot_ec_enable(void);
+void acpi_ec_suspend_transactions(void);
+void acpi_ec_resume_transactions(void);
/*--------------------------------------------------------------------------
Suspend/Resume
Index: linux-2.6/drivers/acpi/sleep.c
===================================================================
--- linux-2.6.orig/drivers/acpi/sleep.c
+++ linux-2.6/drivers/acpi/sleep.c
@@ -110,11 +110,12 @@ void __init acpi_old_suspend_ordering(vo
}
/**
- * acpi_pm_disable_gpes - Disable the GPEs.
+ * acpi_pm_freeze - Disable the GPEs and suspend EC transactions.
*/
-static int acpi_pm_disable_gpes(void)
+static int acpi_pm_freeze(void)
{
acpi_disable_all_gpes();
+ acpi_ec_suspend_transactions();
return 0;
}
@@ -142,7 +143,8 @@ static int acpi_pm_prepare(void)
int error = __acpi_pm_prepare();
if (!error)
- acpi_disable_all_gpes();
+ acpi_pm_freeze();
+
return error;
}
@@ -276,6 +278,8 @@ static int acpi_suspend_enter(suspend_st
*/
acpi_disable_all_gpes();
+ acpi_ec_resume_transactions();
+
local_irq_restore(flags);
printk(KERN_DEBUG "Back to C!\n");
@@ -333,7 +337,7 @@ static int acpi_suspend_begin_old(suspen
static struct platform_suspend_ops acpi_suspend_ops_old = {
.valid = acpi_suspend_state_valid,
.begin = acpi_suspend_begin_old,
- .prepare_late = acpi_pm_disable_gpes,
+ .prepare_late = acpi_pm_freeze,
.enter = acpi_suspend_enter,
.wake = acpi_pm_finish,
.end = acpi_pm_end,
@@ -522,6 +526,7 @@ static int acpi_hibernation_enter(void)
status = acpi_enter_sleep_state(ACPI_STATE_S4);
/* Reprogram control registers and execute _BFS */
acpi_leave_sleep_state_prep(ACPI_STATE_S4);
+ acpi_ec_resume_transactions();
local_irq_restore(flags);
return ACPI_SUCCESS(status) ? 0 : -EFAULT;
@@ -550,6 +555,7 @@ static void acpi_hibernation_leave(void)
}
/* Restore the NVS memory area */
hibernate_nvs_restore();
+ acpi_ec_resume_transactions();
}
static void acpi_pm_enable_gpes(void)
@@ -565,7 +571,7 @@ static struct platform_hibernation_ops a
.prepare = acpi_pm_prepare,
.enter = acpi_hibernation_enter,
.leave = acpi_hibernation_leave,
- .pre_restore = acpi_pm_disable_gpes,
+ .pre_restore = acpi_pm_freeze,
.restore_cleanup = acpi_pm_enable_gpes,
};
@@ -598,12 +604,9 @@ static int acpi_hibernation_begin_old(vo
static int acpi_hibernation_pre_snapshot_old(void)
{
- int error = acpi_pm_disable_gpes();
-
- if (!error)
- hibernate_nvs_save();
-
- return error;
+ acpi_pm_freeze();
+ hibernate_nvs_save();
+ return 0;
}
/*
@@ -615,10 +618,10 @@ static struct platform_hibernation_ops a
.end = acpi_pm_end,
.pre_snapshot = acpi_hibernation_pre_snapshot_old,
.finish = acpi_hibernation_finish,
- .prepare = acpi_pm_disable_gpes,
+ .prepare = acpi_pm_freeze,
.enter = acpi_hibernation_enter,
.leave = acpi_hibernation_leave,
- .pre_restore = acpi_pm_disable_gpes,
+ .pre_restore = acpi_pm_freeze,
.restore_cleanup = acpi_pm_enable_gpes,
.recover = acpi_pm_finish,
};
On Wednesday 03 February 2010, Rafael J. Wysocki wrote:
> On Sunday 31 January 2010, Rafael J. Wysocki wrote:
> > Hi,
> >
> > While Maxim is testing if the patch below helps with
> > http://bugzilla.kernel.org/show_bug.cgi?id=14668
> > I think it's necessary anyway.
> >
> > The problem is that currently there's nothing to prevent us from suspending in
> > the middle of an EC transaction in progress, at least as far as I can see.
> > As a result, we can suspend with the ACPI global lock held or something like
> > this, which leads to problems especially for hibernation (if the resume kernel
> > passes control to the image kernel in the middle of an EC transaction, things
> > aren't nice). For this reason I think we should wait until there are no EC
> > transactions in progress before we suspend and we should prevent any new
> > EC transactions from starting after that point. The patch below does that.
>
> After discussing the patch with Alex I have a new version which is appended
> below. This one causes EC transactions to be discarded after we've executed
> _PTS (which is reasonable IMO) and allows them to happen again very early
> during wake-up, so effectively the resume behavior should be unchanged.
Unfortunately I overlooked the fact that this patch would break the error paths
and test modes where we also have to allow EC transactions to happen again
after they've been "suspended". Sorry about that.
Updated patch follows.
Rafael
---
From: Rafael J. Wysocki <[email protected]>
Subject: ACPI / EC: Remove race between EC driver and suspend process (rev. 3)
There is a race between the suspend process and the EC driver that
may result in suspending in the middle of an EC transaction in
progress, which in turn may lead to unpredictable behavior of the
platform.
To remove that race condition, add a helpers for suspending and
resuming EC transactions in a safe way to be executed by the ACPI
platform suspend/hibernate callbacks. Modify these callbacks so
that the EC transactions are suspended right after executing the
_PTS global control method and are allowed to happen right after
the low-level wake-up.
Signed-off-by: Rafael J. Wysocki <[email protected]>
Reported-by: Maxim Levitsky <[email protected]>
---
drivers/acpi/ec.c | 45 ++++++++++++++++++++++++++++++++++++++++++++-
drivers/acpi/internal.h | 3 +++
drivers/acpi/sleep.c | 47 +++++++++++++++++++++++++++++------------------
3 files changed, 76 insertions(+), 19 deletions(-)
Index: linux-2.6/drivers/acpi/ec.c
===================================================================
--- linux-2.6.orig/drivers/acpi/ec.c
+++ linux-2.6/drivers/acpi/ec.c
@@ -76,8 +76,9 @@ enum ec_command {
enum {
EC_FLAGS_QUERY_PENDING, /* Query is pending */
EC_FLAGS_GPE_STORM, /* GPE storm detected */
- EC_FLAGS_HANDLERS_INSTALLED /* Handlers for GPE and
+ EC_FLAGS_HANDLERS_INSTALLED, /* Handlers for GPE and
* OpReg are installed */
+ EC_FLAGS_SUSPENDED, /* Driver is suspended */
};
/* If we find an EC via the ECDT, we need to keep a ptr to its context */
@@ -291,6 +292,12 @@ static int acpi_ec_transaction(struct ac
if (t->rdata)
memset(t->rdata, 0, t->rlen);
mutex_lock(&ec->lock);
+ if (test_bit(EC_FLAGS_SUSPENDED, &ec->flags)) {
+ pr_err(PREFIX
+ "transaction discarded due to power transition\n");
+ status = -EINVAL;
+ goto unlock;
+ }
if (ec->global_lock) {
status = acpi_acquire_global_lock(ACPI_EC_UDELAY_GLK, &glk);
if (ACPI_FAILURE(status)) {
@@ -445,6 +452,42 @@ int ec_transaction(u8 command,
EXPORT_SYMBOL(ec_transaction);
+void acpi_ec_suspend_transactions(void)
+{
+ struct acpi_ec *ec = first_ec;
+
+ if (!ec)
+ return;
+
+ mutex_lock(&ec->lock);
+ /* Prevent transactions from happening while suspended */
+ set_bit(EC_FLAGS_SUSPENDED, &ec->flags);
+ mutex_unlock(&ec->lock);
+}
+
+void acpi_ec_resume_transactions(void)
+{
+ struct acpi_ec *ec = first_ec;
+
+ if (!ec)
+ return;
+
+ mutex_lock(&ec->lock);
+ /* Allow transactions to happen again */
+ clear_bit(EC_FLAGS_SUSPENDED, &ec->flags);
+ mutex_unlock(&ec->lock);
+}
+
+void acpi_ec_resume_transactions_noirq(void)
+{
+ /*
+ * Allow transactions to happen again (this function is called from
+ * atomic context during wake-up, so we don't need to acquire the mutex)
+ */
+ if (first_ec)
+ clear_bit(EC_FLAGS_SUSPENDED, &first_ec->flags);
+}
+
static int acpi_ec_query_unlocked(struct acpi_ec *ec, u8 * data)
{
int result;
Index: linux-2.6/drivers/acpi/internal.h
===================================================================
--- linux-2.6.orig/drivers/acpi/internal.h
+++ linux-2.6/drivers/acpi/internal.h
@@ -49,6 +49,9 @@ void acpi_early_processor_set_pdc(void);
int acpi_ec_init(void);
int acpi_ec_ecdt_probe(void);
int acpi_boot_ec_enable(void);
+void acpi_ec_suspend_transactions(void);
+void acpi_ec_resume_transactions(void);
+void acpi_ec_resume_transactions_noirq(void);
/*--------------------------------------------------------------------------
Suspend/Resume
Index: linux-2.6/drivers/acpi/sleep.c
===================================================================
--- linux-2.6.orig/drivers/acpi/sleep.c
+++ linux-2.6/drivers/acpi/sleep.c
@@ -110,11 +110,12 @@ void __init acpi_old_suspend_ordering(vo
}
/**
- * acpi_pm_disable_gpes - Disable the GPEs.
+ * acpi_pm_freeze - Disable the GPEs and suspend EC transactions.
*/
-static int acpi_pm_disable_gpes(void)
+static int acpi_pm_freeze(void)
{
acpi_disable_all_gpes();
+ acpi_ec_suspend_transactions();
return 0;
}
@@ -142,7 +143,8 @@ static int acpi_pm_prepare(void)
int error = __acpi_pm_prepare();
if (!error)
- acpi_disable_all_gpes();
+ acpi_pm_freeze();
+
return error;
}
@@ -276,6 +278,8 @@ static int acpi_suspend_enter(suspend_st
*/
acpi_disable_all_gpes();
+ acpi_ec_resume_transactions_noirq();
+
local_irq_restore(flags);
printk(KERN_DEBUG "Back to C!\n");
@@ -286,6 +290,12 @@ static int acpi_suspend_enter(suspend_st
return ACPI_SUCCESS(status) ? 0 : -EFAULT;
}
+static void acpi_suspend_finish(void)
+{
+ acpi_ec_resume_transactions();
+ acpi_pm_finish();
+}
+
static int acpi_suspend_state_valid(suspend_state_t pm_state)
{
u32 acpi_state;
@@ -307,7 +317,7 @@ static struct platform_suspend_ops acpi_
.begin = acpi_suspend_begin,
.prepare_late = acpi_pm_prepare,
.enter = acpi_suspend_enter,
- .wake = acpi_pm_finish,
+ .wake = acpi_suspend_finish,
.end = acpi_pm_end,
};
@@ -333,9 +343,9 @@ static int acpi_suspend_begin_old(suspen
static struct platform_suspend_ops acpi_suspend_ops_old = {
.valid = acpi_suspend_state_valid,
.begin = acpi_suspend_begin_old,
- .prepare_late = acpi_pm_disable_gpes,
+ .prepare_late = acpi_pm_freeze,
.enter = acpi_suspend_enter,
- .wake = acpi_pm_finish,
+ .wake = acpi_suspend_finish,
.end = acpi_pm_end,
.recover = acpi_pm_finish,
};
@@ -522,6 +532,7 @@ static int acpi_hibernation_enter(void)
status = acpi_enter_sleep_state(ACPI_STATE_S4);
/* Reprogram control registers and execute _BFS */
acpi_leave_sleep_state_prep(ACPI_STATE_S4);
+ acpi_ec_resume_transactions_noirq();
local_irq_restore(flags);
return ACPI_SUCCESS(status) ? 0 : -EFAULT;
@@ -530,6 +541,7 @@ static int acpi_hibernation_enter(void)
static void acpi_hibernation_finish(void)
{
hibernate_nvs_free();
+ acpi_ec_resume_transactions();
acpi_pm_finish();
}
@@ -550,10 +562,12 @@ static void acpi_hibernation_leave(void)
}
/* Restore the NVS memory area */
hibernate_nvs_restore();
+ acpi_ec_resume_transactions_noirq();
}
-static void acpi_pm_enable_gpes(void)
+static void acpi_pm_thaw(void)
{
+ acpi_ec_resume_transactions();
acpi_enable_all_runtime_gpes();
}
@@ -565,8 +579,8 @@ static struct platform_hibernation_ops a
.prepare = acpi_pm_prepare,
.enter = acpi_hibernation_enter,
.leave = acpi_hibernation_leave,
- .pre_restore = acpi_pm_disable_gpes,
- .restore_cleanup = acpi_pm_enable_gpes,
+ .pre_restore = acpi_pm_freeze,
+ .restore_cleanup = acpi_pm_thaw,
};
/**
@@ -598,12 +612,9 @@ static int acpi_hibernation_begin_old(vo
static int acpi_hibernation_pre_snapshot_old(void)
{
- int error = acpi_pm_disable_gpes();
-
- if (!error)
- hibernate_nvs_save();
-
- return error;
+ acpi_pm_freeze();
+ hibernate_nvs_save();
+ return 0;
}
/*
@@ -615,11 +626,11 @@ static struct platform_hibernation_ops a
.end = acpi_pm_end,
.pre_snapshot = acpi_hibernation_pre_snapshot_old,
.finish = acpi_hibernation_finish,
- .prepare = acpi_pm_disable_gpes,
+ .prepare = acpi_pm_freeze,
.enter = acpi_hibernation_enter,
.leave = acpi_hibernation_leave,
- .pre_restore = acpi_pm_disable_gpes,
- .restore_cleanup = acpi_pm_enable_gpes,
+ .pre_restore = acpi_pm_freeze,
+ .restore_cleanup = acpi_pm_thaw,
.recover = acpi_pm_finish,
};
#endif /* CONFIG_HIBERNATION */