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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CANkg5ewRc5zhBtxLhazo8Wsfa3Srj32AwZD9mD=W=Qqpqi7zJQ@mail.gmail.com>
Date:   Thu, 14 Jul 2022 14:55:29 -0600
From:   Tim Van Patten <timvp@...gle.com>
To:     Prashant Malani <pmalani@...omium.org>
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,

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.

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