Warn about long delays #754

paulmenzel · 2022-03-02T08:01:47Z

No description provided.

`HowLong` refers to different units in both functions, so make it more clear, what unit they expect. That also makes one comment superfluous.

rafaeljw

If this is a spec violation, the commit log should at least mention which section of the spec is violated and how.

source/components/executer/exsystem.c

Values greater than 100 microseconds violate the ACPI specification, so warn users about it. From ACPI Specification version 6.2 Errata A, 19.6.128 *Stall (Stall for a Short Time)*: > The implementation of Stall is OS-specific, but must not relinquish > control of the processor. Because of this, delays longer than 100 > microseconds must use Sleep instead of Stall.

paulmenzel · 2022-03-13T07:55:48Z

If this is a spec violation, the commit log should at least mention which section of the spec is violated and how.

Only the stall is a specification violation.

The sleep warning is just for good implementation, so sleeps longer than 10 ms hopefully get reimplemented by a polling approach or something similar.

acpibob · 2022-03-15T15:41:59Z

I’m seeing some long sleeps in existing ASL code, some examples: MSI/MS/MS-7752/14FAD414B696: Sleep (0x64) TONK/TN/TN1402/ABC6298CB633: Sleep (0x64) ZOTAC/NM/NM10/27F12A855946: Sleep (0x0BB8) TONK/C/C31/7F7E8D75C7FF: Sleep (0xFA) Supermicro/X8/X8SIL/40AECBFF4573: Sleep (0x0BB8) MSI/MS-7/MS-7A38/53B5D217D45C: Sleep (0x012C) MSI/MS/MS-7721/3EF15301B047: Sleep (0xFA) From: Paul Menzel ***@***.***> Sent: Saturday, March 12, 2022 11:54 PM To: acpica/acpica ***@***.***> Cc: Subscribed ***@***.***> Subject: Re: [acpica/acpica] Warn about long delays (PR #754) @paulmenzel commented on this pull request.

________________________________ In source/components/executer/exsystem.c<#754 (comment)>:

@@ -338,6 +338,12 @@ AcpiExSystemDoSleep (

AcpiExExitInterpreter (); + if (HowLongMs > 50) Indeed. Fixed. — Reply to this email directly, view it on GitHub<#754 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AAHL7LB22QRF73UWRGDRRZDU7WNLHANCNFSM5PWTFSTA>. Triage notifications on the go with GitHub Mobile for iOS<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>. You are receiving this because you are subscribed to this thread.Message ID: ***@***.******@***.***>>

paulmenzel · 2022-03-15T15:53:04Z

[If replying to GitHub emails, please always remove the citation.]

I’m seeing some long sleeps in existing ASL code, some examples:

MSI/MS/MS-7752/14FAD414B696:                Sleep (0x64)
TONK/TN/TN1402/ABC6298CB633:                Sleep (0x64)
ZOTAC/NM/NM10/27F12A855946:            Sleep (0x0BB8)
TONK/C/C31/7F7E8D75C7FF:                                    Sleep (0xFA)
Supermicro/X8/X8SIL/40AECBFF4573:                    Sleep (0x0BB8)
MSI/MS-7/MS-7A38/53B5D217D45C:            Sleep (0x012C)
MSI/MS/MS-7721/3EF15301B047:                                Sleep (0xFA)

Yes, and these should be warned/notified about. For example, 0x64 ms is 100 ms, 0xfa ms is 250 ms and 0xbb8 ms is 3000 ms greater than the current 2 second maximum. It’s non-optimal, that means bad, implementation in firmware, and gives a bad experience. (The question remains, if the OS like GNU/Linux hits these code paths.)

acpibob · 2022-03-15T15:56:22Z

This of course means that these machines will always generate warnings from ACPI. This is discouraged, especially in Linux.

Perhaps implementing this warning in the iASL compiler would be better.

paulmenzel · 2022-03-15T16:03:25Z

This of course means that these machines will always generate warnings from ACPI.

Did you test, that these code paths from your examples are actually executed (all the time). Nevertheless, if these delays are always happening, the user should be made aware of them and the firmware should be fixed, so it’s exactly the wanted outcome of these changes.

This is discouraged, especially in Linux.

Where is that documented, that it’s discouraged? Elaborate log messages, and actionable warnings are encouraged in my experience.

Perhaps implementing this warning in the iASL compiler would be better.

It would not hurt, but then only folks using an iASL with that patch will see the warning, and would be able to fix it. The user should still be informed about the bad implementation.

acpibob · 2022-03-15T16:15:53Z

No, I didn’t test for actual execution.

We can try adding this warning, but if we get too many complaints about it, we will need to back it out.

paulmenzel · 2022-03-15T16:19:52Z

Understood.

By the way, https://bugzilla.kernel.org/show_bug.cgi?id=208705 motivated me to look into this. Also to figure out, which of these sleeps is actually executed.

acpibob · 2022-03-15T16:26:22Z

A couple things:

The description and the warning mention 10ms, but the code checks for >50ms:

executer/exsystem: Warn about sleeps greater than 10 ms
Quick boottime is important, so warn about sleeps greater than 10 ms.

if (HowLongMs > 50)
{
    ACPI_WARNING ((AE_INFO,
        "Firmware issue: Excessive delay (%llu ms > 10 ms) in ACPI Control Method", HowLongUs));

The comment appears after the code that checks the time:
ACPI_WARNING ((AE_INFO,
"Firmware issue: Excessive delay (%llu ms > 10 ms) in ACPI Control Method", HowLongUs));
}

/*
 * For compatibility with other ACPI implementations and to prevent
 * accidental deep sleeps, limit the sleep time to something reasonable.

Also, I would say "Excessive sleep" instead of "excessive delay".

paulmenzel · 2022-03-15T16:33:42Z

A couple things:

The description and the warning mention 10ms, but the code checks for >50ms:

Hmm, yes, that was in the first revision, but after @rafaeljw’s comment I force pushed the fixed code. Did I miss something?

https://github.com/acpica/acpica/pull/754/files#diff-d218a4975ac8a3e06ab6253dba4e7407edebde1c739e8a7b1c306acd8b406ff2R341

executer/exsystem: Warn about sleeps greater than 10 ms Quick boottime is important, so warn about sleeps greater than 10 ms.
if (HowLongMs > 50)
{
    ACPI_WARNING ((AE_INFO,
        "Firmware issue: Excessive delay (%llu ms > 10 ms) in ACPI Control Method", HowLongUs));
The comment appears after the code that checks the time: ACPI_WARNING ((AE_INFO, "Firmware issue: Excessive delay (%llu ms > 10 ms) in ACPI Control Method", HowLongUs)); }
/*
 * For compatibility with other ACPI implementations and to prevent
 * accidental deep sleeps, limit the sleep time to something reasonable.

Hmm, that comment was already there, and is the for the time limitation (to currently two seconds), isn’t it.

Also, I would say "Excessive sleep" instead of "excessive delay".

Will fix.

acpibob · 2022-03-15T16:36:20Z

I would add a comment at the start of the new code.

paulmenzel · 2022-03-15T16:40:58Z

I am happy to do that. I thought the – hopefully understandable warning – would make a comment obsolete. Do you have a suggestion? Do you mean something like:

/* Notify users about excessive sleep times, so ASL firmware code can be improved to use polling or similar. */

acpibob · 2022-03-15T16:42:29Z

Yes, this is ok. Probably split it into 2 lines, however.

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.

paulmenzel · 2022-03-16T11:24:19Z

I force pushed the improved commits.

rafaeljw

All 3 commits look good to me now.

acpibob · 2022-03-25T16:37:41Z

Ok, got it. Thanks, Bob

paulmenzel · 2022-09-06T10:27:26Z

Sorry for the mistakes that crept in, and thank you all for fixing them:

commit 441747f
commit 82a46ba

For the record, on the Acer TravelMate 5735Z/BA51_MV, BIOS V1.14 07/26/2011, there are some sleeps related to brightness configuration, that take 100 ms (0x64):

[   11.169493] IPv6: ADDRCONF(NETDEV_CHANGE): enp5s0: link becomes ready
[   11.984097] ACPI Warning: Firmware issue: Excessive sleep time (0x0000000000000064 ms > 10 ms) in ACPI Control Method (20220331/exsystem-177)
[   12.092233] ACPI Warning: Firmware issue: Excessive sleep time (0x0000000000000064 ms > 10 ms) in ACPI Control Method (20220331/exsystem-177)
[   12.200228] ACPI Warning: Firmware issue: Excessive sleep time (0x0000000000000064 ms > 10 ms) in ACPI Control Method (20220331/exsystem-177)
[   12.309446] ACPI Warning: Firmware issue: Excessive sleep time (0x0000000000000064 ms > 10 ms) in ACPI Control Method (20220331/exsystem-177)
[   12.417404] ACPI Warning: Firmware issue: Excessive sleep time (0x0000000000000064 ms > 10 ms) in ACPI Control Method (20220331/exsystem-177)
[   12.526249] ACPI Warning: Firmware issue: Excessive sleep time (0x0000000000000064 ms > 10 ms) in ACPI Control Method (20220331/exsystem-177)
[   13.566840] rfkill: input handler disabled

executer/exsystem: Add units to time variable names

b69cbef

`HowLong` refers to different units in both functions, so make it more clear, what unit they expect. That also makes one comment superfluous.

rafaeljw suggested changes Mar 3, 2022

View reviewed changes

source/components/executer/exsystem.c Outdated Show resolved Hide resolved

paulmenzel force-pushed the warn-about-long-delays branch from 5c3dbc3 to e2d68b5 Compare March 13, 2022 07:52

paulmenzel force-pushed the warn-about-long-delays branch from e2d68b5 to 2a0d1d4 Compare March 16, 2022 11:23

rafaeljw approved these changes Mar 18, 2022

View reviewed changes

acpibob merged commit b3c9e44 into acpica:master Mar 25, 2022

paulmenzel deleted the warn-about-long-delays branch April 2, 2022 07:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Warn about long delays #754

Warn about long delays #754

paulmenzel commented Mar 2, 2022

rafaeljw left a comment

paulmenzel commented Mar 13, 2022

acpibob commented Mar 15, 2022 via email

paulmenzel commented Mar 15, 2022

acpibob commented Mar 15, 2022

paulmenzel commented Mar 15, 2022

acpibob commented Mar 15, 2022

paulmenzel commented Mar 15, 2022

acpibob commented Mar 15, 2022

paulmenzel commented Mar 15, 2022

acpibob commented Mar 15, 2022

paulmenzel commented Mar 15, 2022

acpibob commented Mar 15, 2022

paulmenzel commented Mar 16, 2022

rafaeljw left a comment

acpibob commented Mar 25, 2022

paulmenzel commented Sep 6, 2022

Warn about long delays #754

Warn about long delays #754

Conversation

paulmenzel commented Mar 2, 2022

rafaeljw left a comment

Choose a reason for hiding this comment

paulmenzel commented Mar 13, 2022

acpibob commented Mar 15, 2022 via email

paulmenzel commented Mar 15, 2022

acpibob commented Mar 15, 2022

paulmenzel commented Mar 15, 2022

acpibob commented Mar 15, 2022

paulmenzel commented Mar 15, 2022

acpibob commented Mar 15, 2022

paulmenzel commented Mar 15, 2022

acpibob commented Mar 15, 2022

paulmenzel commented Mar 15, 2022

acpibob commented Mar 15, 2022

paulmenzel commented Mar 16, 2022

rafaeljw left a comment

Choose a reason for hiding this comment

acpibob commented Mar 25, 2022

paulmenzel commented Sep 6, 2022