linux-kernel - Re: [PATCH v4 3/5] platform/chrome: cros_ec: initialize `wake_enabled` in cros_ec

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] [day] [month] [year] [list]

Message-ID: <Ygyrr1U9JbqHDSCK@google.com>
Date:   Wed, 16 Feb 2022 15:45:51 +0800
From:   Tzung-Bi Shih <tzungbi@...gle.com>
To:     Prashant Malani <pmalani@...omium.org>
Cc:     bleung@...omium.org, groeck@...omium.org,
        chrome-platform@...ts.linux.dev, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v4 3/5] platform/chrome: cros_ec: initialize
 `wake_enabled` in cros_ec_register()

On Tue, Feb 15, 2022 at 09:47:09PM -0800, Prashant Malani wrote:
> On Tue, Feb 15, 2022 at 8:37 PM Tzung-Bi Shih <tzungbi@...gle.com> wrote:
> >
> > `wake_enabled` indicates cros_ec_resume() needs to call
> > disable_irq_wake() to undo enable_irq_wake() in cros_ec_suspend().
> >
> > Initialize `wake_enabled` in cros_ec_register() and determine the flag
> > in cros_ec_suspend() instead of reset-after-used in cros_ec_resume().

After reconsidering the 2 options in [1], I feel the flag needs to be set in
cros_ec_suspend() just in case if someone changes the wakeup capability per
[2].

[1]: https://patchwork.kernel.org/project/chrome-platform/patch/20220209045035.380615-4-tzungbi@google.com/#24739778
[2]: https://patchwork.kernel.org/project/chrome-platform/patch/20220209045035.380615-4-tzungbi@google.com/#24740205

Will change it back in the next version, pardon me.

> > @@ -383,10 +384,9 @@ int cros_ec_resume(struct cros_ec_device *ec_dev)
> >                 dev_dbg(ec_dev->dev, "Error %d sending resume event to ec",
> >                         ret);
> >
> > -       if (ec_dev->wake_enabled) {
> > +       if (ec_dev->wake_enabled)
> >                 disable_irq_wake(ec_dev->irq);
> > -               ec_dev->wake_enabled = 0;
> > -       }
> > +
> 
> Better to leave it as is, and ensure "wake_enabled" is cleared after resume?
> Will result in a smaller diff.

No, cros_ec_suspend() uses the flag to tell cros_ec_resume(): don't forget to
call disable_irq_wake().  It shouldn't be reset after used by
cros_ec_resume().