On Tue, Mar 24, 2020 at 09:02:38PM +0100, Mark Kettenis wrote:
> > Date: Tue, 24 Mar 2020 12:52:55 -0500
> > From: Scott Cheloha <scottchel...@gmail.com>
> >
> > On Tue, Mar 24, 2020 at 09:17:34AM +0100, Mark Kettenis wrote:
> > > > Date: Mon, 23 Mar 2020 21:57:46 -0500
> > > > From: Scott Cheloha <scottchel...@gmail.com>
> > > >
> > > > On Sun, Mar 15, 2020 at 09:55:53PM -0500, Scott Cheloha wrote:
> > > > >
> > > > > [...]
> > > > >
> > > > > This is a straightforward ticks-to-milliseconds conversion, but IIRC
> > > > > pirofti@ wanted me to get some tests before committing it.
> > > > >
> > > > > The only users of acpi_sleep() are (a) acpitz(4) and (b) any AML code
> > > > > that uses AMLOP_SLEEP. AMLOP_SLEEP seems to trigger just before a
> > > > > suspend. I don't know when else it is used.
> > > > >
> > > > > If you have an acpi(4) laptop with suspend/resume support, please
> > > > > apply this patch and let me know if anything doesn't work,
> > > > > particularly with suspend/resume.
> > > > >
> > > > > [...]
> > > >
> > > > 1 week bump.
> > > >
> > > > I have one test report. I'm hoping for a few more.
> > > >
> > > > I think acpi(4) machines with suspend/resume support should be
> > > > somewhat common amongst tech@ readers.
> > >
> > > AMLOP_SLEEP can occur anywhere when executing AML code.
> >
> > Oh, good to know.
> >
> > > The current code tries to protect against negative timeouts, but
> > > your new code doesn't?
> >
> > What would a negative value here actually mean? A firmware bug?
> > Should we log that? Or panic?
> >
> > The simplest thing would be to just MAX it up to 1. Which I have
> > done in this patch.
> >
> > But it looks like there are other "what if we get a negative value
> > from the firmware" problems in this file. I dunno if you want to
> > address all of them.
> >
> > Related:
> >
> > I'm looking around at these functions accepting .v_integer as
> > argument: acpi_stall, acpi_sleep(), acpi_event_wait(). They all take
> > ints, but aml_value.v_integer is an int64_t. So there's implicit
> > type-casting.
> >
> > Should we change these functions to handle 64-bit integers? Or should
> > we clamp .v_integer to within [INT_MIN, INT_MAX]?
>
> The ACPI code isn't the best code in our tree I fear. One issue here
> is that ACPI 1.0 had 32-bit integers, which became 640-bit in ACPI
> 2.0. Treating all integers as 64-bit numbers seems to work fine
> though. So acpi_sleep() should really accept a 64-bit integer as an
> argument. And ACPI says that integers are unsigned, so the sign
> problem should never occur and our codebase is just plain doing it
> wrong...
>
> Here is the description of the operation lifted form the ACPI 6.3 spec:
>
> 19.6.125 Sleep (Milliseconds Sleep)
>
> Syntax
>
> Sleep (MilliSeconds)
>
> Arguments
>
> The Sleep term is used to implement long-term timing
> requirements. Execution is delayed for at least the required
> number of milliseconds.
>
> Description
>
> The implementation of Sleep is to round the request up to the
> closest sleep time supported by the OS and relinquish the
> processor.
That sounds like a much deeper problem.
The following diff converts the tsleep(9) to tsleep_nsec(9) without
changing the current behavior. I have left a note about the larger
problem.
I have several successful test reports.
Is anyone OK with this?
Index: dsdt.c
===================================================================
RCS file: /cvs/src/sys/dev/acpi/dsdt.c,v
retrieving revision 1.249
diff -u -p -r1.249 dsdt.c
--- dsdt.c 16 Oct 2019 01:43:50 -0000 1.249
+++ dsdt.c 6 Apr 2020 20:29:51 -0000
@@ -465,15 +465,14 @@ void
acpi_sleep(int ms, char *reason)
{
static int acpinowait;
- int to = ms * hz / 1000;
+
+ /* XXX ACPI integers are supposed to be unsigned. */
+ ms = MAX(1, ms);
if (cold)
delay(ms * 1000);
- else {
- if (to <= 0)
- to = 1;
- tsleep(&acpinowait, PWAIT, reason, to);
- }
+ else
+ tsleep_nsec(&acpinowait, PWAIT, reason, MSEC_TO_NSEC(ms));
}
void
