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]?
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
