. . w f On Tue, Jan 8, 2019 at 6:17 PM Rafael J. Wysocki
<rafael@xxxxxxxxxx> wrote:
>
> 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.
Well, it looks like commit 5a8361f7ecce is the first bad one at least
in the first bug listed by your patch, so indeed the case that we
assume to be failing here appears to be the failing one on that
system.
So the problem appears to be that before commit 5a8361f7ecce on some
platforms acpi_load_tables() was actually called before
acpi_ec_ecdt_probe() despite what the comment next to the latter says.
So, on those platforms running acpi_ec_ecdt_probe() after
acpi_load_tables() is fine, but question is if it also is fine on the
other platforms.
Before commit 5a8361f7ecce acpi_gbl_group_module_level_code and
acpi_gbl_parse_table_as_term_list were used for deciding whether or
not to call acpi_load_tables() from acpi_early_init(). Then,
acpi_gbl_group_module_level_code was always "true" and
acpi_gbl_parse_table_as_term_list was "false" on all platforms except
for the ones included in acpi_quirks_dmi_table[]. Hence,
acpi_load_tables() was actually loaded from acpi_early_init(), i.e.
before acpi_ec_ecdt_probe(), on all platforms except the ones from
acpi_quirks_dmi_table[], so it should be safe to move
acpi_ec_ecdt_probe() after acpi_load_tables() on any platform that was
not in acpi_quirks_dmi_table[] before commit 5a8361f7ecce. If it
turns out to not work on the platforms that were in
acpi_quirks_dmi_table[] before commit 5a8361f7ecce, we can address
that by adding the DMI quirks table back.
