[SRU][OEM-OSP1-B/E][PATCH 0/6] Fix on dock devices reprobe after resume

classic Classic list List threaded Threaded
11 messages Options
Reply | Threaded
Open this post in threaded view
|

[SRU][OEM-OSP1-B/E][PATCH 0/6] Fix on dock devices reprobe after resume

You-Sheng Yang
BugLink: https://bugs.launchpad.net/bugs/1859407

[Impact]
If user un-plugs and then re-plugs the thunderbolt 3 cable of Dell
WD19TB into the laptop during suspend state, peripheral devices on the
docking station cannot be re-probed after the system resumes.

[Fix]
Upstream commit 56b991849009f ("PM: sleep: Simplify suspend-to-idle control
flow") along with its dependencies are required to fix this issue:

  * 56b991849009f PM: sleep: Simplify suspend-to-idle control flow
  * 41275eb5c7181 ACPI: PM: Set s2idle_wakeup earlier and clear it later
  * 9089f16e053af ACPI: EC: Return bool from acpi_ec_dispatch_gpe()
  * 6921de898ba8f ACPICA: Return u32 from acpi_dispatch_gpe()
  * 3a79bc63d9075 PCI: irq: Introduce rearm_wake_irq()

And 56b991849009f takes an additional fix as we've done for bug 1858424:

  * 016b87ca5c8c6 ACPI: EC: Rework flushing of pending work

[Test Case]
1. Please make sure that the WD19 (TBT 3 cable) connects to the laptop
2. Suspend the system
3. Un-plug the TBT 3 cable of WD19 docking station from laptop
4. Re-plug the TBT 3 cable of WD19 docking station into the laptop
5. Press the Enter key or power button to wake up the system
6. Connect an Ethernet cable to a Ethernet port of WD19 docking station
7. Check if the Ethernet interface is available and networking connection is up.

[Regression Potential]
Medium. The first four patches should not affect original functionalities. The
last two mostly simplify the process and resolve a regression.

Rafael J. Wysocki (6):
  PCI: irq: Introduce rearm_wake_irq()
  ACPICA: Return u32 from acpi_dispatch_gpe()
  ACPI: EC: Return bool from acpi_ec_dispatch_gpe()
  ACPI: PM: Set s2idle_wakeup earlier and clear it later
  PM: sleep: Simplify suspend-to-idle control flow
  ACPI: EC: Rework flushing of pending work

 drivers/acpi/acpica/evxfgpe.c |  6 ++--
 drivers/acpi/ec.c             | 47 ++++++++++++++-----------------
 drivers/acpi/internal.h       |  2 +-
 drivers/acpi/sleep.c          | 53 +++++++++++++++++++----------------
 drivers/base/power/main.c     |  5 ----
 include/acpi/acpixf.h         |  8 +++++-
 include/linux/interrupt.h     |  1 +
 include/linux/suspend.h       |  1 -
 kernel/irq/pm.c               | 20 +++++++++++++
 kernel/power/suspend.c        | 53 +++++++++++++++--------------------
 10 files changed, 105 insertions(+), 91 deletions(-)

--
2.24.0


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

[SRU][OEM-OSP1-B/E][PATCH 1/6] PCI: irq: Introduce rearm_wake_irq()

You-Sheng Yang
From: "Rafael J. Wysocki" <[hidden email]>

BugLink: https://bugs.launchpad.net/bugs/1859407

Introduce a new function, rearm_wake_irq(), allowing a wakeup IRQ
to be armed for systen wakeup detection again without running any
action handlers associated with it after it has been armed for
wakeup detection and triggered.

That is useful for IRQs, like ACPI SCI, that may deliver wakeup
as well as non-wakeup interrupts when armed for systen wakeup
detection.  In those cases, it may be possible to determine whether
or not the delivered interrupt is a systen wakeup one without
running the entire action handler (or handlers, if the IRQ is
shared) for the IRQ, and if the interrupt turns out to be a
non-wakeup one, the IRQ can be rearmed with the help of the
new function.

Signed-off-by: Rafael J. Wysocki <[hidden email]>
Acked-by: Thomas Gleixner <[hidden email]>
(cherry picked from commit 3a79bc63d90750f737ab9d7219bd3091d2fd6d84)
Signed-off-by: You-Sheng Yang <[hidden email]>
---
 include/linux/interrupt.h |  1 +
 kernel/irq/pm.c           | 20 ++++++++++++++++++++
 2 files changed, 21 insertions(+)

diff --git a/include/linux/interrupt.h b/include/linux/interrupt.h
index 4a728dba02e22..3e7497432dab5 100644
--- a/include/linux/interrupt.h
+++ b/include/linux/interrupt.h
@@ -220,6 +220,7 @@ extern void irq_wake_thread(unsigned int irq, void *dev_id);
 /* The following three functions are for the core kernel use only. */
 extern void suspend_device_irqs(void);
 extern void resume_device_irqs(void);
+extern void rearm_wake_irq(unsigned int irq);
 
 /**
  * struct irq_affinity_notify - context for notification of IRQ affinity changes
diff --git a/kernel/irq/pm.c b/kernel/irq/pm.c
index d6961d3c6f9e2..8f557fa1f4fe4 100644
--- a/kernel/irq/pm.c
+++ b/kernel/irq/pm.c
@@ -176,6 +176,26 @@ static void resume_irqs(bool want_early)
  }
 }
 
+/**
+ * rearm_wake_irq - rearm a wakeup interrupt line after signaling wakeup
+ * @irq: Interrupt to rearm
+ */
+void rearm_wake_irq(unsigned int irq)
+{
+ unsigned long flags;
+ struct irq_desc *desc = irq_get_desc_buslock(irq, &flags, IRQ_GET_DESC_CHECK_GLOBAL);
+
+ if (!desc || !(desc->istate & IRQS_SUSPENDED) ||
+    !irqd_is_wakeup_set(&desc->irq_data))
+ return;
+
+ desc->istate &= ~IRQS_SUSPENDED;
+ irqd_set(&desc->irq_data, IRQD_WAKEUP_ARMED);
+ __enable_irq(desc);
+
+ irq_put_desc_busunlock(desc, flags);
+}
+
 /**
  * irq_pm_syscore_ops - enable interrupt lines early
  *
--
2.24.0


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

[SRU][OEM-OSP1-B/E][PATCH 2/6] ACPICA: Return u32 from acpi_dispatch_gpe()

You-Sheng Yang
In reply to this post by You-Sheng Yang
From: "Rafael J. Wysocki" <[hidden email]>

BugLink: https://bugs.launchpad.net/bugs/1859407

In some cases it is useful to know whether or not the
acpi_ev_detect_gpe() called by acpi_dispatch_gpe() has found
the GPE to be active, so return the return value of it (whose
data type is u32) from latter.

Signed-off-by: Rafael J. Wysocki <[hidden email]>
Acked-by: Thomas Gleixner <[hidden email]>
(cherry picked from commit 6921de898ba8f2ec91cfea70e7160b89c477382e)
Signed-off-by: You-Sheng Yang <[hidden email]>
---
 drivers/acpi/acpica/evxfgpe.c | 6 +++---
 include/acpi/acpixf.h         | 8 +++++++-
 2 files changed, 10 insertions(+), 4 deletions(-)

diff --git a/drivers/acpi/acpica/evxfgpe.c b/drivers/acpi/acpica/evxfgpe.c
index 4188731e7c406..9990fb72db83e 100644
--- a/drivers/acpi/acpica/evxfgpe.c
+++ b/drivers/acpi/acpica/evxfgpe.c
@@ -644,17 +644,17 @@ ACPI_EXPORT_SYMBOL(acpi_get_gpe_status)
  * PARAMETERS:  gpe_device          - Parent GPE Device. NULL for GPE0/GPE1
  *              gpe_number          - GPE level within the GPE block
  *
- * RETURN:      None
+ * RETURN:      INTERRUPT_HANDLED or INTERRUPT_NOT_HANDLED
  *
  * DESCRIPTION: Detect and dispatch a General Purpose Event to either a function
  *              (e.g. EC) or method (e.g. _Lxx/_Exx) handler.
  *
  ******************************************************************************/
-void acpi_dispatch_gpe(acpi_handle gpe_device, u32 gpe_number)
+u32 acpi_dispatch_gpe(acpi_handle gpe_device, u32 gpe_number)
 {
  ACPI_FUNCTION_TRACE(acpi_dispatch_gpe);
 
- acpi_ev_detect_gpe(gpe_device, NULL, gpe_number);
+ return acpi_ev_detect_gpe(gpe_device, NULL, gpe_number);
 }
 
 ACPI_EXPORT_SYMBOL(acpi_dispatch_gpe)
diff --git a/include/acpi/acpixf.h b/include/acpi/acpixf.h
index 7aa38b648564f..39f7bc8f9f487 100644
--- a/include/acpi/acpixf.h
+++ b/include/acpi/acpixf.h
@@ -305,6 +305,9 @@ ACPI_GLOBAL(u8, acpi_gbl_system_awake_and_running);
 #define ACPI_HW_DEPENDENT_RETURN_OK(prototype) \
  ACPI_EXTERNAL_RETURN_OK(prototype)
 
+#define ACPI_HW_DEPENDENT_RETURN_UINT32(prototype) \
+ ACPI_EXTERNAL_RETURN_UINT32(prototype)
+
 #define ACPI_HW_DEPENDENT_RETURN_VOID(prototype) \
  ACPI_EXTERNAL_RETURN_VOID(prototype)
 
@@ -315,6 +318,9 @@ ACPI_GLOBAL(u8, acpi_gbl_system_awake_and_running);
 #define ACPI_HW_DEPENDENT_RETURN_OK(prototype) \
  static ACPI_INLINE prototype {return(AE_OK);}
 
+#define ACPI_HW_DEPENDENT_RETURN_UINT32(prototype) \
+ static ACPI_INLINE prototype {return(0);}
+
 #define ACPI_HW_DEPENDENT_RETURN_VOID(prototype) \
  static ACPI_INLINE prototype {return;}
 
@@ -746,7 +752,7 @@ ACPI_HW_DEPENDENT_RETURN_STATUS(acpi_status
      u32 gpe_number,
      acpi_event_status
      *event_status))
-ACPI_HW_DEPENDENT_RETURN_VOID(void acpi_dispatch_gpe(acpi_handle gpe_device, u32 gpe_number))
+ACPI_HW_DEPENDENT_RETURN_UINT32(u32 acpi_dispatch_gpe(acpi_handle gpe_device, u32 gpe_number))
 ACPI_HW_DEPENDENT_RETURN_STATUS(acpi_status acpi_disable_all_gpes(void))
 ACPI_HW_DEPENDENT_RETURN_STATUS(acpi_status acpi_enable_all_runtime_gpes(void))
 ACPI_HW_DEPENDENT_RETURN_STATUS(acpi_status acpi_enable_all_wakeup_gpes(void))
--
2.24.0


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

[SRU][OEM-OSP1-B/E][PATCH 3/6] ACPI: EC: Return bool from acpi_ec_dispatch_gpe()

You-Sheng Yang
In reply to this post by You-Sheng Yang
From: "Rafael J. Wysocki" <[hidden email]>

BugLink: https://bugs.launchpad.net/bugs/1859407

On some systems, if suspend-to-idle is used, the EC may signal system
wakeup events (power button events, for example) as well as events
that should not cause the system to resume and acpi_ec_dispatch_gpe()
needs to be called to determine whether or not the system should
resume then.  In particular, if acpi_ec_dispatch_gpe() doesn't detect
any EC events at all, the system should remain suspended, so it is
useful to know when that is the case.

For this reason, make acpi_ec_dispatch_gpe() return a bool value
indicating whether or not any EC events have been detected by it.

Signed-off-by: Rafael J. Wysocki <[hidden email]>
Acked-by: Thomas Gleixner <[hidden email]>
(cherry picked from commit 9089f16e053afc5e18feaeb9f64cc7c90d6bd687)
Signed-off-by: You-Sheng Yang <[hidden email]>
---
 drivers/acpi/ec.c       | 11 ++++++++---
 drivers/acpi/internal.h |  2 +-
 2 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
index 49e16f0090957..17c416aa46a68 100644
--- a/drivers/acpi/ec.c
+++ b/drivers/acpi/ec.c
@@ -1071,10 +1071,15 @@ void acpi_ec_set_gpe_wake_mask(u8 action)
  acpi_set_gpe_wake_mask(NULL, first_ec->gpe, action);
 }
 
-void acpi_ec_dispatch_gpe(void)
+bool acpi_ec_dispatch_gpe(void)
 {
- if (first_ec)
- acpi_dispatch_gpe(NULL, first_ec->gpe);
+ u32 ret;
+
+ if (!first_ec)
+ return false;
+
+ ret = acpi_dispatch_gpe(NULL, first_ec->gpe);
+ return ret == ACPI_INTERRUPT_HANDLED;
 }
 
 /* --------------------------------------------------------------------------
diff --git a/drivers/acpi/internal.h b/drivers/acpi/internal.h
index 6a9e1fb8913ae..587238211be16 100644
--- a/drivers/acpi/internal.h
+++ b/drivers/acpi/internal.h
@@ -198,7 +198,7 @@ void acpi_ec_block_transactions(void);
 void acpi_ec_unblock_transactions(void);
 void acpi_ec_mark_gpe_for_wake(void);
 void acpi_ec_set_gpe_wake_mask(u8 action);
-void acpi_ec_dispatch_gpe(void);
+bool acpi_ec_dispatch_gpe(void);
 int acpi_ec_add_query_handler(struct acpi_ec *ec, u8 query_bit,
       acpi_handle handle, acpi_ec_query_func func,
       void *data);
--
2.24.0


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

[SRU][OEM-OSP1-B/E][PATCH 4/6] ACPI: PM: Set s2idle_wakeup earlier and clear it later

You-Sheng Yang
In reply to this post by You-Sheng Yang
From: "Rafael J. Wysocki" <[hidden email]>

BugLink: https://bugs.launchpad.net/bugs/1859407

The role of the s2idle_wakeup variable is to cause
acpi_pm_wakeup_event() and acpi_pm_notify_handler() to
increment pm_abort_suspend and trigger a wakeup from
suspend-to-idle in case the ACPI SCI wakeup was canceled
by acpi_s2idle_wake().

However, for this purpose it need not be set in acpi_s2idle_wake()
and cleared in acpi_s2idle_sync(), respectively.  In fact, it
may be set as early as in acpi_s2idle_prepare() and cleared as
late as in acpi_s2idle_restore(), so do that to allow subsequent
changes to be simpler.

This change is not expected to alter functionality.

Signed-off-by: Rafael J. Wysocki <[hidden email]>
Acked-by: Thomas Gleixner <[hidden email]>
(cherry picked from commit 41275eb5c7181febdfaa63c3a0ad9b7acdadcd52)
Signed-off-by: You-Sheng Yang <[hidden email]>
---
 drivers/acpi/sleep.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/acpi/sleep.c b/drivers/acpi/sleep.c
index e52f1238d2d66..ac428405e318b 100644
--- a/drivers/acpi/sleep.c
+++ b/drivers/acpi/sleep.c
@@ -982,6 +982,8 @@ static int acpi_s2idle_prepare(void)
  /* Change the configuration of GPEs to avoid spurious wakeup. */
  acpi_enable_all_wakeup_gpes();
  acpi_os_wait_events_complete();
+
+ s2idle_wakeup = true;
  return 0;
 }
 
@@ -1001,7 +1003,6 @@ static void acpi_s2idle_wake(void)
  if (acpi_sci_irq_valid() &&
     !irqd_is_wakeup_armed(irq_get_irq_data(acpi_sci_irq))) {
  pm_system_cancel_wakeup();
- s2idle_wakeup = true;
  /*
  * On some platforms with the LPS0 _DSM device noirq resume
  * takes too much time for EC wakeup events to survive, so look
@@ -1022,11 +1023,12 @@ static void acpi_s2idle_sync(void)
  acpi_os_wait_events_complete(); /* synchronize SCI IRQ handling */
  acpi_ec_flush_work();
  acpi_os_wait_events_complete(); /* synchronize Notify handling */
- s2idle_wakeup = false;
 }
 
 static void acpi_s2idle_restore(void)
 {
+ s2idle_wakeup = false;
+
  acpi_enable_all_runtime_gpes();
 
  acpi_disable_wakeup_devices(ACPI_STATE_S0);
--
2.24.0


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

[SRU][OEM-OSP1-B/E][PATCH 5/6] PM: sleep: Simplify suspend-to-idle control flow

You-Sheng Yang
In reply to this post by You-Sheng Yang
From: "Rafael J. Wysocki" <[hidden email]>

BugLink: https://bugs.launchpad.net/bugs/1859407

After commit 33e4f80ee69b ("ACPI / PM: Ignore spurious SCI wakeups
from suspend-to-idle") the "noirq" phases of device suspend and
resume may run for multiple times during suspend-to-idle, if there
are spurious system wakeup events while suspended.  However, this
is complicated and fragile and actually unnecessary.

The main reason for doing this is that on some systems the EC may
signal system wakeup events (power button events, for example) as
well as events that should not cause the system to resume (spurious
system wakeup events).  Thus, in order to determine whether or not
a given event signaled by the EC while suspended is a proper system
wakeup one, the EC GPE needs to be dispatched and to start with that
was achieved by allowing the ACPI SCI action handler to run, which
was only possible after calling resume_device_irqs().

However, dispatching the EC GPE this way turned out to take too much
time in some cases and some EC events might be missed due to that, so
commit 68e22011856f ("ACPI: EC: Dispatch the EC GPE directly on
s2idle wake") started to dispatch the EC GPE right after a wakeup
event has been detected, so in fact the full ACPI SCI action handler
doesn't need to run any more to deal with the wakeups coming from the
EC.

Use this observation to simplify the suspend-to-idle control flow
so that the "noirq" phases of device suspend and resume are each
run only once in every suspend-to-idle cycle, which is reported to
significantly reduce power drawn by some systems when suspended to
idle (by allowing them to reach a deep platform-wide low-power state
through the suspend-to-idle flow).  [What appears to happen is that
the "noirq" resume of devices after a spurious EC wakeup brings some
devices into a state in which they prevent the platform from reaching
the deep low-power state going forward, even after a subsequent
"noirq" suspend phase, and on some systems the EC triggers such
wakeups already when the "noirq" suspend of devices is running for
the first time in the given suspend/resume cycle, so the platform
cannot reach the deep low-power state at all.]

First, make acpi_s2idle_wake() use the acpi_ec_dispatch_gpe() return
value to determine whether or not the wakeup may have been triggered
by the EC (in which case the system wakeup is canceled and ACPI
events are processed in order to determine whether or not the event
is a proper system wakeup one) and use rearm_wake_irq() (introduced
by a previous change) in it to rearm the ACPI SCI for system wakeup
detection in case the system will remain suspended.

Second, drop acpi_s2idle_sync(), which is not needed any more, and
the corresponding global platform suspend-to-idle callback.

Next, drop the pm_wakeup_pending() check (which is an optimization
only) from __device_suspend_noirq() to prevent it from returning
errors on system wakeups occurring before the "noirq" phase of
device suspend is complete (as in the case of suspend-to-idle it is
not known whether or not these wakeups are suprious at that point),
in order to avoid having to carry out a "noirq" resume of devices
on a spurious system wakeup.

Finally, change the code flow in s2idle_loop() to (1) run the
"noirq" suspend of devices once before starting the loop, (2) check
for spurious EC wakeups (via the platform ->wake callback) for the
first time before calling s2idle_enter(), and (3) run the "noirq"
resume of devices once after leaving the loop.

Signed-off-by: Rafael J. Wysocki <[hidden email]>
Acked-by: Thomas Gleixner <[hidden email]>
(cherry picked from commit 56b991849009f5def0443bfb2f48c8321d888e15)
Signed-off-by: You-Sheng Yang <[hidden email]>
---
 drivers/acpi/sleep.c      | 47 ++++++++++++++++++----------------
 drivers/base/power/main.c |  5 ----
 include/linux/suspend.h   |  1 -
 kernel/power/suspend.c    | 53 +++++++++++++++++----------------------
 4 files changed, 48 insertions(+), 58 deletions(-)

diff --git a/drivers/acpi/sleep.c b/drivers/acpi/sleep.c
index ac428405e318b..9428f94fb4dfa 100644
--- a/drivers/acpi/sleep.c
+++ b/drivers/acpi/sleep.c
@@ -996,33 +996,37 @@ static void acpi_s2idle_wake(void)
  lpi_check_constraints();
 
  /*
- * If IRQD_WAKEUP_ARMED is not set for the SCI at this point, it means
- * that the SCI has triggered while suspended, so cancel the wakeup in
- * case it has not been a wakeup event (the GPEs will be checked later).
+ * If IRQD_WAKEUP_ARMED is set for the SCI at this point, the SCI has
+ * not triggered while suspended, so bail out.
  */
- if (acpi_sci_irq_valid() &&
-    !irqd_is_wakeup_armed(irq_get_irq_data(acpi_sci_irq))) {
+ if (!acpi_sci_irq_valid() ||
+    irqd_is_wakeup_armed(irq_get_irq_data(acpi_sci_irq)))
+ return;
+
+ /*
+ * If there are EC events to process, the wakeup may be a spurious one
+ * coming from the EC.
+ */
+ if (acpi_ec_dispatch_gpe()) {
+ /*
+ * Cancel the wakeup and process all pending events in case
+ * there are any wakeup ones in there.
+ *
+ * Note that if any non-EC GPEs are active at this point, the
+ * SCI will retrigger after the rearming below, so no events
+ * should be missed by canceling the wakeup here.
+ */
  pm_system_cancel_wakeup();
  /*
- * On some platforms with the LPS0 _DSM device noirq resume
- * takes too much time for EC wakeup events to survive, so look
- * for them now.
+ * The EC driver uses the system workqueue and an additional
+ * special one, so those need to be flushed too.
  */
- acpi_ec_dispatch_gpe();
+ acpi_os_wait_events_complete(); /* synchronize EC GPE processing */
+ acpi_ec_flush_work();
+ acpi_os_wait_events_complete(); /* synchronize Notify handling */
  }
-}
 
-static void acpi_s2idle_sync(void)
-{
- /*
- * Process all pending events in case there are any wakeup ones.
- *
- * The EC driver uses the system workqueue and an additional special
- * one, so those need to be flushed too.
- */
- acpi_os_wait_events_complete(); /* synchronize SCI IRQ handling */
- acpi_ec_flush_work();
- acpi_os_wait_events_complete(); /* synchronize Notify handling */
+ rearm_wake_irq(acpi_sci_irq);
 }
 
 static void acpi_s2idle_restore(void)
@@ -1054,7 +1058,6 @@ static const struct platform_s2idle_ops acpi_s2idle_ops = {
  .begin = acpi_s2idle_begin,
  .prepare = acpi_s2idle_prepare,
  .wake = acpi_s2idle_wake,
- .sync = acpi_s2idle_sync,
  .restore = acpi_s2idle_restore,
  .end = acpi_s2idle_end,
 };
diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c
index 7900debc5ce44..ca3c8700c1f96 100644
--- a/drivers/base/power/main.c
+++ b/drivers/base/power/main.c
@@ -1299,11 +1299,6 @@ static int __device_suspend_noirq(struct device *dev, pm_message_t state, bool a
  if (async_error)
  goto Complete;
 
- if (pm_wakeup_pending()) {
- async_error = -EBUSY;
- goto Complete;
- }
-
  if (dev->power.syscore || dev->power.direct_complete)
  goto Complete;
 
diff --git a/include/linux/suspend.h b/include/linux/suspend.h
index 50e81bff37df5..7d0dfb1ea34e6 100644
--- a/include/linux/suspend.h
+++ b/include/linux/suspend.h
@@ -191,7 +191,6 @@ struct platform_s2idle_ops {
  int (*begin)(void);
  int (*prepare)(void);
  void (*wake)(void);
- void (*sync)(void);
  void (*restore)(void);
  void (*end)(void);
 };
diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c
index 07a32e99aa644..09ac6da691ae2 100644
--- a/kernel/power/suspend.c
+++ b/kernel/power/suspend.c
@@ -115,48 +115,41 @@ static void s2idle_enter(void)
 
 static void s2idle_loop(void)
 {
+ int error;
+
+ dpm_noirq_begin();
+ error = dpm_noirq_suspend_devices(PMSG_SUSPEND);
+ if (error)
+ goto resume;
+
  pm_pr_dbg("suspend-to-idle\n");
 
+ /*
+ * Suspend-to-idle equals:
+ * frozen processes + suspended devices + idle processors.
+ * Thus s2idle_enter() should be called right after all devices have
+ * been suspended.
+ *
+ * Wakeups during the noirq suspend of devices may be spurious, so try
+ * to avoid them upfront.
+ */
  for (;;) {
- int error;
-
- dpm_noirq_begin();
-
- /*
- * Suspend-to-idle equals
- * frozen processes + suspended devices + idle processors.
- * Thus s2idle_enter() should be called right after
- * all devices have been suspended.
- *
- * Wakeups during the noirq suspend of devices may be spurious,
- * so prevent them from terminating the loop right away.
- */
- error = dpm_noirq_suspend_devices(PMSG_SUSPEND);
- if (!error)
- s2idle_enter();
- else if (error == -EBUSY && pm_wakeup_pending())
- error = 0;
-
- if (!error && s2idle_ops && s2idle_ops->wake)
+ if (s2idle_ops && s2idle_ops->wake)
  s2idle_ops->wake();
 
- dpm_noirq_resume_devices(PMSG_RESUME);
-
- dpm_noirq_end();
-
- if (error)
- break;
-
- if (s2idle_ops && s2idle_ops->sync)
- s2idle_ops->sync();
-
  if (pm_wakeup_pending())
  break;
 
  pm_wakeup_clear(false);
+
+ s2idle_enter();
  }
 
  pm_pr_dbg("resume from suspend-to-idle\n");
+
+resume:
+ dpm_noirq_resume_devices(PMSG_RESUME);
+ dpm_noirq_end();
 }
 
 void s2idle_wake(void)
--
2.24.0


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

[SRU][OEM-OSP1-B/E][PATCH 6/6] ACPI: EC: Rework flushing of pending work

You-Sheng Yang
In reply to this post by You-Sheng Yang
From: "Rafael J. Wysocki" <[hidden email]>

BugLink: https://bugs.launchpad.net/bugs/1859407

There is a race condition in the ACPI EC driver, between
__acpi_ec_flush_event() and acpi_ec_event_handler(), that may
cause systems to stay in suspended-to-idle forever after a wakeup
event coming from the EC.

Namely, acpi_s2idle_wake() calls acpi_ec_flush_work() to wait until
the delayed work resulting from the handling of the EC GPE in
acpi_ec_dispatch_gpe() is processed, and that function invokes
__acpi_ec_flush_event() which uses wait_event() to wait for
ec->nr_pending_queries to become zero on ec->wait, and that wait
queue may be woken up too early.

Suppose that acpi_ec_dispatch_gpe() has caused acpi_ec_gpe_handler()
to run, so advance_transaction() has been called and it has invoked
acpi_ec_submit_query() to queue up an event work item, so
ec->nr_pending_queries has been incremented (under ec->lock).  The
work function of that work item, acpi_ec_event_handler() runs later
and calls acpi_ec_query() to process the event.  That function calls
acpi_ec_transaction() which invokes acpi_ec_transaction_unlocked()
and the latter wakes up ec->wait under ec->lock, but it drops that
lock before returning.

When acpi_ec_query() returns, acpi_ec_event_handler() acquires
ec->lock and decrements ec->nr_pending_queries, but at that point
__acpi_ec_flush_event() (woken up previously) may already have
acquired ec->lock, checked the value of ec->nr_pending_queries (and
it would not have been zero then) and decided to go back to sleep.
Next, if ec->nr_pending_queries is equal to zero now, the loop
in acpi_ec_event_handler() terminates, ec->lock is released and
acpi_ec_check_event() is called, but it does nothing unless
ec_event_clearing is equal to ACPI_EC_EVT_TIMING_EVENT (which is
not the case by default).  In the end, if no more event work items
have been queued up while executing acpi_ec_transaction_unlocked(),
there is nothing to wake up __acpi_ec_flush_event() again and it
sleeps forever, so the suspend-to-idle loop cannot make progress and
the system is permanently suspended.

To avoid this issue, notice that it actually is not necessary to
wait for ec->nr_pending_queries to become zero in every case in
which __acpi_ec_flush_event() is used.

First, during platform-based system suspend (not suspend-to-idle),
__acpi_ec_flush_event() is called by acpi_ec_disable_event() after
clearing the EC_FLAGS_QUERY_ENABLED flag, which prevents
acpi_ec_submit_query() from submitting any new event work items,
so calling flush_scheduled_work() and flushing ec_query_wq
subsequently (in order to wait until all of the queries in that
queue have been processed) would be sufficient to flush all of
the pending EC work in that case.

Second, the purpose of the flushing of pending EC work while
suspended-to-idle described above really is to wait until the
first event work item coming from acpi_ec_dispatch_gpe() is
complete, because it should produce system wakeup events if
that is a valid EC-based system wakeup, so calling
flush_scheduled_work() followed by flushing ec_query_wq is also
sufficient for that purpose.

Rework the code to follow the above observations.

Fixes: 56b9918490 ("PM: sleep: Simplify suspend-to-idle control flow")
Reported-by: Kenneth R. Crudup <[hidden email]>
Tested-by: Kenneth R. Crudup <[hidden email]>
Cc: 5.4+ <[hidden email]> # 5.4+
Signed-off-by: Rafael J. Wysocki <[hidden email]>
(cherry picked from commit 016b87ca5c8c6e9e87db442f04dc99609b11ed36)
Signed-off-by: You-Sheng Yang <[hidden email]>
---
 drivers/acpi/ec.c | 36 +++++++++++++-----------------------
 1 file changed, 13 insertions(+), 23 deletions(-)

diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
index 17c416aa46a68..9b193a3193380 100644
--- a/drivers/acpi/ec.c
+++ b/drivers/acpi/ec.c
@@ -535,26 +535,10 @@ static void acpi_ec_enable_event(struct acpi_ec *ec)
 }
 
 #ifdef CONFIG_PM_SLEEP
-static bool acpi_ec_query_flushed(struct acpi_ec *ec)
+static void __acpi_ec_flush_work(void)
 {
- bool flushed;
- unsigned long flags;
-
- spin_lock_irqsave(&ec->lock, flags);
- flushed = !ec->nr_pending_queries;
- spin_unlock_irqrestore(&ec->lock, flags);
- return flushed;
-}
-
-static void __acpi_ec_flush_event(struct acpi_ec *ec)
-{
- /*
- * When ec_freeze_events is true, we need to flush events in
- * the proper position before entering the noirq stage.
- */
- wait_event(ec->wait, acpi_ec_query_flushed(ec));
- if (ec_query_wq)
- flush_workqueue(ec_query_wq);
+ flush_scheduled_work(); /* flush ec->work */
+ flush_workqueue(ec_query_wq); /* flush queries */
 }
 
 static void acpi_ec_disable_event(struct acpi_ec *ec)
@@ -564,15 +548,21 @@ static void acpi_ec_disable_event(struct acpi_ec *ec)
  spin_lock_irqsave(&ec->lock, flags);
  __acpi_ec_disable_event(ec);
  spin_unlock_irqrestore(&ec->lock, flags);
- __acpi_ec_flush_event(ec);
+
+ /*
+ * When ec_freeze_events is true, we need to flush events in
+ * the proper position before entering the noirq stage.
+ */
+ __acpi_ec_flush_work();
 }
 
 void acpi_ec_flush_work(void)
 {
- if (first_ec)
- __acpi_ec_flush_event(first_ec);
+ /* Without ec_query_wq there is nothing to flush. */
+ if (!ec_query_wq)
+ return;
 
- flush_scheduled_work();
+ __acpi_ec_flush_work();
 }
 #endif /* CONFIG_PM_SLEEP */
 
--
2.24.0


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

ACK/Cmnt: [SRU][OEM-OSP1-B/E][PATCH 0/6] Fix on dock devices reprobe after resume

Stefan Bader-2
In reply to this post by You-Sheng Yang
On 13.01.20 10:40, You-Sheng Yang wrote:

> BugLink: https://bugs.launchpad.net/bugs/1859407
>
> [Impact]
> If user un-plugs and then re-plugs the thunderbolt 3 cable of Dell
> WD19TB into the laptop during suspend state, peripheral devices on the
> docking station cannot be re-probed after the system resumes.
>
> [Fix]
> Upstream commit 56b991849009f ("PM: sleep: Simplify suspend-to-idle control
> flow") along with its dependencies are required to fix this issue:
>
>   * 56b991849009f PM: sleep: Simplify suspend-to-idle control flow
>   * 41275eb5c7181 ACPI: PM: Set s2idle_wakeup earlier and clear it later
>   * 9089f16e053af ACPI: EC: Return bool from acpi_ec_dispatch_gpe()
>   * 6921de898ba8f ACPICA: Return u32 from acpi_dispatch_gpe()
>   * 3a79bc63d9075 PCI: irq: Introduce rearm_wake_irq()
>
> And 56b991849009f takes an additional fix as we've done for bug 1858424:
>
>   * 016b87ca5c8c6 ACPI: EC: Rework flushing of pending work
>
> [Test Case]
> 1. Please make sure that the WD19 (TBT 3 cable) connects to the laptop
> 2. Suspend the system
> 3. Un-plug the TBT 3 cable of WD19 docking station from laptop
> 4. Re-plug the TBT 3 cable of WD19 docking station into the laptop
> 5. Press the Enter key or power button to wake up the system
> 6. Connect an Ethernet cable to a Ethernet port of WD19 docking station
> 7. Check if the Ethernet interface is available and networking connection is up.
>
> [Regression Potential]
> Medium. The first four patches should not affect original functionalities. The
> last two mostly simplify the process and resolve a regression.

In general I would agree. Just from personal experience in the past, any change
on the ACPI EC code is like a game of whack-a-mole. So I would not be too
surprised if something else is found to be broken after. But since this is for
now only geared towards an intermediate release it sounds acceptable.

>
> Rafael J. Wysocki (6):
>   PCI: irq: Introduce rearm_wake_irq()
>   ACPICA: Return u32 from acpi_dispatch_gpe()
>   ACPI: EC: Return bool from acpi_ec_dispatch_gpe()
>   ACPI: PM: Set s2idle_wakeup earlier and clear it later
>   PM: sleep: Simplify suspend-to-idle control flow
>   ACPI: EC: Rework flushing of pending work
>
>  drivers/acpi/acpica/evxfgpe.c |  6 ++--
>  drivers/acpi/ec.c             | 47 ++++++++++++++-----------------
>  drivers/acpi/internal.h       |  2 +-
>  drivers/acpi/sleep.c          | 53 +++++++++++++++++++----------------
>  drivers/base/power/main.c     |  5 ----
>  include/acpi/acpixf.h         |  8 +++++-
>  include/linux/interrupt.h     |  1 +
>  include/linux/suspend.h       |  1 -
>  kernel/irq/pm.c               | 20 +++++++++++++
>  kernel/power/suspend.c        | 53 +++++++++++++++--------------------
>  10 files changed, 105 insertions(+), 91 deletions(-)
>

Acked-by: Stefan Bader <[hidden email]>

--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

ACK: [SRU][OEM-OSP1-B/E][PATCH 0/6] Fix on dock devices reprobe after resume

Marcelo Henrique Cerri
In reply to this post by You-Sheng Yang
Clean cherry-picks and sane changes. Although it has medium risk of
regression it's targeted only to linux-oem-osp1:

Acked-by: Marcelo Henrique Cerri <[hidden email]>

--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team

signature.asc (673 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

APPLIED [OEM-OSP1-B] Re: [SRU][OEM-OSP1-B/E][PATCH 0/6] Fix on dock devices reprobe after resume

Timo Aaltonen-6
In reply to this post by You-Sheng Yang
On 13.1.2020 11.40, You-Sheng Yang wrote:
> BugLink: https://bugs.launchpad.net/bugs/1859407
>
> [Impact]
> If user un-plugs and then re-plugs the thunderbolt 3 cable of Dell
> WD19TB into the laptop during suspend state, peripheral devices on the
> docking station cannot be re-probed after the system resumes.

applied to ops1 oem-next, thanks


--
t

--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

APPLIED[E]: [SRU][OEM-OSP1-B/E][PATCH 0/6] Fix on dock devices reprobe after resume

Kleber Souza
In reply to this post by You-Sheng Yang
On 13.01.20 10:40, You-Sheng Yang wrote:

> BugLink: https://bugs.launchpad.net/bugs/1859407
>
> [Impact]
> If user un-plugs and then re-plugs the thunderbolt 3 cable of Dell
> WD19TB into the laptop during suspend state, peripheral devices on the
> docking station cannot be re-probed after the system resumes.
>
> [Fix]
> Upstream commit 56b991849009f ("PM: sleep: Simplify suspend-to-idle control
> flow") along with its dependencies are required to fix this issue:
>
>   * 56b991849009f PM: sleep: Simplify suspend-to-idle control flow
>   * 41275eb5c7181 ACPI: PM: Set s2idle_wakeup earlier and clear it later
>   * 9089f16e053af ACPI: EC: Return bool from acpi_ec_dispatch_gpe()
>   * 6921de898ba8f ACPICA: Return u32 from acpi_dispatch_gpe()
>   * 3a79bc63d9075 PCI: irq: Introduce rearm_wake_irq()
>
> And 56b991849009f takes an additional fix as we've done for bug 1858424:
>
>   * 016b87ca5c8c6 ACPI: EC: Rework flushing of pending work
>
> [Test Case]
> 1. Please make sure that the WD19 (TBT 3 cable) connects to the laptop
> 2. Suspend the system
> 3. Un-plug the TBT 3 cable of WD19 docking station from laptop
> 4. Re-plug the TBT 3 cable of WD19 docking station into the laptop
> 5. Press the Enter key or power button to wake up the system
> 6. Connect an Ethernet cable to a Ethernet port of WD19 docking station
> 7. Check if the Ethernet interface is available and networking connection is up.
>
> [Regression Potential]
> Medium. The first four patches should not affect original functionalities. The
> last two mostly simplify the process and resolve a regression.
>
> Rafael J. Wysocki (6):
>   PCI: irq: Introduce rearm_wake_irq()
>   ACPICA: Return u32 from acpi_dispatch_gpe()
>   ACPI: EC: Return bool from acpi_ec_dispatch_gpe()
>   ACPI: PM: Set s2idle_wakeup earlier and clear it later
>   PM: sleep: Simplify suspend-to-idle control flow
>   ACPI: EC: Rework flushing of pending work
>
>  drivers/acpi/acpica/evxfgpe.c |  6 ++--
>  drivers/acpi/ec.c             | 47 ++++++++++++++-----------------
>  drivers/acpi/internal.h       |  2 +-
>  drivers/acpi/sleep.c          | 53 +++++++++++++++++++----------------
>  drivers/base/power/main.c     |  5 ----
>  include/acpi/acpixf.h         |  8 +++++-
>  include/linux/interrupt.h     |  1 +
>  include/linux/suspend.h       |  1 -
>  kernel/irq/pm.c               | 20 +++++++++++++
>  kernel/power/suspend.c        | 53 +++++++++++++++--------------------
>  10 files changed, 105 insertions(+), 91 deletions(-)
>

Applied to eoan/linux.

Thanks,
Kleber

--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team