On Wed, Aug 03, 2016 at 01:16:11PM +0200, Mark Kettenis wrote:
> > Date: Wed, 3 Aug 2016 13:22:33 +0300
> > From: Paul Irofti <p...@irofti.net>
> >
> > Hi,
> >
> > I was looking at fixing powerdown on the x260 Skylake machine and ran
> > into the EC XXX comment from acpi_powerdown().
> >
> > I think that grabbing the global lock before doing the AML calls is a
> > good start.
> >
> > What we are missing now is incrementing the global_lock_count variable
> > from the ACPI thread so that calls to acpi_glk_leave() take into account
> > our hold of the lock.
> >
> > Should we make that a global variable and protect increments and
> > decrements from acpi_glk_{enter,leave}? This way we could also increment
> > it from acpi_powerdown() and that would put an end to this issue.
>
> Where in the acpi spec does it say you have to grab the acpi global lock?
>
> Unless it explicitly says that, we defenitely shouldn't grab it. The
> global lock is all about locking out the firmware, not the acpi
> thread. And by locking out the firmware when we shouldn't, we will
> probably cause hangs on some machines.
>
> I don't think that XXX is a big issue by the way. By the time
> acpi_powerdown() runs we've halted the secondary CPUs and are running
> on the primary CPU anyway. As long as the acpi_powerdown() doesn't
> sleep, the acpi thread shouldn't interfere. And from a firmware
> perspective this is indistinguishable from running the code in the
> acpi thread.
>
> Mike Larkin was talking recently about moving the S5 transition
> handling into acpi_sleep_state(). If that happens it would be trivial
> to run from the acpi thread. That is probably a better strategy to
> get rid of that XXX.
>
Yes, this is a better way. But I have not made much progress there (lack
of time).
-ml
> Cheers,
>
> Mark
>
>
> > Index: acpi.c
> > ===================================================================
> > RCS file: /cvs/src/sys/dev/acpi/acpi.c,v
> > retrieving revision 1.313
> > diff -u -p -u -p -r1.313 acpi.c
> > --- acpi.c 28 Jul 2016 21:57:56 -0000 1.313
> > +++ acpi.c 3 Aug 2016 10:12:15 -0000
> > @@ -2497,27 +2497,31 @@ fail_tts:
> > return (error);
> > }
> >
> > -/* XXX
> > - * We are going to do AML execution but are not in the acpi thread.
> > - * We do not know if the acpi thread is sleeping on acpiec in some
> > - * intermediate context. Wish us luck.
> > - */
> > void
> > acpi_powerdown(void)
> > {
> > int state = ACPI_STATE_S5, s;
> > struct acpi_softc *sc = acpi_softc;
> > + int st = 0;
> >
> > if (acpi_enabled == 0)
> > return;
> >
> > + /*
> > + * We are going to do AML execution but are not in the acpi thread.
> > + * Grab the global lock to make sure that the acpi thread is not
> > + * sleeping on acpiec in some intermediate context.
> > + */
> > + while (!st)
> > + st = acpi_acquire_glk(&sc->sc_facs->global_lock);
> > +
> > s = splhigh();
> > disable_intr();
> > cold = 1;
> >
> > /* 1st powerdown AML step: _PTS(tostate) */
> > aml_node_setval(sc, sc->sc_pts, state);
> > -
> > +
> > acpi_disable_allgpes(sc);
> > acpi_enable_wakegpes(sc, state);
> >
> >
> >
>
