Bug 218945

Summary: The fix in 218789 breaks ac and battery readings for asus laptops (gu605)
Product: ACPI Reporter: VitaliiT (vitaly.torshyn)
Component: ECAssignee: Rafael J. Wysocki (rjw)
Status: CLOSED CODE_FIX    
Severity: blocking CC: rjw, W_Armin
Priority: P3    
Hardware: Intel   
OS: Linux   
Kernel Version: 6.10-rc master Subsystem:
Regression: Yes Bisected commit-id: 60fa6ae6e6d09e377fce6f8d9b6f6a4d88769f63
Attachments: Workaround for asus gu605
Asus gu605 (BIOS 319) acpidump
ACPI: EC: Evaluate _REG in EC device scope explicitly
ACPI: EC: Evaluate orphan _REG under EC device

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/