> Date: Mon, 6 Apr 2020 15:32:09 -0500
> From: Scott Cheloha <[email protected]>
>
> 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 <[email protected]>
> > >
> > > 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 <[email protected]>
> > > > >
> > > > > 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?
ok kettenis@
> 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
>
