> 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.
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);
>
>
>
