Bug 227191 - Cannot check battery status after upgrading to 12-CURRENT after r330957 (ACPI _STA method removed)
Summary: Cannot check battery status after upgrading to 12-CURRENT after r330957 (ACPI...
Status: In Progress
Alias: None
Product: Base System
Classification: Unclassified
Component: misc (show other bugs)
Version: CURRENT
Hardware: Any Any
: --- Affects Many People
Assignee: Andriy Gapon
URL:
Keywords: needs-qa, patch, regression
: 230428 (view as bug list)
Depends on:
Blocks: 230428
  Show dependency treegraph
 
Reported: 2018-04-01 21:24 UTC by Mateusz Piotrowski
Modified: 2018-12-07 15:51 UTC (History)
11 users (show)

See Also:
koobs: mfc-stable12?


Attachments
Fix battery detection code (698 bytes, patch)
2018-08-25 11:01 UTC, Ali Abdallah
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mateusz Piotrowski freebsd_committer 2018-04-01 21:24:32 UTC
I've recently upgraded my box running 12-CURRENT from r330529 to r331748. After the update I can no longer check the battery status:

> $ acpiconf -i 0
> acpiconf: get battery info (0) failed: Operation not permitted
> # acpiconf -i 0
> acpiconf: get battery info (0) failed: Device not configured
> # sysctl hw.acpi.battery
> sysctl: unknown oid 'hw.acpi.battery'

The machine is Yoga 3 14[1].

[1]: https://wiki.freebsd.org/Laptops/Lenovo_Yoga_3_14
Comment 1 Mateusz Piotrowski freebsd_committer 2018-04-04 23:29:57 UTC
I found the following lines in my dmesg:

> ACPI Error: No handler for Region [ERAM] (0xfffff80003621b80) [EmbeddedControl] (20180313/evregion-288)
> ACPI Error: Region EmbeddedControl (ID=3) has no handler (20180313/exfldio-428)
> ACPI Error: Method parse/execution failed \134_SB.PCI0.LPCB.EC0.BAT0._STA, AE_NOT_EXIST (20180313/psparse-677)

I am not sure if they are new but they mention BAT0.
Comment 2 Mateusz Piotrowski freebsd_committer 2018-04-06 14:36:10 UTC
It is not a hardware issue apparently. I've live booted Ubuntu and it detects my battery just fine.
Comment 3 Conrad Meyer freebsd_committer 2018-04-06 15:15:08 UTC
r330957 from that period is the 20180313 ACPICA merge.  Probably suspect.  I don't see any other ACPI changes in that interval that relate to x86 laptops.

Any chance you can try r330956 and r330957 and report whether that isolates it?  Thanks.
Comment 4 Conrad Meyer freebsd_committer 2018-04-06 15:27:14 UTC
Your dmesg line seems related (BAT0):
ACPI Error: Method parse/execution failed \134_SB.PCI0.LPCB.EC0.BAT0._STA, AE_NOT_EXIST

Nothing changed in any of the dmesg-mentioned files (evregion, exfldio, psparse) in that commit.  Hmm.

However, these changes are suspect: sys/contrib/dev/acpica/components/namespace/nsxfname.c

- * For Device and Processor objects, run the Device _HID, _UID, _CID, _STA,
+ * For Device and Processor objects, run the Device _HID, _UID, _CID,
...

- * this was the fate of the _SUB method which was found to cause such
- * problems and was removed (11/2015).
+ * Because of this reason support for the following methods has been removed:
+ * 1) _SUB method was removed (11/2015)
+ * 2) _STA method was removed (02/2018)

-         *
-         * For _STA, if the method does not exist, then (as per the ACPI
-         * specification), the returned CurrentStatus flags will indicate
-         * that the device is present/functional/enabled. Otherwise, the
-         * CurrentStatus flags reflect the value returned from _STA.
          */

-        /* Execute the Device._STA method */
-
-        Status = AcpiUtExecute_STA (Node, &Info->CurrentStatus);
-        if (ACPI_SUCCESS (Status))
-        {
-            Valid |= ACPI_VALID_STA;
-        }


sys/contrib/dev/acpica/changes.txt

+AcpiGetObjectInfo - removed support for the _STA method. This was causing
+problems on some platforms.


And removing it clearly causes problems on others, so...

sys/dev/acpica/acpi.c

 BOOLEAN
 acpi_DeviceIsPresent(device_t dev)
 {
-    ACPI_DEVICE_INFO   *devinfo;
-    ACPI_HANDLE                h;
-    BOOLEAN            present;
+       ACPI_HANDLE h;
+       UINT32 s;
+       ACPI_STATUS status;

-    if ((h = acpi_get_handle(dev)) == NULL ||
-       ACPI_FAILURE(AcpiGetObjectInfo(h, &devinfo)))
-       return (FALSE);
+       h = acpi_get_handle(dev);
+       if (h == NULL)
+               return (FALSE);
+       status = acpi_GetInteger(h, "_STA", &s);

-    /* If no _STA method, must be present */
-    present = (devinfo->Valid & ACPI_VALID_STA) == 0 ||
-       ACPI_DEVICE_PRESENT(devinfo->CurrentStatus) ? TRUE : FALSE;
+       /* If no _STA method, must be present */
+       if (ACPI_FAILURE(status))
+               return (status == AE_NOT_FOUND ? TRUE : FALSE);
...
+       return (ACPI_DEVICE_PRESENT(s) ? TRUE : FALSE);

Ok, this function should behave more or less the same as before...

A similar change was made to acpi_BatteryIsPresent(), but again I don't see the problem.
Comment 5 Mateusz Piotrowski freebsd_committer 2018-04-06 16:15:32 UTC
(In reply to Conrad Meyer from comment #3)

I'm building r330956 at the moment. I'll report back.
Comment 6 Mateusz Piotrowski freebsd_committer 2018-04-08 21:19:35 UTC
I can confirm that `acpiconf -i 0` works as expected on r330956 while being broken on r330957.
Comment 7 Mateusz Piotrowski freebsd_committer 2018-04-09 11:50:29 UTC
(In reply to Mateusz Piotrowski from comment #1)

Also, I get those ACPI ERROR lines from dmesg mentioned even when the battery is detected (before r330957) so they are probably not related.
Comment 8 Mateusz Piotrowski freebsd_committer 2018-05-08 11:34:18 UTC
Same on 12.0-CURRENT r333344.
Comment 9 Mateusz Piotrowski freebsd_committer 2018-05-29 15:54:50 UTC
(In reply to Mateusz Piotrowski from comment #8)

Can I provide any logs/recompile the system with debugging on to help debug this issue?
Comment 10 Ali Abdallah 2018-08-25 11:01:17 UTC
Created attachment 196523 [details]
Fix battery detection code

I worked out a patch to fix the problem. It fixes the issue for me on my corebooted Thinkpad x230. 

On non *BAT0_STA method, acpi_GetInteger returns AE_NOT_EXIST on the new acpica code, since _STA core was removed. 

Give it a try.
Comment 11 Mateusz Piotrowski freebsd_committer 2018-08-25 11:29:51 UTC
(In reply to Ali Abdallah from comment #10)

Thank you for the patch.

Unfortunately, I am unable to this it out soon as the machine stopped booting due to hardware issues a few weeks ago.
Comment 12 Conrad Meyer freebsd_committer 2018-08-26 21:57:10 UTC
(In reply to Ali Abdallah from comment #10)
Patch looks good to me.  I have an X230 but don't know that I can promise to test any time soon.
Comment 13 Andriy Gapon freebsd_committer 2018-09-14 10:51:35 UTC
(In reply to Ali Abdallah from comment #10)

Based on Conrad's investigation in comment #4, why not just return TRUE for any failure of acpi_GetInteger(h, "_STA", &s) ?
I think that would be completely backward compatible with the previous behaviour of AcpiGetObjectInfo.
But this is just a minor suggestion.
Comment 14 Conrad Meyer freebsd_committer 2018-10-09 05:01:11 UTC
*** Bug 230428 has been marked as a duplicate of this bug. ***
Comment 15 Mateusz Piotrowski freebsd_committer 2018-10-25 15:29:40 UTC
Do you think we get it committed before 12.0-RELEASE?
Comment 16 Matías Pizarro 2018-11-24 14:02:38 UTC
Hi all,

FYI, I had the same issue with 13-CURRENT but it now works fine in today';s stable/12 | 12-PRERELEASE which I understand should be the same as 12-RC2.

Thanks for your work on this,

All the best,

-- matías
Comment 17 Mark Johnston freebsd_committer 2018-11-26 14:54:50 UTC
(In reply to Matías Pizarro from comment #16)
There have been several ACPICA updates since the bug was filed, but 13-CURRENT and 12.0 should be in sync.  Which revision of 13-CURRENT did you test?

Can anyone confirm that this is still an issue, either on -CURRENT on 12?
Comment 18 Matías Pizarro 2018-11-26 15:58:00 UTC
I was still having this problem on 13-CURRENT r340703 ( https://svnweb.freebsd.org/base?view=revision&revision=340703 ). But that was nearly 6 days ago, I have not tested further down the 13-CURRENT development branch and, as you said, there's been quite a few ACPI-related commits since then
Comment 19 Andriy Gapon freebsd_committer 2018-12-06 12:07:18 UTC
I am curious if anyone who had this problem before still has it.
Especially, I am curious if they had an error message like in comment#1 and if that message went way.

In addition to the prior analysis I'd like to add the following summary.
- before base r330957 we ignored any _STA evaluation failure (which was performed in ACPICA contrib code) for the purpose of acpi_DeviceIsPresent and acpi_BatteryIsPresent
- ACPICA 20180313 stopped evaluating _STA altogether
- so, we added evaluation of _STA to acpi_DeviceIsPresent and acpi_BatteryIsPresent
- one important difference is that now we ignore a failure only if _STA does not exist (AE_NOT_FOUND)
- any other kind of failure is treated as a failure
- apparently, on some systems we can get AE_NOT_EXIST when evaluating _STA
- that error is not an evil twin of AE_NOT_FOUND, despite a very similar name, but a distinct error related to a missing handler for embedded controller (EC) address space
- it's possible that for some people the problem was fixed by some changes in ACPICA and/or acpi_ec that fixed the AE_NOT_EXIST failure

Still, I would like to re-iterate my proposal that we restore full pre-r330957 behaviour by ignoring any _STA error.
Comment 20 commit-hook freebsd_committer 2018-12-06 12:35:14 UTC
A commit references this bug:

Author: avg
Date: Thu Dec  6 12:34:34 UTC 2018
New revision: 341632
URL: https://svnweb.freebsd.org/changeset/base/341632

Log:
  acpi_{Device,Battery}IsPresent: restore pre-r330957 behaviour

  Specifically, assume that the device is present if evaluation of _STA
  method fails.

  Before r330957 we ignored any _STA evaluation failure (which was
  performed by AcpiGetObjectInfo in ACPICA contrib code) for the purpose
  of acpi_DeviceIsPresent and acpi_BatteryIsPresent.  ACPICA 20180313
  removed evaluation of _STA from AcpiGetObjectInfo.  So, we added
  evaluation of _STA to acpi_DeviceIsPresent and acpi_BatteryIsPresent.
  One important difference is that the new code ignored a failure only if
  _STA did not exist (AE_NOT_FOUND).  Any other kind of failure was
  treated as a fatal failure.  Apparently, on some systems we can get
  AE_NOT_EXIST when evaluating _STA.  And that error is not an evil twin
  of AE_NOT_FOUND, despite a very similar name, but a distinct error
  related to a missing handler for an ACPI operation region.

  It's possible that for some people the problem was already fixed by
  changes in ACPICA and/or in acpi_ec driver (or even in BIOS) that fixed
  the AE_NOT_EXIST failure related to EC operation region.

  This work is based on a great analysis by cem and an earlier patch by
  Ali Abdallah <aliovx@gmail.com>.

  PR:		227191
  Reported by:	0mp
  MFC after:	2 weeks

Changes:
  head/sys/dev/acpica/acpi.c