Subject: Re: acpi_ec locking
To: YAMAMOTO Takashi <yamt@mwd.biglobe.ne.jp>
From: Charles M. Hannum <abuse@spamalicious.com>
List: tech-kern
Date: 11/03/2003 16:10:01
--Boundary-00=_Z3np/WstGxXyCNg
Content-Type: text/plain;
charset="iso-8859-1"
Content-Transfer-Encoding: 7bit
Content-Disposition: inline
On Monday 03 November 2003 01:41 pm, YAMAMOTO Takashi wrote:
> hi,
>
> is attached diff correct? ideas mostly taken from freebsd.
> i don't see why AcpiGbl_GpeLock is needed here.
>
> i guess _GLK handling should be in more generic place, though...
This isn't *quite* right.
1) Most of the systems I've looked at have no _GLK method, so we need to deal
with that without spewing an error message.
2) If we're using lockmgr(), we might as well use lockstatus() rather than
keeping our own "locked" bit.
3) I'm not convinced it makes sense to have a lock timeout.
Anyway, I've fixed the first two. Can someone test these on machines with
acpiec0?
--Boundary-00=_Z3np/WstGxXyCNg
Content-Type: text/x-diff;
charset="iso-8859-1";
name="ec-diff"
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment;
filename="ec-diff"
Index: acpi_ec.c
===================================================================
RCS file: /cvsroot/src/sys/dev/acpi/acpi_ec.c,v
retrieving revision 1.16
diff -u -r1.16 acpi_ec.c
--- acpi_ec.c 3 Nov 2003 06:03:47 -0000 1.16
+++ acpi_ec.c 3 Nov 2003 16:05:30 -0000
@@ -207,6 +207,10 @@
int sc_flags; /* see below */
uint32_t sc_csrvalue; /* saved control register */
+
+ struct lock sc_lock; /* serialize operations to this EC */
+ UINT32 sc_glkhandle; /* global lock handle */
+ UINT32 sc_glk; /* need global lock? */
};
static const char * const ec_hid[] = {
@@ -214,9 +218,14 @@
NULL
};
-#define EC_F_LOCKED 0x01 /* EC is locked */
#define EC_F_PENDQUERY 0x02 /* query is pending */
+/*
+ * how long to wait to acquire global lock.
+ * the value is taken from FreeBSD driver.
+ */
+#define EC_LOCK_TIMEOUT 1000
+
#define EC_DATA_READ(sc) \
bus_space_read_1((sc)->sc_data_st, (sc)->sc_data_sh, 0)
#define EC_DATA_WRITE(sc, v) \
@@ -231,23 +240,39 @@
EcIsLocked(struct acpi_ec_softc *sc)
{
- return (sc->sc_flags & EC_F_LOCKED);
+ return (lockstatus(&sc->sc_lock) == LK_EXCLUSIVE);
}
static __inline void
EcLock(struct acpi_ec_softc *sc)
{
+ ACPI_STATUS status;
- sc->sc_flags |= EC_F_LOCKED;
- AcpiOsAcquireLock (AcpiGbl_GpeLock, ACPI_NOT_ISR);
+ lockmgr(&sc->sc_lock, LK_EXCLUSIVE, NULL);
+ if (sc->sc_glk) {
+ status = AcpiAcquireGlobalLock(EC_LOCK_TIMEOUT,
+ &sc->sc_glkhandle);
+ if (ACPI_FAILURE(status)) {
+ printf("%s: failed to acquire global lock\n",
+ sc->sc_dev.dv_xname);
+ lockmgr(&sc->sc_lock, LK_RELEASE, NULL);
+ return;
+ }
+ }
}
static __inline void
EcUnlock(struct acpi_ec_softc *sc)
{
+ ACPI_STATUS status;
- AcpiOsReleaseLock (AcpiGbl_GpeLock, ACPI_NOT_ISR);
- sc->sc_flags &= ~EC_F_LOCKED;
+ if (sc->sc_glk) {
+ status = AcpiReleaseGlobalLock(sc->sc_glkhandle);
+ if (ACPI_FAILURE(status))
+ printf("%s: failed to release global lock\n",
+ sc->sc_dev.dv_xname);
+ }
+ lockmgr(&sc->sc_lock, LK_RELEASE, NULL);
}
typedef struct {
@@ -312,6 +337,8 @@
printf(": ACPI Embedded Controller\n");
+ lockinit(&sc->sc_lock, PWAIT, "eclock", 0, 0);
+
sc->sc_node = aa->aa_node;
/* Parse our resources. */
@@ -363,6 +390,18 @@
printf("%s: unable to evaluate _GPE: %d\n",
sc->sc_dev.dv_xname, rv);
return;
+ }
+
+ /*
+ * evaluate _GLK to see if we should acquire global lock
+ * when accessing the EC.
+ */
+ if ((rv = acpi_eval_integer(sc->sc_node->ad_handle, "_GLK",
+ &sc->sc_glk)) != AE_OK) {
+ if (rv != AE_NOT_FOUND)
+ printf("%s: unable to evaluate _GLK: %d\n",
+ sc->sc_dev.dv_xname, rv);
+ sc->sc_glk = 0;
}
/*
--Boundary-00=_Z3np/WstGxXyCNg--