From: Peter Wu <peter@lekensteyn.nl>
To: Lv Zheng <lv.zheng@intel.com>
Cc: "Rafael J. Wysocki" <rafael.j.wysocki@intel.com>,
"Rafael J. Wysocki" <rjw@rjwysocki.net>,
Len Brown <len.brown@intel.com>, Lv Zheng <zetalog@gmail.com>,
linux-acpi@vger.kernel.org,
Luya Tshimbalanga <luya@fedoraproject.org>
Subject: Re: [PATCH 3/4] ACPI / EC: Fix a gap that ECDT EC cannot handle EC events
Date: Sat, 3 Sep 2016 18:48:58 +0200 [thread overview]
Message-ID: <20160903164858.GC5252@al> (raw)
In-Reply-To: <1be8e280092735b7f2e0dd4797b8eb4f96d6cdba.1472802172.git.lv.zheng@intel.com>
On Fri, Sep 02, 2016 at 03:46:46PM +0800, Lv Zheng wrote:
> It is possible to register _Qxx from namespace and use the ECDT EC to
> perform event handling. The reported bug reveals that Windows is using ECDT
> in this way in case the namespace EC is not present. This patch facilitates
> Linux to support ECDT in this way.
>
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=115021
> Reported-and-tested-by: Luya Tshimbalanga <luya@fedoraproject.org>
> Suggested-by: Peter Wu <peter@lekensteyn.nl>
> Cc: Luya Tshimbalanga <luya@fedoraproject.org>
> Cc: Peter Wu <peter@lekensteyn.nl>
> Signed-off-by: Lv Zheng <lv.zheng@intel.com>
> ---
> drivers/acpi/ec.c | 119 ++++++++++++++++++++++++++++++++++++++---------
> drivers/acpi/internal.h | 1 +
> drivers/acpi/scan.c | 1 +
> 3 files changed, 98 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
> index 4b4c0cb..847e665 100644
> --- a/drivers/acpi/ec.c
> +++ b/drivers/acpi/ec.c
> @@ -108,6 +108,7 @@ enum {
> EC_FLAGS_QUERY_GUARDING, /* Guard for SCI_EVT check */
> EC_FLAGS_GPE_HANDLER_INSTALLED, /* GPE handler installed */
> EC_FLAGS_EC_HANDLER_INSTALLED, /* OpReg handler installed */
> + EC_FLAGS_EVT_HANDLER_INSTALLED, /* _Qxx handlers installed */
> EC_FLAGS_STARTED, /* Driver is started */
> EC_FLAGS_STOPPED, /* Driver is stopped */
> EC_FLAGS_COMMAND_STORM, /* GPE storms occurred to the
> @@ -1305,7 +1306,7 @@ ec_parse_device(acpi_handle handle, u32 Level, void *context, void **retval)
> * handler is not installed, which means "not able to handle
> * transactions".
> */
> -static int ec_install_handlers(struct acpi_ec *ec)
> +static int ec_install_handlers(struct acpi_ec *ec, bool handle_events)
> {
> acpi_status status;
>
> @@ -1334,6 +1335,16 @@ static int ec_install_handlers(struct acpi_ec *ec)
> set_bit(EC_FLAGS_EC_HANDLER_INSTALLED, &ec->flags);
> }
>
> + if (!handle_events)
> + return 0;
> +
> + if (!test_bit(EC_FLAGS_EVT_HANDLER_INSTALLED, &ec->flags)) {
> + /* Find and register all query methods */
> + acpi_walk_namespace(ACPI_TYPE_METHOD, ec->handle, 1,
> + acpi_ec_register_query_methods,
> + NULL, ec, NULL);
> + set_bit(EC_FLAGS_EVT_HANDLER_INSTALLED, &ec->flags);
> + }
> if (!test_bit(EC_FLAGS_GPE_HANDLER_INSTALLED, &ec->flags)) {
> status = acpi_install_gpe_raw_handler(NULL, ec->gpe,
> ACPI_GPE_EDGE_TRIGGERED,
> @@ -1344,6 +1355,9 @@ static int ec_install_handlers(struct acpi_ec *ec)
> if (test_bit(EC_FLAGS_STARTED, &ec->flags) &&
> ec->reference_count >= 1)
> acpi_ec_enable_gpe(ec, true);
> +
> + /* EC is fully operational, allow queries */
> + clear_bit(EC_FLAGS_QUERY_PENDING, &ec->flags);
It makes sense to allow queries only if it can receive GPEs for such
events. You do not set this flag again in acpi_ec_remove_query_handlers
because the EC is destroyed and not re-used right?
> }
> }
>
> @@ -1378,13 +1392,17 @@ static void ec_remove_handlers(struct acpi_ec *ec)
> pr_err("failed to remove gpe handler\n");
> clear_bit(EC_FLAGS_GPE_HANDLER_INSTALLED, &ec->flags);
> }
> + if (test_bit(EC_FLAGS_EVT_HANDLER_INSTALLED, &ec->flags)) {
> + acpi_ec_remove_query_handlers(ec, true, 0);
> + clear_bit(EC_FLAGS_EVT_HANDLER_INSTALLED, &ec->flags);
> + }
> }
>
> -static int acpi_ec_setup(struct acpi_ec *ec)
> +static int acpi_ec_setup(struct acpi_ec *ec, bool handle_events)
> {
> int ret;
>
> - ret = ec_install_handlers(ec);
> + ret = ec_install_handlers(ec, handle_events);
> if (ret)
> return ret;
>
> @@ -1400,18 +1418,33 @@ static int acpi_ec_setup(struct acpi_ec *ec)
> return ret;
> }
>
> -static int acpi_config_boot_ec(struct acpi_ec *ec, bool is_ecdt)
> +static int acpi_config_boot_ec(struct acpi_ec *ec, acpi_handle handle,
> + bool handle_events, bool is_ecdt)
> {
> int ret;
>
> - if (boot_ec)
> + /*
> + * 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.
> + */
> + if (boot_ec && boot_ec->handle != handle)
> ec_remove_handlers(boot_ec);
>
> /* Unset old boot EC */
> if (boot_ec != ec)
> acpi_ec_free(boot_ec);
>
> - ret = acpi_ec_setup(ec);
> + /*
> + * ECDT device creation is split into acpi_ec_ecdt_probe() and
> + * acpi_ec_ecdt_start(). This function takes care of completing the
> + * ECDT parsing logic as the handle update should be performed
> + * between the installation/uninstallation of the handlers.
> + */
> + if (ec->handle != handle)
> + ec->handle = handle;
> +
> + ret = acpi_ec_setup(ec, handle_events);
> if (ret)
> return ret;
>
> @@ -1419,9 +1452,12 @@ static int acpi_config_boot_ec(struct acpi_ec *ec, bool is_ecdt)
> if (!boot_ec) {
> boot_ec = ec;
> boot_ec_is_ecdt = is_ecdt;
> - acpi_handle_info(boot_ec->handle, "Used as boot %s EC\n",
> - is_ecdt ? "ECDT" : "DSDT");
> }
> +
> + acpi_handle_info(boot_ec->handle,
> + "Used as boot %s EC to handle transactions%s\n",
> + is_ecdt ? "ECDT" : "DSDT",
> + handle_events ? " and events" : "");
> return ret;
> }
>
> @@ -1442,11 +1478,7 @@ static int acpi_ec_add(struct acpi_device *device)
> goto error;
> }
>
> - /* Find and register all query methods */
> - acpi_walk_namespace(ACPI_TYPE_METHOD, ec->handle, 1,
> - acpi_ec_register_query_methods, NULL, ec, NULL);
> -
> - ret = acpi_config_boot_ec(ec, false);
> + ret = acpi_config_boot_ec(ec, device->handle, true, false);
> if (ret)
> goto error;
>
> @@ -1461,9 +1493,6 @@ static int acpi_ec_add(struct acpi_device *device)
> /* Reprobe devices depending on the EC */
> acpi_walk_dep_device_list(ec->handle);
>
> - /* EC is fully operational, allow queries */
> - clear_bit(EC_FLAGS_QUERY_PENDING, &ec->flags);
> -
> /* Clear stale _Q events if hardware might require that */
> if (EC_FLAGS_CLEAR_ON_RESUME)
> acpi_ec_clear(ec);
> @@ -1482,7 +1511,6 @@ static int acpi_ec_remove(struct acpi_device *device)
>
> ec = acpi_driver_data(device);
> ec_remove_handlers(ec);
> - acpi_ec_remove_query_handlers(ec, true, 0);
> release_region(ec->data_addr, 1);
> release_region(ec->command_addr, 1);
> device->driver_data = NULL;
> @@ -1528,9 +1556,8 @@ int __init acpi_ec_dsdt_probe(void)
> if (!ec)
> return -ENOMEM;
> /*
> - * Finding EC from DSDT if there is no ECDT EC available. When this
> - * function is invoked, ACPI tables have been fully loaded, we can
> - * walk namespace now.
> + * At this point, the namespace is initialized, so start to find
> + * the namespace objects.
> */
> status = acpi_get_devices(ec_device_ids[0].id,
> ec_parse_device, ec, NULL);
> @@ -1538,13 +1565,55 @@ int __init acpi_ec_dsdt_probe(void)
> ret = -ENODEV;
> goto error;
> }
> - ret = acpi_config_boot_ec(ec, false);
> + /*
> + * When the DSDT EC is available, always re-configure boot EC to
> + * have _REG evaluated. _REG can only be evaluated after the
> + * namespace initialization.
> + * At this point, the GPE is not fully initialized, so do not to
> + * handle the events.
> + */
> + ret = acpi_config_boot_ec(ec, ec->handle, false, false);
> error:
> if (ret)
> acpi_ec_free(ec);
> return ret;
> }
>
> +/*
> + * If the DSDT EC is not functioning, we still need to prepare a fully
> + * functioning ECDT EC first in order to handle the events.
> + * https://bugzilla.kernel.org/show_bug.cgi?id=115021
> + */
> +int __init acpi_ec_ecdt_start(void)
> +{
> + struct acpi_table_ecdt *ecdt_ptr;
> + acpi_status status;
> + acpi_handle handle;
> +
> + if (!boot_ec)
> + return -ENODEV;
> + /*
> + * The DSDT EC should have already been started in
> + * acpi_ec_add().
> + */
> + if (!boot_ec_is_ecdt)
> + return -ENODEV;
> +
> + status = acpi_get_table(ACPI_SIG_ECDT, 1,
> + (struct acpi_table_header **)&ecdt_ptr);
> + if (ACPI_FAILURE(status))
> + return -ENODEV;
> +
> + /*
> + * At this point, the namespace and the GPE is initialized, so
> + * start to find the namespace objects and handle the events.
> + */
> + status = acpi_get_handle(NULL, ecdt_ptr->id, &handle);
> + if (ACPI_FAILURE(status))
> + return -ENODEV;
> + return acpi_config_boot_ec(boot_ec, handle, true, true);
> +}
> +
> #if 0
> /*
> * Some EC firmware variations refuses to respond QR_EC when SCI_EVT is not
> @@ -1647,8 +1716,12 @@ int __init acpi_ec_ecdt_probe(void)
> ec->data_addr = ecdt_ptr->data.address;
> }
> ec->gpe = ecdt_ptr->gpe;
> - ec->handle = ACPI_ROOT_OBJECT;
> - ret = acpi_config_boot_ec(ec, true);
> +
> + /*
> + * At this point, the namespace is not initialized, so do not find
> + * the namespace objects, or handle the events.
> + */
> + ret = acpi_config_boot_ec(ec, ACPI_ROOT_OBJECT, false, true);
> error:
> if (ret)
> acpi_ec_free(ec);
> diff --git a/drivers/acpi/internal.h b/drivers/acpi/internal.h
> index 940218f..4727722 100644
> --- a/drivers/acpi/internal.h
> +++ b/drivers/acpi/internal.h
> @@ -185,6 +185,7 @@ typedef int (*acpi_ec_query_func) (void *data);
> int acpi_ec_init(void);
> int acpi_ec_ecdt_probe(void);
> int acpi_ec_dsdt_probe(void);
> +int acpi_ec_ecdt_start(void);
> void acpi_ec_block_transactions(void);
> void acpi_ec_unblock_transactions(void);
> void acpi_ec_unblock_transactions_early(void);
> diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
> index ad9fc84..763c0da 100644
> --- a/drivers/acpi/scan.c
> +++ b/drivers/acpi/scan.c
> @@ -2044,6 +2044,7 @@ int __init acpi_scan_init(void)
> }
>
> acpi_update_all_gpes();
> + acpi_ec_ecdt_start();
Ok, so the order of invocation via acpi_init() is:
acpi_ec_ecdt_probe() via acpi_bus_init()
acpi_ec_dsdt_probe() via acpi_bus_init()
acpi_ec_ecdt_start() via acpi_scan_init
acpi_ec_add() via acpi_ec_init()
So the fallback in this patch should work and it should not accidentally
be activated when the DSDT has a valid EC.
Reviewed-by: Peter Wu <peter@lekensteyn.nl>
>
> acpi_scan_initialized = true;
>
> --
> 1.7.10
>
next prev parent reply other threads:[~2016-09-03 16:50 UTC|newest]
Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-09-02 7:46 [PATCH 0/4] ACPI / EC: Fix ECDT and boot_ec support Lv Zheng
2016-09-02 7:46 ` [PATCH 1/4] ACPI / EC: Cleanup first_ec/boot_ec code Lv Zheng
2016-09-03 16:21 ` Peter Wu
2016-09-05 5:57 ` Zheng, Lv
2016-09-06 9:35 ` Peter Wu
2016-09-07 2:34 ` Zheng, Lv
2016-09-02 7:46 ` [PATCH 2/4] ACPI / EC: Fix a memory leakage issue in acpi_ec_add() Lv Zheng
2016-09-03 16:28 ` Peter Wu
2016-09-02 7:46 ` [PATCH 3/4] ACPI / EC: Fix a gap that ECDT EC cannot handle EC events Lv Zheng
2016-09-03 16:48 ` Peter Wu [this message]
2016-09-05 7:48 ` Zheng, Lv
2016-09-02 7:46 ` [PATCH 4/4] ACPI / EC: Fix issues related to boot_ec Lv Zheng
2016-09-07 8:49 ` [PATCH v2 0/4] ACPI / EC: Fix ECDT and boot_ec support Lv Zheng
2016-09-07 8:50 ` [PATCH v2 1/4] ACPI / EC: Cleanup first_ec/boot_ec code Lv Zheng
2016-09-07 8:50 ` [PATCH v2 2/4] ACPI / EC: Fix a memory leakage issue in acpi_ec_add() Lv Zheng
2016-09-07 8:50 ` [PATCH v2 3/4] ACPI / EC: Fix a gap that ECDT EC cannot handle EC events Lv Zheng
2016-09-07 8:50 ` [PATCH v2 4/4] ACPI / EC: Fix issues related to boot_ec Lv Zheng
2016-09-12 21:55 ` [PATCH v2 0/4] ACPI / EC: Fix ECDT and boot_ec support Rafael J. Wysocki
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20160903164858.GC5252@al \
--to=peter@lekensteyn.nl \
--cc=len.brown@intel.com \
--cc=linux-acpi@vger.kernel.org \
--cc=luya@fedoraproject.org \
--cc=lv.zheng@intel.com \
--cc=rafael.j.wysocki@intel.com \
--cc=rjw@rjwysocki.net \
--cc=zetalog@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).