linux-kernel - Re: [RFC v4 3/5] platform/x86/intel/pmc: core: Drop check

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: <CAM7w-FW8fGPxv0djzkzt4B88xcsMhjXJZ4i6n2ueaC81h4giaw@mail.gmail.com>
Date:   Wed, 23 Nov 2022 12:47:23 -0500
From:   Sven van Ashbrook <svenva@...omium.org>
To:     Mario Limonciello <mario.limonciello@....com>
Cc:     "Rafael J . Wysocki" <rafael@...nel.org>,
        Rajneesh Bhardwaj <irenic.rajneesh@...il.com>,
        David E Box <david.e.box@...el.com>,
        Raul Rangel <rrangel@...omium.org>, linux-pm@...r.kernel.org,
        platform-driver-x86@...r.kernel.org, Pavel Machek <pavel@....cz>,
        Len Brown <len.brown@...el.com>,
        John Stultz <jstultz@...gle.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        Stephen Boyd <sboyd@...nel.org>,
        S-k Shyam-sundar <Shyam-sundar.S-k@....com>,
        Rajat Jain <rajatja@...gle.com>,
        Hans de Goede <hdegoede@...hat.com>,
        linux-kernel@...r.kernel.org, Mark Gross <markgross@...nel.org>
Subject: Re: [RFC v4 3/5] platform/x86/intel/pmc: core: Drop check_counters

Hi Mario, comments below.

On Thu, Nov 17, 2022 at 5:58 PM Mario Limonciello
<mario.limonciello@....com> wrote:
>
> `check_counters` is a stateful variable for indicating whether or
> not to be checking if counters incremented on resume from s2idle.
>
> As the module already has code to gate whether to check the counters
> that will fail the suspend when this is enabled, use that instead.
>
> Signed-off-by: Mario Limonciello <mario.limonciello@....com>
> ---
> RFC v3->v4:
>  * No changes
> ---
>  drivers/platform/x86/intel/pmc/core.c | 7 ++-----
>  drivers/platform/x86/intel/pmc/core.h | 1 -
>  2 files changed, 2 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/platform/x86/intel/pmc/core.c b/drivers/platform/x86/intel/pmc/core.c
> index 17ec5825d13d..adc2cae4db28 100644
> --- a/drivers/platform/x86/intel/pmc/core.c
> +++ b/drivers/platform/x86/intel/pmc/core.c
> @@ -2059,8 +2059,6 @@ static __maybe_unused int pmc_core_suspend(struct device *dev)
>  {
>         struct pmc_dev *pmcdev = dev_get_drvdata(dev);
>
> -       pmcdev->check_counters = false;
> -
>         /* No warnings on S0ix failures */
>         if (!warn_on_s0ix_failures)
>                 return 0;
> @@ -2077,7 +2075,6 @@ static __maybe_unused int pmc_core_suspend(struct device *dev)
>         if (pmc_core_dev_state_get(pmcdev, &pmcdev->s0ix_counter))
>                 return -EIO;
>
> -       pmcdev->check_counters = true;
>         return 0;
>  }
>
> @@ -2113,10 +2110,10 @@ static __maybe_unused int pmc_core_resume(struct device *dev)
>         const struct pmc_bit_map **maps = pmcdev->map->lpm_sts;
>         int offset = pmcdev->map->lpm_status_offset;
>
> -       if (!pmcdev->check_counters)
> +       if (!pmc_core_is_s0ix_failed(pmcdev))

Will this break the "CPU did not enter SLP_S0!!!" warning?

As far as I can tell,
If an Intel system uses S3 instead of S0ix, pmcdev->s0ix_counter will
not get updated in the
suspend callback. In the resume callback, the counter check in
pmc_core_is_s0ix_failed()
no longer makes any sense. It either fails all the time (if
pmcdev->s0ix_counter was inited with a non-
zero value) or succeeds all the time (if pmcdev->s0ix_counter was zero-inited).

>                 return 0;
>
> -       if (!pmc_core_is_s0ix_failed(pmcdev))
> +       if (!warn_on_s0ix_failures)
>                 return 0;
>
>         if (pmc_core_is_pc10_failed(pmcdev)) {
> diff --git a/drivers/platform/x86/intel/pmc/core.h b/drivers/platform/x86/intel/pmc/core.h
> index 7a059e02c265..5687e91e884c 100644
> --- a/drivers/platform/x86/intel/pmc/core.h
> +++ b/drivers/platform/x86/intel/pmc/core.h
> @@ -316,7 +316,6 @@ struct pmc_reg_map {
>   * @pmc_xram_read_bit: flag to indicate whether PMC XRAM shadow registers
>   *                     used to read MPHY PG and PLL status are available
>   * @mutex_lock:                mutex to complete one transcation
> - * @check_counters:    On resume, check if counters are getting incremented
>   * @pc10_counter:      PC10 residency counter
>   * @s0ix_counter:      S0ix residency (step adjusted)
>   * @num_lpm_modes:     Count of enabled modes
> --
> 2.34.1
>