MPSAFE locking

* Serialize ops in acpi_cmbat_notify_handler(), acpi_cmbat_ioctl(),
  acpi_cmbat_init_battery(), and acpi_cmbat_get_battinfo().
* Get the softc directly in acpi_cmbat_get_total_battinfo() rather than
  build an array of them.
* Don't queue a _BIF query after receiving a notify.  Since we clear the
  timespec, a _BIF query will be done in the context of the next caller.
* Add asserts to leaf functions that operate on shared data.
* Remove the bst/bif updating flags now that we hold the lock over the
  full query.
* Explain various comments in more detail.

1 files changed, 127 insertions, 171 deletions

diff --git a/sys/dev/acpica/acpi_cmbat.c b/sys/dev/acpica/acpi_cmbat.c
index 362d0a1..e849346 100644
--- a/sys/dev/acpica/acpi_cmbat.c
+++ b/sys/dev/acpica/acpi_cmbat.c
@@ -49,14 +49,14 @@ MALLOC_DEFINE(M_ACPICMBAT, "acpicmbat", "ACPI control method battery data");
 #define ACPI_CMBAT_RETRY_MAX	6
 
 /* Check the battery once a minute. */
-#define	CMBAT_POLLRATE	(60 * hz)
+#define	CMBAT_POLLRATE		(60 * hz)
 
 /* Hooks for the ACPI CA debugging infrastructure */
 #define	_COMPONENT	ACPI_BATTERY
 ACPI_MODULE_NAME("BATTERY")
 
-#define	ACPI_BATTERY_BST_CHANGE 0x80
-#define	ACPI_BATTERY_BIF_CHANGE 0x81
+#define	ACPI_BATTERY_BST_CHANGE	0x80
+#define	ACPI_BATTERY_BIF_CHANGE	0x81
 
 struct acpi_cmbat_softc {
     device_t	    dev;
@@ -65,8 +65,6 @@ struct acpi_cmbat_softc {
     struct acpi_bst bst;
     struct timespec bif_lastupdated;
     struct timespec bst_lastupdated;
-    int		    bif_updating;
-    int		    bst_updating;
 
     int		    present;
     int		    cap;
@@ -77,6 +75,7 @@ struct acpi_cmbat_softc {
 };
 
 static struct timespec	acpi_cmbat_info_lastupdated;
+ACPI_SERIAL_DECL(cmbat, "ACPI cmbat");
 
 /* XXX: devclass_get_maxunit() don't give us the current allocated units. */
 static int		acpi_cmbat_units = 0;
@@ -121,10 +120,12 @@ acpi_cmbat_info_expired(struct timespec *lastupdated)
 {
     struct timespec	curtime;
 
+    ACPI_SERIAL_ASSERT(cmbat);
+
     if (lastupdated == NULL)
-	return (1);
+	return (TRUE);
     if (!timespecisset(lastupdated))
-	return (1);
+	return (TRUE);
 
     getnanotime(&curtime);
     timespecsub(&curtime, lastupdated);
@@ -132,10 +133,12 @@ acpi_cmbat_info_expired(struct timespec *lastupdated)
 	    curtime.tv_sec > acpi_battery_get_info_expire());
 }
 
-
 static void
 acpi_cmbat_info_updated(struct timespec *lastupdated)
 {
+
+    ACPI_SERIAL_ASSERT(cmbat);
+
     if (lastupdated != NULL)
 	getnanotime(lastupdated);
 }
@@ -150,18 +153,17 @@ acpi_cmbat_get_bst(void *context)
     ACPI_HANDLE	h;
     ACPI_BUFFER	bst_buffer;
 
+    ACPI_SERIAL_ASSERT(cmbat);
+
     dev = context;
     sc = device_get_softc(dev);
     h = acpi_get_handle(dev);
+    bst_buffer.Pointer = NULL;
+    bst_buffer.Length = ACPI_ALLOCATE_BUFFER;
 
     if (!acpi_cmbat_info_expired(&sc->bst_lastupdated))
-	return;
-    if (sc->bst_updating)
-	return;
-    sc->bst_updating = 1;
+	goto end;
 
-    bst_buffer.Pointer = NULL;
-    bst_buffer.Length = ACPI_ALLOCATE_BUFFER;
     as = AcpiEvaluateObject(h, "_BST", NULL, &bst_buffer);
     if (ACPI_FAILURE(as)) {
 	ACPI_VPRINT(dev, acpi_device_get_parent_softc(dev),
@@ -190,7 +192,6 @@ acpi_cmbat_get_bst(void *context)
 end:
     if (bst_buffer.Pointer != NULL)
 	AcpiOsFree(bst_buffer.Pointer);
-    sc->bst_updating = 0;
 }
 
 static void
@@ -203,18 +204,17 @@ acpi_cmbat_get_bif(void *context)
     ACPI_HANDLE	h;
     ACPI_BUFFER	bif_buffer;
 
+    ACPI_SERIAL_ASSERT(cmbat);
+
     dev = context;
     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))
-	return;
-    if (sc->bif_updating)
-	return;
-    sc->bif_updating = 1;
+	goto end;
 
-    bif_buffer.Pointer = NULL;
-    bif_buffer.Length = ACPI_ALLOCATE_BUFFER;
     as = AcpiEvaluateObject(h, "_BIF", NULL, &bif_buffer);
     if (ACPI_FAILURE(as)) {
 	ACPI_VPRINT(dev, acpi_device_get_parent_softc(dev),
@@ -230,23 +230,23 @@ acpi_cmbat_get_bif(void *context)
 	goto end;
     }
 
-    if (acpi_PkgInt32(res,  0, &sc->bif.units) != 0)
+    if (acpi_PkgInt32(res, 0, &sc->bif.units) != 0)
 	goto end;
-    if (acpi_PkgInt32(res,  1, &sc->bif.dcap) != 0)
+    if (acpi_PkgInt32(res, 1, &sc->bif.dcap) != 0)
 	goto end;
-    if (acpi_PkgInt32(res,  2, &sc->bif.lfcap) != 0)
+    if (acpi_PkgInt32(res, 2, &sc->bif.lfcap) != 0)
 	goto end;
-    if (acpi_PkgInt32(res,  3, &sc->bif.btech) != 0)
+    if (acpi_PkgInt32(res, 3, &sc->bif.btech) != 0)
 	goto end;
-    if (acpi_PkgInt32(res,  4, &sc->bif.dvol) != 0)
+    if (acpi_PkgInt32(res, 4, &sc->bif.dvol) != 0)
 	goto end;
-    if (acpi_PkgInt32(res,  5, &sc->bif.wcap) != 0)
+    if (acpi_PkgInt32(res, 5, &sc->bif.wcap) != 0)
 	goto end;
-    if (acpi_PkgInt32(res,  6, &sc->bif.lcap) != 0)
+    if (acpi_PkgInt32(res, 6, &sc->bif.lcap) != 0)
 	goto end;
-    if (acpi_PkgInt32(res,  7, &sc->bif.gra1) != 0)
+    if (acpi_PkgInt32(res, 7, &sc->bif.gra1) != 0)
 	goto end;
-    if (acpi_PkgInt32(res,  8, &sc->bif.gra2) != 0)
+    if (acpi_PkgInt32(res, 8, &sc->bif.gra2) != 0)
 	goto end;
     if (acpi_PkgStr(res,  9, sc->bif.model, ACPI_CMBAT_MAXSTRLEN) != 0)
 	goto end;
@@ -261,7 +261,6 @@ acpi_cmbat_get_bif(void *context)
 end:
     if (bif_buffer.Pointer != NULL)
 	AcpiOsFree(bif_buffer.Pointer);
-    sc->bif_updating = 0;
 }
 
 static void
@@ -271,11 +270,11 @@ acpi_cmbat_notify_handler(ACPI_HANDLE h, UINT32 notify, void *context)
     struct acpi_cmbat_softc	*sc;
 
     dev = (device_t)context;
-    if ((sc = device_get_softc(dev)) == NULL)
-	return;
+    sc = device_get_softc(dev);
 
     acpi_UserNotify("CMBAT", h, notify);
 
+    ACPI_SERIAL_BEGIN(cmbat);
     switch (notify) {
     case ACPI_NOTIFY_DEVICE_CHECK:
     case ACPI_BATTERY_BST_CHANGE:
@@ -284,11 +283,11 @@ acpi_cmbat_notify_handler(ACPI_HANDLE h, UINT32 notify, void *context)
     case ACPI_NOTIFY_BUS_CHECK:
     case ACPI_BATTERY_BIF_CHANGE:
 	timespecclear(&sc->bif_lastupdated);
-	AcpiOsQueueForExecution(OSD_PRIORITY_LO, acpi_cmbat_get_bif, dev);
 	break;
     default:
 	break;
     }
+    ACPI_SERIAL_END(cmbat);
 }
 
 static int
@@ -311,10 +310,9 @@ acpi_cmbat_attach(device_t dev)
     ACPI_HANDLE	handle;
     struct acpi_cmbat_softc *sc;
 
-    if ((sc = device_get_softc(dev)) == NULL)
-	return (ENXIO);
-
+    sc = device_get_softc(dev);
     handle = acpi_get_handle(dev);
+    sc->dev = dev;
 
     /*
      * Install a system notify handler in addition to the device notify.
@@ -323,31 +321,35 @@ acpi_cmbat_attach(device_t dev)
     AcpiInstallNotifyHandler(handle, ACPI_ALL_NOTIFY,
 			     acpi_cmbat_notify_handler, dev);
 
-    sc->bif_updating = sc->bst_updating = 0;
-    sc->dev = dev;
-
+    ACPI_SERIAL_BEGIN(cmbat);
     timespecclear(&sc->bif_lastupdated);
     timespecclear(&sc->bst_lastupdated);
 
     if (acpi_cmbat_units == 0) {
 	error = acpi_register_ioctl(ACPIIO_CMBAT_GET_BIF,
 				    acpi_cmbat_ioctl, NULL);
-	if (error != 0)
+	if (error != 0) {
+	    device_printf(dev, "register bif ioctl failed\n");
 	    return (error);
+	}
 	error = acpi_register_ioctl(ACPIIO_CMBAT_GET_BST,
 				    acpi_cmbat_ioctl, NULL);
-	if (error != 0)
-		return (error);
+	if (error != 0) {
+	    device_printf(dev, "register bst ioctl failed\n");
+	    return (error);
+	}
     }
 
     sc->phys_unit = acpi_cmbat_units;
     error = acpi_battery_register(ACPI_BATT_TYPE_CMBAT, sc->phys_unit);
-    if (error != 0)
+    if (error != 0) {
+    	device_printf(dev, "registering battery %d failed\n", sc->phys_unit);
 	return (error);
-
+    }
     acpi_cmbat_units++;
     timespecclear(&acpi_cmbat_info_lastupdated);
-    sc->initializing = 0;
+    ACPI_SERIAL_END(cmbat);
+
     AcpiOsQueueForExecution(OSD_PRIORITY_LO, acpi_cmbat_init_battery, dev);
 
     return (0);
@@ -359,8 +361,10 @@ acpi_cmbat_detach(device_t dev)
     struct acpi_cmbat_softc *sc;
 
     sc = device_get_softc(dev);
+    ACPI_SERIAL_BEGIN(cmbat);
     acpi_battery_remove(ACPI_BATT_TYPE_CMBAT, sc->phys_unit);
     acpi_cmbat_units--;
+    ACPI_SERIAL_END(cmbat);
     return (0);
 }
 
@@ -374,7 +378,7 @@ acpi_cmbat_resume(device_t dev)
 static int
 acpi_cmbat_ioctl(u_long cmd, caddr_t addr, void *arg)
 {
-    device_t	dev;
+    device_t dev;
     union acpi_battery_ioctl_arg *ioctl_arg;
     struct acpi_cmbat_softc *sc;
     struct acpi_bif	*bifp;
@@ -385,13 +389,12 @@ acpi_cmbat_ioctl(u_long cmd, caddr_t addr, void *arg)
     if (dev == NULL)
 	return (ENXIO);
     sc = device_get_softc(dev);
-    if (sc == NULL)
-	return (ENXIO);
 
     /*
      * No security check required: information retrieval only.  If
      * new functions are added here, a check might be required.
      */
+    ACPI_SERIAL_BEGIN(cmbat);
     switch (cmd) {
     case ACPIIO_CMBAT_GET_BIF:
 	acpi_cmbat_get_bif(dev);
@@ -425,6 +428,7 @@ acpi_cmbat_ioctl(u_long cmd, caddr_t addr, void *arg)
     default:
 	break;
     }
+    ACPI_SERIAL_END(cmbat);
 
     return (0);
 }
@@ -434,19 +438,18 @@ acpi_cmbat_is_bst_valid(struct acpi_bst *bst)
 {
     if (bst->state >= ACPI_BATT_STAT_MAX || bst->cap == 0xffffffff ||
 	bst->volt == 0xffffffff)
-
-	return (0);
+	return (FALSE);
     else
-	return (1);
+	return (TRUE);
 }
 
 static int
 acpi_cmbat_is_bif_valid(struct acpi_bif *bif)
 {
     if (bif->lfcap == 0)
-	return (0);
+	return (FALSE);
     else
-	return (1);
+	return (TRUE);
 }
 
 static int
@@ -458,136 +461,105 @@ acpi_cmbat_get_total_battinfo(struct acpi_battinfo *battinfo)
     int		valid_rate, valid_units;
     int		cap, min;
     int		total_cap, total_min, total_full;
-    device_t	dev;
     struct acpi_cmbat_softc *sc;
-    static int	bat_units = 0;
-    static struct acpi_cmbat_softc **bat = NULL;
+
+    ACPI_SERIAL_ASSERT(cmbat);
 
     cap = min = -1;
     batt_stat = ACPI_BATT_STAT_NOT_PRESENT;
     error = 0;
 
-    /* Allocate array of softc pointers */
-    if (bat_units != acpi_cmbat_units) {
-	if (bat != NULL) {
-	    free(bat, M_ACPICMBAT);
-	    bat = NULL;
-	}
-	bat_units = 0;
-    }
-    if (bat == NULL) {
-	bat_units = acpi_cmbat_units;
-	bat = malloc(sizeof(struct acpi_cmbat_softc *) * bat_units,
-		     M_ACPICMBAT, M_NOWAIT);
-	if (bat == NULL) {
-	    error = ENOMEM;
-	    goto out;
-	}
-
-	/* Collect softc pointers */
-	for (i = 0; i < acpi_cmbat_units; i++) {
-	    if ((dev = devclass_get_device(acpi_cmbat_devclass, i)) == NULL) {
-		error = ENXIO;
-		goto out;
-	    }
-	    if ((sc = device_get_softc(dev)) == NULL) {
-		error = ENXIO;
-		goto out;
-	    }
-	    bat[i] = sc;
-	}
-    }
-
     /* Get battery status, valid rate and valid units */
     batt_stat = valid_rate = valid_units = 0;
     for (i = 0; i < acpi_cmbat_units; i++) {
-	bat[i]->present = acpi_BatteryIsPresent(bat[i]->dev);
-	if (!bat[i]->present)
+	sc = devclass_get_softc(acpi_cmbat_devclass, i);
+	if (sc == NULL)
 	    continue;
-
-	acpi_cmbat_get_bst(bat[i]->dev);
+	sc->present = acpi_BatteryIsPresent(sc->dev);
+	if (!sc->present)
+	    continue;
+	acpi_cmbat_get_bst(sc->dev);
 
 	/* If battery not installed, we get strange values */
-	if (!acpi_cmbat_is_bst_valid(&(bat[i]->bst)) ||
-	    !acpi_cmbat_is_bif_valid(&(bat[i]->bif))) {
-
-	    bat[i]->present = 0;
+	if (!acpi_cmbat_is_bst_valid(&sc->bst) ||
+	    !acpi_cmbat_is_bif_valid(&sc->bif)) {
+	    sc->present = FALSE;
 	    continue;
 	}
 
 	valid_units++;
-	bat[i]->cap = 100 * bat[i]->bst.cap / bat[i]->bif.lfcap;
+	sc->cap = 100 * sc->bst.cap / sc->bif.lfcap;
 
 	/* 
 	 * Some laptops report the "design-capacity" instead of the 
 	 * "real-capacity" when the battery is fully charged.
 	 * That breaks the above arithmetic as it needs to be 100% maximum.
 	 */
-	if (bat[i]->cap > 100)
-	    bat[i]->cap = 100;
-
-	batt_stat |= bat[i]->bst.state;
-
-	if (bat[i]->bst.rate > 0) {
-	    /*
-	     * XXX Hack to calculate total battery time.
-	     * Systems with 2 or more battries, they may get used
-	     * one by one, thus bst.rate is set only to the one
-	     * in use. For remaining batteries bst.rate = 0, which
-	     * makes it impossible to calculate remaining time.
-	     * Some other systems may need sum of bst.rate in
-	     * dis-charging state.
-	     * There for we sum up the bst.rate that is valid
-	     * (in dis-charging state), and use the sum to
-	     * calcutate remaining batteries' time.
-	     */
-	    if (bat[i]->bst.state & ACPI_BATT_STAT_DISCHARG)
-		valid_rate += bat[i]->bst.rate;
+	if (sc->cap > 100)
+	    sc->cap = 100;
+
+	batt_stat |= sc->bst.state;
+
+	/*
+	 * XXX Hack to calculate total battery time.
+	 *
+	 * On systems with more than one battery, they may get used
+	 * sequentially, thus bst.rate may only signify the one in use.
+	 * For the remaining batteries, bst.rate will be zero, which
+	 * makes it impossible to calculate the remaining time.  Some
+	 * other systems may need the sum of all the bst.rate values
+	 * when discharging.  Therefore, we sum the bst.rate for valid
+	 * batteries (ones in the discharging state) and use the sum
+	 * to calculate the total remaining time.
+	 */
+	if (sc->bst.rate > 0) {
+	    if (sc->bst.state & ACPI_BATT_STAT_DISCHARG)
+		valid_rate += sc->bst.rate;
 	}
     }
 
     /* Calculate total battery capacity and time */
     total_cap = total_min = total_full = 0;
     for (i = 0; i < acpi_cmbat_units; i++) {
-	if (!bat[i]->present)
+	sc = devclass_get_softc(acpi_cmbat_devclass, i);
+	if (!sc->present)
 	    continue;
 
-	if (valid_rate > 0) {
-	    /* Use the sum of bst.rate */
-	    bat[i]->min = 60 * bat[i]->bst.cap / valid_rate;
-	} else if (bat[i]->full_charge_time > 0) {
-	    bat[i]->min = (bat[i]->full_charge_time * bat[i]->cap) / 100;
-	} else {
-	    /* Couldn't find valid rate and full battery time */
-	    bat[i]->min = 0;
-	}
-	total_min += bat[i]->min;
-	total_cap += bat[i]->cap;
-	total_full += bat[i]->full_charge_time;
+	/*
+	 * If any batteries are discharging, use the sum of the bst.rate
+	 * values.  Otherwise, use the full charge time to estimate
+	 * remaining time.  If neither are available, assume no charge.
+	 */
+	if (valid_rate > 0)
+	    sc->min = 60 * sc->bst.cap / valid_rate;
+	else if (sc->full_charge_time > 0)
+	    sc->min = (sc->full_charge_time * sc->cap) / 100;
+	else
+	    sc->min = 0;
+	total_min += sc->min;
+	total_cap += sc->cap;
+	total_full += sc->full_charge_time;
     }
 
     /* Battery life */
     if (valid_units == 0) {
 	cap = -1;
 	batt_stat = ACPI_BATT_STAT_NOT_PRESENT;
-    } else {
+    } else
 	cap = total_cap / valid_units;
-    }
 
     /* Battery time */
-    if (valid_units == 0) {
+    if (valid_units == 0)
 	min = -1;
-    } else if (valid_rate == 0 || (batt_stat & ACPI_BATT_STAT_CHARGING)) {
+    else if (valid_rate == 0 || (batt_stat & ACPI_BATT_STAT_CHARGING)) {
 	if (total_full == 0)
 	    min = -1;
 	else
 	    min = (total_full * cap) / 100;
-    } else {
+    } else
 	min = total_min;
-    }
     acpi_cmbat_info_updated(&acpi_cmbat_info_lastupdated);
 
-out:
     battinfo->cap = cap;
     battinfo->min = min;
     battinfo->state = batt_stat;
@@ -598,14 +570,12 @@ out:
 static void
 acpi_cmbat_init_battery(void *arg)
 {
+    struct acpi_cmbat_softc *sc;
     int		retry;
-    device_t	dev = (device_t)arg;
-    struct acpi_cmbat_softc *sc = device_get_softc(dev);
-
-    if (sc->initializing)
-	return;
+    device_t	dev;
 
-    sc->initializing = 1;
+    dev = (device_t)arg;
+    sc = device_get_softc(dev);
     ACPI_VPRINT(dev, acpi_device_get_parent_softc(dev),
 		"battery initialization start\n");
 
@@ -614,20 +584,18 @@ acpi_cmbat_init_battery(void *arg)
 	if (!sc->present)
 	    continue;
 
+	ACPI_SERIAL_BEGIN(cmbat);
 	timespecclear(&sc->bst_lastupdated);
 	timespecclear(&sc->bif_lastupdated);
-
 	acpi_cmbat_get_bst(dev);
-	if (!acpi_cmbat_is_bst_valid(&sc->bst))
-	    continue;
-
 	acpi_cmbat_get_bif(dev);
-	if (!acpi_cmbat_is_bif_valid(&sc->bif))
-	    continue;
-	break;
+	ACPI_SERIAL_END(cmbat);
+
+	if (acpi_cmbat_is_bst_valid(&sc->bst) &&
+	    acpi_cmbat_is_bif_valid(&sc->bif))
+	    break;
     }
 
-    sc->initializing = 0;
     if (retry == ACPI_CMBAT_RETRY_MAX) {
 	ACPI_VPRINT(dev, acpi_device_get_parent_softc(dev),
 		    "battery initialization failed, giving up\n");
@@ -644,32 +612,18 @@ int
 acpi_cmbat_get_battinfo(int unit, struct acpi_battinfo *battinfo)
 {
     int		error;
-    device_t	dev;
     struct acpi_cmbat_softc *sc;
 
-    if (unit == -1)
-	return (acpi_cmbat_get_total_battinfo(battinfo));
-
-    if (acpi_cmbat_info_expired(&acpi_cmbat_info_lastupdated)) {
-	error = acpi_cmbat_get_total_battinfo(battinfo);
-	if (error)
-	    goto out;
-    }
-
-    error = 0;
-    if (unit >= acpi_cmbat_units) {
-	error = ENXIO;
+    ACPI_SERIAL_BEGIN(cmbat);
+    error = acpi_cmbat_get_total_battinfo(battinfo);
+    if (unit == -1 || error)
 	goto out;
-    }
 
-    if ((dev = devclass_get_device(acpi_cmbat_devclass, unit)) == NULL) {
-	error = ENXIO;
+    error = ENXIO;
+    if (unit >= acpi_cmbat_units)
 	goto out;
-    }
-    if ((sc = device_get_softc(dev)) == NULL) {
-	error = ENXIO;
+    if ((sc = devclass_get_softc(acpi_cmbat_devclass, unit)) == NULL)
 	goto out;
-    }
 
     if (!sc->present) {
 	battinfo->cap = -1;
@@ -680,7 +634,9 @@ acpi_cmbat_get_battinfo(int unit, struct acpi_battinfo *battinfo)
 	battinfo->min = sc->min;
 	battinfo->state = sc->bst.state;
     }
+    error = 0;
 
 out:
+    ACPI_SERIAL_END(cmbat);
     return (error);
 }