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;
Message-ID: <fc37371bb116cabdd9d2ae114c0f34a818e9c4c2.camel@linux.intel.com>
Subject: Re: [PATCH v8 4/4] platform/x86/intel/pmc: core: Report duration of
 time in HW sleep state
From:   "David E. Box" <david.e.box@linux.intel.com>
Reply-To: david.e.box@linux.intel.com
To:     "Limonciello, Mario" <Mario.Limonciello@amd.com>,
        Ilpo =?ISO-8859-1?Q?J=E4rvinen?= <ilpo.jarvinen@linux.intel.com>,
        Box David E <david.e.box@intel.com>
Cc:     "jstultz@google.com" <jstultz@google.com>,
        "pavel@ucw.cz" <pavel@ucw.cz>,
        "svenva@chromium.org" <svenva@chromium.org>,
        Rajneesh Bhardwaj <irenic.rajneesh@gmail.com>,
        "S-k, Shyam-sundar" <Shyam-sundar.S-k@amd.com>,
        "rrangel@chromium.org" <rrangel@chromium.org>,
        Jain Rajat <rajatja@google.com>,
        "hdegoede@redhat.com" <hdegoede@redhat.com>,
        Mark Gross <markgross@kernel.org>,
        "platform-driver-x86@vger.kernel.org" 
        <platform-driver-x86@vger.kernel.org>,
        LKML <linux-kernel@vger.kernel.org>
Date:   Thu, 13 Apr 2023 17:35:51 -0700
In-Reply-To: <MN0PR12MB6101F7E14E3AD1ECFD6C2F25E2989@MN0PR12MB6101.namprd12.prod.outlook.com>
References: <20230412194917.7164-1-mario.limonciello@amd.com>
         <20230412194917.7164-5-mario.limonciello@amd.com>
         <5d904d5a-d25e-7d6d-57e4-ca451bcead57@linux.intel.com>
         <MN0PR12MB6101F7E14E3AD1ECFD6C2F25E2989@MN0PR12MB6101.namprd12.prod.outlook.com>
Organization: David E. Box
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.44.4 (3.44.4-2.fc36) 
MIME-Version: 1.0
Precedence: bulk

On Thu, 2023-04-13 at 22:40 +0000, Limonciello, Mario wrote:
> [Public]
>=20
>=20
>=20
> > -----Original Message-----
> > From: Ilpo J=C3=A4rvinen <ilpo.jarvinen@linux.intel.com>
> > Sent: Thursday, April 13, 2023 04:24
> > To: Limonciello, Mario <Mario.Limonciello@amd.com>
> > Cc: Box David E <david.e.box@intel.com>; jstultz@google.com;
> > pavel@ucw.cz; svenva@chromium.org; Rajneesh Bhardwaj
> > <irenic.rajneesh@gmail.com>; S-k, Shyam-sundar <Shyam-sundar.S-
> > k@amd.com>; rrangel@chromium.org; Jain Rajat <rajatja@google.com>;
> > hdegoede@redhat.com; Mark Gross <markgross@kernel.org>; platform-
> > driver-x86@vger.kernel.org; LKML <linux-kernel@vger.kernel.org>
> > Subject: Re: [PATCH v8 4/4] platform/x86/intel/pmc: core: Report durati=
on of
> > time in HW sleep state
> >=20
> > On Wed, 12 Apr 2023, Mario Limonciello wrote:
> >=20
> > > 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.
> > >=20
> > > Report this to the standard kernel reporting infrastructure so that
> > > userspace software can query after the suspend cycle is done.
> > >=20
> > > Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> > > ---
> > > v7->v8:
> > > =C2=A0* Report max sleep as well
> > > ---
> > > =C2=A0drivers/platform/x86/intel/pmc/core.c | 3 +++
> > > =C2=A01 file changed, 3 insertions(+)
> > >=20
> > > diff --git a/drivers/platform/x86/intel/pmc/core.c
> > b/drivers/platform/x86/intel/pmc/core.c
> > > index 925c5d676a43..f9677104353d 100644
> > > --- a/drivers/platform/x86/intel/pmc/core.c
> > > +++ b/drivers/platform/x86/intel/pmc/core.c
> > > @@ -1153,6 +1153,7 @@ static int pmc_core_probe(struct platform_devic=
e
> > *pdev)
> > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0pmc_core_do_dmi_quirk=
s(pmcdev);
> > >=20
> > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0pmc_core_dbgfs_regist=
er(pmcdev);
> > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0pm_report_max_hw_sleep(((1=
UL << 32) - 1) *
> > pmc_core_adjust_slp_s0_step(pmcdev, 1));
> >=20
> > Technically this is FIELD_MAX(SLP_S0_RES_COUNTER_MASK) *
> > pmc_core_adjust...?
> > Where the define is:
> > #define SLP_S0_RES_COUNTER_MASK=C2=A0GENMASK(31, 0)
>=20
> That's fine by me to switch it over, it certainly makes it a lot more
> readable.
> I took the value from @Box David E to use suggested in v7, so what are yo=
ur
> thoughts?

Ilpo's suggestion is preferable. The warning comes from using 1UL, long bei=
ng 4
bytes on i386.

>=20
> The current version has an overflow error reported by the robot for i386,=
 so
> it
> definitely needs some sort of change.

Resolved by using the macro. With Ilpo's suggestion you can add my reviewed=
 by.
Thanks.

David

>=20
> >=20
> > >=20
> > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0device_initialized =
=3D true;
> > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0dev_info(&pdev->dev, =
" initialized\n");
> > > @@ -1214,6 +1215,8 @@ static inline bool pmc_core_is_s0ix_failed(stru=
ct
> > pmc_dev *pmcdev)
> > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0if (pmc_core_dev_stat=
e_get(pmcdev, &s0ix_counter))
> > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0=C2=A0return false;
> > >=20
> > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0pm_report_hw_sleep_time((u=
32)(s0ix_counter - pmcdev-
> > > s0ix_counter));
> > > +
> > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0if (s0ix_counter =3D=
=3D pmcdev->s0ix_counter)
> > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0=C2=A0return true;
> > >=20
> > >=20
> >=20
> > --
> > =C2=A0i.