[PLEASE TEST] acpi(4): acpi_sleep(): tsleep(9) -> tsleep_nsec(9)

classic Classic list List threaded Threaded
8 messages Options
Reply | Threaded
Open this post in threaded view
|

[PLEASE TEST] acpi(4): acpi_sleep(): tsleep(9) -> tsleep_nsec(9)

Scott Cheloha
Hi,

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.

I've been running with this since November without apparent issue...
But this driver impacts many people, so I think I need more tests.

Cheers,

Scott

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 16 Mar 2020 02:26:25 -0000
@@ -465,15 +465,11 @@ void
 acpi_sleep(int ms, char *reason)
 {
  static int acpinowait;
- int to = ms * hz / 1000;
 
  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

Reply | Threaded
Open this post in threaded view
|

Re: [PLEASE TEST] acpi(4): acpi_sleep(): tsleep(9) -> tsleep_nsec(9)

Lucas Raab
On Sun, Mar 15, 2020, at 21:55, Scott Cheloha wrote:

> Hi,
>
> 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.
>
> I've been running with this since November without apparent issue...
> But this driver impacts many people, so I think I need more tests.
>
> Cheers,
>
> Scott
>

No problems so far on a Thinkpad X1C7

Lucas

Reply | Threaded
Open this post in threaded view
|

Re: [PLEASE TEST] acpi(4): acpi_sleep(): tsleep(9) -> tsleep_nsec(9)

Scott Cheloha
In reply to this post by Scott Cheloha
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.

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 16 Mar 2020 02:26:25 -0000
@@ -465,15 +465,11 @@ void
 acpi_sleep(int ms, char *reason)
 {
  static int acpinowait;
- int to = ms * hz / 1000;
 
  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

Reply | Threaded
Open this post in threaded view
|

Re: [PLEASE TEST] acpi(4): acpi_sleep(): tsleep(9) -> tsleep_nsec(9)

Mark Kettenis
> Date: Mon, 23 Mar 2020 21:57:46 -0500
> From: Scott Cheloha <[hidden email]>
>
> 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.  The current
code tries to protect against negative timeouts, but your new code
doesn't?

> 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 16 Mar 2020 02:26:25 -0000
> @@ -465,15 +465,11 @@ void
>  acpi_sleep(int ms, char *reason)
>  {
>   static int acpinowait;
> - int to = ms * hz / 1000;
>  
>   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
>
>

Reply | Threaded
Open this post in threaded view
|

Re: [PLEASE TEST] acpi(4): acpi_sleep(): tsleep(9) -> tsleep_nsec(9)

Scott Cheloha
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 <[hidden email]>
> >
> > 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

Reply | Threaded
Open this post in threaded view
|

Re: [PLEASE TEST] acpi(4): acpi_sleep(): tsleep(9) -> tsleep_nsec(9)

Mark Kettenis
> Date: Tue, 24 Mar 2020 12:52:55 -0500
> From: Scott Cheloha <[hidden email]>
>
> 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 <[hidden email]>
> > >
> > > 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
>

Reply | Threaded
Open this post in threaded view
|

Re: [PLEASE TEST] acpi(4): acpi_sleep(): tsleep(9) -> tsleep_nsec(9)

Scott Cheloha
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 <[hidden email]>
> >
> > 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 <[hidden email]>
> > > >
> > > > 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

Reply | Threaded
Open this post in threaded view
|

Re: [PLEASE TEST] acpi(4): acpi_sleep(): tsleep(9) -> tsleep_nsec(9)

Mark Kettenis
> Date: Mon, 6 Apr 2020 15:32:09 -0500
> From: Scott Cheloha <[hidden email]>
>
> 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 <[hidden email]>
> > >
> > > 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 <[hidden email]>
> > > > >
> > > > > 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
>