Hi, Peter > From: Peter Wu [mailto:peter@xxxxxxxxxxxxx] > Subject: Re: [PATCH 3/4] ACPI / EC: Fix a gap that ECDT EC cannot handle EC events > > 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@xxxxxxxxxxxxxxxxx> > > Suggested-by: Peter Wu <peter@xxxxxxxxxxxxx> > > Cc: Luya Tshimbalanga <luya@xxxxxxxxxxxxxxxxx> > > Cc: Peter Wu <peter@xxxxxxxxxxxxx> > > Signed-off-by: Lv Zheng <lv.zheng@xxxxxxxxx> > > --- > > 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? This is a hidden logic in old code. It's corrected in the upstream linux-pm.git. When this patch is re-sent, the code here could probably be replaced by acpi_ec_enable_event()/acpi_ec_disable_event(). > > > } > > } > > > > @@ -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. Let me clarify: acpi_ec_ecdt_probe(): enable EC transactions so that EC operation region can be accessed during the table loading. acpi_load_tables(): the order of acpi_ec_ecdt_probe()/acpi_load_tables() will be corrected in newer ACPICA releases. acpi_ec_dsdt_probe(): enable EC transactions so that EC operation region can be accessed in _INI/_STA evaluations. acpi_initialize_objects(): the order of acpi_ec_dsdt_probe()/acpi_initialize_objects() will be corrected in newer ACPICA releases. acpi_ec_add(): takes care of multi-EC devices and boot EC/first EC support, driving EC devices, may enable _Qxx handling. acpi_update_all_gpes()/acpi_ec_ecdt_start(): ensure to enable all _Qxx/_Lxx/_Exx handling at this point. > > Reviewed-by: Peter Wu <peter@xxxxxxxxxxxxx> Thanks for the review. Best regards Lv > > > > > acpi_scan_initialized = true; > > > > -- > > 1.7.10 > > -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
