> 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.
> 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 24 Mar 2020 17:50:05 -0000
> @@ -465,15 +465,13 @@ void
> acpi_sleep(int ms, char *reason)
> {
> static int acpinowait;
> - int to = ms * hz / 1000;
> +
> + 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
>
