Bug 124744

Summary:

[acpi] [patch] incorrect _BST result validation for Tosh Satellite P20

Product:

Base System

Reporter:

Yuri Skripachov <y.skripachov>

Component:

kern

Assignee:

Andriy Gapon <avg>

Status:

Closed FIXED

Severity:

Affects Only Me

Priority:

Normal

Version:

7.0-STABLE

Hardware:

Any

OS:

Any

Attachments:

Description	Flags
file.diff	none

Description Yuri Skripachov 2008-06-19 07:20:02 UTC

"acpiconf -i 0" command does not show battery presence if notebook is AC
powered. that is because of extra bits used in _BST state by toshiba.
according to acpi specs the only wrong _BST state is (ACPI_BATT_STAT_CHARGING
| ACPI_BATT_STAT_DISCHARG). it seems that other bits in _BST have to be
ignored and arithmetic comparison is a wrong way to validate _BST state.
at least for my satellite :( i have been using this patch for 5.x, 6.x
and 7.0-stable.

Fix: patch for /usr/src/sys/dev/acpica/acpi_battery.c is attached.

Patch attached with submission follows:
How-To-Repeat: plug in AC adapter and type acpiconf -i 0.

Comment 1 Mark Linimon freebsd_committer

2008-06-19 22:05:29 UTC

Responsible Changed
From-To: freebsd-bugs->freebsd-acpi

Over to maintainer(s).

Comment 2 Andriy Gapon freebsd_committer

2010-12-05 15:20:53 UTC

Is this still an issue?
If yes, I will try to review and commit your patch.
Thanks!
-- 
Andriy Gapon

Comment 3 Andriy Gapon freebsd_committer

2010-12-14 18:34:21 UTC

Responsible Changed
From-To: freebsd-acpi->avg

I am taking a closer look at this report.

Comment 4 Andriy Gapon 2010-12-16 18:05:26 UTC

I think that the patch is good and I have slightly extended it:
http://people.freebsd.org/~avg/acpi-bat.diff

Part of the problem is that the same definitions are used for interpreting status
returned by _BST and for maintaining internal driver status.
I have added a comment that explains this duality, retired now unused
ACPI_BATT_STAT_MAX, added code for cleaning extended/undefined bits in _BST status
and added a warning about charging+discharging bits being set at the same time.

I'd appreciate reviews and testing.
Thanks!

-- 
Andriy Gapon

Comment 5 dfilter service freebsd_committer

2010-12-17 16:21:38 UTC

Author: avg
Date: Fri Dec 17 16:21:30 2010
New Revision: 216503
URL: http://svn.freebsd.org/changeset/base/216503

Log:
  small cleanup of acpi battery status setting and checking
  
  This is based on the patch submitted by Yuri Skripachov.
  Overview of the changes:
  - clarify double-use of some ACPI_BATT_STAT_* definitions
  - clean up undefined/extended status bits returned by _BST
  - warn about charging+discharging bits being set at the same time
  
  PR:		kern/124744
  Submitted by:	Yuri Skripachov <y.skripachov@gmail.com>
  Tested by:	Yuri Skripachov <y.skripachov@gmail.com>
  MFC after:	2 weeks

Modified:
  head/sys/dev/acpica/acpi_battery.c
  head/sys/dev/acpica/acpi_cmbat.c
  head/sys/dev/acpica/acpi_smbat.c
  head/sys/dev/acpica/acpiio.h

Modified: head/sys/dev/acpica/acpi_battery.c
==============================================================================
--- head/sys/dev/acpica/acpi_battery.c	Fri Dec 17 15:39:55 2010	(r216502)
+++ head/sys/dev/acpica/acpi_battery.c	Fri Dec 17 16:21:30 2010	(r216503)
@@ -102,8 +102,9 @@ acpi_battery_get_info_expire(void)
 int
 acpi_battery_bst_valid(struct acpi_bst *bst)
 {
-    return (bst->state < ACPI_BATT_STAT_MAX && bst->cap != ACPI_BATT_UNKNOWN &&
-	bst->volt != ACPI_BATT_UNKNOWN);
+
+    return (bst->state != ACPI_BATT_STAT_NOT_PRESENT &&
+	bst->cap != ACPI_BATT_UNKNOWN && bst->volt != ACPI_BATT_UNKNOWN);
 }
 
 /* Check _BIF results for validity. */

Modified: head/sys/dev/acpica/acpi_cmbat.c
==============================================================================
--- head/sys/dev/acpica/acpi_cmbat.c	Fri Dec 17 15:39:55 2010	(r216502)
+++ head/sys/dev/acpica/acpi_cmbat.c	Fri Dec 17 16:21:30 2010	(r216503)
@@ -279,6 +279,12 @@ acpi_cmbat_get_bst(void *arg)
 	goto end;
     acpi_cmbat_info_updated(&sc->bst_lastupdated);
 
+    /* Clear out undefined/extended bits that might be set by hardware. */
+    sc->bst.state &= ACPI_BATT_STAT_BST_MASK;
+    if ((sc->bst.state & ACPI_BATT_STAT_INVALID) == ACPI_BATT_STAT_INVALID)
+	ACPI_VPRINT(dev, acpi_device_get_parent_softc(dev),
+	    "battery reports simultaneous charging and discharging\n");
+
     /* XXX If all batteries are critical, perhaps we should suspend. */
     if (sc->bst.state & ACPI_BATT_STAT_CRITICAL) {
     	if ((sc->flags & ACPI_BATT_STAT_CRITICAL) == 0) {

Modified: head/sys/dev/acpica/acpi_smbat.c
==============================================================================
--- head/sys/dev/acpica/acpi_smbat.c	Fri Dec 17 15:39:55 2010	(r216502)
+++ head/sys/dev/acpica/acpi_smbat.c	Fri Dec 17 16:21:30 2010	(r216503)
@@ -390,6 +390,7 @@ acpi_smbat_get_bst(device_t dev, struct 
 
 	if (val > 0) {
 		sc->bst.rate = val * factor;
+		sc->bst.state &= ~SMBATT_BS_DISCHARGING;
 		sc->bst.state |= ACPI_BATT_STAT_CHARGING;
 	} else if (val < 0)
 		sc->bst.rate = (-val) * factor;

Modified: head/sys/dev/acpica/acpiio.h
==============================================================================
--- head/sys/dev/acpica/acpiio.h	Fri Dec 17 15:39:55 2010	(r216502)
+++ head/sys/dev/acpica/acpiio.h	Fri Dec 17 16:21:30 2010	(r216503)
@@ -74,11 +74,22 @@ struct acpi_bst {
     uint32_t volt;			/* Present Voltage */
 };
 
+/*
+ * Note that the following definitions represent status bits for internal
+ * driver state.  The first three of them (charging, discharging and critical)
+ * conveninetly conform to ACPI specification of status returned by _BST
+ * method.  Other definitions (not present, etc) are synthetic.
+ * Also note that according to the specification the charging and discharging
+ * status bits must not be set at the same time.
+ */
 #define ACPI_BATT_STAT_DISCHARG		0x0001
 #define ACPI_BATT_STAT_CHARGING		0x0002
 #define ACPI_BATT_STAT_CRITICAL		0x0004
-#define ACPI_BATT_STAT_NOT_PRESENT	0x0007
-#define ACPI_BATT_STAT_MAX		0x0007
+#define ACPI_BATT_STAT_INVALID					\
+    (ACPI_BATT_STAT_DISCHARG | ACPI_BATT_STAT_CHARGING)
+#define ACPI_BATT_STAT_BST_MASK					\
+    (ACPI_BATT_STAT_INVALID | ACPI_BATT_STAT_CRITICAL)
+#define ACPI_BATT_STAT_NOT_PRESENT	ACPI_BATT_STAT_BST_MASK
 
 union acpi_battery_ioctl_arg {
     int			 unit;	/* Device unit or ACPI_BATTERY_ALL_UNITS. */
_______________________________________________
svn-src-all@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscribe@freebsd.org"

Comment 6 Andriy Gapon freebsd_committer

2010-12-17 16:25:44 UTC

State Changed
From-To: open->patched

The workaround is committed to head.

Comment 7 dfilter service freebsd_committer

2011-03-11 16:43:59 UTC

Author: avg
Date: Fri Mar 11 16:43:39 2011
New Revision: 219507
URL: http://svn.freebsd.org/changeset/base/219507

Log:
  MFC r216503: small cleanup of acpi battery status setting and checking
  
  PR:		kern/124744

Modified:
  stable/8/sys/dev/acpica/acpi_battery.c
  stable/8/sys/dev/acpica/acpi_cmbat.c
  stable/8/sys/dev/acpica/acpi_smbat.c
  stable/8/sys/dev/acpica/acpiio.h
Directory Properties:
  stable/8/sys/   (props changed)
  stable/8/sys/amd64/include/xen/   (props changed)
  stable/8/sys/cddl/contrib/opensolaris/   (props changed)
  stable/8/sys/contrib/dev/acpica/   (props changed)
  stable/8/sys/contrib/pf/   (props changed)

Modified: stable/8/sys/dev/acpica/acpi_battery.c
==============================================================================
--- stable/8/sys/dev/acpica/acpi_battery.c	Fri Mar 11 16:30:30 2011	(r219506)
+++ stable/8/sys/dev/acpica/acpi_battery.c	Fri Mar 11 16:43:39 2011	(r219507)
@@ -102,8 +102,9 @@ acpi_battery_get_info_expire(void)
 int
 acpi_battery_bst_valid(struct acpi_bst *bst)
 {
-    return (bst->state < ACPI_BATT_STAT_MAX && bst->cap != ACPI_BATT_UNKNOWN &&
-	bst->volt != ACPI_BATT_UNKNOWN);
+
+    return (bst->state != ACPI_BATT_STAT_NOT_PRESENT &&
+	bst->cap != ACPI_BATT_UNKNOWN && bst->volt != ACPI_BATT_UNKNOWN);
 }
 
 /* Check _BIF results for validity. */

Modified: stable/8/sys/dev/acpica/acpi_cmbat.c
==============================================================================
--- stable/8/sys/dev/acpica/acpi_cmbat.c	Fri Mar 11 16:30:30 2011	(r219506)
+++ stable/8/sys/dev/acpica/acpi_cmbat.c	Fri Mar 11 16:43:39 2011	(r219507)
@@ -279,6 +279,12 @@ acpi_cmbat_get_bst(void *arg)
 	goto end;
     acpi_cmbat_info_updated(&sc->bst_lastupdated);
 
+    /* Clear out undefined/extended bits that might be set by hardware. */
+    sc->bst.state &= ACPI_BATT_STAT_BST_MASK;
+    if ((sc->bst.state & ACPI_BATT_STAT_INVALID) == ACPI_BATT_STAT_INVALID)
+	ACPI_VPRINT(dev, acpi_device_get_parent_softc(dev),
+	    "battery reports simultaneous charging and discharging\n");
+
     /* XXX If all batteries are critical, perhaps we should suspend. */
     if (sc->bst.state & ACPI_BATT_STAT_CRITICAL) {
     	if ((sc->flags & ACPI_BATT_STAT_CRITICAL) == 0) {

Modified: stable/8/sys/dev/acpica/acpi_smbat.c
==============================================================================
--- stable/8/sys/dev/acpica/acpi_smbat.c	Fri Mar 11 16:30:30 2011	(r219506)
+++ stable/8/sys/dev/acpica/acpi_smbat.c	Fri Mar 11 16:43:39 2011	(r219507)
@@ -390,6 +390,7 @@ acpi_smbat_get_bst(device_t dev, struct 
 
 	if (val > 0) {
 		sc->bst.rate = val * factor;
+		sc->bst.state &= ~SMBATT_BS_DISCHARGING;
 		sc->bst.state |= ACPI_BATT_STAT_CHARGING;
 	} else if (val < 0)
 		sc->bst.rate = (-val) * factor;

Modified: stable/8/sys/dev/acpica/acpiio.h
==============================================================================
--- stable/8/sys/dev/acpica/acpiio.h	Fri Mar 11 16:30:30 2011	(r219506)
+++ stable/8/sys/dev/acpica/acpiio.h	Fri Mar 11 16:43:39 2011	(r219507)
@@ -74,11 +74,22 @@ struct acpi_bst {
     uint32_t volt;			/* Present Voltage */
 };
 
+/*
+ * Note that the following definitions represent status bits for internal
+ * driver state.  The first three of them (charging, discharging and critical)
+ * conveninetly conform to ACPI specification of status returned by _BST
+ * method.  Other definitions (not present, etc) are synthetic.
+ * Also note that according to the specification the charging and discharging
+ * status bits must not be set at the same time.
+ */
 #define ACPI_BATT_STAT_DISCHARG		0x0001
 #define ACPI_BATT_STAT_CHARGING		0x0002
 #define ACPI_BATT_STAT_CRITICAL		0x0004
-#define ACPI_BATT_STAT_NOT_PRESENT	0x0007
-#define ACPI_BATT_STAT_MAX		0x0007
+#define ACPI_BATT_STAT_INVALID					\
+    (ACPI_BATT_STAT_DISCHARG | ACPI_BATT_STAT_CHARGING)
+#define ACPI_BATT_STAT_BST_MASK					\
+    (ACPI_BATT_STAT_INVALID | ACPI_BATT_STAT_CRITICAL)
+#define ACPI_BATT_STAT_NOT_PRESENT	ACPI_BATT_STAT_BST_MASK
 
 union acpi_battery_ioctl_arg {
     int			 unit;	/* Device unit or ACPI_BATTERY_ALL_UNITS. */
_______________________________________________
svn-src-all@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscribe@freebsd.org"

Comment 8 dfilter service freebsd_committer

2011-03-11 17:12:54 UTC

Author: avg
Date: Fri Mar 11 17:12:39 2011
New Revision: 219510
URL: http://svn.freebsd.org/changeset/base/219510

Log:
  MFC r216503: small cleanup of acpi battery status setting and checking
  
  PR:		kern/124744

Modified:
  stable/7/sys/dev/acpica/acpi_battery.c
  stable/7/sys/dev/acpica/acpi_cmbat.c
  stable/7/sys/dev/acpica/acpi_smbat.c
  stable/7/sys/dev/acpica/acpiio.h
Directory Properties:
  stable/7/sys/   (props changed)
  stable/7/sys/cddl/contrib/opensolaris/   (props changed)
  stable/7/sys/contrib/dev/acpica/   (props changed)
  stable/7/sys/contrib/pf/   (props changed)

Modified: stable/7/sys/dev/acpica/acpi_battery.c
==============================================================================
--- stable/7/sys/dev/acpica/acpi_battery.c	Fri Mar 11 17:01:14 2011	(r219509)
+++ stable/7/sys/dev/acpica/acpi_battery.c	Fri Mar 11 17:12:39 2011	(r219510)
@@ -101,8 +101,9 @@ acpi_battery_get_info_expire(void)
 int
 acpi_battery_bst_valid(struct acpi_bst *bst)
 {
-    return (bst->state < ACPI_BATT_STAT_MAX && bst->cap != ACPI_BATT_UNKNOWN &&
-	bst->volt != ACPI_BATT_UNKNOWN);
+
+    return (bst->state != ACPI_BATT_STAT_NOT_PRESENT &&
+	bst->cap != ACPI_BATT_UNKNOWN && bst->volt != ACPI_BATT_UNKNOWN);
 }
 
 /* Check _BIF results for validity. */

Modified: stable/7/sys/dev/acpica/acpi_cmbat.c
==============================================================================
--- stable/7/sys/dev/acpica/acpi_cmbat.c	Fri Mar 11 17:01:14 2011	(r219509)
+++ stable/7/sys/dev/acpica/acpi_cmbat.c	Fri Mar 11 17:12:39 2011	(r219510)
@@ -278,6 +278,12 @@ acpi_cmbat_get_bst(void *arg)
 	goto end;
     acpi_cmbat_info_updated(&sc->bst_lastupdated);
 
+    /* Clear out undefined/extended bits that might be set by hardware. */
+    sc->bst.state &= ACPI_BATT_STAT_BST_MASK;
+    if ((sc->bst.state & ACPI_BATT_STAT_INVALID) == ACPI_BATT_STAT_INVALID)
+	ACPI_VPRINT(dev, acpi_device_get_parent_softc(dev),
+	    "battery reports simultaneous charging and discharging\n");
+
     /* XXX If all batteries are critical, perhaps we should suspend. */
     if (sc->bst.state & ACPI_BATT_STAT_CRITICAL) {
     	if ((sc->flags & ACPI_BATT_STAT_CRITICAL) == 0) {

Modified: stable/7/sys/dev/acpica/acpi_smbat.c
==============================================================================
--- stable/7/sys/dev/acpica/acpi_smbat.c	Fri Mar 11 17:01:14 2011	(r219509)
+++ stable/7/sys/dev/acpica/acpi_smbat.c	Fri Mar 11 17:12:39 2011	(r219510)
@@ -389,6 +389,7 @@ acpi_smbat_get_bst(device_t dev, struct 
 
 	if (val > 0) {
 		sc->bst.rate = val * factor;
+		sc->bst.state &= ~SMBATT_BS_DISCHARGING;
 		sc->bst.state |= ACPI_BATT_STAT_CHARGING;
 	} else if (val < 0)
 		sc->bst.rate = (-val) * factor;

Modified: stable/7/sys/dev/acpica/acpiio.h
==============================================================================
--- stable/7/sys/dev/acpica/acpiio.h	Fri Mar 11 17:01:14 2011	(r219509)
+++ stable/7/sys/dev/acpica/acpiio.h	Fri Mar 11 17:12:39 2011	(r219510)
@@ -74,11 +74,22 @@ struct acpi_bst {
     uint32_t volt;			/* Present Voltage */
 };
 
+/*
+ * Note that the following definitions represent status bits for internal
+ * driver state.  The first three of them (charging, discharging and critical)
+ * conveninetly conform to ACPI specification of status returned by _BST
+ * method.  Other definitions (not present, etc) are synthetic.
+ * Also note that according to the specification the charging and discharging
+ * status bits must not be set at the same time.
+ */
 #define ACPI_BATT_STAT_DISCHARG		0x0001
 #define ACPI_BATT_STAT_CHARGING		0x0002
 #define ACPI_BATT_STAT_CRITICAL		0x0004
-#define ACPI_BATT_STAT_NOT_PRESENT	0x0007
-#define ACPI_BATT_STAT_MAX		0x0007
+#define ACPI_BATT_STAT_INVALID					\
+    (ACPI_BATT_STAT_DISCHARG | ACPI_BATT_STAT_CHARGING)
+#define ACPI_BATT_STAT_BST_MASK					\
+    (ACPI_BATT_STAT_INVALID | ACPI_BATT_STAT_CRITICAL)
+#define ACPI_BATT_STAT_NOT_PRESENT	ACPI_BATT_STAT_BST_MASK
 
 union acpi_battery_ioctl_arg {
     int			 unit;	/* Device unit or ACPI_BATTERY_ALL_UNITS. */
_______________________________________________
svn-src-all@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscribe@freebsd.org"

Comment 9 Andriy Gapon freebsd_committer

2011-03-11 18:03:12 UTC

State Changed
From-To: patched->closed

The fixed has been MFCed.