diff mbox series

[15/20] ACPICA: executer/exsystem: Warn about sleeps greater than 10 ms

Message ID 4200238.ejJDZkT8p0@kreacher
State New
Headers show
Series ACPICA: ACPICA 20220331 | expand

Commit Message

Rafael J. Wysocki April 11, 2022, 6:59 p.m. UTC 
From: Paul Menzel <pmenzel@molgen.mpg.de>

ACPICA commit 2a0d1d475e7ea1c815bee1e0692d81db9a7c909c

Quick boottime is important, so warn about sleeps greater than 10 ms.

Distribution Linux kernels reach initrd in 350 ms, so excessive delays
should be called out. 10 ms is chosen randomly, but three of such delays
would already make up ten percent of the boottime.

Link: https://github.com/acpica/acpica/commit/2a0d1d47
Signed-off-by: Bob Moore <robert.moore@intel.com>
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 exsystem.c |   10 ++++++++++
 1 file changed, 10 insertions(+)

Comments

kernel test robot April 12, 2022, 5:22 a.m. UTC | #1 
Hi "Rafael,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on rafael-pm/linux-next]
[also build test ERROR on linus/master linux/master v5.18-rc2 next-20220411]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/intel-lab-lkp/linux/commits/Rafael-J-Wysocki/ACPICA-ACPICA-20220331/20220412-030922
base:   https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git linux-next
config: i386-randconfig-a006-20220411 (https://download.01.org/0day-ci/archive/20220412/202204121322.P9yX0gKP-lkp@intel.com/config)
compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project fe2478d44e4f7f191c43fef629ac7a23d0251e72)
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/22c298d3a077e7ea8503e9acf7ac83e6b1e10148
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Rafael-J-Wysocki/ACPICA-ACPICA-20220331/20220412-030922
        git checkout 22c298d3a077e7ea8503e9acf7ac83e6b1e10148
        # save the config file to linux build tree
        mkdir build_dir
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=i386 SHELL=/bin/bash

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

Note: the linux-review/Rafael-J-Wysocki/ACPICA-ACPICA-20220331/20220412-030922 HEAD 32181ae3d3173aeee41f709612dfa4d52951b39d builds fine.
      It only hurts bisectability.

All errors (new ones prefixed by >>):

   drivers/acpi/acpica/exsystem.c:140:7: error: use of undeclared identifier 'how_long_US'; did you mean 'how_long_us'?
                   if (how_long_US > 100) {
                       ^~~~~~~~~~~
                       how_long_us
   drivers/acpi/acpica/exsystem.c:123:41: note: 'how_long_us' declared here
   acpi_status acpi_ex_system_do_stall(u32 how_long_us)
                                           ^
>> drivers/acpi/acpica/exsystem.c:179:10: error: use of undeclared identifier 'how_long_us'; did you mean 'how_long_ms'?
                                 how_long_us));
                                 ^~~~~~~~~~~
                                 how_long_ms
   include/acpi/acoutput.h:203:54: note: expanded from macro 'ACPI_WARNING'
   #define ACPI_WARNING(plist)             acpi_warning plist
                                                        ^
   drivers/acpi/acpica/exsystem.c:164:41: note: 'how_long_ms' declared here
   acpi_status acpi_ex_system_do_sleep(u64 how_long_ms)
                                           ^
   2 errors generated.


vim +179 drivers/acpi/acpica/exsystem.c

   150	
   151	/*******************************************************************************
   152	 *
   153	 * FUNCTION:    acpi_ex_system_do_sleep
   154	 *
   155	 * PARAMETERS:  how_long_ms     - The amount of time to sleep,
   156	 *                                in milliseconds
   157	 *
   158	 * RETURN:      None
   159	 *
   160	 * DESCRIPTION: Sleep the running thread for specified amount of time.
   161	 *
   162	 ******************************************************************************/
   163	
   164	acpi_status acpi_ex_system_do_sleep(u64 how_long_ms)
   165	{
   166		ACPI_FUNCTION_ENTRY();
   167	
   168		/* Since this thread will sleep, we must release the interpreter */
   169	
   170		acpi_ex_exit_interpreter();
   171	
   172		/*
   173		 * Warn users about excessive sleep times, so ASL code can be improved to
   174		 * use polling or similar techniques.
   175		 */
   176		if (how_long_ms > 10) {
   177			ACPI_WARNING((AE_INFO,
   178				      "Firmware issue: Excessive sleep time (%llu ms > 10 ms) in ACPI Control Method",
 > 179				      how_long_us));
   180		}
   181	
   182		/*
   183		 * For compatibility with other ACPI implementations and to prevent
   184		 * accidental deep sleeps, limit the sleep time to something reasonable.
   185		 */
   186		if (how_long_ms > ACPI_MAX_SLEEP) {
   187			how_long_ms = ACPI_MAX_SLEEP;
   188		}
   189	
   190		acpi_os_sleep(how_long_ms);
   191	
   192		/* And now we must get the interpreter again */
   193	
   194		acpi_ex_enter_interpreter();
   195		return (AE_OK);
   196	}
   197
Rafael J. Wysocki May 21, 2022, 4:11 p.m. UTC | #2 
On Mon, Apr 11, 2022 at 9:04 PM Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>
> From: Paul Menzel <pmenzel@molgen.mpg.de>
>
> ACPICA commit 2a0d1d475e7ea1c815bee1e0692d81db9a7c909c
>
> Quick boottime is important, so warn about sleeps greater than 10 ms.
>
> Distribution Linux kernels reach initrd in 350 ms, so excessive delays
> should be called out. 10 ms is chosen randomly, but three of such delays
> would already make up ten percent of the boottime.
>
> Link: https://github.com/acpica/acpica/commit/2a0d1d47
> Signed-off-by: Bob Moore <robert.moore@intel.com>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

I have decided to revert this, because it spams logs with unuseful
warnings on many production systems.

Power management AML uses sleeps above 10 ms for PCI spec compliance
and polling is not useful in those cases.

I will submit an analogous revert for upstream ACPICA.

> ---
>  exsystem.c |   10 ++++++++++
>  1 file changed, 10 insertions(+)
>
> diff -Nurp linux.before_name/drivers/acpi/acpica/exsystem.c linux.after_name/drivers/acpi/acpica/exsystem.c
> --- linux.before_name/drivers/acpi/acpica/exsystem.c    2022-04-01 18:26:54.958046947 +0200
> +++ linux.after_name/drivers/acpi/acpica/exsystem.c     2022-04-01 18:26:51.760086923 +0200
> @@ -170,6 +170,16 @@ acpi_status acpi_ex_system_do_sleep(u64
>         acpi_ex_exit_interpreter();
>
>         /*
> +        * Warn users about excessive sleep times, so ASL code can be improved to
> +        * use polling or similar techniques.
> +        */
> +       if (how_long_ms > 10) {
> +               ACPI_WARNING((AE_INFO,
> +                             "Firmware issue: Excessive sleep time (%llu ms > 10 ms) in ACPI Control Method",
> +                             how_long_us));
> +       }
> +
> +       /*
>          * For compatibility with other ACPI implementations and to prevent
>          * accidental deep sleeps, limit the sleep time to something reasonable.
>          */
>
>
>
Paul Menzel May 21, 2022, 11:28 p.m. UTC | #3 
Dear Rafael,


Am 21.05.22 um 18:11 schrieb Rafael J. Wysocki:
> On Mon, Apr 11, 2022 at 9:04 PM Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>>
>> From: Paul Menzel <pmenzel@molgen.mpg.de>
>>
>> ACPICA commit 2a0d1d475e7ea1c815bee1e0692d81db9a7c909c
>>
>> Quick boottime is important, so warn about sleeps greater than 10 ms.
>>
>> Distribution Linux kernels reach initrd in 350 ms, so excessive delays
>> should be called out. 10 ms is chosen randomly, but three of such delays
>> would already make up ten percent of the boottime.
>>
>> Link: https://github.com/acpica/acpica/commit/2a0d1d47
>> Signed-off-by: Bob Moore <robert.moore@intel.com>
>> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> I have decided to revert this, because it spams logs with unuseful
> warnings on many production systems.

Thank you for the information. Can you please give an example?

> Power management AML uses sleeps above 10 ms for PCI spec compliance
> and polling is not useful in those cases.

Can you please tell me what delays are used? Maybe we can increase the 
threshold to the one required by the PCI spec?


Kind regards,

Paul


> I will submit an analogous revert for upstream ACPICA.
> 
>> ---
>>   exsystem.c |   10 ++++++++++
>>   1 file changed, 10 insertions(+)
>>
>> diff -Nurp linux.before_name/drivers/acpi/acpica/exsystem.c linux.after_name/drivers/acpi/acpica/exsystem.c
>> --- linux.before_name/drivers/acpi/acpica/exsystem.c    2022-04-01 18:26:54.958046947 +0200
>> +++ linux.after_name/drivers/acpi/acpica/exsystem.c     2022-04-01 18:26:51.760086923 +0200
>> @@ -170,6 +170,16 @@ acpi_status acpi_ex_system_do_sleep(u64
>>          acpi_ex_exit_interpreter();
>>
>>          /*
>> +        * Warn users about excessive sleep times, so ASL code can be improved to
>> +        * use polling or similar techniques.
>> +        */
>> +       if (how_long_ms > 10) {
>> +               ACPI_WARNING((AE_INFO,
>> +                             "Firmware issue: Excessive sleep time (%llu ms > 10 ms) in ACPI Control Method",
>> +                             how_long_us));
>> +       }
>> +
>> +       /*
>>           * For compatibility with other ACPI implementations and to prevent
>>           * accidental deep sleeps, limit the sleep time to something reasonable.
>>           */
>>
>>
>>
Rafael J. Wysocki June 14, 2022, 1:25 p.m. UTC | #4 
Hi Paul,

Sorry for the delay.

On Sun, May 22, 2022 at 1:28 AM Paul Menzel <pmenzel@molgen.mpg.de> wrote:
>
> Dear Rafael,
>
>
> Am 21.05.22 um 18:11 schrieb Rafael J. Wysocki:
> > On Mon, Apr 11, 2022 at 9:04 PM Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> >>
> >> From: Paul Menzel <pmenzel@molgen.mpg.de>
> >>
> >> ACPICA commit 2a0d1d475e7ea1c815bee1e0692d81db9a7c909c
> >>
> >> Quick boottime is important, so warn about sleeps greater than 10 ms.
> >>
> >> Distribution Linux kernels reach initrd in 350 ms, so excessive delays
> >> should be called out. 10 ms is chosen randomly, but three of such delays
> >> would already make up ten percent of the boottime.
> >>
> >> Link: https://github.com/acpica/acpica/commit/2a0d1d47
> >> Signed-off-by: Bob Moore <robert.moore@intel.com>
> >> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >
> > I have decided to revert this, because it spams logs with unuseful
> > warnings on many production systems.
>
> Thank you for the information. Can you please give an example?

Personally, I saw this on Dell XPS13 9360 and 9380 machines in my
office, but it has been reported to be that it was visible on multiple
systems in the Linux client power lab at Intel.

You can also see here that Linux is not the only affected OS:
https://github.com/acpica/acpica/pull/780

Thanks!
diff mbox series

Patch

diff -Nurp linux.before_name/drivers/acpi/acpica/exsystem.c linux.after_name/drivers/acpi/acpica/exsystem.c
--- linux.before_name/drivers/acpi/acpica/exsystem.c	2022-04-01 18:26:54.958046947 +0200
+++ linux.after_name/drivers/acpi/acpica/exsystem.c	2022-04-01 18:26:51.760086923 +0200
@@ -170,6 +170,16 @@  acpi_status acpi_ex_system_do_sleep(u64
 	acpi_ex_exit_interpreter();
 
 	/*
+	 * Warn users about excessive sleep times, so ASL code can be improved to
+	 * use polling or similar techniques.
+	 */
+	if (how_long_ms > 10) {
+		ACPI_WARNING((AE_INFO,
+			      "Firmware issue: Excessive sleep time (%llu ms > 10 ms) in ACPI Control Method",
+			      how_long_us));
+	}
+
+	/*
 	 * For compatibility with other ACPI implementations and to prevent
 	 * accidental deep sleeps, limit the sleep time to something reasonable.
 	 */