On Thu, Aug 8, 2024 at 7:22 PM Rafael J. Wysocki <rafael@xxxxxxxxxx> wrote: > > Hi, > > On Mon, Aug 5, 2024 at 2:47 PM Hans de Goede <hdegoede@xxxxxxxxxx> wrote: > > > > Hi, > > > > On 8/5/24 1:28 PM, Hans de Goede wrote: > > > Hi, > > > > > > On 8/1/24 4:28 PM, Hans de Goede wrote: > > >> Hi, > > >> > > >> On 7/29/24 1:15 PM, Hans de Goede wrote: > > >>> Hi Rafael, > > >>> > > >>> There are 2 bug reports: > > >>> > > >>> 1. Brightness up/down key-presses no longer working on LG laptop (acpi-video related): > > >>> https://lists.fedoraproject.org/archives/list/devel@xxxxxxxxxxxxxxxxxxxxxxx/thread/V2KWAGZIAX4TOWPCH6A6FSIT66PR3KMZ/ > > >> > > >> I have filed: > > >> > > >> https://bugzilla.redhat.com/show_bug.cgi?id=2302253 > > >> > > >> to track this now and an acpidump of the troublesome LG laptop > > >> is attached there. I have also requested dmesg output of > > >> a non working kernel to be attached there. > > >> > > >> As a reminder this is the bug where it has been confirmed that > > >> reverting "EC: Install address space handler at the namespace root" > > >> helps, with the caveat that there is a Thunderbolt related IRQ > > >> storm on the ACPI event IRQ after the revert ... > > > > > > Ok, so the bugzilla now has 2 different dmesg outputs: > > > > > > 1. 6.9.6, this kernel works without problems > > > > > > 2. 6.9.12 with the following patch you suggested on top: > > > > > > --- a/drivers/acpi/ec.c > > > +++ b/drivers/acpi/ec.c > > > @@ -1788,7 +1788,7 @@ void __init acpi_ec_dsdt_probe(void) > > > * At this point, the GPE is not fully initialized, so do not to > > > * handle the events. > > > */ > > > - ret = acpi_ec_setup(ec, NULL, true); > > > + ret = acpi_ec_setup(ec, NULL, false); > > > if (ret) { > > > acpi_ec_free(ec); > > > return; > > > > > > Unfortunately this does not help. dmesg shows some EC _REG errors, which > > > are now (with the above diff applied) shown just before the > > > "Boot DSDT EC initialization complete" message, which shows that _REG now > > > runs from acpi_ec_add() rather then before: > > > > > > [ 1.007566] ACPI BIOS Error (bug): Could not resolve symbol [\_TZ.FN00._OFF], AE_NOT_FOUND (20230628/psargs-330) > > > [ 1.007576] ACPI Error: Aborting method \_SB.PC00.LPCB.H_EC.EREG due to previous error (AE_NOT_FOUND) (20230628/psparse-52 > > > [ 1.007580] ACPI Error: Aborting method \_SB.PC00.LPCB.H_EC._REG due to previous error (AE_NOT_FOUND) (20230628/psparse-52 > > > [ 1.007639] ACPI: EC: interrupt unblocked > > > [ 1.007640] ACPI: EC: event unblocked > > > [ 1.007675] ACPI: EC: EC_CMD/EC_SC=0x66, EC_DATA=0x62 > > > [ 1.007676] ACPI: EC: GPE=0x6e > > > [ 1.007677] ACPI: \_SB_.PC00.LPCB.LGEC: Boot DSDT EC initialization complete > > > [ 1.007679] ACPI: \_SB_.PC00.LPCB.LGEC: EC: Used to handle transactions and events > > > > > > Note that the errors are from calling _REG on \_SB.PC00.LPCB.H_EC, where as the actual > > > EC (and the only acpi_device on which _REG would get called for the EC Opregion before) is: > > > \_SB_.PC00.LPCB.LGEC. > > > > > > Looking at the DSDT it seems that the H_EC is not used and is leftover from a copy/paste > > > from some reference design DSDT. Its _REG however does write to the EC before hitting the error > > > and I think that that write may be causing the issue... > > > > > > The H_EC device does have an _STA method and looking closer the troublesome EREG method is > > > also called from _INI. So I guess that _STA is returning 0 causing _INI to not run and > > > that is the reason why we are not seeing the same EREG errors with kernel 6.9.6 where _REG is > > > only called for the EC opregion on \_SB_.PC00.LPCB.LGEC and not for the entire ACPI device > > > hierarchy as is done with >= 6.9.7 . > > > > > > Maybe we should only call _REG for the EC opregion on present devices (and devices without > > > a _STA)? > > > > > > Also note that both LGEC and H_EC use the same cmd + data ports. > > > > > > I'll go and ask the reporter to retrieve the status of both LGEC and H_EC and then see > > > from there. > > > > The reporter has confirmed that of the 2 EC devices ( H_EC / LGEC ) only LGEC returns > > a _STA of non 0: > > > > > Here it is, with kernel 6.9.6: > > > > > > $ cat /sys/bus/acpi/devices/PNP0C09\:00/path > > > \_SB_.PC00.LPCB.H_EC > > > $ cat /sys/bus/acpi/devices/PNP0C09\:00/status > > > 0 > > > $ cat /sys/bus/acpi/devices/PNP0C09\:01/path > > > \_SB_.PC00.LPCB.LGEC > > > $ cat /sys/bus/acpi/devices/PNP0C09\:01/status > > > 15 > > > > And taking a second look at the other bug: > > https://bugzilla.redhat.com/show_bug.cgi?id=2298938 > > > > That one also has 2 EC ACPI devices and the errors come from calling _REG on the one > > which is not picked as the boot_ec : > > > > jul 19 17:33:41 kernel: ACPI: EC: EC started > > jul 19 17:33:41 kernel: ACPI: EC: interrupt blocked > > jul 19 17:33:41 kernel: ACPI Error: Aborting method \_SB.PCI0.LPCB.H_EC.ECMD due to previous error (AE_AML_LOOP_TIMEOUT) (20230628/psparse-529) > > jul 19 17:33:41 kernel: ACPI Error: Aborting method \_TZ.FNCL due to previous error (AE_AML_LOOP_TIMEOUT) (20230628/psparse-529) > > jul 19 17:33:41 kernel: ACPI Error: Aborting method \_TZ.FN00._OFF due to previous error (AE_AML_LOOP_TIMEOUT) (20230628/psparse-529) > > jul 19 17:33:41 kernel: ACPI Error: Aborting method \_SB.PCI0.LPCB.H_EC._REG due to previous error (AE_AML_LOOP_TIMEOUT) (20230628/psparse-529) > > jul 19 17:33:41 kernel: ACPI: EC: EC_CMD/EC_SC=0x66, EC_DATA=0x62 > > jul 19 17:33:41 kernel: ACPI: \_SB_.PCI0.LPCB.EC0_: Boot DSDT EC used to handle transactions > > > > Note the H_EC vs EC0_ in the errors vs the "Boot DSDT EC used to > > handle transactions" message. > > > > So the issue in both cases seems to be calling _REG on an unused EC acpi_device. > > Not sure how to best fix this though ... > > I have created an experimental acpi-ec-fixes branch: > > https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git/log/?h=acpi-ec-fixes > > for this which illustrates my idea (untested so far). This has just been updated to actually register the EC _REG for all devices (which would have been skipped for the majority of them due to the previous location of the relevant function call). > The underlying observation is that _REG only needs to be evaluated for > EC operation regions located in the scopes of ACPI device objects > representing valid devices, so it is better to do it for each of these > objects individually in acpi_bus_attach(). > > For the EC itself, it is better to do what was done before the > $subject commit, so evaluate _REG for the EC operation regions in the > EC scope (including the "orphan" _REG). > > Accordingly, commit 0e6b6dedf168 ("Revert "ACPI: EC: Evaluate orphan > _REG under EC device") is reverted, acpi_execute_reg_methods() is > modified to take an additional depth argument and it is called for > each device object representing a valid device with that argument > equal to 1.
