Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20230330194439.14361-1-mario.limonciello@amd.com>
 <20230330194439.14361-5-mario.limonciello@amd.com> <CAJZ5v0jabz27wk-g=iZRSnpUCmWimfjSUZO+iuNeRbuzPxW=DA@mail.gmail.com>
 <59127d2ac2e60d59b5711517eb049eee334a3cdd.camel@intel.com> <3f7ec97b-5fa6-9c54-b4b2-58ebf4f88449@amd.com>
In-Reply-To: <3f7ec97b-5fa6-9c54-b4b2-58ebf4f88449@amd.com>
From:   "Rafael J. Wysocki" <rafael@kernel.org>
Date:   Mon, 3 Apr 2023 20:32:59 +0200
Message-ID: <CAJZ5v0iE-cikqtMFaJCs_2cMENh65KRJhZs3Z_voogJ4qZpTBA@mail.gmail.com>
Subject: Re: [PATCH v5 4/4] platform/x86/intel/pmc: core: Report duration of
 time in HW sleep state
To:     "Limonciello, Mario" <mario.limonciello@amd.com>
Cc:     "Box, David E" <david.e.box@intel.com>,
        "rafael@kernel.org" <rafael@kernel.org>,
        "jstultz@google.com" <jstultz@google.com>,
        "Shyam-sundar.S-k@amd.com" <Shyam-sundar.S-k@amd.com>,
        "markgross@kernel.org" <markgross@kernel.org>,
        "rrangel@chromium.org" <rrangel@chromium.org>,
        "Jain, Rajat" <rajatja@google.com>,
        "irenic.rajneesh@gmail.com" <irenic.rajneesh@gmail.com>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "hdegoede@redhat.com" <hdegoede@redhat.com>,
        "svenva@chromium.org" <svenva@chromium.org>,
        "linux-pm@vger.kernel.org" <linux-pm@vger.kernel.org>,
        "platform-driver-x86@vger.kernel.org" 
        <platform-driver-x86@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 8BIT
Precedence: bulk

On Mon, Apr 3, 2023 at 8:07 PM Limonciello, Mario
<mario.limonciello@amd.com> wrote:
>
> On 4/3/2023 13:00, Box, David E wrote:
> > On Fri, 2023-03-31 at 20:05 +0200, Rafael J. Wysocki wrote:
> >> On Thu, Mar 30, 2023 at 9:45 PM Mario Limonciello
> >> <mario.limonciello@amd.com> wrote:
> >>>
> >>> intel_pmc_core displays a warning when the module parameter
> >>> `warn_on_s0ix_failures` is set and a suspend didn't get to a HW sleep
> >>> state.
> >>>
> >>> Report this to the standard kernel reporting infrastructure so that
> >>> userspace software can query after the suspend cycle is done.
> >>>
> >>> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> >>> ---
> >>> v4->v5:
> >>>   * Reword commit message
> >>> ---
> >>>   drivers/platform/x86/intel/pmc/core.c | 2 ++
> >>>   1 file changed, 2 insertions(+)
> >>>
> >>> diff --git a/drivers/platform/x86/intel/pmc/core.c
> >>> b/drivers/platform/x86/intel/pmc/core.c
> >>> index e2f171fac094..980af32dd48a 100644
> >>> --- a/drivers/platform/x86/intel/pmc/core.c
> >>> +++ b/drivers/platform/x86/intel/pmc/core.c
> >>> @@ -1203,6 +1203,8 @@ static inline bool pmc_core_is_s0ix_failed(struct
> >>> pmc_dev *pmcdev)
> >>>          if (pmc_core_dev_state_get(pmcdev, &s0ix_counter))
> >>>                  return false;
> >>>
> >>> +       pm_set_hw_sleep_time(s0ix_counter - pmcdev->s0ix_counter);
> >>> +
> >>
> >> Maybe check if this is really accumulating?  In case of a counter
> >> overflow, for instance?
> >
> > Overflow is likely on some systems. The counter is only 32-bit and at our
> > smallest granularity of 30.5us per tick it could overflow after a day and a half
> > of s0ix time, though most of our systems have a higher granularity that puts
> > them around 6 days.
> >
> > This brings up an issue that the attribute cannot be trusted if the system is
> > suspended for longer than the maximum hardware counter time. Should be noted in
> > the Documentation.
>
> I think it would be rather confusing for userspace having to account for
> this and it's better to abstract it in the kernel.
>
> How can you discover the granularity a system can support?
> How would you know overflow actually happened?  Is there a bit somewhere
> else that could tell you?

I'm not really sure if there is a generally usable overflow detection for this.

> In terms of ABI how about when we know overflow occurred and userspace
> reads the sysfs file we return -EOVERFLOW instead of a potentially bad
> value?

So if the new value is greater than the old one, you don't really know
whether or not an overflow has taken place.

And so I would just document the fact that the underlying HW/firmware
counter overflows as suggested by Dave.