Re: [PATCH v2] platform/chrome: cros_ec: Send host event for prepare/complete

[Tim Van Patten]

Hi,

> > We had issues measuring overall suspend and resume times, which this
> > patch attempts to help resolve.   As stated in the description, the EC
> > host command logs couldn't be used reliably, since they weren't
> > generated at the start of suspend/end of resume, but instead at some
> > point in the middle of the procedures.   This CL moves when the
> > commands are sent from the AP to the EC to as early/late as possible
> > in an attempt to deterministically capture as much of each process as
> > possible.
>
> If the timing of the EC host command logs is unreliable, why not add
> new host commands specifically for this, and then call those at the moment(s)
> you deem is more appropriate, instead of moving the suspend/resume itself
> (which may be introducing its own timing ramifications)?
>
> Calling such a theoretical new EC host command from the userspace power manager
> would probably accomplish the same thing.

We investigated something like that internally initially:
https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/3646238)

We decided to take this simpler approach instead, since it
accomplishes the same goal without requiring new host commands.

> From the kernel documentation[2], "The prepare phase is meant to
> prevent races by preventing new devices from being registered; the PM core
> would never know that all the children of a device had been suspended if new
> children could be registered at will." and "The method may also prepare the
> device or driver in some way for the upcoming system power transition, but
> it should not put the device into a low-power state."
>
> So it seems like an incorrect usage of this callback.

Why is this usage incorrect?

Sending the message to the EC is the preparation for the AP to enter
the system power transition.   For example, it allows the EC to begin
watchdogging the AP once the suspend begins and stop once the resume
completes. This allows the EC to watchdog the entire process, without
any gaps - a beneficial side effect of this change.

> > While this patch could potentially be split, both the logging and PM
> > changes are means to the same end: improving logging behavior to make
> > it easier for developers to measure suspend/resume time and aid in
> > debugging.
>
> That alone is not sufficient cause to combine 2 different changes into
> a single patch.
> The flip side is: the patch to move the suspend/resume host commands into
> prepare/complete() can itself introduce regressions. We should be able to
> revert that without touching the patch adding the logging (assuming concerns
> regarding that are addressed).

Keeping the logging while reverting the PM change would be misleading
to developers while debugging, since the log messages are no longer
indicating the start/end of suspend/resume.

Regardless, I can move them into a separate patch if this necessary.

-- 

Tim Van Patten | ChromeOS | timvp@xxxxxxxxxx | (720) 432-0997