[Fwd: cvs commit: src/sys/dev/acpica acpi_cmbat.c]

Here is a patch that should fix the battery hangs on RELENG_6.  It was 
tested to work fine, although I need testing from an affected user to 
verify it fixes the problem.  It was committed to HEAD and will be MFCed 
if it fixes the problem.

I'm a bit disappointed that no one reported this problem in the 2 weeks 
it was present in 7-current.  If you have the time to run -current on at 
least one partition of your laptop, that would assist me greatly.

Thanks,
-Nate

-------- Original Message --------
Subject: cvs commit: src/sys/dev/acpica acpi_cmbat.c
Date: Wed, 23 Nov 2005 00:58:05 +0000 (GMT)
From: Nate Lawson <njl_at_FreeBSD.org>
To: njl_at_FreeBSD.ORG

njl         2005-11-23 00:57:51 UTC

   FreeBSD src repository

   Modified files:
     sys/dev/acpica       acpi_cmbat.c
   Log:
   Try to fix problems with periodic hangs by never directly calling _BIF.
   Instead, re-evaluate _BIF only when we get a notify and use the cached
   results.  We also still evaluate _BIF once on boot.  Also, optimize the
   init loop a little by only querying for a particular info if it's not 
valid.

   MFC after:      2 days

   Revision  Changes    Path
   1.42      +34 -22    src/sys/dev/acpica/acpi_cmbat.c


Index: src/sys/dev/acpica/acpi_cmbat.c
diff -u src/sys/dev/acpica/acpi_cmbat.c:1.41 
src/sys/dev/acpica/acpi_cmbat.c:1.42
--- src/sys/dev/acpica/acpi_cmbat.c:1.41	Sun Sep 11 18:39:01 2005
+++ src/sys/dev/acpica/acpi_cmbat.c	Wed Nov 23 00:57:51 2005
_at__at_ -66,7 +66,6 _at__at_

      struct acpi_bif bif;
      struct acpi_bst bst;
-    struct timespec bif_lastupdated;
      struct timespec bst_lastupdated;
  };

_at__at_ -80,8 +79,8 _at__at_
  			    void *context);
  static int		acpi_cmbat_info_expired(struct timespec *lastupdated);
  static void		acpi_cmbat_info_updated(struct timespec *lastupdated);
-static void		acpi_cmbat_get_bst(device_t dev);
-static void		acpi_cmbat_get_bif(device_t dev);
+static void		acpi_cmbat_get_bst(void *arg);
+static void		acpi_cmbat_get_bif(void *arg);
  static int		acpi_cmbat_bst(device_t dev, struct acpi_bst *bstp);
  static int		acpi_cmbat_bif(device_t dev, struct acpi_bif *bifp);
  static void		acpi_cmbat_init_battery(void *arg);
_at__at_ -134,7 +133,6 _at__at_
      handle = acpi_get_handle(dev);
      sc->dev = dev;

-    timespecclear(&sc->bif_lastupdated);
      timespecclear(&sc->bst_lastupdated);

      error = acpi_battery_register(dev);
_at__at_ -180,20 +178,22 _at__at_
      dev = (device_t)context;
      sc = device_get_softc(dev);

-    /*
-     * Clear the appropriate last updated time.  The next call to retrieve
-     * the battery status will get the new value for us.  We don't need to
-     * acquire a lock since we are only clearing the time stamp and since
-     * calling _BST/_BIF can trigger a notify, we could deadlock also.
-     */
      switch (notify) {
      case ACPI_NOTIFY_DEVICE_CHECK:
      case ACPI_BATTERY_BST_CHANGE:
+	/*
+	 * Clear the last updated time.  The next call to retrieve the
+	 * battery status will get the new value for us.
+	 */
  	timespecclear(&sc->bst_lastupdated);
  	break;
      case ACPI_NOTIFY_BUS_CHECK:
      case ACPI_BATTERY_BIF_CHANGE:
-	timespecclear(&sc->bif_lastupdated);
+	/*
+	 * Queue a callback to get the current battery info from thread
+	 * context.  It's not safe to block in a notify handler.
+	 */
+	AcpiOsQueueForExecution(OSD_PRIORITY_LO, acpi_cmbat_get_bif, dev);
  	break;
      }

_at__at_ -229,16 +229,18 _at__at_
  }

  static void
-acpi_cmbat_get_bst(device_t dev)
+acpi_cmbat_get_bst(void *arg)
  {
      struct acpi_cmbat_softc *sc;
      ACPI_STATUS	as;
      ACPI_OBJECT	*res;
      ACPI_HANDLE	h;
      ACPI_BUFFER	bst_buffer;
+    device_t dev;

      ACPI_SERIAL_ASSERT(cmbat);

+    dev = arg;
      sc = device_get_softc(dev);
      h = acpi_get_handle(dev);
      bst_buffer.Pointer = NULL;
_at__at_ -287,24 +289,23 _at__at_
  }

  static void
-acpi_cmbat_get_bif(device_t dev)
+acpi_cmbat_get_bif(void *arg)
  {
      struct acpi_cmbat_softc *sc;
      ACPI_STATUS	as;
      ACPI_OBJECT	*res;
      ACPI_HANDLE	h;
      ACPI_BUFFER	bif_buffer;
+    device_t dev;

      ACPI_SERIAL_ASSERT(cmbat);

+    dev = arg;
      sc = device_get_softc(dev);
      h = acpi_get_handle(dev);
      bif_buffer.Pointer = NULL;
      bif_buffer.Length = ACPI_ALLOCATE_BUFFER;

-    if (!acpi_cmbat_info_expired(&sc->bif_lastupdated))
-	goto end;
-
      as = AcpiEvaluateObject(h, "_BIF", NULL, &bif_buffer);
      if (ACPI_FAILURE(as)) {
  	ACPI_VPRINT(dev, acpi_device_get_parent_softc(dev),
_at__at_ -346,7 +347,6 _at__at_
  	goto end;
      if (acpi_PkgStr(res, 12, sc->bif.oeminfo, ACPI_CMBAT_MAXSTRLEN) != 0)
  	goto end;
-    acpi_cmbat_info_updated(&sc->bif_lastupdated);

  end:
      if (bif_buffer.Pointer != NULL)
_at__at_ -360,8 +360,13 _at__at_

      sc = device_get_softc(dev);

+    /*
+     * Just copy the data.  The only value that should change is the
+     * last-full capacity, so we only update when we get a notify that says
+     * the info has changed.  Many systems apparently take a long time to
+     * process a _BIF call so we avoid it if possible.
+     */
      ACPI_SERIAL_BEGIN(cmbat);
-    acpi_cmbat_get_bif(dev);
      bifp->units = sc->bif.units;
      bifp->dcap = sc->bif.dcap;
      bifp->lfcap = sc->bif.lfcap;
_at__at_ -422,11 +427,18 _at__at_
  	if (!acpi_BatteryIsPresent(dev))
  	    continue;

+	/*
+	 * Only query the battery if this is the first try or the specific
+	 * type of info is still invalid.
+	 */
  	ACPI_SERIAL_BEGIN(cmbat);
-	timespecclear(&sc->bst_lastupdated);
-	timespecclear(&sc->bif_lastupdated);
-	acpi_cmbat_get_bst(dev);
-	acpi_cmbat_get_bif(dev);
+	if (retry == 0 || !acpi_battery_bst_valid(&sc->bst)) {
+	    timespecclear(&sc->bst_lastupdated);
+	    acpi_cmbat_get_bst(dev);
+	}
+	if (retry == 0 || !acpi_battery_bif_valid(&sc->bif))
+	    acpi_cmbat_get_bif(dev);
+
  	valid = acpi_battery_bst_valid(&sc->bst) &&
  	    acpi_battery_bif_valid(&sc->bif);
  	ACPI_SERIAL_END(cmbat);

-- 
Nate