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--