The last resume result exposing logic in cros_ec_sleep_event()
incorrectly requires S0ix support, which doesn't work on ARM based
systems where S0ix doesn't exist. That's because cros_ec_sleep_event()
only reports the last resume result when the EC indicates the last sleep
event was an S0ix resume. On ARM systems, the last sleep event is always
S3 resume, but the EC can still detect sleep hang events in case some
other part of the AP is blocking sleep.
Always expose the last resume result if the EC supports it so that this
works on all devices regardless of S0ix support. This fixes sleep hang
detection on ARM based chromebooks like Trogdor.
Cc: Evan Green <[email protected]>
Cc: Rajat Jain <[email protected]>
Cc: Matthias Kaehlcke <[email protected]>
Cc: Hsin-Yi Wang <[email protected]>
Fixes: 7235560ac77a ("platform/chrome: Add support for v1 of host sleep event")
Signed-off-by: Stephen Boyd <[email protected]>
---
Changes from v1 (https://lore.kernel.org/r/[email protected]):
* Fixed typo
drivers/platform/chrome/cros_ec.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/platform/chrome/cros_ec.c b/drivers/platform/chrome/cros_ec.c
index b3e94cdf7d1a..3abef9747482 100644
--- a/drivers/platform/chrome/cros_ec.c
+++ b/drivers/platform/chrome/cros_ec.c
@@ -135,10 +135,10 @@ static int cros_ec_sleep_event(struct cros_ec_device *ec_dev, u8 sleep_event)
buf.msg.command = EC_CMD_HOST_SLEEP_EVENT;
ret = cros_ec_cmd_xfer_status(ec_dev, &buf.msg);
-
- /* For now, report failure to transition to S0ix with a warning. */
+ /* Report failure to transition to system wide suspend with a warning. */
if (ret >= 0 && ec_dev->host_sleep_v1 &&
- (sleep_event == HOST_SLEEP_EVENT_S0IX_RESUME)) {
+ (sleep_event == HOST_SLEEP_EVENT_S0IX_RESUME ||
+ sleep_event == HOST_SLEEP_EVENT_S3_RESUME)) {
ec_dev->last_resume_result =
buf.u.resp1.resume_response.sleep_transitions;
base-commit: f2906aa863381afb0015a9eb7fefad885d4e5a56
--
https://chromeos.dev
On Fri, Jun 10, 2022 at 11:18 PM Stephen Boyd <[email protected]> wrote:
>
> The last resume result exposing logic in cros_ec_sleep_event()
> incorrectly requires S0ix support, which doesn't work on ARM based
> systems where S0ix doesn't exist. That's because cros_ec_sleep_event()
> only reports the last resume result when the EC indicates the last sleep
> event was an S0ix resume. On ARM systems, the last sleep event is always
> S3 resume, but the EC can still detect sleep hang events in case some
> other part of the AP is blocking sleep.
>
> Always expose the last resume result if the EC supports it so that this
> works on all devices regardless of S0ix support. This fixes sleep hang
> detection on ARM based chromebooks like Trogdor.
>
> Cc: Evan Green <[email protected]>
> Cc: Rajat Jain <[email protected]>
> Cc: Matthias Kaehlcke <[email protected]>
> Cc: Hsin-Yi Wang <[email protected]>
> Fixes: 7235560ac77a ("platform/chrome: Add support for v1 of host sleep event")
> Signed-off-by: Stephen Boyd <[email protected]>
Reviewed-by: Guenter Roeck <[email protected]>
> ---
>
> Changes from v1 (https://lore.kernel.org/r/[email protected]):
> * Fixed typo
>
> drivers/platform/chrome/cros_ec.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/platform/chrome/cros_ec.c b/drivers/platform/chrome/cros_ec.c
> index b3e94cdf7d1a..3abef9747482 100644
> --- a/drivers/platform/chrome/cros_ec.c
> +++ b/drivers/platform/chrome/cros_ec.c
> @@ -135,10 +135,10 @@ static int cros_ec_sleep_event(struct cros_ec_device *ec_dev, u8 sleep_event)
> buf.msg.command = EC_CMD_HOST_SLEEP_EVENT;
>
> ret = cros_ec_cmd_xfer_status(ec_dev, &buf.msg);
> -
> - /* For now, report failure to transition to S0ix with a warning. */
> + /* Report failure to transition to system wide suspend with a warning. */
> if (ret >= 0 && ec_dev->host_sleep_v1 &&
> - (sleep_event == HOST_SLEEP_EVENT_S0IX_RESUME)) {
> + (sleep_event == HOST_SLEEP_EVENT_S0IX_RESUME ||
> + sleep_event == HOST_SLEEP_EVENT_S3_RESUME)) {
> ec_dev->last_resume_result =
> buf.u.resp1.resume_response.sleep_transitions;
>
>
> base-commit: f2906aa863381afb0015a9eb7fefad885d4e5a56
> --
> https://chromeos.dev
>
On Fri, Jun 10, 2022 at 11:18 PM Stephen Boyd <[email protected]> wrote:
>
> The last resume result exposing logic in cros_ec_sleep_event()
> incorrectly requires S0ix support, which doesn't work on ARM based
> systems where S0ix doesn't exist. That's because cros_ec_sleep_event()
> only reports the last resume result when the EC indicates the last sleep
> event was an S0ix resume. On ARM systems, the last sleep event is always
> S3 resume, but the EC can still detect sleep hang events in case some
> other part of the AP is blocking sleep.
>
> Always expose the last resume result if the EC supports it so that this
> works on all devices regardless of S0ix support. This fixes sleep hang
> detection on ARM based chromebooks like Trogdor.
>
> Cc: Evan Green <[email protected]>
> Cc: Rajat Jain <[email protected]>
> Cc: Matthias Kaehlcke <[email protected]>
> Cc: Hsin-Yi Wang <[email protected]>
> Fixes: 7235560ac77a ("platform/chrome: Add support for v1 of host sleep event")
> Signed-off-by: Stephen Boyd <[email protected]>
I looked at the EC code for firmware-hatch-12669.B, which is I think
the oldest board shipping the suspend_transitions support, and saw
that it correctly initializes resume_response.sleep_transitions for
both S0IX_RESUME and S3_RESUME. I think that mitigates my only concern
that we might be interpreting garbage sleep_transitions from the EC on
some older platforms. So,
Reviewed-by: Evan Green <[email protected]>
> ---
>
> Changes from v1 (https://lore.kernel.org/r/[email protected]):
> * Fixed typo
>
> drivers/platform/chrome/cros_ec.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/platform/chrome/cros_ec.c b/drivers/platform/chrome/cros_ec.c
> index b3e94cdf7d1a..3abef9747482 100644
> --- a/drivers/platform/chrome/cros_ec.c
> +++ b/drivers/platform/chrome/cros_ec.c
> @@ -135,10 +135,10 @@ static int cros_ec_sleep_event(struct cros_ec_device *ec_dev, u8 sleep_event)
> buf.msg.command = EC_CMD_HOST_SLEEP_EVENT;
>
> ret = cros_ec_cmd_xfer_status(ec_dev, &buf.msg);
> -
> - /* For now, report failure to transition to S0ix with a warning. */
> + /* Report failure to transition to system wide suspend with a warning. */
> if (ret >= 0 && ec_dev->host_sleep_v1 &&
> - (sleep_event == HOST_SLEEP_EVENT_S0IX_RESUME)) {
> + (sleep_event == HOST_SLEEP_EVENT_S0IX_RESUME ||
> + sleep_event == HOST_SLEEP_EVENT_S3_RESUME)) {
> ec_dev->last_resume_result =
> buf.u.resp1.resume_response.sleep_transitions;
>
>
> base-commit: f2906aa863381afb0015a9eb7fefad885d4e5a56
> --
> https://chromeos.dev
>
On Fri, Jun 10, 2022 at 11:18:25PM -0700, Stephen Boyd wrote:
> @@ -135,10 +135,10 @@ static int cros_ec_sleep_event(struct cros_ec_device *ec_dev, u8 sleep_event)
> buf.msg.command = EC_CMD_HOST_SLEEP_EVENT;
>
> ret = cros_ec_cmd_xfer_status(ec_dev, &buf.msg);
> -
> - /* For now, report failure to transition to S0ix with a warning. */
> + /* Report failure to transition to system wide suspend with a warning. */
> if (ret >= 0 && ec_dev->host_sleep_v1 &&
> - (sleep_event == HOST_SLEEP_EVENT_S0IX_RESUME)) {
> + (sleep_event == HOST_SLEEP_EVENT_S0IX_RESUME ||
> + sleep_event == HOST_SLEEP_EVENT_S3_RESUME)) {
> ec_dev->last_resume_result =
> buf.u.resp1.resume_response.sleep_transitions;
The warning in the if-block:
WARN_ONCE(buf.u.resp1.resume_response.sleep_transitions &
EC_HOST_RESUME_SLEEP_TIMEOUT,
"EC detected sleep transition timeout. Total slp_s0 transitions: %d",
buf.u.resp1.resume_response.sleep_transitions &
EC_HOST_RESUME_SLEEP_TRANSITIONS_MASK);
Not sure and I wonder if the term "slp_s0" is still generic enough to the case.
And also [1].
[1]: https://crrev.com/a311a7657ca0a488f507fa78fd9c2d825cbf083e/power_manager/powerd/system/suspend_configurator.cc#98
At least we should fix [2] once the patch landed.
[2]: https://crrev.com/a311a7657ca0a488f507fa78fd9c2d825cbf083e/power_manager/tools/suspend_stress_test#369
Quoting Tzung-Bi Shih (2022-06-13 22:30:18)
> On Fri, Jun 10, 2022 at 11:18:25PM -0700, Stephen Boyd wrote:
> > @@ -135,10 +135,10 @@ static int cros_ec_sleep_event(struct cros_ec_device *ec_dev, u8 sleep_event)
> > buf.msg.command = EC_CMD_HOST_SLEEP_EVENT;
> >
> > ret = cros_ec_cmd_xfer_status(ec_dev, &buf.msg);
> > -
> > - /* For now, report failure to transition to S0ix with a warning. */
> > + /* Report failure to transition to system wide suspend with a warning. */
> > if (ret >= 0 && ec_dev->host_sleep_v1 &&
> > - (sleep_event == HOST_SLEEP_EVENT_S0IX_RESUME)) {
> > + (sleep_event == HOST_SLEEP_EVENT_S0IX_RESUME ||
> > + sleep_event == HOST_SLEEP_EVENT_S3_RESUME)) {
> > ec_dev->last_resume_result =
> > buf.u.resp1.resume_response.sleep_transitions;
>
> The warning in the if-block:
> WARN_ONCE(buf.u.resp1.resume_response.sleep_transitions &
> EC_HOST_RESUME_SLEEP_TIMEOUT,
> "EC detected sleep transition timeout. Total slp_s0 transitions: %d",
> buf.u.resp1.resume_response.sleep_transitions &
> EC_HOST_RESUME_SLEEP_TRANSITIONS_MASK);
>
> Not sure and I wonder if the term "slp_s0" is still generic enough to the case.
> And also [1].
>
> [1]: https://crrev.com/a311a7657ca0a488f507fa78fd9c2d825cbf083e/power_manager/powerd/system/suspend_configurator.cc#98
I don't know what "slp_s0" means. I guess it's some intelism? It could
change to "sleep" and work for me.
>
> At least we should fix [2] once the patch landed.
>
> [2]: https://crrev.com/a311a7657ca0a488f507fa78fd9c2d825cbf083e/power_manager/tools/suspend_stress_test#369
And by fix you mean change "S0iX" to "suspend"?
On Mon, Jun 13, 2022 at 11:17:50PM -0700, Stephen Boyd wrote:
> Quoting Tzung-Bi Shih (2022-06-13 22:30:18)
> > On Fri, Jun 10, 2022 at 11:18:25PM -0700, Stephen Boyd wrote:
> > > @@ -135,10 +135,10 @@ static int cros_ec_sleep_event(struct cros_ec_device *ec_dev, u8 sleep_event)
> > > buf.msg.command = EC_CMD_HOST_SLEEP_EVENT;
> > >
> > > ret = cros_ec_cmd_xfer_status(ec_dev, &buf.msg);
> > > -
> > > - /* For now, report failure to transition to S0ix with a warning. */
> > > + /* Report failure to transition to system wide suspend with a warning. */
> > > if (ret >= 0 && ec_dev->host_sleep_v1 &&
> > > - (sleep_event == HOST_SLEEP_EVENT_S0IX_RESUME)) {
> > > + (sleep_event == HOST_SLEEP_EVENT_S0IX_RESUME ||
> > > + sleep_event == HOST_SLEEP_EVENT_S3_RESUME)) {
> > > ec_dev->last_resume_result =
> > > buf.u.resp1.resume_response.sleep_transitions;
> >
> > The warning in the if-block:
> > WARN_ONCE(buf.u.resp1.resume_response.sleep_transitions &
> > EC_HOST_RESUME_SLEEP_TIMEOUT,
> > "EC detected sleep transition timeout. Total slp_s0 transitions: %d",
> > buf.u.resp1.resume_response.sleep_transitions &
> > EC_HOST_RESUME_SLEEP_TRANSITIONS_MASK);
> >
> > Not sure and I wonder if the term "slp_s0" is still generic enough to the case.
> > And also [1].
> >
> > [1]: https://crrev.com/a311a7657ca0a488f507fa78fd9c2d825cbf083e/power_manager/powerd/system/suspend_configurator.cc#98
>
> I don't know what "slp_s0" means. I guess it's some intelism? It could
> change to "sleep" and work for me.
>
> >
> > At least we should fix [2] once the patch landed.
> >
> > [2]: https://crrev.com/a311a7657ca0a488f507fa78fd9c2d825cbf083e/power_manager/tools/suspend_stress_test#369
>
> And by fix you mean change "S0iX" to "suspend"?
Yes.