On 一, 2019-01-07 at 14:45 +0100, Rafael J. Wysocki wrote:
> On Monday, January 7, 2019 1:43:29 PM CET Rafael J. Wysocki wrote:
> >
> > On Mon, Jan 7, 2019 at 12:51 PM Rafael J. Wysocki <rafael@xxxxxxxxx
> > g> wrote:
> > >
> > >
> > > ca On Sat, Jan 5, 2019 at 2:17 PM Zhang Rui <rui.zhang@intel.c
> > > om> wrote:
> > > >
> > > >
> > > > From 6f2fac0ffcd7fd61036baa0798ab171496cff50f Mon Sep 17
> > > > 00:00:00 2001
> > > > From: Zhang Rui <rui.zhang@xxxxxxxxx>
> > > > Date: Fri, 4 Jan 2019 14:35:52 +0800
> > > > Subject: [PATCH] ACPI/ACPICA: Run EC _REG explicitly for ECDT
> > > >
> > > > commit d737f333b211 ("ACPI: probe ECDT before loading AML
> > > > tables
> > > > regardless of module-level code flag") probes ECDT before
> > > > loading
> > > > the AML tables.
> > > >
> > > > This is the right thing to do according to the ACPI Spec, but
> > > > unfortunately, it breaks the current kernel EC/ECDT support,
> > > > and makes
> > > > many devices, including battery, lid, etc, fails to work on a
> > > > variety
> > > > of platforms.
> > > >
> > > > This is because:
> > > > 1. Probing ECDT requires installing EC address space handler
> > > > 2. EC _REG can not be evaluated at the time when probing ECDT
> > > > because AML
> > > > tables have not been loaded yet.
> > > > 3. Many devices fail to work because EC _REG is not evaluated.
> > > >
> > > > To fix this, a solution is proposed in this patch to evaluate
> > > > EC _REG
> > > > explicitly in ACPICA, if ECDT has been probed.
> > > It would be good to give some more details here as the patch
> > > itself
> > > appears to be rather convoluted.
> > >
> > > Also, the description above doesn't actually explain why the
> > > problem
> > > is there, as it doesn't explain why probing the ECDT early causes
> > > the
> > > EC _REG to be not evaluated.
> > >
> > > It looks like the failure is due to the change of the ordering
> > > between
> > > acpi_load_tables() and acpi_ec_ecdt_probe() in the
> > > acpi_gbl_group_module_level_code case which causes the EC to be
> > > probed
> > > before instantiating the namespace and _REG obviously cannot be
> > > evaluated then.
> > So what happens is:
> >
> > acpi_ec_ecdt_probe()
> > acpi_ec_ecdt_probe()
> > acpi_config_boot_ec(ec, ACPI_ROOT_OBJECT, false, true)
> > ...
> > ec->handle = ACPI_ROOT_OBJECT;
> > ...
> > acpi_ec_setup(ec, false)
> > ec_install_handlers(ec, false)
> >
> > acpi_install_address_space_handler(ACPI_ROOT_OBJECT,
> > ACPI_ADR_SPACE_EC, &acpi_ec_space_handler, NULL, ec)
> > ...
> > set_bit(EC_FLAGS_EC_HANDLER_INSTALLED, &ec-
> > >flags)
> >
> > and now it returns, because handle_events is "false".
> >
> > Next time acpi_ec_setup() is called from acpi_ec_add() that can be
> > invoked in two ways, either through
> > acpi_bus_register_driver(&acpi_ec_driver) which runs it for the
> > DSDT
> > EC (if found), or through acpi_ec_ecdt_start() and
> > acpi_bus_register_early_device().
> >
> > There are two cases in there, the acpi_config_boot_ec() one and the
> > direct invocation of acpi_ec_setup() when acpi_is_boot_ec(ec)
> > returns
> > "false". The former is the failing case AFAICS, because
> > acpi_ec_ecdt_probe() (if successful) has set boot_ec as well as
> > ec->command_addr and ec->data_addr.
> >
> > So acpi_config_boot_ec() runs again and since boot_ec->handle has
> > not
> > been updated, it doesn't call ec_remove_handlers(), and because the
> > EC_FLAGS_EC_HANDLER_INSTALLED is set, the address space handler
> > installed previously is retained and _REG is not evaluated. Since
> > it
> > wasn't evaluated before (as it was not present then), it is never
> > evaluated and the failure occurs.
> >
> > So it looks like clearing ec-> handle before invoking
> > acpi_config_boot_ec() in the is_ecdt case in acpi_ec_add() can
> > help,
> > can't it?
> Well, that wouldn't work, because ec->handle is checked by
> acpi_config_boot_ec()
> too. What might work would be to clear it and then pass the original
> to
> acpi_config_boot_ec() as 'handle'. Anyway, the patch below should
> cover
> all of the different cases just fine, so can you try this one,
> please?
>
I was thinking of fixing it in EC driver in the very beginning, and I
have a debug patch similar to the patch below
https://bugzilla.kernel.org/show_bug.cgi?id=200011#c71
and it indeed fixes the problem.
But then _REG is evaluated in the driver probe time, which I think it
might be too late, and the best time to evaluate _REG is
1. after ACPI namespace ready (_REG available)
2. before any _STA being evaluated in ACPICA core.
acpi_early_init()
acpi_ecdt_probe()
acpi_bus_init()
acpi_load_tables()
...
acpi_initialize_objects()
acpi_ns_initialize_devices()
...
acpi_ev_initialize_op_regions()
...
acpi_ns_init_one_device()
acpi_ut_execute_STA()
That's why I prefer to fix in it acpi_ev_initialize_op_regions().
To avoid _REG being evaluated twice, we can fix EC driver to
1. install EC address space handler for ECDT and never remove it
2. install EC address space handler for DSDT EC only if a) ECDT does
not exists, or b) ECDT handle does not equal DSDT handle.
what do you think?
thanks,
rui
> ---
> drivers/acpi/ec.c | 11 ++++++-----
> 1 file changed, 6 insertions(+), 5 deletions(-)
>
> Index: linux-pm/drivers/acpi/ec.c
> ===================================================================
> --- linux-pm.orig/drivers/acpi/ec.c
> +++ linux-pm/drivers/acpi/ec.c
> @@ -1545,12 +1545,13 @@ static int acpi_config_boot_ec(struct ac
> int ret;
>
> /*
> - * Changing the ACPI handle results in a re-configuration of
> the
> - * boot EC. And if it happens after the namespace
> initialization,
> - * it causes _REG evaluations.
> + * It is necessary to evaluate the _REG method for the EC
> address space
> + * handler, but it might not be evaluated when installing
> that handler
> + * previously (as that might happen when the namespace was
> not present
> + * yet), so remove the handler at this point to force the
> evaluation of
> + * _REG when it is installed again by acpi_ec_setup() below.
> */
> - if (boot_ec && boot_ec->handle != handle)
> - ec_remove_handlers(boot_ec);
> + ec_remove_handlers(ec);
>
> /* Unset old boot EC */
> if (boot_ec != ec)
>
