`how_long` uses different units in both functions, so make it more
clear, what unit they expect.
Signed-off-by: Paul Menzel <[email protected]>
---
drivers/acpi/acpica/exsystem.c | 20 ++++++++++----------
1 file changed, 10 insertions(+), 10 deletions(-)
diff --git a/drivers/acpi/acpica/exsystem.c b/drivers/acpi/acpica/exsystem.c
index 1281c07112de..6bc5b46e6927 100644
--- a/drivers/acpi/acpica/exsystem.c
+++ b/drivers/acpi/acpica/exsystem.c
@@ -107,7 +107,7 @@ acpi_status acpi_ex_system_wait_mutex(acpi_mutex mutex, u16 timeout)
*
* FUNCTION: acpi_ex_system_do_stall
*
- * PARAMETERS: how_long - The amount of time to stall,
+ * PARAMETERS: how_long_us - The amount of time to stall,
* in microseconds
*
* RETURN: Status
@@ -120,13 +120,13 @@ acpi_status acpi_ex_system_wait_mutex(acpi_mutex mutex, u16 timeout)
*
******************************************************************************/
-acpi_status acpi_ex_system_do_stall(u32 how_long)
+acpi_status acpi_ex_system_do_stall(u32 how_long_us)
{
acpi_status status = AE_OK;
ACPI_FUNCTION_ENTRY();
- if (how_long > 255) { /* 255 microseconds */
+ if (how_long_us > 255) { /* 255 microseconds */
/*
* Longer than 255 usec, this is an error
*
@@ -134,10 +134,10 @@ acpi_status acpi_ex_system_do_stall(u32 how_long)
* order to support existing BIOSs)
*/
ACPI_ERROR((AE_INFO,
- "Time parameter is too large (%u)", how_long));
+ "Time parameter is too large (%u)", how_long_us));
status = AE_AML_OPERAND_VALUE;
} else {
- acpi_os_stall(how_long);
+ acpi_os_stall(how_long_us);
}
return (status);
@@ -147,7 +147,7 @@ acpi_status acpi_ex_system_do_stall(u32 how_long)
*
* FUNCTION: acpi_ex_system_do_sleep
*
- * PARAMETERS: how_long - The amount of time to sleep,
+ * PARAMETERS: how_long_ms - The amount of time to sleep,
* in milliseconds
*
* RETURN: None
@@ -156,7 +156,7 @@ acpi_status acpi_ex_system_do_stall(u32 how_long)
*
******************************************************************************/
-acpi_status acpi_ex_system_do_sleep(u64 how_long)
+acpi_status acpi_ex_system_do_sleep(u64 how_long_ms)
{
ACPI_FUNCTION_ENTRY();
@@ -168,11 +168,11 @@ acpi_status acpi_ex_system_do_sleep(u64 how_long)
* For compatibility with other ACPI implementations and to prevent
* accidental deep sleeps, limit the sleep time to something reasonable.
*/
- if (how_long > ACPI_MAX_SLEEP) {
- how_long = ACPI_MAX_SLEEP;
+ if (how_long_ms > ACPI_MAX_SLEEP) {
+ how_long_ms = ACPI_MAX_SLEEP;
}
- acpi_os_sleep(how_long);
+ acpi_os_sleep(how_long_ms);
/* And now we must get the interpreter again */
--
2.35.1
Inform users if firmware violates the ACPI specification.
Signed-off-by: Paul Menzel <[email protected]>
---
drivers/acpi/acpica/exsystem.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/acpi/acpica/exsystem.c b/drivers/acpi/acpica/exsystem.c
index 6bc5b46e6927..00f66af31ffa 100644
--- a/drivers/acpi/acpica/exsystem.c
+++ b/drivers/acpi/acpica/exsystem.c
@@ -137,6 +137,9 @@ acpi_status acpi_ex_system_do_stall(u32 how_long_us)
"Time parameter is too large (%u)", how_long_us));
status = AE_AML_OPERAND_VALUE;
} else {
+ if (how_long_us > 100) /* 100 microseconds */
+ ACPI_WARNING((AE_INFO,
+ "Time parameter %u us > 100 us violating ACPI spec, please fix the firmware.", how_long_us));
acpi_os_stall(how_long_us);
}
--
2.35.1
Quick boottime is important, so warn about sleeps greater than 50 ms in
ACPI.
50 ms is still long compared to distribution Linux kernels reaching initrd
in 350 ms, so should probably changed to 10 ms, so people are aware
about this.
Signed-off-by: Paul Menzel <[email protected]>
---
drivers/acpi/acpica/exsystem.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/drivers/acpi/acpica/exsystem.c b/drivers/acpi/acpica/exsystem.c
index 00f66af31ffa..bdffb8aac05c 100644
--- a/drivers/acpi/acpica/exsystem.c
+++ b/drivers/acpi/acpica/exsystem.c
@@ -167,6 +167,11 @@ acpi_status acpi_ex_system_do_sleep(u64 how_long_ms)
acpi_ex_exit_interpreter();
+ if (how_long_ms > 50) {
+ ACPI_WARNING((AE_INFO,
+ "Time parameter %llu > 50 ms. Please contact firmware vendor for more responsive system.", how_long_ms));
+ }
+
/*
* For compatibility with other ACPI implementations and to prevent
* accidental deep sleeps, limit the sleep time to something reasonable.
--
2.35.1
So, this is the current implementation:
/*
* For compatibility with other ACPI implementations and to prevent
* accidental deep sleeps, limit the sleep time to something reasonable.
*/
if (HowLong > ACPI_MAX_SLEEP)
{
HowLong = ACPI_MAX_SLEEP;
}
AcpiOsSleep (HowLong);
Where ACPI_MAX_SLEEP is:
#define ACPI_MAX_SLEEP 2000 /* 2000 millisec == two seconds */
-----Original Message-----
From: Paul Menzel <[email protected]>
Sent: Thursday, February 24, 2022 3:38 AM
To: Moore, Robert <[email protected]>; Wysocki, Rafael J <[email protected]>; Len Brown <[email protected]>
Cc: Paul Menzel <[email protected]>; [email protected]; [email protected]; [email protected]
Subject: [PATCH 3/4] acpi: exsystem: Warn about sleeps greater than 50 ms
Quick boottime is important, so warn about sleeps greater than 50 ms in ACPI.
50 ms is still long compared to distribution Linux kernels reaching initrd in 350 ms, so should probably changed to 10 ms, so people are aware about this.
Signed-off-by: Paul Menzel <[email protected]>
---
drivers/acpi/acpica/exsystem.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/drivers/acpi/acpica/exsystem.c b/drivers/acpi/acpica/exsystem.c index 00f66af31ffa..bdffb8aac05c 100644
--- a/drivers/acpi/acpica/exsystem.c
+++ b/drivers/acpi/acpica/exsystem.c
@@ -167,6 +167,11 @@ acpi_status acpi_ex_system_do_sleep(u64 how_long_ms)
acpi_ex_exit_interpreter();
+ if (how_long_ms > 50) {
+ ACPI_WARNING((AE_INFO,
+ "Time parameter %llu > 50 ms. Please contact firmware vendor for more responsive system.", how_long_ms));
+ }
+
/*
* For compatibility with other ACPI implementations and to prevent
* accidental deep sleeps, limit the sleep time to something reasonable.
--
2.35.1
Dear Robert,
Am 24.02.22 um 17:19 schrieb Moore, Robert:
> So, this is the current implementation:
>
> /*
> * For compatibility with other ACPI implementations and to prevent
> * accidental deep sleeps, limit the sleep time to something reasonable.
> */
> if (HowLong > ACPI_MAX_SLEEP)
> {
> HowLong = ACPI_MAX_SLEEP;
> }
>
> AcpiOsSleep (HowLong);
>
> Where ACPI_MAX_SLEEP is:
>
> #define ACPI_MAX_SLEEP 2000 /* 2000 millisec == two seconds */
Exactly. I am not changing it though, but just add a warning.
Kind regards,
Paul
> -----Original Message-----
> From: Paul Menzel <[email protected]>
> Sent: Thursday, February 24, 2022 3:38 AM
> To: Moore, Robert <[email protected]>; Wysocki, Rafael J <[email protected]>; Len Brown <[email protected]>
> Cc: Paul Menzel <[email protected]>; [email protected]; [email protected]; [email protected]
> Subject: [PATCH 3/4] acpi: exsystem: Warn about sleeps greater than 50 ms
>
> Quick boottime is important, so warn about sleeps greater than 50 ms in ACPI.
>
> 50 ms is still long compared to distribution Linux kernels reaching initrd in 350 ms, so should probably changed to 10 ms, so people are aware about this.
>
> Signed-off-by: Paul Menzel <[email protected]>
> ---
> drivers/acpi/acpica/exsystem.c | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/drivers/acpi/acpica/exsystem.c b/drivers/acpi/acpica/exsystem.c index 00f66af31ffa..bdffb8aac05c 100644
> --- a/drivers/acpi/acpica/exsystem.c
> +++ b/drivers/acpi/acpica/exsystem.c
> @@ -167,6 +167,11 @@ acpi_status acpi_ex_system_do_sleep(u64 how_long_ms)
>
> acpi_ex_exit_interpreter();
>
> + if (how_long_ms > 50) {
> + ACPI_WARNING((AE_INFO,
> + "Time parameter %llu > 50 ms. Please contact firmware vendor for more responsive system.", how_long_ms));
> + }
> +
> /*
> * For compatibility with other ACPI implementations and to prevent
> * accidental deep sleeps, limit the sleep time to something reasonable.
> --
> 2.35.1
On Thu, Feb 24, 2022 at 12:38 PM Paul Menzel <[email protected]> wrote:
>
> Quick boottime is important, so warn about sleeps greater than 50 ms in
> ACPI.
>
> 50 ms is still long compared to distribution Linux kernels reaching initrd
> in 350 ms, so should probably changed to 10 ms, so people are aware
> about this.
>
> Signed-off-by: Paul Menzel <[email protected]>
First off, as ACPICA material, this should be submitted to the
upstream project via https://github.com/acpica/acpica/.
> ---
> drivers/acpi/acpica/exsystem.c | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/drivers/acpi/acpica/exsystem.c b/drivers/acpi/acpica/exsystem.c
> index 00f66af31ffa..bdffb8aac05c 100644
> --- a/drivers/acpi/acpica/exsystem.c
> +++ b/drivers/acpi/acpica/exsystem.c
> @@ -167,6 +167,11 @@ acpi_status acpi_ex_system_do_sleep(u64 how_long_ms)
>
> acpi_ex_exit_interpreter();
>
> + if (how_long_ms > 50) {
> + ACPI_WARNING((AE_INFO,
Second, the log level is somewhat high for something like this.
> + "Time parameter %llu > 50 ms. Please contact firmware vendor for more responsive system.", how_long_ms));
Also, I would rephrase the warning message to something like "Firmware
issue: Excessive delay (%llu ms) in ACPI Control Method".
> + }
> +
> /*
> * For compatibility with other ACPI implementations and to prevent
> * accidental deep sleeps, limit the sleep time to something reasonable.
> --
> 2.35.1
>
On Thu, Feb 24, 2022 at 12:38 PM Paul Menzel <[email protected]> wrote:
>
> `how_long` uses different units in both functions, so make it more
> clear, what unit they expect.
>
> Signed-off-by: Paul Menzel <[email protected]>
> ---
> drivers/acpi/acpica/exsystem.c | 20 ++++++++++----------
As ACPICA material, this should be submitted to the upstream project
via https://github.com/acpica/acpica/.
This applies to the other patches in the series too.
> 1 file changed, 10 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/acpi/acpica/exsystem.c b/drivers/acpi/acpica/exsystem.c
> index 1281c07112de..6bc5b46e6927 100644
> --- a/drivers/acpi/acpica/exsystem.c
> +++ b/drivers/acpi/acpica/exsystem.c
> @@ -107,7 +107,7 @@ acpi_status acpi_ex_system_wait_mutex(acpi_mutex mutex, u16 timeout)
> *
> * FUNCTION: acpi_ex_system_do_stall
> *
> - * PARAMETERS: how_long - The amount of time to stall,
> + * PARAMETERS: how_long_us - The amount of time to stall,
> * in microseconds
> *
> * RETURN: Status
> @@ -120,13 +120,13 @@ acpi_status acpi_ex_system_wait_mutex(acpi_mutex mutex, u16 timeout)
> *
> ******************************************************************************/
>
> -acpi_status acpi_ex_system_do_stall(u32 how_long)
> +acpi_status acpi_ex_system_do_stall(u32 how_long_us)
> {
> acpi_status status = AE_OK;
>
> ACPI_FUNCTION_ENTRY();
>
> - if (how_long > 255) { /* 255 microseconds */
> + if (how_long_us > 255) { /* 255 microseconds */
> /*
> * Longer than 255 usec, this is an error
> *
> @@ -134,10 +134,10 @@ acpi_status acpi_ex_system_do_stall(u32 how_long)
> * order to support existing BIOSs)
> */
> ACPI_ERROR((AE_INFO,
> - "Time parameter is too large (%u)", how_long));
> + "Time parameter is too large (%u)", how_long_us));
> status = AE_AML_OPERAND_VALUE;
> } else {
> - acpi_os_stall(how_long);
> + acpi_os_stall(how_long_us);
> }
>
> return (status);
> @@ -147,7 +147,7 @@ acpi_status acpi_ex_system_do_stall(u32 how_long)
> *
> * FUNCTION: acpi_ex_system_do_sleep
> *
> - * PARAMETERS: how_long - The amount of time to sleep,
> + * PARAMETERS: how_long_ms - The amount of time to sleep,
> * in milliseconds
> *
> * RETURN: None
> @@ -156,7 +156,7 @@ acpi_status acpi_ex_system_do_stall(u32 how_long)
> *
> ******************************************************************************/
>
> -acpi_status acpi_ex_system_do_sleep(u64 how_long)
> +acpi_status acpi_ex_system_do_sleep(u64 how_long_ms)
> {
> ACPI_FUNCTION_ENTRY();
>
> @@ -168,11 +168,11 @@ acpi_status acpi_ex_system_do_sleep(u64 how_long)
> * For compatibility with other ACPI implementations and to prevent
> * accidental deep sleeps, limit the sleep time to something reasonable.
> */
> - if (how_long > ACPI_MAX_SLEEP) {
> - how_long = ACPI_MAX_SLEEP;
> + if (how_long_ms > ACPI_MAX_SLEEP) {
> + how_long_ms = ACPI_MAX_SLEEP;
> }
>
> - acpi_os_sleep(how_long);
> + acpi_os_sleep(how_long_ms);
>
> /* And now we must get the interpreter again */
>
> --
> 2.35.1
>
Dear Rafael,
Am 01.03.22 um 20:40 schrieb Rafael J. Wysocki:
> On Thu, Feb 24, 2022 at 12:38 PM Paul Menzel <[email protected]> wrote:
>>
>> `how_long` uses different units in both functions, so make it more
>> clear, what unit they expect.
>>
>> Signed-off-by: Paul Menzel <[email protected]>
>> ---
>> drivers/acpi/acpica/exsystem.c | 20 ++++++++++----------
>
> As ACPICA material, this should be submitted to the upstream project
> via https://github.com/acpica/acpica/.
>
> This applies to the other patches in the series too.
Thank you for the hint. Done [1].
[…]
Kind regards,
Paul
PS: I should have read [2] before manually porting my patches from Linux
to ACPICA.
[1]: https://github.com/acpica/acpica/pull/754
[2]: Documentation/driver-api/acpi/linuxized-acpica.rst