Bug 227191
| Summary: | Cannot check battery status after upgrading to 12-CURRENT after r330957 (ACPI _STA method removed) | ||||||
|---|---|---|---|---|---|---|---|
| Product: | Base System | Reporter: | Mateusz Piotrowski <0mp> | ||||
| Component: | misc | Assignee: | Andriy Gapon <avg> | ||||
| Status: | Closed FIXED | ||||||
| Severity: | Affects Many People | CC: | acpi, ali.abdallah, cem, current, doctorwhoguy, emaste, grahamperrin, jkim, markj, matias, pi, slava | ||||
| Priority: | --- | Keywords: | patch, regression | ||||
| Version: | CURRENT | Flags: | koobs:
mfc-stable12+
|
||||
| Hardware: | Any | ||||||
| OS: | Any | ||||||
| Bug Depends on: | |||||||
| Bug Blocks: | 230428 | ||||||
| Attachments: |
|
||||||
|
Description
Mateusz Piotrowski
2018-04-01 21:24:32 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.
It is not a hardware issue apparently. I've live booted Ubuntu and it detects my battery just fine. 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. 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.
(In reply to Conrad Meyer from comment #3) I'm building r330956 at the moment. I'll report back. I can confirm that `acpiconf -i 0` works as expected on r330956 while being broken on r330957. (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. Same on 12.0-CURRENT r333344. (In reply to Mateusz Piotrowski from comment #8) Can I provide any logs/recompile the system with debugging on to help debug this issue? 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.
(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. (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. (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. *** Bug 230428 has been marked as a duplicate of this bug. *** Do you think we get it committed before 12.0-RELEASE? 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 (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? 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 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. 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 A commit references this bug: Author: avg Date: Thu Dec 20 08:45:41 UTC 2018 New revision: 342278 URL: https://svnweb.freebsd.org/changeset/base/342278 Log: MFC r341632: acpi_{Device,Battery}IsPresent: restore pre-r330957 behaviour Specifically, assume that the device is present if evaluation of _STA method fails. PR: 227191 Changes: _U stable/12/ stable/12/sys/dev/acpica/acpi.c Hello, I'm having the same issue on Lenovo X1 Carbon gen1, Coreboot 4.7 TianoCore on Freebsd 12.0-RELEASE-p3. It was working since Freebsd 11.1 when I just set this up over a year ago and in 11.2. Stopped working after update to 12. [moose@beast /usr/home/moose]$ acpiconf -i 0 acpiconf: get battery info (0) failed: Device not configured [moose@beast /usr/home/moose]$ dmesg | grep -i acpi | grep -i bat ACPI Error: Method parse/execution failed \134_SB.PCI0.LPCB.EC.BAT0._STA, AE_NOT_EXIST (20181003/psparse-677) ACPI Error: Method parse/execution failed \134_SB.PCI0.LPCB.EC.BAT1._STA, AE_NOT_EXIST (20181003/psparse-677) ACPI Error: Method parse/execution failed \134_SB.PCI0.LPCB.EC.BAT0._STA, AE_NOT_EXIST (20181003/psparse-677) ACPI Error: Method parse/execution failed \134_SB.PCI0.LPCB.EC.BAT1._STA, AE_NOT_EXIST (20181003/psparse-677) [moose@beast /usr/home/moose]$ uname -a FreeBSD beast 12.0-RELEASE-p3 FreeBSD 12.0-RELEASE-p3 GENERIC amd64 Please let me know if you need any additional information and/or if I need to apply any patches manually. Thank you (In reply to Slava from comment #22) The fix has been committed and merged to the stable/12 branch, and will be part of the next (12.1) FreeBSD release cut from that branch. If you would not like to wait for that release, you may update to the stable/12 branch. For more information on tracking development branches, see: https://www.freebsd.org/doc/handbook/current-stable.html (In reply to Slava from comment #22) Alternatively, one can apply the commit that was merged to stable/12 in base r342278, and rebuild/reinstall the kernel |