Bug 218945 - The fix in 218789 breaks ac and battery readings for asus laptops (gu605)
Summary: The fix in 218789 breaks ac and battery readings for asus laptops (gu605)
Status: CLOSED CODE_FIX
Alias: None
Product: ACPI
Classification: Unclassified
Component: EC (show other bugs)
Hardware: Intel Linux
: P3 blocking
Assignee: Rafael J. Wysocki
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2024-06-07 12:43 UTC by VitaliiT
Modified: 2024-07-12 13:27 UTC (History)
2 users (show)

See Also:
Kernel Version: 6.10-rc master
Subsystem:
Regression: Yes
Bisected commit-id: 60fa6ae6e6d09e377fce6f8d9b6f6a4d88769f63


Attachments
Workaround for asus gu605 (1.14 KB, patch)
2024-06-07 12:43 UTC, VitaliiT
Details | Diff
Asus gu605 (BIOS 319) acpidump (3.04 MB, application/octet-stream)
2024-06-11 18:46 UTC, VitaliiT
Details
ACPI: EC: Evaluate _REG in EC device scope explicitly (583 bytes, patch)
2024-06-11 20:35 UTC, Rafael J. Wysocki
Details | Diff
ACPI: EC: Evaluate orphan _REG under EC device (4.50 KB, patch)
2024-06-12 11:08 UTC, Rafael J. Wysocki
Details | Diff

Description VitaliiT 2024-06-07 12:43:14 UTC
Created attachment 306438 [details]
Workaround for asus gu605

Creating this ticket to highlight a regression introduced with a fix proposed in https://bugzilla.kernel.org/show_bug.cgi?id=218789

Actually that patch breaks ac and battery modules. 
Sysfs readings for gu605 reported: 
- that AC is always online independently if power source connected
- battery readings are wrong: capacity always 100, voltage is always 5000, current is always 1000000; charge_full and charge_full_design is always 5000. All values are not correct. 

I've updated defect 218789 with a workaround patch that makes ac and battery modules working again with 6.10 source tree. 

Unfortunately, I can not provide correct solution since I am not acpi expert, but I feel that Lenovo laptop might need quirks or separate logic for EC initialization flow.

Additionally, not sure if it is correct to use ACPI_ROOT_OBJECT as acpi_handle when actual acpi_ec requested for initialization. An ACPI expert might need to look into this. 

Here are some results of investigation: 

In 218789 due to reported error proposed to use ACPI_ROOT_OBJECT which supposed to initalize EC's, but on asus laptops this change breaks backward compatibility.

So ec_install_handlers is called from acpi_ec_setup and first_ec will be set to ec in the beginning, so in ec_install_handlers() ACPI_ROOT_OBJECT will be used as handler. 
 static int ec_install_handlers(struct acpi_ec *ec, struct acpi_device *device,
                               bool call_reg)
 {
       acpi_handle scope_handle = ec == first_ec ? ACPI_ROOT_OBJECT : ec->handle;

And this method is called from acpi_ec_setup where first_ec will be set to ec: 

static int acpi_ec_setup(struct acpi_ec *ec, struct acpi_device *device, bool call_reg)
{
	int ret;

	/* First EC capable of handling transactions */
	if (!first_ec)
		first_ec = ec;

I understand that original defect should be fixed, but preserving things which used to work should be also priority. 

Please let me know if additional information is needed. 

The fix which just works (probably it is not "proper" fix, but that patch makes asus battery and ac modules working as supposed to).
Comment 1 Rafael J. Wysocki 2024-06-11 12:34:38 UTC
First off, it is correct to use ACPI_ROOT_OBJECT to install the EC address space handler (and this works on the majority of systems).

Second, if the EC address space handler is installed at ACPI_ROOT_OBJECT, it should cover all of the EC opregions throughout the ACPI namespace including the ones below ec->handle, so something strange is going on here.

Can you please attach the output of "dmesg | grep -i ACPI" after a fresh boot from both the failing and working cases?
Comment 2 Armin Wolf 2024-06-11 12:50:39 UTC
Could you also share the output of "acpidump"?
Comment 3 VitaliiT 2024-06-11 18:46:02 UTC
Created attachment 306451 [details]
Asus gu605 (BIOS 319) acpidump

Attached acpidump for asus gu605mv/bios 319 (most recent).
Comment 4 Armin Wolf 2024-06-11 19:02:31 UTC
It seems that "ECOK" remains zero, causing the AML code to abort all attempts to access the EC and return bogus data instead.

It seem to me that we somehow forget to call the _REG method underneath the EC0 device.
Comment 5 Armin Wolf 2024-06-11 19:04:49 UTC
Ops, seems that we call acpi_execute_reg_methods() on ACPI_ROOT_OBJECT, seems that we should call it with the handle of the first ec instead.
Comment 6 Armin Wolf 2024-06-11 19:16:31 UTC
Part 6.5.4 of the ACPI specification seems to suggest that we call _REG in the scope of the EC device.
Comment 7 Armin Wolf 2024-06-11 19:20:17 UTC
But it also seems that the original device with the strange UCSI implementation also wants _REG to be executed under \_REG.
Comment 8 Rafael J. Wysocki 2024-06-11 20:35:17 UTC
Created attachment 306452 [details]
ACPI: EC: Evaluate _REG in EC device scope explicitly

We can do both in principle, so what about the attached patch?
Comment 9 VitaliiT 2024-06-11 21:11:45 UTC
Rafael, are you requesting to test that patch and you do believe it is a fix for regression?
Comment 10 Andy Shevchenko 2024-06-11 21:43:50 UTC
(In reply to VitaliiT from comment #9)
> Rafael, are you requesting to test that patch and you do believe it is a fix
> for regression?

Does it fix your issue? I believe it's a PoC (or even ready-to-submit) change that needs to be tested.
Comment 11 Rafael J. Wysocki 2024-06-12 10:32:13 UTC
(In reply to VitaliiT from comment #9)
> Rafael, are you requesting to test that patch and you do believe it is a fix
> for regression?

Please test it and we'll see if it helps.
Comment 12 Rafael J. Wysocki 2024-06-12 11:08:56 UTC
Created attachment 306454 [details]
ACPI: EC: Evaluate orphan _REG under EC device

After starting to install the EC address space handler at the ACPI namespace root, if there is an "orphan" _REG method in the EC device's scope, it will not be evaluated any more.

Lo and behold, there is "orphan" _REG in the EC0 scope in this DSDT and it sets ECOK, so this patch should help.

Please test (if this works, you don't need to test the previous one).
Comment 13 VitaliiT 2024-06-12 13:09:48 UTC
Rafael, confirmed that the fix with  check if (scope_handle != ec->handle) and execution of acpi_execute_orphan_reg_method() works. The AC and BAT1 sysfs values are correct with your patch https://bugzilla.kernel.org/attachment.cgi?id=306454&action=diff 

Please let me know if you need additional information (e.g. dmesg or acpidump).
Comment 14 VitaliiT 2024-06-12 13:11:00 UTC
Also, can you please confirm that the fix will be merged into 6.10?
Comment 15 Rafael J. Wysocki 2024-06-12 13:56:18 UTC
I am going to submit it for 6.10 inclusion and chances are that it will end up in 6.10.
Comment 16 Rafael J. Wysocki 2024-06-13 09:29:55 UTC
Submitted:

https://lore.kernel.org/linux-acpi/12466682.O9o76ZdvQC@kreacher/

Note You need to log in before you can comment on or make changes to this bug.