[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <MN0PR12MB6101D5A3F649F912BAD9D741E2149@MN0PR12MB6101.namprd12.prod.outlook.com>
Date: Thu, 1 Dec 2022 20:12:01 +0000
From: "Limonciello, Mario" <Mario.Limonciello@....com>
To: Sven van Ashbrook <svenva@...omium.org>
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" <linux-pm@...r.kernel.org>,
"platform-driver-x86@...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" <linux-kernel@...r.kernel.org>,
Mark Gross <markgross@...nel.org>
Subject: RE: [RFC v4 3/5] platform/x86/intel/pmc: core: Drop check_counters
[Public]
> -----Original Message-----
> From: Sven van Ashbrook <svenva@...omium.org>
> Sent: Wednesday, November 23, 2022 11:47
> To: Limonciello, Mario <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).
Ah yeah; So this patch probably doesn't make sense as is. It would mean
either needing to check pm_suspend_via_firmware() in the resume callback
or just skipping it. I'll drop it in the next revision of this.
>
> > 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
> >
Download attachment "winmail.dat" of type "application/ms-tnef" (19283 bytes)
Powered by blists - more mailing lists