linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Mario Limonciello <mario.limonciello@amd.com>
To: Sven van Ashbrook <svenva@chromium.org>
Cc: Rajneesh Bhardwaj <irenic.rajneesh@gmail.com>,
	Hans de Goede <hdegoede@redhat.com>,
	LKML <linux-kernel@vger.kernel.org>,
	"S-k, Shyam-sundar" <Shyam-sundar.S-k@amd.com>,
	"rrangel@chromium.org" <rrangel@chromium.org>,
	"platform-driver-x86@vger.kernel.org" 
	<platform-driver-x86@vger.kernel.org>,
	Rajneesh Bhardwaj <rajneesh.bhardwaj@intel.com>,
	Rafael J Wysocki <rjw@rjwysocki.net>,
	Rajat Jain <rajatja@google.com>,
	David E Box <david.e.box@intel.com>,
	Mark Gross <markgross@kernel.org>
Subject: Re: [PATCH v1] platform/x86: intel_pmc_core: promote S0ix failure warn() to WARN()
Date: Mon, 31 Oct 2022 20:58:27 -0500	[thread overview]
Message-ID: <cb5ab68e-5034-d937-e28e-e838e50172a8@amd.com> (raw)
In-Reply-To: <CAG-rBijSASfbfWQNarjGqj2UxQDOSdwM-qj5YA5A9ur=DNJf-g@mail.gmail.com>

On 10/31/22 20:38, Sven van Ashbrook wrote:
> On Mon, Oct 31, 2022 at 3:39 PM Limonciello, Mario
> <Mario.Limonciello@amd.com> wrote:
>>
>> Just thinking about it a little bit more, it could be a lot nicer to have something like:
>>
>> /sys/power/suspend_stats/last_hw_deepest_state
> 
> While I agree that reporting through a framework is generally better
> than getting infrastructure to grep for specific strings, I believe
> that a simple sysfs file is probably too simplistic.
> 
> 1. We need more sophisticated reporting than just last_hw_deepest_state:
> 
> - sometimes the system enters the deep state we want, yet after a
> while moves back up and gets "stuck" in an intermediate state (below
> S0). Or, the system enters the deep state we want, but moves back to
> S0 after a time without apparent reason. These platform-dependent
> failures are not so easily describable in a generic framework.

I actually thought that by putting the duration of time put in 
last_hw_deepest_state you'll be able to catch this by comparing the 
duration of the suspend to the duration of last_hw_deepest_state.

If you're below some threshold of percent for suspends that are at least 
some other threshold long you can trigger the failure.

This then lets you tune your framework to find the right place for those
thresholds too without needing to change the kernel.

> 
> - ChromeOS in particular has multiple independent S0ix / S3 / s2idle
> failure report sources. We have the kernel warning above; also our
> Embedded Controller monitors suspend failure cases which the simple
> kernel warning cannot catch, reported through a separate WARN_ONCE().
>  > 2. A simple sysfs file will need to be polled by the infrastructure
> after every suspend; it would be preferable to have some signal or
> callback which the infrastructure could register itself with.

The interface to trigger a suspend is writing a value into 
/sys/power/state.  You'll get a return code from this, but this return 
code does not represent whether you got to the deepest state, just 
whether the suspend succeeded or not.

So what would an ideal interface that sends a signal that the last 
"successful" suspend didn't get to the deepest state look like to you?

> 
> The generic infrastructure to support this sounds like quite a bit of
> work, and for what gain? Compared to simply matching a log string and
> sending the whole dmesg if there's a match.

I would like to think it's cheaper to read the sysfs file, do a local 
comparison on HW deepest time to the suspend time and then only send the 
the dmesg up for further analysis.

> 
> Is the light worth the candle?

I wrote an RFC that I sent out for it with my ideas at least.


  reply	other threads:[~2022-11-01  1:58 UTC|newest]

Thread overview: 15+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-10-27 15:19 [PATCH v1] platform/x86: intel_pmc_core: promote S0ix failure warn() to WARN() Sven van Ashbrook
2022-10-27 15:40 ` Hans de Goede
2022-10-27 15:47   ` Limonciello, Mario
2022-10-27 15:51     ` Sven van Ashbrook
     [not found]       ` <CAE2upjS6qRGRcuVYuAB5DMf66A7VcfCKKYEkpsr1My7RnKDFtQ@mail.gmail.com>
2022-10-29  4:11         ` Sven van Ashbrook
2022-10-31 19:39           ` Limonciello, Mario
2022-10-31 20:15             ` Hans de Goede
2022-10-31 20:20               ` Sven van Ashbrook
     [not found]             ` <CACK8Z6E7=xt118d47FTpmgKHgUBgH48FQzTi5iL90C3MjHb-3Q@mail.gmail.com>
2022-10-31 20:55               ` Limonciello, Mario
2022-11-01 17:24                 ` Sven van Ashbrook
2022-11-01 20:30                   ` David E. Box
2022-11-01  1:38             ` Sven van Ashbrook
2022-11-01  1:58               ` Mario Limonciello [this message]
2022-11-01 13:50                 ` Sven van Ashbrook
2022-10-27 16:37   ` David E. Box

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=cb5ab68e-5034-d937-e28e-e838e50172a8@amd.com \
    --to=mario.limonciello@amd.com \
    --cc=Shyam-sundar.S-k@amd.com \
    --cc=david.e.box@intel.com \
    --cc=hdegoede@redhat.com \
    --cc=irenic.rajneesh@gmail.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=markgross@kernel.org \
    --cc=platform-driver-x86@vger.kernel.org \
    --cc=rajatja@google.com \
    --cc=rajneesh.bhardwaj@intel.com \
    --cc=rjw@rjwysocki.net \
    --cc=rrangel@chromium.org \
    --cc=svenva@chromium.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).