Implement Event()/Signal()/Wait() AML operations

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

Implement Event()/Signal()/Wait() AML operations

Mark Kettenis
I have a Shuttle NC02U "nano" PC that ends up with the the acpi0
kernel thread blocked on the "acpievt" wait channel.  I tracked this
down to an incorrect implementation of the Signal() and Wait()
operations.

This changes the implementation to be a proper semaphore.  It also
releases the acpi interpreter lock.  This is necessary because the
acpivout(4) implementation directly executes AML code from the calling
thread instead of using an acpi task.  Maybe that should be fixed
instead but it is non-trivial and the current code results in a hard
hang during the initial mode setting stage.

ok?


Index: dev/acpi/dsdt.c
===================================================================
RCS file: /cvs/src/sys/dev/acpi/dsdt.c,v
retrieving revision 1.243
diff -u -p -r1.243 dsdt.c
--- dev/acpi/dsdt.c 19 Aug 2018 08:23:47 -0000 1.243
+++ dev/acpi/dsdt.c 8 Jan 2019 16:25:12 -0000
@@ -2905,29 +2905,32 @@ int
 acpi_event_wait(struct aml_scope *scope, struct aml_value *evt, int timeout)
 {
  /* Wait for event to occur; do work in meantime */
- evt->v_evt.state = 0;
- while (!evt->v_evt.state) {
- if (!acpi_dotask(acpi_softc) && !cold)
- tsleep(evt, PWAIT, "acpievt", 1);
- else
- delay(100);
+ while (evt->v_evt.state == 0 && timeout >= 0) {
+ if (!acpi_dotask(acpi_softc) && !cold) {
+ rw_exit_write(&acpi_softc->sc_lck);
+ if (tsleep(evt, PWAIT, "acpievt", 1) == EWOULDBLOCK) {
+ if (timeout < 0xffff)
+ timeout -= (1000 / hz);
+ }
+ rw_enter_write(&acpi_softc->sc_lck);
+ } else {
+ delay(1000);
+ if (timeout < 0xffff)
+ timeout--;
+ }
  }
- if (evt->v_evt.state == 1) {
- /* Object is signaled */
- return (0);
- } else if (timeout == 0) {
- /* Zero timeout */
+ if (evt->v_evt.state == 0)
  return (-1);
- }
- /* Wait for timeout or signal */
+ evt->v_evt.state--;
  return (0);
 }
 
 void
 acpi_event_signal(struct aml_scope *scope, struct aml_value *evt)
 {
- evt->v_evt.state = 1;
- /* Wakeup waiters */
+ evt->v_evt.state++;
+ if (evt->v_evt.state > 0)
+ wakeup_one(evt);
 }
 
 void
@@ -4215,7 +4218,7 @@ aml_parse(struct aml_scope *scope, int r
  case AMLOP_EVENT:
  /* Event: N */
  rv = _aml_setvalue(opargs[0], AML_OBJTYPE_EVENT, 0, 0);
- rv->v_integer = 0;
+ rv->v_evt.state = 0;
  break;
  case AMLOP_MUTEX:
  /* Mutex: Nw */

Reply | Threaded
Open this post in threaded view
|

Re: Implement Event()/Signal()/Wait() AML operations

Mark Kettenis
> Date: Tue, 8 Jan 2019 17:46:37 +0100 (CET)
> From: Mark Kettenis <[hidden email]>
>
> I have a Shuttle NC02U "nano" PC that ends up with the the acpi0
> kernel thread blocked on the "acpievt" wait channel.  I tracked this
> down to an incorrect implementation of the Signal() and Wait()
> operations.
>
> This changes the implementation to be a proper semaphore.  It also
> releases the acpi interpreter lock.  This is necessary because the
> acpivout(4) implementation directly executes AML code from the calling
> thread instead of using an acpi task.  Maybe that should be fixed
> instead but it is non-trivial and the current code results in a hard
> hang during the initial mode setting stage.
>
> ok?

Better diff.  We shouldn't call delay() if we ran a task.

ok?

Index: dev/acpi/dsdt.c
===================================================================
RCS file: /cvs/src/sys/dev/acpi/dsdt.c,v
retrieving revision 1.243
diff -u -p -r1.243 dsdt.c
--- dev/acpi/dsdt.c 19 Aug 2018 08:23:47 -0000 1.243
+++ dev/acpi/dsdt.c 8 Jan 2019 17:01:50 -0000
@@ -2905,29 +2905,34 @@ int
 acpi_event_wait(struct aml_scope *scope, struct aml_value *evt, int timeout)
 {
  /* Wait for event to occur; do work in meantime */
- evt->v_evt.state = 0;
- while (!evt->v_evt.state) {
- if (!acpi_dotask(acpi_softc) && !cold)
- tsleep(evt, PWAIT, "acpievt", 1);
- else
- delay(100);
+ while (evt->v_evt.state == 0 && timeout >= 0) {
+ if (acpi_dotask(acpi_softc))
+    continue;
+ if (!cold) {
+ rw_exit_write(&acpi_softc->sc_lck);
+ if (tsleep(evt, PWAIT, "acpievt", 1) == EWOULDBLOCK) {
+ if (timeout < 0xffff)
+ timeout -= (1000 / hz);
+ }
+ rw_enter_write(&acpi_softc->sc_lck);
+ } else {
+ delay(1000);
+ if (timeout < 0xffff)
+ timeout--;
+ }
  }
- if (evt->v_evt.state == 1) {
- /* Object is signaled */
- return (0);
- } else if (timeout == 0) {
- /* Zero timeout */
+ if (evt->v_evt.state == 0)
  return (-1);
- }
- /* Wait for timeout or signal */
+ evt->v_evt.state--;
  return (0);
 }
 
 void
 acpi_event_signal(struct aml_scope *scope, struct aml_value *evt)
 {
- evt->v_evt.state = 1;
- /* Wakeup waiters */
+ evt->v_evt.state++;
+ if (evt->v_evt.state > 0)
+ wakeup_one(evt);
 }
 
 void
@@ -4215,7 +4220,7 @@ aml_parse(struct aml_scope *scope, int r
  case AMLOP_EVENT:
  /* Event: N */
  rv = _aml_setvalue(opargs[0], AML_OBJTYPE_EVENT, 0, 0);
- rv->v_integer = 0;
+ rv->v_evt.state = 0;
  break;
  case AMLOP_MUTEX:
  /* Mutex: Nw */

Reply | Threaded
Open this post in threaded view
|

Re: Implement Event()/Signal()/Wait() AML operations

Mark Kettenis
> Date: Tue, 8 Jan 2019 18:02:27 +0100 (CET)
> From: Mark Kettenis <[hidden email]>
>
> > Date: Tue, 8 Jan 2019 17:46:37 +0100 (CET)
> > From: Mark Kettenis <[hidden email]>
> >
> > I have a Shuttle NC02U "nano" PC that ends up with the the acpi0
> > kernel thread blocked on the "acpievt" wait channel.  I tracked this
> > down to an incorrect implementation of the Signal() and Wait()
> > operations.
> >
> > This changes the implementation to be a proper semaphore.  It also
> > releases the acpi interpreter lock.  This is necessary because the
> > acpivout(4) implementation directly executes AML code from the calling
> > thread instead of using an acpi task.  Maybe that should be fixed
> > instead but it is non-trivial and the current code results in a hard
> > hang during the initial mode setting stage.
> >
> > ok?
>
> Better diff.  We shouldn't call delay() if we ran a task.

And instead of tsleep(9), use rwlseep(9).

Index: dev/acpi/dsdt.c
===================================================================
RCS file: /cvs/src/sys/dev/acpi/dsdt.c,v
retrieving revision 1.243
diff -u -p -r1.243 dsdt.c
--- dev/acpi/dsdt.c 19 Aug 2018 08:23:47 -0000 1.243
+++ dev/acpi/dsdt.c 8 Jan 2019 18:55:14 -0000
@@ -2905,29 +2905,33 @@ int
 acpi_event_wait(struct aml_scope *scope, struct aml_value *evt, int timeout)
 {
  /* Wait for event to occur; do work in meantime */
- evt->v_evt.state = 0;
- while (!evt->v_evt.state) {
- if (!acpi_dotask(acpi_softc) && !cold)
- tsleep(evt, PWAIT, "acpievt", 1);
- else
- delay(100);
+ while (evt->v_evt.state == 0 && timeout >= 0) {
+ if (acpi_dotask(acpi_softc))
+    continue;
+ if (!cold) {
+ if (rwsleep(evt, &acpi_softc->sc_lck, PWAIT,
+    "acpievt", 1) == EWOULDBLOCK) {
+ if (timeout < 0xffff)
+ timeout -= (1000 / hz);
+ }
+ } else {
+ delay(1000);
+ if (timeout < 0xffff)
+ timeout--;
+ }
  }
- if (evt->v_evt.state == 1) {
- /* Object is signaled */
- return (0);
- } else if (timeout == 0) {
- /* Zero timeout */
+ if (evt->v_evt.state == 0)
  return (-1);
- }
- /* Wait for timeout or signal */
+ evt->v_evt.state--;
  return (0);
 }
 
 void
 acpi_event_signal(struct aml_scope *scope, struct aml_value *evt)
 {
- evt->v_evt.state = 1;
- /* Wakeup waiters */
+ evt->v_evt.state++;
+ if (evt->v_evt.state > 0)
+ wakeup_one(evt);
 }
 
 void
@@ -4215,7 +4219,7 @@ aml_parse(struct aml_scope *scope, int r
  case AMLOP_EVENT:
  /* Event: N */
  rv = _aml_setvalue(opargs[0], AML_OBJTYPE_EVENT, 0, 0);
- rv->v_integer = 0;
+ rv->v_evt.state = 0;
  break;
  case AMLOP_MUTEX:
  /* Mutex: Nw */

Reply | Threaded
Open this post in threaded view
|

Re: Implement Event()/Signal()/Wait() AML operations

Ted Unangst-6
Mark Kettenis wrote:

> Index: dev/acpi/dsdt.c
> ===================================================================
> RCS file: /cvs/src/sys/dev/acpi/dsdt.c,v
> retrieving revision 1.243
> diff -u -p -r1.243 dsdt.c
> --- dev/acpi/dsdt.c 19 Aug 2018 08:23:47 -0000 1.243
> +++ dev/acpi/dsdt.c 8 Jan 2019 18:55:14 -0000
> @@ -2905,29 +2905,33 @@ int
>  acpi_event_wait(struct aml_scope *scope, struct aml_value *evt, int timeout)
>  {
>   /* Wait for event to occur; do work in meantime */
> - evt->v_evt.state = 0;
> - while (!evt->v_evt.state) {
> - if (!acpi_dotask(acpi_softc) && !cold)
> - tsleep(evt, PWAIT, "acpievt", 1);
> - else
> - delay(100);
> + while (evt->v_evt.state == 0 && timeout >= 0) {
> + if (acpi_dotask(acpi_softc))
> +    continue;
> + if (!cold) {
> + if (rwsleep(evt, &acpi_softc->sc_lck, PWAIT,
> +    "acpievt", 1) == EWOULDBLOCK) {
> + if (timeout < 0xffff)
> + timeout -= (1000 / hz);
> + }
> + } else {
> + delay(1000);
> + if (timeout < 0xffff)
> + timeout--;
> + }

Does 0xffff come from ACPI? Can we give that a name?

I thought sleeping for one tick is kinda weird, but I see what it's doing with
the acpi_dotask loop. This feels precarious, but whatever.

Reply | Threaded
Open this post in threaded view
|

Re: Implement Event()/Signal()/Wait() AML operations

Ted Unangst-6
Ted Unangst wrote:
>
> Does 0xffff come from ACPI? Can we give that a name?
>
> I thought sleeping for one tick is kinda weird, but I see what it's doing with
> the acpi_dotask loop. This feels precarious, but whatever.

So upon further thought, this is pretty bad. If the new task also calls sem
wait, we'll end up back here, recursing on the event loop. And of course, the
first sem can't wakeup until after the second one does. If you want to say it
was like this when you got here, ok, but pretending to pause like this is
troublesome. There was a bug in libc rpc code where it tried to do something
similar and blew up the stack.

Reply | Threaded
Open this post in threaded view
|

Re: Implement Event()/Signal()/Wait() AML operations

Theo de Raadt-2
Ted Unangst <[hidden email]> wrote:

> Ted Unangst wrote:
> >
> > Does 0xffff come from ACPI? Can we give that a name?
> >
> > I thought sleeping for one tick is kinda weird, but I see what it's doing with
> > the acpi_dotask loop. This feels precarious, but whatever.
>
> So upon further thought, this is pretty bad. If the new task also calls sem
> wait, we'll end up back here, recursing on the event loop. And of course, the
> first sem can't wakeup until after the second one does. If you want to say it
> was like this when you got here, ok, but pretending to pause like this is
> troublesome. There was a bug in libc rpc code where it tried to do something
> similar and blew up the stack.

Not sure I understand.  There is only one acpi task.  

Reply | Threaded
Open this post in threaded view
|

Re: Implement Event()/Signal()/Wait() AML operations

Mark Kettenis
In reply to this post by Ted Unangst-6
> From: "Ted Unangst" <[hidden email]>
> Date: Wed, 09 Jan 2019 19:01:09 -0500
>
> Mark Kettenis wrote:
> > Index: dev/acpi/dsdt.c
> > ===================================================================
> > RCS file: /cvs/src/sys/dev/acpi/dsdt.c,v
> > retrieving revision 1.243
> > diff -u -p -r1.243 dsdt.c
> > --- dev/acpi/dsdt.c 19 Aug 2018 08:23:47 -0000 1.243
> > +++ dev/acpi/dsdt.c 8 Jan 2019 18:55:14 -0000
> > @@ -2905,29 +2905,33 @@ int
> >  acpi_event_wait(struct aml_scope *scope, struct aml_value *evt, int timeout)
> >  {
> >   /* Wait for event to occur; do work in meantime */
> > - evt->v_evt.state = 0;
> > - while (!evt->v_evt.state) {
> > - if (!acpi_dotask(acpi_softc) && !cold)
> > - tsleep(evt, PWAIT, "acpievt", 1);
> > - else
> > - delay(100);
> > + while (evt->v_evt.state == 0 && timeout >= 0) {
> > + if (acpi_dotask(acpi_softc))
> > +    continue;
> > + if (!cold) {
> > + if (rwsleep(evt, &acpi_softc->sc_lck, PWAIT,
> > +    "acpievt", 1) == EWOULDBLOCK) {
> > + if (timeout < 0xffff)
> > + timeout -= (1000 / hz);
> > + }
> > + } else {
> > + delay(1000);
> > + if (timeout < 0xffff)
> > + timeout--;
> > + }
>
> Does 0xffff come from ACPI? Can we give that a name?

Sure.  Committed with an additional AML_NO_TIMEOUT define.

Reply | Threaded
Open this post in threaded view
|

Re: Implement Event()/Signal()/Wait() AML operations

Mark Kettenis
In reply to this post by Theo de Raadt-2
> From: "Theo de Raadt" <[hidden email]>
> Date: Thu, 10 Jan 2019 10:23:27 -0700
>
> Ted Unangst <[hidden email]> wrote:
>
> > Ted Unangst wrote:
> > >
> > > Does 0xffff come from ACPI? Can we give that a name?
> > >
> > > I thought sleeping for one tick is kinda weird, but I see what
> > > it's doing with the acpi_dotask loop. This feels precarious, but
> > > whatever.
> >
> > So upon further thought, this is pretty bad. If the new task also
> > calls sem wait, we'll end up back here, recursing on the event
> > loop. And of course, the first sem can't wakeup until after the
> > second one does. If you want to say it was like this when you got
> > here, ok, but pretending to pause like this is troublesome. There
> > was a bug in libc rpc code where it tried to do something similar
> > and blew up the stack.
>
> Not sure I understand.  There is only one acpi task.  

No there can be many acpi tasks, so there is indeed a risk.  But we
have to allow other acpi tasks to run while we're blocked on the
semaphore otherwise we'll never wake up!