diff mbox series

[v8,4/4] platform/x86/intel/pmc: core: Report duration of time in HW sleep state

Message ID 20230412194917.7164-5-mario.limonciello@amd.com
State New
Headers
Series Add vendor agnostic mechanism to report hardware sleep |

Commit Message

Mario Limonciello April 12, 2023, 7:49 p.m. UTC 
  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>
---
v7->v8:
 * Report max sleep as well
---
 drivers/platform/x86/intel/pmc/core.c | 3 +++
 1 file changed, 3 insertions(+)

Comments

kernel test robot April 13, 2023, 1:50 a.m. UTC | #1 
Hi Mario,

kernel test robot noticed the following build warnings:

[auto build test WARNING on 09a9639e56c01c7a00d6c0ca63f4c7c41abe075d]

url:    https://github.com/intel-lab-lkp/linux/commits/Mario-Limonciello/PM-Add-sysfs-files-to-represent-time-spent-in-hardware-sleep-state/20230413-035220
base:   09a9639e56c01c7a00d6c0ca63f4c7c41abe075d
patch link:    https://lore.kernel.org/r/20230412194917.7164-5-mario.limonciello%40amd.com
patch subject: [PATCH v8 4/4] platform/x86/intel/pmc: core: Report duration of time in HW sleep state
config: i386-randconfig-a004-20230410 (https://download.01.org/0day-ci/archive/20230413/202304130908.LOiMWRYR-lkp@intel.com/config)
compiler: clang version 14.0.6 (https://github.com/llvm/llvm-project f28c006a5895fc0e329fe15fead81e37457cb1d1)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/315b1dd23cbedfd2848c8ac8ec1f77c3610b955e
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Mario-Limonciello/PM-Add-sysfs-files-to-represent-time-spent-in-hardware-sleep-state/20230413-035220
        git checkout 315b1dd23cbedfd2848c8ac8ec1f77c3610b955e
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=i386 olddefconfig
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=i386 SHELL=/bin/bash drivers/platform/x86/intel/pmc/

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202304130908.LOiMWRYR-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/platform/x86/intel/pmc/core.c:1156:31: warning: shift count >= width of type [-Wshift-count-overflow]
           pm_report_max_hw_sleep(((1UL << 32) - 1) * pmc_core_adjust_slp_s0_step(pmcdev, 1));
                                        ^  ~~
   1 warning generated.


vim +1156 drivers/platform/x86/intel/pmc/core.c

  1097	
  1098	static int pmc_core_probe(struct platform_device *pdev)
  1099	{
  1100		static bool device_initialized;
  1101		struct pmc_dev *pmcdev;
  1102		const struct x86_cpu_id *cpu_id;
  1103		void (*core_init)(struct pmc_dev *pmcdev);
  1104		u64 slp_s0_addr;
  1105	
  1106		if (device_initialized)
  1107			return -ENODEV;
  1108	
  1109		pmcdev = devm_kzalloc(&pdev->dev, sizeof(*pmcdev), GFP_KERNEL);
  1110		if (!pmcdev)
  1111			return -ENOMEM;
  1112	
  1113		platform_set_drvdata(pdev, pmcdev);
  1114		pmcdev->pdev = pdev;
  1115	
  1116		cpu_id = x86_match_cpu(intel_pmc_core_ids);
  1117		if (!cpu_id)
  1118			return -ENODEV;
  1119	
  1120		core_init = (void  (*)(struct pmc_dev *))cpu_id->driver_data;
  1121	
  1122		/*
  1123		 * Coffee Lake has CPU ID of Kaby Lake and Cannon Lake PCH. So here
  1124		 * Sunrisepoint PCH regmap can't be used. Use Cannon Lake PCH regmap
  1125		 * in this case.
  1126		 */
  1127		if (core_init == spt_core_init && !pci_dev_present(pmc_pci_ids))
  1128			core_init = cnp_core_init;
  1129	
  1130		mutex_init(&pmcdev->lock);
  1131		core_init(pmcdev);
  1132	
  1133	
  1134		if (lpit_read_residency_count_address(&slp_s0_addr)) {
  1135			pmcdev->base_addr = PMC_BASE_ADDR_DEFAULT;
  1136	
  1137			if (page_is_ram(PHYS_PFN(pmcdev->base_addr)))
  1138				return -ENODEV;
  1139		} else {
  1140			pmcdev->base_addr = slp_s0_addr - pmcdev->map->slp_s0_offset;
  1141		}
  1142	
  1143		pmcdev->regbase = ioremap(pmcdev->base_addr,
  1144					  pmcdev->map->regmap_length);
  1145		if (!pmcdev->regbase)
  1146			return -ENOMEM;
  1147	
  1148		if (pmcdev->core_configure)
  1149			pmcdev->core_configure(pmcdev);
  1150	
  1151		pmcdev->pmc_xram_read_bit = pmc_core_check_read_lock_bit(pmcdev);
  1152		pmc_core_get_low_power_modes(pdev);
  1153		pmc_core_do_dmi_quirks(pmcdev);
  1154	
  1155		pmc_core_dbgfs_register(pmcdev);
> 1156		pm_report_max_hw_sleep(((1UL << 32) - 1) * pmc_core_adjust_slp_s0_step(pmcdev, 1));
  1157	
  1158		device_initialized = true;
  1159		dev_info(&pdev->dev, " initialized\n");
  1160	
  1161		return 0;
  1162	}
  1163
Ilpo Järvinen April 13, 2023, 9:23 a.m. UTC | #2 
On Wed, 12 Apr 2023, Mario Limonciello 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>
> ---
> v7->v8:
>  * Report max sleep as well
> ---
>  drivers/platform/x86/intel/pmc/core.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> 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_device *pdev)
>  	pmc_core_do_dmi_quirks(pmcdev);
>  
>  	pmc_core_dbgfs_register(pmcdev);
> +	pm_report_max_hw_sleep(((1UL << 32) - 1) * pmc_core_adjust_slp_s0_step(pmcdev, 1));

Technically this is FIELD_MAX(SLP_S0_RES_COUNTER_MASK) * pmc_core_adjust...? 
Where the define is:
#define SLP_S0_RES_COUNTER_MASK	GENMASK(31, 0)

>  
>  	device_initialized = true;
>  	dev_info(&pdev->dev, " initialized\n");
> @@ -1214,6 +1215,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_report_hw_sleep_time((u32)(s0ix_counter - pmcdev->s0ix_counter));
> +
>  	if (s0ix_counter == pmcdev->s0ix_counter)
>  		return true;
>  
>
Mario Limonciello April 13, 2023, 10:40 p.m. UTC | #3 
[Public]



> -----Original Message-----
> From: Ilpo Järvinen <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 duration of
> time in HW sleep state
> 
> On Wed, 12 Apr 2023, Mario Limonciello 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>
> > ---
> > v7->v8:
> >  * Report max sleep as well
> > ---
> >  drivers/platform/x86/intel/pmc/core.c | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > 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_device
> *pdev)
> >  	pmc_core_do_dmi_quirks(pmcdev);
> >
> >  	pmc_core_dbgfs_register(pmcdev);
> > +	pm_report_max_hw_sleep(((1UL << 32) - 1) *
> pmc_core_adjust_slp_s0_step(pmcdev, 1));
> 
> Technically this is FIELD_MAX(SLP_S0_RES_COUNTER_MASK) *
> pmc_core_adjust...?
> Where the define is:
> #define SLP_S0_RES_COUNTER_MASK	GENMASK(31, 0)

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 your
thoughts?

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

> 
> >
> >  	device_initialized = true;
> >  	dev_info(&pdev->dev, " initialized\n");
> > @@ -1214,6 +1215,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_report_hw_sleep_time((u32)(s0ix_counter - pmcdev-
> >s0ix_counter));
> > +
> >  	if (s0ix_counter == pmcdev->s0ix_counter)
> >  		return true;
> >
> >
> 
> --
>  i.
David E. Box April 14, 2023, 12:35 a.m. UTC | #4 
On Thu, 2023-04-13 at 22:40 +0000, Limonciello, Mario wrote:
> [Public]
> 
> 
> 
> > -----Original Message-----
> > From: Ilpo Järvinen <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 duration of
> > time in HW sleep state
> > 
> > On Wed, 12 Apr 2023, Mario Limonciello 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>
> > > ---
> > > v7->v8:
> > >  * Report max sleep as well
> > > ---
> > >  drivers/platform/x86/intel/pmc/core.c | 3 +++
> > >  1 file changed, 3 insertions(+)
> > > 
> > > 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_device
> > *pdev)
> > >         pmc_core_do_dmi_quirks(pmcdev);
> > > 
> > >         pmc_core_dbgfs_register(pmcdev);
> > > +       pm_report_max_hw_sleep(((1UL << 32) - 1) *
> > pmc_core_adjust_slp_s0_step(pmcdev, 1));
> > 
> > Technically this is FIELD_MAX(SLP_S0_RES_COUNTER_MASK) *
> > pmc_core_adjust...?
> > Where the define is:
> > #define SLP_S0_RES_COUNTER_MASK GENMASK(31, 0)
> 
> 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 your
> thoughts?

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

> 
> 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

> 
> > 
> > > 
> > >         device_initialized = true;
> > >         dev_info(&pdev->dev, " initialized\n");
> > > @@ -1214,6 +1215,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_report_hw_sleep_time((u32)(s0ix_counter - pmcdev-
> > > s0ix_counter));
> > > +
> > >         if (s0ix_counter == pmcdev->s0ix_counter)
> > >                 return true;
> > > 
> > > 
> > 
> > --
> >  i.
kernel test robot April 14, 2023, 1:58 a.m. UTC | #5 
Hi Mario,

kernel test robot noticed the following build warnings:

[auto build test WARNING on 09a9639e56c01c7a00d6c0ca63f4c7c41abe075d]

url:    https://github.com/intel-lab-lkp/linux/commits/Mario-Limonciello/PM-Add-sysfs-files-to-represent-time-spent-in-hardware-sleep-state/20230413-035220
base:   09a9639e56c01c7a00d6c0ca63f4c7c41abe075d
patch link:    https://lore.kernel.org/r/20230412194917.7164-5-mario.limonciello%40amd.com
patch subject: [PATCH v8 4/4] platform/x86/intel/pmc: core: Report duration of time in HW sleep state
config: i386-randconfig-a001 (https://download.01.org/0day-ci/archive/20230414/202304140957.hkvWzLzM-lkp@intel.com/config)
compiler: gcc-11 (Debian 11.3.0-8) 11.3.0
reproduce (this is a W=1 build):
        # https://github.com/intel-lab-lkp/linux/commit/315b1dd23cbedfd2848c8ac8ec1f77c3610b955e
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Mario-Limonciello/PM-Add-sysfs-files-to-represent-time-spent-in-hardware-sleep-state/20230413-035220
        git checkout 315b1dd23cbedfd2848c8ac8ec1f77c3610b955e
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        make W=1 O=build_dir ARCH=i386 olddefconfig
        make W=1 O=build_dir ARCH=i386 SHELL=/bin/bash drivers/platform/x86/intel/pmc/

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202304140957.hkvWzLzM-lkp@intel.com/

All warnings (new ones prefixed by >>):

   drivers/platform/x86/intel/pmc/core.c: In function 'pmc_core_probe':
>> drivers/platform/x86/intel/pmc/core.c:1156:38: warning: left shift count >= width of type [-Wshift-count-overflow]
    1156 |         pm_report_max_hw_sleep(((1UL << 32) - 1) * pmc_core_adjust_slp_s0_step(pmcdev, 1));
         |                                      ^~


vim +1156 drivers/platform/x86/intel/pmc/core.c

  1097	
  1098	static int pmc_core_probe(struct platform_device *pdev)
  1099	{
  1100		static bool device_initialized;
  1101		struct pmc_dev *pmcdev;
  1102		const struct x86_cpu_id *cpu_id;
  1103		void (*core_init)(struct pmc_dev *pmcdev);
  1104		u64 slp_s0_addr;
  1105	
  1106		if (device_initialized)
  1107			return -ENODEV;
  1108	
  1109		pmcdev = devm_kzalloc(&pdev->dev, sizeof(*pmcdev), GFP_KERNEL);
  1110		if (!pmcdev)
  1111			return -ENOMEM;
  1112	
  1113		platform_set_drvdata(pdev, pmcdev);
  1114		pmcdev->pdev = pdev;
  1115	
  1116		cpu_id = x86_match_cpu(intel_pmc_core_ids);
  1117		if (!cpu_id)
  1118			return -ENODEV;
  1119	
  1120		core_init = (void  (*)(struct pmc_dev *))cpu_id->driver_data;
  1121	
  1122		/*
  1123		 * Coffee Lake has CPU ID of Kaby Lake and Cannon Lake PCH. So here
  1124		 * Sunrisepoint PCH regmap can't be used. Use Cannon Lake PCH regmap
  1125		 * in this case.
  1126		 */
  1127		if (core_init == spt_core_init && !pci_dev_present(pmc_pci_ids))
  1128			core_init = cnp_core_init;
  1129	
  1130		mutex_init(&pmcdev->lock);
  1131		core_init(pmcdev);
  1132	
  1133	
  1134		if (lpit_read_residency_count_address(&slp_s0_addr)) {
  1135			pmcdev->base_addr = PMC_BASE_ADDR_DEFAULT;
  1136	
  1137			if (page_is_ram(PHYS_PFN(pmcdev->base_addr)))
  1138				return -ENODEV;
  1139		} else {
  1140			pmcdev->base_addr = slp_s0_addr - pmcdev->map->slp_s0_offset;
  1141		}
  1142	
  1143		pmcdev->regbase = ioremap(pmcdev->base_addr,
  1144					  pmcdev->map->regmap_length);
  1145		if (!pmcdev->regbase)
  1146			return -ENOMEM;
  1147	
  1148		if (pmcdev->core_configure)
  1149			pmcdev->core_configure(pmcdev);
  1150	
  1151		pmcdev->pmc_xram_read_bit = pmc_core_check_read_lock_bit(pmcdev);
  1152		pmc_core_get_low_power_modes(pdev);
  1153		pmc_core_do_dmi_quirks(pmcdev);
  1154	
  1155		pmc_core_dbgfs_register(pmcdev);
> 1156		pm_report_max_hw_sleep(((1UL << 32) - 1) * pmc_core_adjust_slp_s0_step(pmcdev, 1));
  1157	
  1158		device_initialized = true;
  1159		dev_info(&pdev->dev, " initialized\n");
  1160	
  1161		return 0;
  1162	}
  1163
diff mbox series

Patch

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_device *pdev)
 	pmc_core_do_dmi_quirks(pmcdev);
 
 	pmc_core_dbgfs_register(pmcdev);
+	pm_report_max_hw_sleep(((1UL << 32) - 1) * pmc_core_adjust_slp_s0_step(pmcdev, 1));
 
 	device_initialized = true;
 	dev_info(&pdev->dev, " initialized\n");
@@ -1214,6 +1215,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_report_hw_sleep_time((u32)(s0ix_counter - pmcdev->s0ix_counter));
+
 	if (s0ix_counter == pmcdev->s0ix_counter)
 		return true;