s On Tue, Jan 8, 2019 at 2:49 PM Zhang Rui <rui.zhang@xxxxxxxxx> wrote:
>
> 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
That unless we are going to switch over to the DSDT EC later.
> 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.
Right: use the DSDT EC as a "boot EC" only if the ECDT EC is not there.
> what do you think?
Something's fishy. :-)
AFAICS acpi_gbl_group_module_level_code has been "false" since commit
5a8361f7ecce (ACPICA: Integrate package handling with module-level
code) which was way before commit d737f333b211. This means that the
case that we regard as failing, i.e. acpi_load_tables() called from
acpi_early_init(), which was before acpi_bus_init() that called
acpi_ec_ecdt_probe(), in fact was not there at all since commit
5a8361f7ecce. Thus the code removed by commit d737f333b211 from
acpi_early_init() was actually dead already at that time and
acpi_ec_ecdt_probe() was always called before acpi_load_tables() even
before commit d737f333b211 and that commit couldn't break it.
Hence, the only other thing done by commit d737f333b211, which was to
move the acpi_ec_ecdt_probe() to acpi_early_init() was wrong and so
reverting that part of commit d737f333b211 should fix the problem.
