linux-stable.git/drivers/acpi/ec.c, branch linux-4.2.y Linux kernel stable tree ACPI / EC: Fix a code coverity issue when QR_EC transactions are failed. 2015-06-15T12:35:59+00:00 Lv Zheng lv.zheng@intel.com 2015-06-11T05:21:51+00:00 66db383439b51b1aa920f3579da644fb5fdb2b7c When the QR_EC transaction fails, the EC_FLAGS_QUERY_PENDING flag prevents the event handling work queue from being scheduled again. Though there shouldn't be failed QR_EC transactions, and this gap was efficiently used for catching and learning the SCI_EVT clearing timing compliance issues, we need to fix this as we are not fully compatible with all platforms/Windows to handle SCI_EVT clearing timing correctly. Fixing this gives the EC driver the chances to recover from a state machine failure. So this patch fixes this issue. When nr_pending_queries drops to 0, it clears EC_FLAGS_QUERY_PENDING at the proper position for different modes in order to ensure that the SCI_EVT handling can proceed. In order to be clearer for future ec_event_clearing modes, all checks in this patch are written in the inclusive style, not the exclusive style. Cc: 3.16+ <stable@vger.kernel.org> # 3.16+ Signed-off-by: Lv Zheng <lv.zheng@intel.com> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> 
When the QR_EC transaction fails, the EC_FLAGS_QUERY_PENDING flag prevents
the event handling work queue from being scheduled again.

Though there shouldn't be failed QR_EC transactions, and this gap was
efficiently used for catching and learning the SCI_EVT clearing timing
compliance issues, we need to fix this as we are not fully compatible
with all platforms/Windows to handle SCI_EVT clearing timing correctly.
Fixing this gives the EC driver the chances to recover from a state machine
failure.

So this patch fixes this issue. When nr_pending_queries drops to 0, it
clears EC_FLAGS_QUERY_PENDING at the proper position for different modes in
order to ensure that the SCI_EVT handling can proceed.

In order to be clearer for future ec_event_clearing modes, all checks in
this patch are written in the inclusive style, not the exclusive style.

Cc: 3.16+ <stable@vger.kernel.org> # 3.16+
Signed-off-by: Lv Zheng <lv.zheng@intel.com>
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
ACPI / EC: Fix EC_FLAGS_QUERY_HANDSHAKE platforms using new event clearing timing. 2015-06-15T12:35:59+00:00 Lv Zheng lv.zheng@intel.com 2015-06-11T05:21:45+00:00 3cb02aeb28dd3f1b8a132fa3ecf6db17afd518d6 It is reported that on several platforms, EC firmware will not respond non-expected QR_EC (see EC_FLAGS_QUERY_HANDSHAKE, only write QR_EC when SCI_EVT is set). Unfortunately, ACPI specification doesn't define when the SCI_EVT should be cleared by the firmware, thus the original implementation queued up second QR_EC right after writing QR_EC command and before reading the returned event value as at that time the SCI_EVT is ensured not cleared. This behavior is also based on the assumption that the firmware should be able to return 0x00 to indicate "no outstanding event". This behavior did fix issues on Samsung platforms where the spurious query value of 0x00 is supported and didn't break platforms in my test queue. But recently, specific Acer, Asus, Lenovo platforms keep on blaming this change. This patch changes the behavior to re-check the SCI_EVT a bit later and removes EC_FLAGS_QUERY_HANDSHAKE quirks, hoping this is the Windows compliant EC driver behavior. In order to be robust to the possible regressions, instead of removing the quirk directly, this patch keeps the quirk code, removes the quirk users and keeps old behavior for Samsung platforms. Cc: 3.16+ <stable@vger.kernel.org> # 3.16+ Link: https://bugzilla.kernel.org/show_bug.cgi?id=94411 Link: https://bugzilla.kernel.org/show_bug.cgi?id=97381 Link: https://bugzilla.kernel.org/show_bug.cgi?id=98111 Reported-and-tested-by: Gabriele Mazzotta <gabriele.mzt@gmail.com> Reported-and-tested-by: Tigran Gabrielyan <tigrangab@gmail.com> Reported-and-tested-by: Adrien D <ghbdtn@openmailbox.org> Signed-off-by: Lv Zheng <lv.zheng@intel.com> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> 
It is reported that on several platforms, EC firmware will not respond
non-expected QR_EC (see EC_FLAGS_QUERY_HANDSHAKE, only write QR_EC when
SCI_EVT is set).

Unfortunately, ACPI specification doesn't define when the SCI_EVT should be
cleared by the firmware, thus the original implementation queued up second
QR_EC right after writing QR_EC command and before reading the returned
event value as at that time the SCI_EVT is ensured not cleared. This
behavior is also based on the assumption that the firmware should be able
to return 0x00 to indicate "no outstanding event". This behavior did fix
issues on Samsung platforms where the spurious query value of 0x00 is
supported and didn't break platforms in my test queue.

But recently, specific Acer, Asus, Lenovo platforms keep on blaming this
change.

This patch changes the behavior to re-check the SCI_EVT a bit later and
removes EC_FLAGS_QUERY_HANDSHAKE quirks, hoping this is the Windows
compliant EC driver behavior.

In order to be robust to the possible regressions, instead of removing the
quirk directly, this patch keeps the quirk code, removes the quirk users
and keeps old behavior for Samsung platforms.

Cc: 3.16+ <stable@vger.kernel.org> # 3.16+
Link: https://bugzilla.kernel.org/show_bug.cgi?id=94411
Link: https://bugzilla.kernel.org/show_bug.cgi?id=97381
Link: https://bugzilla.kernel.org/show_bug.cgi?id=98111
Reported-and-tested-by: Gabriele Mazzotta <gabriele.mzt@gmail.com>
Reported-and-tested-by: Tigran Gabrielyan <tigrangab@gmail.com>
Reported-and-tested-by: Adrien D <ghbdtn@openmailbox.org>
Signed-off-by: Lv Zheng <lv.zheng@intel.com>
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
ACPI / EC: Add event clearing variation support. 2015-06-15T12:35:59+00:00 Lv Zheng lv.zheng@intel.com 2015-06-11T05:21:38+00:00 1d68d2612c2e7309166fa43d8e27eb163435527f We've been suffering from the uncertainty of the SCI_EVT clearing timing. This patch implements 3 of 4 possible modes to handle SCI_EVT clearing variations. The old behavior is kept in this patch. Status: QR_EC is re-checked as early as possible after checking previous SCI_EVT. This always leads to 2 QR_EC transactions per SCI_EVT indication and the target may implement event queue which returns 0x00 indicating "no outstanding event". This is proven to be a conflict against Windows behavior, but is still kept in this patch to make the EC driver robust to the possible regressions that may occur on Samsung platforms. Query: QR_EC is re-checked after the target has handled the QR_EC query request command pushed by the host. Event: QR_EC is re-checked after the target has noticed the query event response data pulled by the host. This timing is not determined by any IRQs, so we may need to use a guard period in this mode, which may explain the existence of the ec_guard() code used by the old EC driver where the re-check timing is implemented in the similar way as this mode. Method: QR_EC is re-checked as late as possible after completing the _Qxx evaluation. The target may implement SCI_EVT like a level triggered interrupt. It is proven on kernel bugzilla 94411 that, Windows will have all _Qxx evaluations parallelized. Thus unless required by further evidences, we needn't implement this mode as it is a conflict of the _Qxx parallelism requirement. Note that, according to the reports, there are platforms that cannot be handled using the "Status" mode without enabling the EC_FLAGS_QUERY_HANDSHAKE quirk. But they can be handled with the other modes according to the tests (kernel bugzilla 97381). The following log entry can be used to confirm the differences of the 3 modes as it should appear at the different positions for the 3 modes: Command(QR_EC) unblocked Status: appearing after EC_SC(W) = 0x84 Query: appearing after EC_DATA(R) = 0xXX where XX is the event number used to determine _QXX Event: appearing after first EC_SC(R) = 0xX0 SCI_EVT=x BURST=0 CMD=0 IBF=0 OBF=0 that is next to the following log entry: Command(QR_EC) completed by hardware Link: https://bugzilla.kernel.org/show_bug.cgi?id=94411 Link: https://bugzilla.kernel.org/show_bug.cgi?id=97381 Link: https://bugzilla.kernel.org/show_bug.cgi?id=98111 Reported-and-tested-by: Gabriele Mazzotta <gabriele.mzt@gmail.com> Reported-and-tested-by: Tigran Gabrielyan <tigrangab@gmail.com> Reported-and-tested-by: Adrien D <ghbdtn@openmailbox.org> Signed-off-by: Lv Zheng <lv.zheng@intel.com> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> 
We've been suffering from the uncertainty of the SCI_EVT clearing timing.
This patch implements 3 of 4 possible modes to handle SCI_EVT clearing
variations. The old behavior is kept in this patch.

Status: QR_EC is re-checked as early as possible after checking previous
        SCI_EVT. This always leads to 2 QR_EC transactions per SCI_EVT
        indication and the target may implement event queue which returns
        0x00 indicating "no outstanding event".
        This is proven to be a conflict against Windows behavior, but is
        still kept in this patch to make the EC driver robust to the
        possible regressions that may occur on Samsung platforms.
Query: QR_EC is re-checked after the target has handled the QR_EC query
       request command pushed by the host.
Event: QR_EC is re-checked after the target has noticed the query event
       response data pulled by the host.
       This timing is not determined by any IRQs, so we may need to use a
       guard period in this mode, which may explain the existence of the
       ec_guard() code used by the old EC driver where the re-check timing
       is implemented in the similar way as this mode.
Method: QR_EC is re-checked as late as possible after completing the _Qxx
        evaluation. The target may implement SCI_EVT like a level triggered
        interrupt.
        It is proven on kernel bugzilla 94411 that, Windows will have all
        _Qxx evaluations parallelized. Thus unless required by further
        evidences, we needn't implement this mode as it is a conflict of
        the _Qxx parallelism requirement.

Note that, according to the reports, there are platforms that cannot be
handled using the "Status" mode without enabling the
EC_FLAGS_QUERY_HANDSHAKE quirk. But they can be handled with the other
modes according to the tests (kernel bugzilla 97381).

The following log entry can be used to confirm the differences of the 3
modes as it should appear at the different positions for the 3 modes:
  Command(QR_EC) unblocked
Status: appearing after
         EC_SC(W) = 0x84
Query: appearing after
         EC_DATA(R) = 0xXX
       where XX is the event number used to determine _QXX
Event: appearing after first
         EC_SC(R) = 0xX0 SCI_EVT=x BURST=0 CMD=0 IBF=0 OBF=0
       that is next to the following log entry:
         Command(QR_EC) completed by hardware

Link: https://bugzilla.kernel.org/show_bug.cgi?id=94411
Link: https://bugzilla.kernel.org/show_bug.cgi?id=97381
Link: https://bugzilla.kernel.org/show_bug.cgi?id=98111
Reported-and-tested-by: Gabriele Mazzotta <gabriele.mzt@gmail.com>
Reported-and-tested-by: Tigran Gabrielyan <tigrangab@gmail.com>
Reported-and-tested-by: Adrien D <ghbdtn@openmailbox.org>
Signed-off-by: Lv Zheng <lv.zheng@intel.com>
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
ACPI / EC: Convert event handling work queue into loop style. 2015-06-15T12:35:59+00:00 Lv Zheng lv.zheng@intel.com 2015-06-11T05:21:32+00:00 9d8993be2d9149bc8b3132dad030ff5960f5abcc During the period that a work queue is scheduled (queued up for run) but hasn't been run, second schedule_work() could fail. This may not lead to the loss of queries because QR_EC is always ensured to be submitted after the work queue has been in the running state. The event handling work queue can be changed into the loop style to allow us to control the code in a more flexible way: 1. Makes it possible to add event=0x00 termination condition in the loop. 2. Increases the thoughput of the QR_EC transactions as the 2nd+ QR_EC transactions may be handled in the same work item used for the 1st QR_EC transaction, thus the delay caused by the 2nd+ work item scheduling can be eliminated. Except the logging message changes and the throughput improvement, this patch is just a funcitonal no-op. Signed-off-by: Lv Zheng <lv.zheng@intel.com> Tested-by: Gabriele Mazzotta <gabriele.mzt@gmail.com> Tested-by: Tigran Gabrielyan <tigrangab@gmail.com> Tested-by: Adrien D <ghbdtn@openmailbox.org> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> 
During the period that a work queue is scheduled (queued up for run) but
hasn't been run, second schedule_work() could fail. This may not lead to
the loss of queries because QR_EC is always ensured to be submitted after
the work queue has been in the running state.

The event handling work queue can be changed into the loop style to allow
us to control the code in a more flexible way:
1. Makes it possible to add event=0x00 termination condition in the loop.
2. Increases the thoughput of the QR_EC transactions as the 2nd+ QR_EC
   transactions may be handled in the same work item used for the 1st QR_EC
   transaction, thus the delay caused by the 2nd+ work item scheduling can
   be eliminated.

Except the logging message changes and the throughput improvement, this
patch is just a funcitonal no-op.

Signed-off-by: Lv Zheng <lv.zheng@intel.com>
Tested-by: Gabriele Mazzotta <gabriele.mzt@gmail.com>
Tested-by: Tigran Gabrielyan <tigrangab@gmail.com>
Tested-by: Adrien D <ghbdtn@openmailbox.org>
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
ACPI / EC: Cleanup transaction state transition. 2015-06-15T12:35:58+00:00 Lv Zheng lv.zheng@intel.com 2015-06-11T05:21:23+00:00 f8b8eb71533338654f39211a87efeca35055566b This patch collects transaction state transition code into one function. We then could have a single function to maintain transaction transition related behaviors. No functional changes. Signed-off-by: Lv Zheng <lv.zheng@intel.com> Tested-by: Gabriele Mazzotta <gabriele.mzt@gmail.com> Tested-by: Tigran Gabrielyan <tigrangab@gmail.com> Tested-by: Adrien D <ghbdtn@openmailbox.org> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> 
This patch collects transaction state transition code into one function. We
then could have a single function to maintain transaction transition
related behaviors. No functional changes.

Signed-off-by: Lv Zheng <lv.zheng@intel.com>
Tested-by: Gabriele Mazzotta <gabriele.mzt@gmail.com>
Tested-by: Tigran Gabrielyan <tigrangab@gmail.com>
Tested-by: Adrien D <ghbdtn@openmailbox.org>
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
ACPI / EC: Remove non-root-caused busy polling quirks. 2015-05-15T23:53:03+00:00 Lv Zheng lv.zheng@intel.com 2015-05-15T06:37:11+00:00 3174abcfea6a05aa25038156d6722b6c8876fb36 { Update to correct 1 patch subject in the description } We have fixed a lot of race issues in the EC driver recently. The following commit introduces MSI udelay()/msleep() quirk to MSI laptops to make EC firmware working for bug 12011 without root causing any EC driver race issues: Commit: 5423a0cb3f74c16e90683f8ee1cec6c240a9556e Subject: ACPI: EC: Add delay for slow MSI controller Commit: 34ff4dbccccce54c83b1234d39b7ad9e548a75dd Subject: ACPI: EC: Separate delays for MSI hardware The following commit extends ECDT validation quirk to MSI laptops to make EC driver locating EC registers properly for bug 12461: Commit: a5032bfdd9c80e0231a6324661e123818eb46ecd Subject: ACPI: EC: Always parse EC device This is a different quirk than the MSI udelay()/msleep() quirk. This patch keeps validating ECDT for only "Micro-Star MS-171F" as reported. The following commit extends MSI udelay()/msleep() quirk to Quanta laptops to make EC firmware working for bug 20242, there is no requirement to validate ECDT for Quanta laptops: Commit: 534bc4e3d27096e2f3fc00c14a20efd597837a4f Mon Sep 17 00:00:00 2001 Subject: ACPI EC: enable MSI workaround for Quanta laptops The following commit extends MSI udelay()/msleep() quirk to Clevo laptops to make EC firmware working for bug 77431, there is no requirement to validate ECDT for Clevo laptops: Commit: 777cb382958851c88763253fe00a26529be4c0e9 Subject: ACPI / EC: Add msi quirk for Clevo W350etq All udelay()/msleep() quirks for MSI/Quanta/Clevo seem to be the wrong fixes generated without fixing the EC driver race issues. And even if it is not wrong, the guarding can be covered by the following commits in wait polling mode: Commit: 9e295ac14d6a59180beed0735e6a504c2ee87761 Subject: ACPI / EC: Reduce ec_poll() by referencing the last register access timestamp. Commit: commit in the same series Subject: ACPI / EC: Fix and clean up register access guarding logics. The only case that is not covered is the inter-transaction guarding. And there is no evidence that we need the inter-transaction guarding upon reading the noted bug entries. So it is time to remove the quirks and let the users to try again. If there is a regression, the only thing we need to do is to restore the inter-transaction guarding for the reported platforms. Link: https://bugzilla.kernel.org/show_bug.cgi?id=12011 Link: https://bugzilla.kernel.org/show_bug.cgi?id=12461 Link: https://bugzilla.kernel.org/show_bug.cgi?id=20242 Link: https://bugzilla.kernel.org/show_bug.cgi?id=77431 Signed-off-by: Lv Zheng <lv.zheng@intel.com> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> 
{ Update to correct 1 patch subject in the description }

We have fixed a lot of race issues in the EC driver recently.

The following commit introduces MSI udelay()/msleep() quirk to MSI laptops
to make EC firmware working for bug 12011 without root causing any EC
driver race issues:
  Commit: 5423a0cb3f74c16e90683f8ee1cec6c240a9556e
  Subject: ACPI: EC: Add delay for slow MSI controller
  Commit: 34ff4dbccccce54c83b1234d39b7ad9e548a75dd
  Subject: ACPI: EC: Separate delays for MSI hardware

The following commit extends ECDT validation quirk to MSI laptops to make
EC driver locating EC registers properly for bug 12461:
  Commit: a5032bfdd9c80e0231a6324661e123818eb46ecd
  Subject: ACPI: EC: Always parse EC device
This is a different quirk than the MSI udelay()/msleep() quirk. This patch
keeps validating ECDT for only "Micro-Star MS-171F" as reported.

The following commit extends MSI udelay()/msleep() quirk to Quanta laptops
to make EC firmware working for bug 20242, there is no requirement to
validate ECDT for Quanta laptops:
  Commit: 534bc4e3d27096e2f3fc00c14a20efd597837a4f Mon Sep 17 00:00:00 2001
  Subject: ACPI EC: enable MSI workaround for Quanta laptops

The following commit extends MSI udelay()/msleep() quirk to Clevo laptops
to make EC firmware working for bug 77431, there is no requirement to
validate ECDT for Clevo laptops:
  Commit: 777cb382958851c88763253fe00a26529be4c0e9
  Subject: ACPI / EC: Add msi quirk for Clevo W350etq

All udelay()/msleep() quirks for MSI/Quanta/Clevo seem to be the wrong
fixes generated without fixing the EC driver race issues.
And even if it is not wrong, the guarding can be covered by the following
commits in wait polling mode:
  Commit: 9e295ac14d6a59180beed0735e6a504c2ee87761
  Subject: ACPI / EC: Reduce ec_poll() by referencing the last register access timestamp.
  Commit: commit in the same series
  Subject: ACPI / EC: Fix and clean up register access guarding logics.
The only case that is not covered is the inter-transaction guarding. And
there is no evidence that we need the inter-transaction guarding upon
reading the noted bug entries.

So it is time to remove the quirks and let the users to try again. If there
is a regression, the only thing we need to do is to restore the
inter-transaction guarding for the reported platforms.

Link: https://bugzilla.kernel.org/show_bug.cgi?id=12011
Link: https://bugzilla.kernel.org/show_bug.cgi?id=12461
Link: https://bugzilla.kernel.org/show_bug.cgi?id=20242
Link: https://bugzilla.kernel.org/show_bug.cgi?id=77431
Signed-off-by: Lv Zheng <lv.zheng@intel.com>
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
ACPI / EC: Add module params for polling modes. 2015-05-15T23:51:18+00:00 Lv Zheng lv.zheng@intel.com 2015-05-15T06:16:48+00:00 15de603b04b229b5582fd148fd851801a79472cc We have 2 polling modes in the EC driver: 1. busy polling: originally used for the MSI quirks. udelay() is used to perform register access guarding. 2. wait polling: normal code path uses wait_event_timeout() and it can be woken up as soon as the transaction is completed in the interrupt mode. It also contains the register acces guarding logic in case the interrupt doesn't arrive and the EC driver is about to advance the transaction in task context (the polling mode). The wait polling is useful for interrupt mode to allow other tasks to use the CPU during the wait. But for the polling mode, the busy polling takes less time than the wait polling, because if no interrupt arrives, the wait polling has to wait the minimal HZ interval. We have a new use case for using the busy polling mode. Some GPIO drivers initialize PIN configuration which cause a GPIO multiplexed EC GPE to be disabled out of the GPE register's control. Busy polling mode is useful here as it takes less time than the wait polling. But the guarding logic prevents it from responding even faster. We should spinning around the EC status rather than spinning around the nop execution lasted a determined period. This patch introduces 2 module params for the polling mode switch and the guard time, so that users can use the busy polling mode without the guarding in case the guarding is not necessary. This is an example to use the 2 module params for this purpose: acpi.ec_busy_polling acpi.ec_polling_guard=0 We've tested the patch on a test platform. The platform suffers from such kind of the GPIO PIN issue. The GPIO driver resets all PIN configuration and after that, EC interrupt cannot arrive because of the multiplexing. Then the platform suffers from a long delay carried out by the wait_event_timeout() as all further EC transactions will run in the polling mode. We switched the EC driver to use the busy polling mechanism instead of the wait timeout polling mechanism and the delay is still high: [ 44.283005] calling PNP0C0B:00+ @ 1305, parent: platform [ 44.417548] call PNP0C0B:00+ returned 0 after 131323 usecs And this patch can significantly reduce the delay: [ 44.502625] calling PNP0C0B:00+ @ 1308, parent: platform [ 44.503760] call PNP0C0B:00+ returned 0 after 1103 usecs Tested-by: Chen Yu <yu.c.chen@intel.com> Signed-off-by: Lv Zheng <lv.zheng@intel.com> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> 
We have 2 polling modes in the EC driver:
1. busy polling: originally used for the MSI quirks. udelay() is used to
   perform register access guarding.
2. wait polling: normal code path uses wait_event_timeout() and it can be
   woken up as soon as the transaction is completed in the interrupt mode.
   It also contains the register acces guarding logic in case the interrupt
   doesn't arrive and the EC driver is about to advance the transaction in
   task context (the polling mode).
The wait polling is useful for interrupt mode to allow other tasks to use
the CPU during the wait.
But for the polling mode, the busy polling takes less time than the wait
polling, because if no interrupt arrives, the wait polling has to wait the
minimal HZ interval.

We have a new use case for using the busy polling mode. Some GPIO drivers
initialize PIN configuration which cause a GPIO multiplexed EC GPE to be
disabled out of the GPE register's control. Busy polling mode is useful
here as it takes less time than the wait polling. But the guarding logic
prevents it from responding even faster. We should spinning around the EC
status rather than spinning around the nop execution lasted a determined
period.

This patch introduces 2 module params for the polling mode switch and the
guard time, so that users can use the busy polling mode without the
guarding in case the guarding is not necessary. This is an example to use
the 2 module params for this purpose:
  acpi.ec_busy_polling acpi.ec_polling_guard=0

We've tested the patch on a test platform. The platform suffers from such
kind of the GPIO PIN issue. The GPIO driver resets all PIN configuration
and after that, EC interrupt cannot arrive because of the multiplexing.
Then the platform suffers from a long delay carried out by the
wait_event_timeout() as all further EC transactions will run in the polling
mode. We switched the EC driver to use the busy polling mechanism instead
of the wait timeout polling mechanism and the delay is still high:
[   44.283005] calling  PNP0C0B:00+ @ 1305, parent: platform
[   44.417548] call PNP0C0B:00+ returned 0 after 131323 usecs
And this patch can significantly reduce the delay:
[   44.502625] calling  PNP0C0B:00+ @ 1308, parent: platform
[   44.503760] call PNP0C0B:00+ returned 0 after 1103 usecs

Tested-by: Chen Yu <yu.c.chen@intel.com>
Signed-off-by: Lv Zheng <lv.zheng@intel.com>
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
ACPI / EC: Fix and clean up register access guarding logics. 2015-05-15T23:51:18+00:00 Lv Zheng lv.zheng@intel.com 2015-05-15T06:16:42+00:00 d8d031a605bff183b76611e0d18e2ca7021fb99f In the polling mode, EC driver shouldn't access the EC registers too frequently. Though this statement is concluded from the non-root caused bugs (see links below), we've maintained the register access guarding logics in the current EC driver. The guarding logics can be found here and there, makes it hard to root cause real timing issues. This patch collects the guarding logics into one single function so that all hidden logics related to this can be seen clearly. The current guarding related code also has several issues: 1. Per-transaction timestamp prevents inter-transaction guarding from being implemented in the same place. We have an inter-transaction udelay() in acpi_ec_transaction_unblocked(), this logic can be merged into ec_poll() if we can use per-device timestamp. This patch completes such merge to form a new ec_guard() function and collects all guarding related hidden logics in it. One hidden logic is: there is no inter-transaction guarding performed for non MSI quirk (wait polling mode), this patch skips inter-transaction guarding before wait_event_timeout() for the wait polling mode to reveal the hidden logic. The other hidden logic is: there is msleep() inter-transaction guarding performed when the GPE storming is observed. As after merging this commit: Commit: e1d4d90fc0313d3d58cbd7912c90f8ef24df45ff Subject: ACPI / EC: Refine command storm prevention support EC_FLAGS_COMMAND_STORM is ensured to be cleared after invoking acpi_ec_transaction_unlocked(), the msleep() guard logic will never happen now. Since no one complains such change, this logic is likely added during the old times where the EC race issues are not fixed and the bugs are false root-caused to the timing issue. This patch simply removes the out-dated logic. We can restore it by stop skipping inter-transaction guarding for wait polling mode. Two different delay values are defined for msleep() and udelay() while they are merged in this patch to 550us. 2. time_after() causes additional delay in the polling mode (can only be observed in noirq suspend/resume processes where polling mode is always used) before advance_transaction() is invoked ("wait polling" log is added before wait_event_timeout()). We can see 2 wait_event_timeout() invocations. This is because time_after() ensures a ">" validation while we only need a ">=" validation here: [ 86.739909] ACPI: Waking up from system sleep state S3 [ 86.742857] ACPI : EC: 2: Increase command [ 86.742859] ACPI : EC: ***** Command(RD_EC) started ***** [ 86.742861] ACPI : EC: ===== TASK (0) ===== [ 86.742871] ACPI : EC: EC_SC(R) = 0x20 SCI_EVT=1 BURST=0 CMD=0 IBF=0 OBF=0 [ 86.742873] ACPI : EC: EC_SC(W) = 0x80 [ 86.742876] ACPI : EC: ***** Event started ***** [ 86.742880] ACPI : EC: ~~~~~ wait polling ~~~~~ [ 86.743972] ACPI : EC: ~~~~~ wait polling ~~~~~ [ 86.747966] ACPI : EC: ===== TASK (0) ===== [ 86.747977] ACPI : EC: EC_SC(R) = 0x20 SCI_EVT=1 BURST=0 CMD=0 IBF=0 OBF=0 [ 86.747978] ACPI : EC: EC_DATA(W) = 0x06 [ 86.747981] ACPI : EC: ~~~~~ wait polling ~~~~~ [ 86.751971] ACPI : EC: ~~~~~ wait polling ~~~~~ [ 86.755969] ACPI : EC: ===== TASK (0) ===== [ 86.755991] ACPI : EC: EC_SC(R) = 0x21 SCI_EVT=1 BURST=0 CMD=0 IBF=0 OBF=1 [ 86.755993] ACPI : EC: EC_DATA(R) = 0x03 [ 86.755994] ACPI : EC: ~~~~~ wait polling ~~~~~ [ 86.755995] ACPI : EC: ***** Command(RD_EC) stopped ***** [ 86.755996] ACPI : EC: 1: Decrease command This patch corrects this by using time_before() instead in ec_guard(): [ 54.283146] ACPI: Waking up from system sleep state S3 [ 54.285414] ACPI : EC: 2: Increase command [ 54.285415] ACPI : EC: ***** Command(RD_EC) started ***** [ 54.285416] ACPI : EC: ~~~~~ wait polling ~~~~~ [ 54.285417] ACPI : EC: ===== TASK (0) ===== [ 54.285424] ACPI : EC: EC_SC(R) = 0x20 SCI_EVT=1 BURST=0 CMD=0 IBF=0 OBF=0 [ 54.285425] ACPI : EC: EC_SC(W) = 0x80 [ 54.285427] ACPI : EC: ***** Event started ***** [ 54.285429] ACPI : EC: ~~~~~ wait polling ~~~~~ [ 54.287209] ACPI : EC: ===== TASK (0) ===== [ 54.287218] ACPI : EC: EC_SC(R) = 0x20 SCI_EVT=1 BURST=0 CMD=0 IBF=0 OBF=0 [ 54.287219] ACPI : EC: EC_DATA(W) = 0x06 [ 54.287222] ACPI : EC: ~~~~~ wait polling ~~~~~ [ 54.291190] ACPI : EC: ===== TASK (0) ===== [ 54.291210] ACPI : EC: EC_SC(R) = 0x21 SCI_EVT=1 BURST=0 CMD=0 IBF=0 OBF=1 [ 54.291213] ACPI : EC: EC_DATA(R) = 0x03 [ 54.291214] ACPI : EC: ~~~~~ wait polling ~~~~~ [ 54.291215] ACPI : EC: ***** Command(RD_EC) stopped ***** [ 54.291216] ACPI : EC: 1: Decrease command After cleaning up all guarding logics, we have one single function ec_guard() collecting all old, non-root-caused, hidden logics. Then we can easily tune the logics in one place to respond to the bug reports. Except the time_before() change, all other changes do not change the behavior of the EC driver. Link: https://bugzilla.kernel.org/show_bug.cgi?id=12011 Link: https://bugzilla.kernel.org/show_bug.cgi?id=20242 Link: https://bugzilla.kernel.org/show_bug.cgi?id=77431 Signed-off-by: Lv Zheng <lv.zheng@intel.com> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> 
In the polling mode, EC driver shouldn't access the EC registers too
frequently. Though this statement is concluded from the non-root caused
bugs (see links below), we've maintained the register access guarding
logics in the current EC driver. The guarding logics can be found here and
there, makes it hard to root cause real timing issues. This patch collects
the guarding logics into one single function so that all hidden logics
related to this can be seen clearly.

The current guarding related code also has several issues:
1. Per-transaction timestamp prevents inter-transaction guarding from being
   implemented in the same place. We have an inter-transaction udelay() in
   acpi_ec_transaction_unblocked(), this logic can be merged into ec_poll()
   if we can use per-device timestamp. This patch completes such merge to
   form a new ec_guard() function and collects all guarding related hidden
   logics in it.
   One hidden logic is: there is no inter-transaction guarding performed
   for non MSI quirk (wait polling mode), this patch skips
   inter-transaction guarding before wait_event_timeout() for the wait
   polling mode to reveal the hidden logic.
   The other hidden logic is: there is msleep() inter-transaction guarding
   performed when the GPE storming is observed. As after merging this
   commit:
     Commit: e1d4d90fc0313d3d58cbd7912c90f8ef24df45ff
     Subject: ACPI / EC: Refine command storm prevention support
   EC_FLAGS_COMMAND_STORM is ensured to be cleared after invoking
   acpi_ec_transaction_unlocked(), the msleep() guard logic will never
   happen now. Since no one complains such change, this logic is likely
   added during the old times where the EC race issues are not fixed and
   the bugs are false root-caused to the timing issue. This patch simply
   removes the out-dated logic. We can restore it by stop skipping
   inter-transaction guarding for wait polling mode.
   Two different delay values are defined for msleep() and udelay() while
   they are merged in this patch to 550us.
2. time_after() causes additional delay in the polling mode (can only be
   observed in noirq suspend/resume processes where polling mode is always
   used) before advance_transaction() is invoked ("wait polling" log is
   added before wait_event_timeout()). We can see 2 wait_event_timeout()
   invocations. This is because time_after() ensures a ">" validation while
   we only need a ">=" validation here:
   [   86.739909] ACPI: Waking up from system sleep state S3
   [   86.742857] ACPI : EC: 2: Increase command
   [   86.742859] ACPI : EC: ***** Command(RD_EC) started *****
   [   86.742861] ACPI : EC: ===== TASK (0) =====
   [   86.742871] ACPI : EC: EC_SC(R) = 0x20 SCI_EVT=1 BURST=0 CMD=0 IBF=0 OBF=0
   [   86.742873] ACPI : EC: EC_SC(W) = 0x80
   [   86.742876] ACPI : EC: ***** Event started *****
   [   86.742880] ACPI : EC: ~~~~~ wait polling ~~~~~
   [   86.743972] ACPI : EC: ~~~~~ wait polling ~~~~~
   [   86.747966] ACPI : EC: ===== TASK (0) =====
   [   86.747977] ACPI : EC: EC_SC(R) = 0x20 SCI_EVT=1 BURST=0 CMD=0 IBF=0 OBF=0
   [   86.747978] ACPI : EC: EC_DATA(W) = 0x06
   [   86.747981] ACPI : EC: ~~~~~ wait polling ~~~~~
   [   86.751971] ACPI : EC: ~~~~~ wait polling ~~~~~
   [   86.755969] ACPI : EC: ===== TASK (0) =====
   [   86.755991] ACPI : EC: EC_SC(R) = 0x21 SCI_EVT=1 BURST=0 CMD=0 IBF=0 OBF=1
   [   86.755993] ACPI : EC: EC_DATA(R) = 0x03
   [   86.755994] ACPI : EC: ~~~~~ wait polling ~~~~~
   [   86.755995] ACPI : EC: ***** Command(RD_EC) stopped *****
   [   86.755996] ACPI : EC: 1: Decrease command
   This patch corrects this by using time_before() instead in ec_guard():
   [   54.283146] ACPI: Waking up from system sleep state S3
   [   54.285414] ACPI : EC: 2: Increase command
   [   54.285415] ACPI : EC: ***** Command(RD_EC) started *****
   [   54.285416] ACPI : EC: ~~~~~ wait polling ~~~~~
   [   54.285417] ACPI : EC: ===== TASK (0) =====
   [   54.285424] ACPI : EC: EC_SC(R) = 0x20 SCI_EVT=1 BURST=0 CMD=0 IBF=0 OBF=0
   [   54.285425] ACPI : EC: EC_SC(W) = 0x80
   [   54.285427] ACPI : EC: ***** Event started *****
   [   54.285429] ACPI : EC: ~~~~~ wait polling ~~~~~
   [   54.287209] ACPI : EC: ===== TASK (0) =====
   [   54.287218] ACPI : EC: EC_SC(R) = 0x20 SCI_EVT=1 BURST=0 CMD=0 IBF=0 OBF=0
   [   54.287219] ACPI : EC: EC_DATA(W) = 0x06
   [   54.287222] ACPI : EC: ~~~~~ wait polling ~~~~~
   [   54.291190] ACPI : EC: ===== TASK (0) =====
   [   54.291210] ACPI : EC: EC_SC(R) = 0x21 SCI_EVT=1 BURST=0 CMD=0 IBF=0 OBF=1
   [   54.291213] ACPI : EC: EC_DATA(R) = 0x03
   [   54.291214] ACPI : EC: ~~~~~ wait polling ~~~~~
   [   54.291215] ACPI : EC: ***** Command(RD_EC) stopped *****
   [   54.291216] ACPI : EC: 1: Decrease command

After cleaning up all guarding logics, we have one single function
ec_guard() collecting all old, non-root-caused, hidden logics. Then we can
easily tune the logics in one place to respond to the bug reports.

Except the time_before() change, all other changes do not change the
behavior of the EC driver.

Link: https://bugzilla.kernel.org/show_bug.cgi?id=12011
Link: https://bugzilla.kernel.org/show_bug.cgi?id=20242
Link: https://bugzilla.kernel.org/show_bug.cgi?id=77431
Signed-off-by: Lv Zheng <lv.zheng@intel.com>
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
ACPI / EC: Remove irqs_disabled() check. 2015-05-15T23:51:17+00:00 Lv Zheng lv.zheng@intel.com 2015-05-15T06:16:34+00:00 373783e6e9394b0dd5050eba152f436e08577d69 The following commit merges polling and interrupt modes for EC driver: Commit: 2a84cb9852f52c0cd1c48bca41a8792d44ad06cc Mon Sep 17 00:00:00 2001 Subject: ACPI: EC: Merge IRQ and POLL modes The irqs_disabled() check introduced in it tries to fall into busy polling mode when the context of ec_poll() cannot sleep. Actually ec_poll() is ensured to be invoked in the contexts that can sleep (from a sysfs /sys/kernel/debug/ec/ec0/io access, or from acpi_evaluate_object(), or from acpi_ec_gpe_poller()). Without the MSI quirk, we never saw the udelay() logic invoked. Thus this check is useless and can be removed. Signed-off-by: Lv Zheng <lv.zheng@intel.com> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> 
The following commit merges polling and interrupt modes for EC driver:
  Commit: 2a84cb9852f52c0cd1c48bca41a8792d44ad06cc Mon Sep 17 00:00:00 2001
  Subject: ACPI: EC: Merge IRQ and POLL modes
The irqs_disabled() check introduced in it tries to fall into busy polling
mode when the context of ec_poll() cannot sleep.

Actually ec_poll() is ensured to be invoked in the contexts that can sleep
(from a sysfs /sys/kernel/debug/ec/ec0/io access, or from
acpi_evaluate_object(), or from acpi_ec_gpe_poller()). Without the MSI
quirk, we never saw the udelay() logic invoked. Thus this check is useless
and can be removed.

Signed-off-by: Lv Zheng <lv.zheng@intel.com>
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
ACPI / EC: Remove storming threashold enlarging quirk. 2015-05-15T23:51:17+00:00 Lv Zheng lv.zheng@intel.com 2015-05-15T06:16:27+00:00 5ab82a11e58b6af704976f5d50ce098472f8abbd This patch removes the storming threashold enlarging quirk. After applying the following commit, we can notice that there is no no-op GPE handling invocation can be observed, thus it is unlikely that the no-op counts can exceed the storming threashold: Commit: ca37bfdfbc8d0a3ec73e4b97bb26dcfa51d515aa Subject: ACPI / EC: Fix several GPE handling issues by deploying ACPI_GPE_DISPATCH_RAW_HANDLER mode. Even when the storming happens, we have already limited its affection to the only transaction and no further transactions will be affected. This is done by this commit: Commit: e1d4d90fc0313d3d58cbd7912c90f8ef24df45ff Subject: ACPI / EC: Refine command storm prevention support So it's time to remove this quirk. Link: https://bugzilla.kernel.org/show_bug.cgi?id=45151 Signed-off-by: Lv Zheng <lv.zheng@intel.com> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> 
This patch removes the storming threashold enlarging quirk.

After applying the following commit, we can notice that there is no no-op
GPE handling invocation can be observed, thus it is unlikely that the
no-op counts can exceed the storming threashold:
  Commit: ca37bfdfbc8d0a3ec73e4b97bb26dcfa51d515aa
  Subject: ACPI / EC: Fix several GPE handling issues by deploying ACPI_GPE_DISPATCH_RAW_HANDLER mode.
Even when the storming happens, we have already limited its affection to
the only transaction and no further transactions will be affected. This is
done by this commit:
  Commit: e1d4d90fc0313d3d58cbd7912c90f8ef24df45ff
  Subject: ACPI / EC: Refine command storm prevention support

So it's time to remove this quirk.

Link: https://bugzilla.kernel.org/show_bug.cgi?id=45151
Signed-off-by: Lv Zheng <lv.zheng@intel.com>
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>