From owner-freebsd-acpi@FreeBSD.ORG Wed Nov 23 01:06:25 2005 Return-Path: X-Original-To: acpi@FreeBSD.org Delivered-To: freebsd-acpi@FreeBSD.ORG Received: from mx1.FreeBSD.org (mx1.freebsd.org [216.136.204.125]) by hub.freebsd.org (Postfix) with ESMTP id 470E916A41F; Wed, 23 Nov 2005 01:06:25 +0000 (GMT) (envelope-from nate@root.org) Received: from ylpvm15.prodigy.net (ylpvm15-ext.prodigy.net [207.115.57.46]) by mx1.FreeBSD.org (Postfix) with ESMTP id 1541A43D53; Wed, 23 Nov 2005 01:06:24 +0000 (GMT) (envelope-from nate@root.org) Received: from pimout7-ext.prodigy.net (pimout7-int.prodigy.net [207.115.4.147]) by ylpvm15.prodigy.net (8.12.10 outbound/8.12.10) with ESMTP id jAN16Lug031527; Tue, 22 Nov 2005 20:06:21 -0500 X-ORBL: [71.139.30.140] Received: from [10.0.5.50] (ppp-71-139-30-140.dsl.snfc21.pacbell.net [71.139.30.140]) by pimout7-ext.prodigy.net (8.13.4 outbound domainkey aix/8.13.4) with ESMTP id jAN16H0K098214; Tue, 22 Nov 2005 20:06:18 -0500 Message-ID: <4383C083.6010300@root.org> Date: Tue, 22 Nov 2005 17:06:11 -0800 From: Nate Lawson User-Agent: Mozilla Thunderbird 1.0.6 (X11/20050723) X-Accept-Language: en-us, en MIME-Version: 1.0 To: acpi@FreeBSD.org Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Cc: stable@FreeBSD.org, FreeBSD Current Subject: [Fwd: cvs commit: src/sys/dev/acpica acpi_cmbat.c] X-BeenThere: freebsd-acpi@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: ACPI and power management development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 23 Nov 2005 01:06:25 -0000 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 To: njl@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 @@ -66,7 +66,6 @@ struct acpi_bif bif; struct acpi_bst bst; - struct timespec bif_lastupdated; struct timespec bst_lastupdated; }; @@ -80,8 +79,8 @@ 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); @@ -134,7 +133,6 @@ handle = acpi_get_handle(dev); sc->dev = dev; - timespecclear(&sc->bif_lastupdated); timespecclear(&sc->bst_lastupdated); error = acpi_battery_register(dev); @@ -180,20 +178,22 @@ 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; } @@ -229,16 +229,18 @@ } 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; @@ -287,24 +289,23 @@ } 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), @@ -346,7 +347,6 @@ 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) @@ -360,8 +360,13 @@ 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; @@ -422,11 +427,18 @@ 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