Re: [please test] acpi: more *sleep(9) -> *sleep_nsec(9) conversions
Paul Irofti writes: > I think Scott should split his diff in two so people can test which > part breaks suspend-resume. Can you try reverting the dsdt.c bits and > leaving the acpiec part of the diff in your tree and see if suspend is > still broken? Reverting dsdt.c and applying acpiec.c part restores resume. I'll keep running with this piece. Thanks Greg
Re: [please test] acpi: more *sleep(9) -> *sleep_nsec(9) conversions
> Date: Sun, 20 Dec 2020 16:23:25 -0600
> From: Scott Cheloha
>
> Short version:
>
> Please test if this patch breaks suspend or hibernate on your
> machine. Reply with the results of your test and your dmesg.
>
> Both are still working on my Lenovo Thinkpad X1 Carbon Gen 6.
>
> Long version:
>
> This patch converts the remaining tick sleeps in dev/acpi to
> nanosecond sleeps.
>
> In acpiec_wait() we have a tsleep(9) for up to 1 tick. There is no
> discernable unit in this code and no timeout so as with all the other
> 1 tick sleeps I think tsleep_nsec(9) for 1ms will work fine. It may
> oversleep 1ms but because there is no timeout it doesn't really
> matter.
>
> In acpi_event_wait() we have a timeout in milliseconds. Currently we
> sleep for up to 1 tick and deduct time from the timeout when the sleep
> returns EWOULDBLOCK. Of note here is that this code is broken on
> kernels where hz > 1000: we will never time out.
>
> I have changed the code to rwsleep_nsec(9) for at least 1ms. The
> timeout is in milliseconds so this seems logical to me.
>
> The caveat is that on default HZ=100 kernels we will oversleep and it
> will take much longer for us to time out in this loop. We can fix
> this by measuring the sleep and deducting the elapsed time from the
> total timeout. Or, if it doesn't really matter, we can just oversleep
> and not worry about it.
>
> Preferences?
>
> Assuming we get no bad tests back, OK?
>
> Index: acpiec.c
> ===
> RCS file: /cvs/src/sys/dev/acpi/acpiec.c,v
> retrieving revision 1.62
> diff -u -p -r1.62 acpiec.c
> --- acpiec.c 26 Aug 2020 03:29:06 - 1.62
> +++ acpiec.c 20 Dec 2020 22:19:14 -
> @@ -105,8 +105,10 @@ acpiec_wait(struct acpiec_softc *sc, uin
> sc->sc_gotsci = 1;
> if (cold || (stat & EC_STAT_BURST))
> delay(1);
> - else
> - tsleep(, PWAIT, "acpiec", 1);
> + else {
> + tsleep_nsec(, PWAIT, "acpiec",
> + MSEC_TO_NSEC(1));
> + }
> }
>
> dnprintf(40, "%s: EC wait_ns, stat: %b\n", DEVNAME(sc), (int)stat,
This ties in with the discussion we had earlier about sleeping without
a wakeup channel. Whatever solution is agreed upon there should
probably be applied here.
> Index: dsdt.c
> ===
> RCS file: /cvs/src/sys/dev/acpi/dsdt.c,v
> retrieving revision 1.257
> diff -u -p -r1.257 dsdt.c
> --- dsdt.c17 Dec 2020 17:57:19 - 1.257
> +++ dsdt.c20 Dec 2020 22:19:15 -
> @@ -2907,10 +2907,10 @@ acpi_event_wait(struct aml_scope *scope,
> if (acpi_dotask(acpi_softc))
> continue;
> if (!cold) {
> - if (rwsleep(evt, _softc->sc_lck, PWAIT,
> - "acpievt", 1) == EWOULDBLOCK) {
> + if (rwsleep_nsec(evt, _softc->sc_lck, PWAIT,
> + "acpievt", MSEC_TO_NSEC(1)) == EWOULDBLOCK) {
> if (timeout < AML_NO_TIMEOUT)
> - timeout -= (1000 / hz);
> + timeout--;
> }
> } else {
> delay(1000);
This is problematic. Since MSEC_TO_NSEC(1) will actually sleep for up
to 20 ms, this will sleep for much longer than timeout milliseconds.
I'd keep this as-is such that we can revisit this when we have a
tickless kernel. But if you really want to get rid of the rwsleep()
call now, maybe using MSEC_TO_NSEC(10) in the loop with timeout -= 10
would probably be the way to go.
Re: [please test] acpi: more *sleep(9) -> *sleep_nsec(9) conversions
I think Scott should split his diff in two so people can test which part breaks suspend-resume. Can you try reverting the dsdt.c bits and leaving the acpiec part of the diff in your tree and see if suspend is still broken? Paul
Re: [please test] acpi: more *sleep(9) -> *sleep_nsec(9) conversions
Scott Cheloha writes: > Short version: > > Please test if this patch breaks suspend or hibernate on your > machine. Reply with the results of your test and your dmesg. > > Both are still working on my Lenovo Thinkpad X1 Carbon Gen 6. This breaks resume from zzz on HP EliteBook 840 G1. The machine comes back with frozen-ish X. Switching between VCs works, so I can reboot. There is nothing undue in dmesg or Xorg.0.log. I shared full machine details (dmesg/acpi) before, e.g. https://www.mail-archive.com/tech@openbsd.org/msg60772.html Sorry for the somewhat unactionable feedback. Let me know if some printfs would help. Thanks Greg
[please test] acpi: more *sleep(9) -> *sleep_nsec(9) conversions
Short version:
Please test if this patch breaks suspend or hibernate on your
machine. Reply with the results of your test and your dmesg.
Both are still working on my Lenovo Thinkpad X1 Carbon Gen 6.
Long version:
This patch converts the remaining tick sleeps in dev/acpi to
nanosecond sleeps.
In acpiec_wait() we have a tsleep(9) for up to 1 tick. There is no
discernable unit in this code and no timeout so as with all the other
1 tick sleeps I think tsleep_nsec(9) for 1ms will work fine. It may
oversleep 1ms but because there is no timeout it doesn't really
matter.
In acpi_event_wait() we have a timeout in milliseconds. Currently we
sleep for up to 1 tick and deduct time from the timeout when the sleep
returns EWOULDBLOCK. Of note here is that this code is broken on
kernels where hz > 1000: we will never time out.
I have changed the code to rwsleep_nsec(9) for at least 1ms. The
timeout is in milliseconds so this seems logical to me.
The caveat is that on default HZ=100 kernels we will oversleep and it
will take much longer for us to time out in this loop. We can fix
this by measuring the sleep and deducting the elapsed time from the
total timeout. Or, if it doesn't really matter, we can just oversleep
and not worry about it.
Preferences?
Assuming we get no bad tests back, OK?
Index: acpiec.c
===
RCS file: /cvs/src/sys/dev/acpi/acpiec.c,v
retrieving revision 1.62
diff -u -p -r1.62 acpiec.c
--- acpiec.c26 Aug 2020 03:29:06 - 1.62
+++ acpiec.c20 Dec 2020 22:19:14 -
@@ -105,8 +105,10 @@ acpiec_wait(struct acpiec_softc *sc, uin
sc->sc_gotsci = 1;
if (cold || (stat & EC_STAT_BURST))
delay(1);
- else
- tsleep(, PWAIT, "acpiec", 1);
+ else {
+ tsleep_nsec(, PWAIT, "acpiec",
+ MSEC_TO_NSEC(1));
+ }
}
dnprintf(40, "%s: EC wait_ns, stat: %b\n", DEVNAME(sc), (int)stat,
Index: dsdt.c
===
RCS file: /cvs/src/sys/dev/acpi/dsdt.c,v
retrieving revision 1.257
diff -u -p -r1.257 dsdt.c
--- dsdt.c 17 Dec 2020 17:57:19 - 1.257
+++ dsdt.c 20 Dec 2020 22:19:15 -
@@ -2907,10 +2907,10 @@ acpi_event_wait(struct aml_scope *scope,
if (acpi_dotask(acpi_softc))
continue;
if (!cold) {
- if (rwsleep(evt, _softc->sc_lck, PWAIT,
- "acpievt", 1) == EWOULDBLOCK) {
+ if (rwsleep_nsec(evt, _softc->sc_lck, PWAIT,
+ "acpievt", MSEC_TO_NSEC(1)) == EWOULDBLOCK) {
if (timeout < AML_NO_TIMEOUT)
- timeout -= (1000 / hz);
+ timeout--;
}
} else {
delay(1000);
