From owner-freebsd-acpi@FreeBSD.ORG Tue Nov 15 19:38:09 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 47A2216A41F; Tue, 15 Nov 2005 19:38:09 +0000 (GMT) (envelope-from pldrouin@pldrouin.net) Received: from smtp.cyberfingers.net (smtp.cyberfingers.net [198.177.254.227]) by mx1.FreeBSD.org (Postfix) with ESMTP id 980E743D5C; Tue, 15 Nov 2005 19:38:07 +0000 (GMT) (envelope-from pldrouin@pldrouin.net) Received: from [134.117.23.170] (pldrouinlap-pc.physics.carleton.ca [134.117.23.170]) by smtp.cyberfingers.net (Postfix) with ESMTP id 5AAEF17002D; Tue, 15 Nov 2005 14:38:06 -0500 (EST) Message-ID: <437A3901.8010001@pldrouin.net> Date: Tue, 15 Nov 2005 14:37:37 -0500 From: Pierre-Luc Drouin User-Agent: Mozilla Thunderbird 1.0.7 (X11/20051107) X-Accept-Language: en-us, en MIME-Version: 1.0 To: freebsd-stable@freebsd.org References: <4377775B.3080606@pldrouin.net> <20051114105854.GA1041@galgenberg.net> <4378CC14.2020109@pldrouin.net> In-Reply-To: Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Cc: acpi@freebsd.org Subject: Re: Performance problem since updating from 6.0-RELEASE to 6.0-STABLE last friday 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: Tue, 15 Nov 2005 19:38:09 -0000 Hajimu UMEMOTO wrote: >Hi, > > > >>>>>>On Mon, 14 Nov 2005 12:40:36 -0500 >>>>>>Pierre-Luc Drouin said: >>>>>> >>>>>> > >pldrouin> Yep, smart battery is definately the problem. The performance of my >pldrouin> laptop is back to normal when I remove the xfce4-battery-plugin. >pldrouin> acpiconf -i loop reproduces the problem for me too. So it looks like >pldrouin> there is something wrong in smart battery. > >The cmbat has similar issue on some laptops. So, acpi_cmbat.c uses >cache for retrieval to reduce its influence, and its expiration >time is set by hw.acpi.battery.info_expire. >However, acpi_smbat.c doesn't use cache. So, I made a patch. Since I >don't have a laptop which has smbat, I cannot test it by myself. >Please test it and let me know the result. > >Index: sys/dev/acpica/acpi_smbat.c >diff -u -p sys/dev/acpica/acpi_smbat.c.orig sys/dev/acpica/acpi_smbat.c >--- sys/dev/acpica/acpi_smbat.c.orig Sun Nov 6 08:55:56 2005 >+++ sys/dev/acpica/acpi_smbat.c Tue Nov 15 16:41:00 2005 >@@ -44,11 +44,18 @@ __FBSDID("$FreeBSD: src/sys/dev/acpica/a > struct acpi_smbat_softc { > uint8_t sb_base_addr; > device_t ec_dev; >+ >+ struct acpi_bif bif; >+ struct acpi_bst bst; >+ struct timespec bif_lastupdated; >+ struct timespec bst_lastupdated; > }; > > static int acpi_smbat_probe(device_t dev); > static int acpi_smbat_attach(device_t dev); > static int acpi_smbat_shutdown(device_t dev); >+static int acpi_smbat_info_expired(struct timespec *lastupdated); >+static void acpi_smbat_info_updated(struct timespec *lastupdated); > static int acpi_smbat_get_bif(device_t dev, struct acpi_bif *bif); > static int acpi_smbat_get_bst(device_t dev, struct acpi_bst *bst); > >@@ -114,6 +121,9 @@ acpi_smbat_attach(device_t dev) > return (ENXIO); > } > >+ timespecclear(&sc->bif_lastupdated); >+ timespecclear(&sc->bst_lastupdated); >+ > if (acpi_battery_register(dev) != 0) { > device_printf(dev, "cannot register battery\n"); > return (ENXIO); >@@ -132,6 +142,34 @@ acpi_smbat_shutdown(device_t dev) > } > > static int >+acpi_smbat_info_expired(struct timespec *lastupdated) >+{ >+ struct timespec curtime; >+ >+ ACPI_SERIAL_ASSERT(smbat); >+ >+ if (lastupdated == NULL) >+ return (TRUE); >+ if (!timespecisset(lastupdated)) >+ return (TRUE); >+ >+ getnanotime(&curtime); >+ timespecsub(&curtime, lastupdated); >+ return (curtime.tv_sec < 0 || >+ curtime.tv_sec > acpi_battery_get_info_expire()); >+} >+ >+static void >+acpi_smbat_info_updated(struct timespec *lastupdated) >+{ >+ >+ ACPI_SERIAL_ASSERT(smbat); >+ >+ if (lastupdated != NULL) >+ getnanotime(lastupdated); >+} >+ >+static int > acpi_smbus_read_2(struct acpi_smbat_softc *sc, uint8_t addr, uint8_t cmd, > uint16_t *ptr) > { >@@ -284,6 +322,11 @@ acpi_smbat_get_bst(device_t dev, struct > error = ENXIO; > sc = device_get_softc(dev); > >+ if (!acpi_smbat_info_expired(&sc->bst_lastupdated)) { >+ error = 0; >+ goto out; >+ } >+ > if (acpi_smbus_read_2(sc, addr, SMBATT_CMD_BATTERY_MODE, &val)) > goto out; > if (val & SMBATT_BM_CAPACITY_MODE) { >@@ -299,7 +342,7 @@ acpi_smbat_get_bst(device_t dev, struct > goto out; > > if (val & SMBATT_BS_DISCHARGING) { >- bst->state |= ACPI_BATT_STAT_DISCHARG; >+ sc->bst.state |= ACPI_BATT_STAT_DISCHARG; > > /* > * If the rate is negative, it is discharging. Otherwise, >@@ -308,27 +351,31 @@ acpi_smbat_get_bst(device_t dev, struct > if (acpi_smbus_read_2(sc, addr, SMBATT_CMD_AT_RATE, &val)) > goto out; > if (val < 0) >- bst->rate = (-val) * factor; >+ sc->bst.rate = (-val) * factor; > else >- bst->rate = -1; >+ sc->bst.rate = -1; > } else { >- bst->state |= ACPI_BATT_STAT_CHARGING; >- bst->rate = -1; >+ sc->bst.state |= ACPI_BATT_STAT_CHARGING; >+ sc->bst.rate = -1; > } > > if (val & SMBATT_BS_REMAINING_CAPACITY_ALARM) >- bst->state |= ACPI_BATT_STAT_CRITICAL; >+ sc->bst.state |= ACPI_BATT_STAT_CRITICAL; > > if (acpi_smbus_read_2(sc, addr, SMBATT_CMD_REMAINING_CAPACITY, &val)) > goto out; >- bst->cap = val * factor; >+ sc->bst.cap = val * factor; > > if (acpi_smbus_read_2(sc, addr, SMBATT_CMD_VOLTAGE, &val)) > goto out; >- bst->volt = val; >+ sc->bst.volt = val; >+ >+ acpi_smbat_info_updated(&sc->bst_lastupdated); >+ > error = 0; > > out: >+ memcpy(bst, &sc->bst, sizeof(sc->bst)); > ACPI_SERIAL_END(smbat); > return (error); > } >@@ -348,55 +395,63 @@ acpi_smbat_get_bif(device_t dev, struct > error = ENXIO; > sc = device_get_softc(dev); > >+ if (!acpi_smbat_info_expired(&sc->bif_lastupdated)) { >+ error = 0; >+ goto out; >+ } >+ > if (acpi_smbus_read_2(sc, addr, SMBATT_CMD_BATTERY_MODE, &val)) > goto out; > if (val & SMBATT_BM_CAPACITY_MODE) { > factor = 10; >- bif->units = ACPI_BIF_UNITS_MW; >+ sc->bif.units = ACPI_BIF_UNITS_MW; > } else { > factor = 1; >- bif->units = ACPI_BIF_UNITS_MA; >+ sc->bif.units = ACPI_BIF_UNITS_MA; > } > > if (acpi_smbus_read_2(sc, addr, SMBATT_CMD_DESIGN_CAPACITY, &val)) > goto out; >- bif->dcap = val * factor; >+ sc->bif.dcap = val * factor; > > if (acpi_smbus_read_2(sc, addr, SMBATT_CMD_FULL_CHARGE_CAPACITY, &val)) > goto out; >- bif->lfcap = val * factor; >- bif->btech = 1; /* secondary (rechargeable) */ >+ sc->bif.lfcap = val * factor; >+ sc->bif.btech = 1; /* secondary (rechargeable) */ > > if (acpi_smbus_read_2(sc, addr, SMBATT_CMD_DESIGN_VOLTAGE, &val)) > goto out; >- bif->dvol = val; >+ sc->bif.dvol = val; > >- bif->wcap = bif->dcap / 10; >- bif->lcap = bif->dcap / 10; >+ sc->bif.wcap = sc->bif.dcap / 10; >+ sc->bif.lcap = sc->bif.dcap / 10; > >- bif->gra1 = factor; /* not supported */ >- bif->gra2 = factor; /* not supported */ >+ sc->bif.gra1 = factor; /* not supported */ >+ sc->bif.gra2 = factor; /* not supported */ > > if (acpi_smbus_read_multi_1(sc, addr, SMBATT_CMD_DEVICE_NAME, >- bif->model, sizeof(bif->model))) >+ sc->bif.model, sizeof(sc->bif.model))) > goto out; > > if (acpi_smbus_read_2(sc, addr, SMBATT_CMD_SERIAL_NUMBER, &val)) > goto out; >- snprintf(bif->serial, sizeof(bif->serial), "0x%04x", val); >+ snprintf(sc->bif.serial, sizeof(sc->bif.serial), "0x%04x", val); > > if (acpi_smbus_read_multi_1(sc, addr, SMBATT_CMD_DEVICE_CHEMISTRY, >- bif->type, sizeof(bif->type))) >+ sc->bif.type, sizeof(sc->bif.type))) > goto out; > > if (acpi_smbus_read_multi_1(sc, addr, SMBATT_CMD_MANUFACTURER_DATA, >- bif->oeminfo, sizeof(bif->oeminfo))) >+ sc->bif.oeminfo, sizeof(sc->bif.oeminfo))) > goto out; > >+ acpi_smbat_info_updated(&sc->bif_lastupdated); >+ > /* XXX check if device was replugged during read? */ > error = 0; > > out: >+ memcpy(bif, &sc->bif, sizeof(sc->bif)); > ACPI_SERIAL_END(smbat); > return (error); > } > > >Sincerely, > >-- >Hajimu UMEMOTO @ Internet Mutual Aid Society Yokohama, Japan >ume@mahoroba.org ume@{,jp.}FreeBSD.org >http://www.imasy.org/~ume/ > > > The patch seams to do its job correctly, but it is still very annoying to have the whole computer to freeze for 1 second when the cache expires. What does make the whole system to freeze? Before the code was changed in 6.0-stable, FreeBSD was able to read the battery status without freezing my laptop... I have been running 3 OSes (FreeBSD, Ubuntu and Win XP) on my laptop for a while and never experienced that kind of problem with either Linux or Win XP. I guess there is something wrong in the new code added after 6.0-release. Thank you! Pierre-Luc Drouin