ACPI: Protection from suspending in the middle of EC transaction
RFC PATCH 1/1 ACPI: Protection from suspending in the middle of EC transaction
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(-)
drivers/acpi/ec.c +16 -1
@@ -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 acif (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 ; }
Quoted 6 lines · Rafael J. Wysocki
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.
Quoted 11 lines
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
Quoted 6 lines · Rafael J. Wysocki
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.
Quoted 11 lines
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
Quoted 7 lines · Alan Jenkins
Quoted 6 lines · Rafael J. Wysocki
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.
Quoted 11 lines
Quoted 11 lines
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
Quoted 7 lines · Alan Jenkins
Quoted 6 lines · Rafael J. Wysocki
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.
Quoted 11 lines
Quoted 11 lines
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
Quoted 3 lines · Maxim Levitsky
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.
...
Quoted 3 lines
I think that not only
_PTSans_WAKare 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.
Signature · 4 lines
"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
Quoted 3 lines · Maxim Levitsky
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.
...
Quoted 3 lines
I think that not only
_PTSans_WAKare 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.
Signature · 4 lines
"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 пишет:
Quoted 5 lines · Maxim Levitsky
Quoted 3 lines
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.
Quoted 8 lines
...
Quoted 3 lines
I think that not only
_PTSans_WAKare 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...
Quoted 5 lines
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.
--
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
Quoted 8 lines · Alexey Starikovskiy
Henrique de Moraes Holschuh пишет:
Quoted 7 lines · Maxim Levitsky
Quoted 3 lines
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.
...
Quoted 8 lines
Quoted 6 lines
Quoted 3 lines
I think that not only
_PTSans_WAKare 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?
Quoted 7 lines
Quoted 4 lines
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
--
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
PATCH 1/1 ACPI / EC: Remover race between EC driver and suspend process (was: Re: [RFC][RFT][PATCH] ACPI: Protection from suspending in the middle of EC transaction)
Quoted 38 lines · Rafael J. Wysocki
Quoted 8 lines · Alexey Starikovskiy
Henrique de Moraes Holschuh пишет:
Quoted 7 lines · Maxim Levitsky
Quoted 3 lines
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.
...
Quoted 8 lines
Quoted 6 lines
Quoted 3 lines
I think that not only
_PTSans_WAKare 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?
Quoted 7 lines
Quoted 4 lines
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.
Quoted 6 lines
So, perhaps instead we should disable the GPEs and call
_PTSbefore the late suspend phase (and analogously, execute_WAKafter 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.
--- drivers/acpi/ec.c | 33 ++++++++++++++++++++++++++++++++- drivers/acpi/internal.h | 2 ++ drivers/acpi/sleep.c | 41 ++++++++++++++++++++++++----------------- 3 files changed, 58 insertions(+), 18 deletions(-)
drivers/acpi/ec.c +32 -1
@@ -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 acif (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;
drivers/acpi/internal.h +2 -0
@@ -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
drivers/acpi/sleep.c +24 -17
107 lines not on this page · raw message
--
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
PATCH 1/1 ACPI / EC: Remover race between EC driver and suspend process (was: Re: [RFC][RFT][PATCH] ACPI: Protection from suspending in the middle of EC transaction)
In reply to Rafael J. WysockiQuoted 38 lines · Rafael J. Wysocki
Quoted 8 lines · Alexey Starikovskiy
Henrique de Moraes Holschuh пишет:
Quoted 7 lines · Maxim Levitsky
Quoted 3 lines
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.
...
Quoted 8 lines
Quoted 6 lines
Quoted 3 lines
I think that not only
_PTSans_WAKare 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?
Quoted 7 lines
Quoted 4 lines
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.
Quoted 6 lines
So, perhaps instead we should disable the GPEs and call
_PTSbefore the late suspend phase (and analogously, execute_WAKafter 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.
--- drivers/acpi/ec.c | 33 ++++++++++++++++++++++++++++++++- drivers/acpi/internal.h | 2 ++ drivers/acpi/sleep.c | 41 ++++++++++++++++++++++++----------------- 3 files changed, 58 insertions(+), 18 deletions(-)
drivers/acpi/ec.c +32 -1
@@ -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 acif (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;
drivers/acpi/internal.h +2 -0
@@ -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
drivers/acpi/sleep.c +24 -17
107 lines not on this page · raw message
_______________________________________________
linux-pm mailing list
[email protected]
https://lists.linux-foundation.org/mailman/listinfo/linux-pm
Quoted 8 lines · Alexey Starikovskiy
Henrique de Moraes Holschuh пишет:
Quoted 7 lines · Maxim Levitsky
Quoted 3 lines
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.
...
Quoted 8 lines
Quoted 6 lines
Quoted 3 lines
I think that not only
_PTSans_WAKare 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?
Quoted 7 lines
Quoted 4 lines
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
_______________________________________________
linux-pm mailing list
[email protected]
https://lists.linux-foundation.org/mailman/listinfo/linux-pm
Henrique de Moraes Holschuh пишет:
Quoted 5 lines · Maxim Levitsky
Quoted 3 lines
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.
Quoted 8 lines
...
Quoted 3 lines
I think that not only
_PTSans_WAKare 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...
Quoted 5 lines
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.
_______________________________________________
linux-pm mailing list
[email protected]
https://lists.linux-foundation.org/mailman/listinfo/linux-pm
Quoted 75 lines · Rafael J. Wysocki
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
_PTSto run, so it might be necessary to do that later. On the other hand, the mechanics behind the ACPI global lock, which is acquired inacpi_ec_transaction(), requires that interrupts work, because otherwise there may be a problem if the global lock is not actually acquired afterACPI_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
===================================================================drivers/acpi/ec.c +11
-1@@ -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 acif (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 ; }
drivers/acpi/ec.c error: · line 1085
Quoted 10 lines
drivers/acpi/ec.c +4
-0static 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.
Quoted 8 lines
drivers/acpi/ec.c +1
-1+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
Quoted 75 lines · Rafael J. Wysocki
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
_PTSto run, so it might be necessary to do that later. On the other hand, the mechanics behind the ACPI global lock, which is acquired inacpi_ec_transaction(), requires that interrupts work, because otherwise there may be a problem if the global lock is not actually acquired afterACPI_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
===================================================================drivers/acpi/ec.c +11
-1@@ -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 acif (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 ; }
drivers/acpi/ec.c error: · line 1085
Quoted 10 lines
drivers/acpi/ec.c +4
-0static 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.
Quoted 8 lines
drivers/acpi/ec.c +1
-1+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
Quoted 1 line · Maxim Levitsky
On Sun, 2010-01-31 at 00:29 +0100, Rafael J. Wysocki wrote:
...
Quoted 11 lines
drivers/acpi/ec.c error: · line 1085
Quoted 10 lines
static int
acpi_ec_resume(struct acpi_device *device){drivers/acpi/ec.c +4
-0struct 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);^^^^^^^^^^^^
Quoted 2 lines
Thats why it doesn't work here....
Will retest now.
Ouch, sorry.
Rafael
Quoted 17 lines · Rafael J. Wysocki
Quoted 1 line · Maxim Levitsky
On Sun, 2010-01-31 at 00:29 +0100, Rafael J. Wysocki wrote:
...
Quoted 11 lines
Review
Quoted 10 lines
static int
acpi_ec_resume(struct acpi_device *device){(quoted diff) +4
-0struct 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);^^^^^^^^^^^^Quoted 2 lines
Thats why it doesn't work here....
Will retest now.
Quoted 1 line
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.
--- drivers/acpi/ec.c | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-)
drivers/acpi/ec.c +16 -1
@@ -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 acif (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 ; }
Quoted 17 lines · Rafael J. Wysocki
Quoted 1 line · Maxim Levitsky
On Sun, 2010-01-31 at 00:29 +0100, Rafael J. Wysocki wrote:
...
Quoted 11 lines
Review
Quoted 10 lines
static int
acpi_ec_resume(struct acpi_device *device){(quoted diff) +4
-0struct 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);^^^^^^^^^^^^Quoted 2 lines
Thats why it doesn't work here....
Will retest now.
Quoted 1 line
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.
--- drivers/acpi/ec.c | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-)
drivers/acpi/ec.c +16 -1
@@ -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 acif (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 ; }
Quoted 1 line · Maxim Levitsky
On Sun, 2010-01-31 at 00:29 +0100, Rafael J. Wysocki wrote:
...
Quoted 11 lines
drivers/acpi/ec.c error: · line 1085
Quoted 10 lines
static int
acpi_ec_resume(struct acpi_device *device){drivers/acpi/ec.c +4
-0struct 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);^^^^^^^^^^^^
Quoted 2 lines
Thats why it doesn't work here....
Will retest now.
Ouch, sorry.
Rafael
PATCH 1/1 ACPI / EC: Remove race between EC driver and suspend process (rev. 2) (was: Re: [RFC][RFT][PATCH] ACPI: Protection from suspending in the middle of EC transaction)
In reply to Rafael J. WysockiQuoted 6 lines · Rafael J. Wysocki
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.
Quoted 8 lines
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.
--- drivers/acpi/ec.c | 32 +++++++++++++++++++++++++++++++- drivers/acpi/internal.h | 2 ++ drivers/acpi/sleep.c | 29 ++++++++++++++++------------- 3 files changed, 49 insertions(+), 14 deletions(-)
drivers/acpi/ec.c +31 -1
@@ -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 acif (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;
drivers/acpi/internal.h +2 -0
@@ -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
drivers/acpi/sleep.c +16 -13
@@ -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(suspenstatic 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(vostatic 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, };
PATCH 1/1 ACPI / EC: Remove race between EC driver and suspend process (rev. 3)
Quoted 16 lines · Rafael J. Wysocki
Quoted 6 lines · Rafael J. Wysocki
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.Quoted 8 lines
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.
Quoted 4 lines
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.
--- drivers/acpi/ec.c | 45 ++++++++++++++++++++++++++++++++++++++++++++- drivers/acpi/internal.h | 3 +++ drivers/acpi/sleep.c | 47 +++++++++++++++++++++++++++++------------------ 3 files changed, 76 insertions(+), 19 deletions(-)
drivers/acpi/ec.c +44 -1
@@ -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 acif (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;
drivers/acpi/internal.h +3 -0
@@ -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
drivers/acpi/sleep.c +29 -18
128 lines not on this page · raw message
PATCH 1/1 ACPI / EC: Remove race between EC driver and suspend process (rev. 3)
In reply to Rafael J. WysockiQuoted 16 lines · Rafael J. Wysocki
Quoted 6 lines · Rafael J. Wysocki
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.Quoted 8 lines
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.
Quoted 4 lines
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.
--- drivers/acpi/ec.c | 45 ++++++++++++++++++++++++++++++++++++++++++++- drivers/acpi/internal.h | 3 +++ drivers/acpi/sleep.c | 47 +++++++++++++++++++++++++++++------------------ 3 files changed, 76 insertions(+), 19 deletions(-)
drivers/acpi/ec.c +44 -1
@@ -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 acif (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;
drivers/acpi/internal.h +3 -0
@@ -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
drivers/acpi/sleep.c +29 -18
128 lines not on this page · raw message
PATCH 1/1 ACPI / EC: Remove race between EC driver and suspend process (rev. 2) (was: Re: [RFC][RFT][PATCH] ACPI: Protection from suspending in the middle of EC transaction)
In reply to Rafael J. WysockiQuoted 6 lines · Rafael J. Wysocki
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.
Quoted 8 lines
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.
--- drivers/acpi/ec.c | 32 +++++++++++++++++++++++++++++++- drivers/acpi/internal.h | 2 ++ drivers/acpi/sleep.c | 29 ++++++++++++++++------------- 3 files changed, 49 insertions(+), 14 deletions(-)
drivers/acpi/ec.c +31 -1
@@ -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 acif (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;
drivers/acpi/internal.h +2 -0
@@ -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
drivers/acpi/sleep.c +16 -13
@@ -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(suspenstatic 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(vostatic 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, };