Message-ID: <CACeCKaer_hacWmLDDMJSK-KFjxqiXV8S1-ydCWGT3mkQVeN-rg@mail.gmail.com>
Date:   Thu, 14 Jul 2022 14:51:28 -0700
From:   Prashant Malani <pmalani@...omium.org>
To:     Tim Van Patten <timvp@...gle.com>
Cc:     LKML <linux-kernel@...r.kernel.org>, rrangel@...omium.org,
        robbarnes@...gle.com, Benson Leung <bleung@...omium.org>,
        Guenter Roeck <groeck@...omium.org>,
        chrome-platform@...ts.linux.dev
Subject: Re: [PATCH v2] platform/chrome: cros_ec: Send host event for prepare/complete

Hi Tim,

On Thu, Jul 14, 2022 at 1:55 PM Tim Van Patten <timvp@...gle.com> wrote:
>
> Hi,

Please avoid top-posting [1]

>
> 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.

>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.

>
> 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).

BR,

-Prashant

[1] See "Top-posting" in https://people.kernel.org/tglx/
[2] https://kernel.org/doc/html/latest/driver-api/pm/devices.html#entering-system-suspend




> This will make it easier to bisect CLs in the event of
> regressions, as well as more deterministically verify optimizations
> and improvements.
>
> @Rob Barnes Please fill in any other gaps you think are relevant.
>
>
> On Thu, Jul 14, 2022 at 12:34 PM Prashant Malani <pmalani@...omium.org> wrote:
> >
> > HI Tim,
> >
> > On Wed, Jul 6, 2022 at 7:51 PM Tim Van Patten <timvp@...gle.com> wrote:
> > >
> > > Update cros_ec_lpc_pm_ops to call cros_ec_lpc_suspend() during PM
> > > .prepare() and cros_ec_lpc_resume() during .complete. This allows the
> > > EC to log entry/exit of AP's suspend/resume more accurately.
> > >
> > > Signed-off-by: Tim Van Patten <timvp@...gle.com>
> > > ---
> > >
> > > Changes in v2:
> > > - Include cros_ec_resume() return value in dev_info() output.
> > > - Guard setting .prepare/.complete with #ifdef CONFIG_PM_SLEEP.
> > >
> > >  drivers/platform/chrome/cros_ec_lpc.c | 14 +++++++++++---
> > >  1 file changed, 11 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/platform/chrome/cros_ec_lpc.c b/drivers/platform/chrome/cros_ec_lpc.c
> > > index 7677ab3c0ead9..ce49fbc4ed2e1 100644
> > > --- a/drivers/platform/chrome/cros_ec_lpc.c
> > > +++ b/drivers/platform/chrome/cros_ec_lpc.c
> > > @@ -534,19 +534,27 @@ static int cros_ec_lpc_suspend(struct device *dev)
> > >  {
> > >         struct cros_ec_device *ec_dev = dev_get_drvdata(dev);
> > >
> > > +       dev_info(dev, "Prepare EC suspend\n");
> >
> > This patch is doing 2 things:
> > 1. Changing the timing of cros_ec_lpc_suspend()/resume() invocation.
> > 2. Adding print logs for these callbacks.
> >
> > Whether 2.) is necessary is already being discussed, so I won't
> > comment on that, but it sounds
> > like this should be 2 different patches.
> >
> > Also, please explain what is wrong with "Previously, those events were sent when
> > suspend/resume were already in progress." IOW, what issue is this
> > solving, besides
> > better ordering of EC logs.
> >
> > BR,
> >
> > -Prashant
>
>
>
> --
>
> Tim Van Patten | ChromeOS | timvp@...gle.com | (720) 432-0997

Powered by blists - more mailing lists