Hi, Peter > From: Peter Wu [mailto:peter@xxxxxxxxxxxxx] > Subject: Re: [PATCH 1/4] ACPI / EC: Cleanup first_ec/boot_ec code > > On Mon, Sep 05, 2016 at 05:57:47AM +0000, Zheng, Lv wrote: > > Hi, Peter > > > > > From: Peter Wu [mailto:peter@xxxxxxxxxxxxx] > > > Subject: Re: [PATCH 1/4] ACPI / EC: Cleanup first_ec/boot_ec code > > > > > > On Fri, Sep 02, 2016 at 03:46:31PM +0800, Lv Zheng wrote: > > > > In order to support full ECDT (driving the ECDT EC after probing the > > > > namespace EC), we need to change our EC device alloc/free algorithm, ensure > > > > not to free old boot EC before qualifying new boot EC. > > > > This patch achieves this by cleaning up first_ec/boot_ec logic: > > > > 1. first_ec: used to perform transactions, so it is assigned in new > > > > acpi_ec_setup() function. > > > > 2. boot_ec: used to track early EC device, so it is assigned in new > > > > acpi_config_boot_ec() function which explictly tells the driver to save > > > > the EC device as early EC device. > > > > > > > > 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: Peter Wu <peter@xxxxxxxxxxxxx> > > > > Cc: Luya Tshimbalanga <luya@xxxxxxxxxxxxxxxxx> > > > > Signed-off-by: Lv Zheng <lv.zheng@xxxxxxxxx> > > > > > > Good idea, the previous acpi_ec_alloc function contained an implicit > > > assumption which was slightly harder to follow. See below for one > > > concern. > > > > > > > --- > > > > drivers/acpi/ec.c | 96 +++++++++++++++++++++++++++++++++++------------------ > > > > 1 file changed, 63 insertions(+), 33 deletions(-) > > > > > > > > diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c > > > > index e7bd57c..4a5f3ab 100644 > > > > --- a/drivers/acpi/ec.c > > > > +++ b/drivers/acpi/ec.c > > > > @@ -179,6 +179,7 @@ static void acpi_ec_event_processor(struct work_struct *work); > > > > > > > > struct acpi_ec *boot_ec, *first_ec; > > > > EXPORT_SYMBOL(first_ec); > > > > +static bool boot_ec_is_ecdt = false; > > > > static struct workqueue_struct *ec_query_wq; > > > > > > > > static int EC_FLAGS_CLEAR_ON_RESUME; /* Needs acpi_ec_clear() on boot/resume */ > > > > @@ -1228,7 +1229,16 @@ acpi_ec_space_handler(u32 function, acpi_physical_address address, > > > > static acpi_status > > > > ec_parse_io_ports(struct acpi_resource *resource, void *context); > > > > > > > > -static struct acpi_ec *make_acpi_ec(void) > > > > +static void acpi_ec_free(struct acpi_ec *ec) > > > > +{ > > > > + if (first_ec == ec) > > > > + first_ec = NULL; > > > > + if (boot_ec == ec) > > > > + boot_ec = NULL; > > > > + kfree(ec); > > > > +} > > > > + > > > > +static struct acpi_ec *acpi_ec_alloc(void) > > > > { > > > > struct acpi_ec *ec = kzalloc(sizeof(struct acpi_ec), GFP_KERNEL); > > > > > > > > @@ -1290,6 +1300,11 @@ ec_parse_device(acpi_handle handle, u32 Level, void *context, void > **retval) > > > > return AE_CTRL_TERMINATE; > > > > } > > > > > > > > +/* > > > > + * Note: This function returns an error code only when the address space > > > > + * handler is not installed, which means "not able to handle > > > > + * transactions". > > > > + */ > > > > static int ec_install_handlers(struct acpi_ec *ec) > > > > { > > > > acpi_status status; > > > > @@ -1365,21 +1380,49 @@ static void ec_remove_handlers(struct acpi_ec *ec) > > > > } > > > > } > > > > > > > > -static struct acpi_ec *acpi_ec_alloc(void) > > > > +static int acpi_ec_setup(struct acpi_ec *ec) > > > > { > > > > - struct acpi_ec *ec; > > > > + int ret; > > > > > > > > - /* Check for boot EC */ > > > > - if (boot_ec) { > > > > - ec = boot_ec; > > > > - boot_ec = NULL; > > > > - ec_remove_handlers(ec); > > > > - if (first_ec == ec) > > > > - first_ec = NULL; > > > > - } else { > > > > - ec = make_acpi_ec(); > > > > + ret = ec_install_handlers(ec); > > > > + if (ret) > > > > + return ret; > > > > + > > > > + /* First EC capable of handling transactions */ > > > > + if (!first_ec) { > > > > + first_ec = ec; > > > > + acpi_handle_info(first_ec->handle, "Used as first EC\n"); > > > > } > > > > - return ec; > > > > + > > > > + acpi_handle_info(ec->handle, > > > > + "GPE=0x%lx, EC_CMD/EC_SC=0x%lx, EC_DATA=0x%lx\n", > > > > + ec->gpe, ec->command_addr, ec->data_addr); > > > > + return ret; > > > > +} > > > > + > > > > +static int acpi_config_boot_ec(struct acpi_ec *ec, bool is_ecdt) > > > > +{ > > > > + int ret; > > > > + > > > > + if (boot_ec) > > > > + ec_remove_handlers(boot_ec); > > > > + > > > > + /* Unset old boot EC */ > > > > + if (boot_ec != ec) > > > > + acpi_ec_free(boot_ec); > > > > + > > > > + ret = acpi_ec_setup(ec); > > > > + if (ret) > > > > + return ret; > > > > + > > > > + /* Set new boot EC */ > > > > + 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"); > > > > + } > > > > + return ret; > > > > } > > > > > > > > static int acpi_ec_add(struct acpi_device *device) > > > > @@ -1395,7 +1438,7 @@ static int acpi_ec_add(struct acpi_device *device) > > > > return -ENOMEM; > > > > if (ec_parse_device(device->handle, 0, ec, NULL) != > > > > AE_CTRL_TERMINATE) { > > > > - kfree(ec); > > > > + acpi_ec_free(ec); > > > > return -EINVAL; > > > > } > > > > > > > > @@ -1403,8 +1446,6 @@ static int acpi_ec_add(struct acpi_device *device) > > > > acpi_walk_namespace(ACPI_TYPE_METHOD, ec->handle, 1, > > > > acpi_ec_register_query_methods, NULL, ec, NULL); > > > > > > > > - if (!first_ec) > > > > - first_ec = ec; > > > > device->driver_data = ec; > > > > > > > > ret = !!request_region(ec->data_addr, 1, "EC data"); > > > > @@ -1412,10 +1453,7 @@ static int acpi_ec_add(struct acpi_device *device) > > > > ret = !!request_region(ec->command_addr, 1, "EC cmd"); > > > > WARN(!ret, "Could not request EC cmd io port 0x%lx", ec->command_addr); > > > > > > > > - pr_info("GPE = 0x%lx, I/O: command/status = 0x%lx, data = 0x%lx\n", > > > > - ec->gpe, ec->command_addr, ec->data_addr); > > > > - > > > > - ret = ec_install_handlers(ec); > > > > + ret = acpi_config_boot_ec(ec, false); > > > > > > > > /* Reprobe devices depending on the EC */ > > > > acpi_walk_dep_device_list(ec->handle); > > > > @@ -1442,9 +1480,7 @@ static int acpi_ec_remove(struct acpi_device *device) > > > > release_region(ec->data_addr, 1); > > > > release_region(ec->command_addr, 1); > > > > device->driver_data = NULL; > > > > - if (ec == first_ec) > > > > - first_ec = NULL; > > > > - kfree(ec); > > > > + acpi_ec_free(ec); > > > > return 0; > > > > } > > > > > > > > @@ -1496,13 +1532,10 @@ int __init acpi_ec_dsdt_probe(void) > > > > ret = -ENODEV; > > > > goto error; > > > > } > > > > - ret = ec_install_handlers(ec); > > > > - > > > > + ret = acpi_config_boot_ec(ec, false); > > > > error: > > > > if (ret) > > > > - kfree(ec); > > > > - else > > > > - first_ec = boot_ec = ec; > > > > + acpi_ec_free(ec); > > > > return ret; > > > > } > > > > > > > > @@ -1600,7 +1633,6 @@ int __init acpi_ec_ecdt_probe(void) > > > > goto error; > > > > } > > > > > > > > - pr_info("EC description table is found, configuring boot EC\n"); > > > > if (EC_FLAGS_CORRECT_ECDT) { > > > > ec->command_addr = ecdt_ptr->data.address; > > > > ec->data_addr = ecdt_ptr->control.address; > > > > @@ -1610,12 +1642,10 @@ int __init acpi_ec_ecdt_probe(void) > > > > } > > > > ec->gpe = ecdt_ptr->gpe; > > > > ec->handle = ACPI_ROOT_OBJECT; > > > > - ret = ec_install_handlers(ec); > > > > + ret = acpi_config_boot_ec(ec, true); > > > > > > acpi_ec_ecdt_probe is called very early, so boot_ec, etc. are NULL and > > > it will invoke acpi_ec_setup. This calls acpi_ec_start, but since no GPE > > > handler is yet registered, it will have no effect. Next it will try to > > > register an address space handler, given the root handle. Does this > > > work? > > > > > > There is no \_REG method, so I would have expected "Fail in evaluating > > > the _REG object of EC device. Broken bios is suspected", but this > > > message was not shown (perhaps it returned AE_BAD_PARAMETER or > > > something?). > > > > > > Assuming that the handler registration failed (AE_BAD_PARAMETER), then > > > acpi_config_boot_ec will have no effect. If it actually did register a > > > handler, I would expect messages like "\: Used as boot ECDT to handle > > > transactions" which is a bit strange. > > > > Your ECDT understanding is very different from mine. > > I actually think there is an "early operation region" concept in ACPI. > > The early operation region can be accessed during the table loading. > > It means: > > If (SREG == One) > > { > > Name(OBJ1, One) > > } > > Can be executed during the table loading. > > This brought a great convenience to the BIOS configurable options. > > > > So what kind of operation regions can be accessed during the table loading? > > Actually they are: > > 1. SystemPort/SystemMemory > > 2. PCI_Config brought by the PCI root bridge > > 2. any other early operation regions made available by the data tables. > > ECDT is a kind of such data table. > > There are spec words in _REG section talking about this. > > > > So if ECDT is not invented for this purpose, why it will be useful? > > If the namespace has been initialized, OSPM ACPI core should always be able to use the namespace EC. > > Like what we do in acpi_ec_dsdt_probe(). > > > > Since ECDT is used before table loading, we'll put acpi_ec_ecdt_probe() before acpi_load_tables(). > > At that time, the namespace is empty, it is not possible to use any handle other than > ACPI_ROOT_OBJECT. > > I see, thanks for the detailed explanation. Originally I was concerned > about the slightly misleading kernel log messages that would refer to > "\" as the EC handle (which is bogus), but this seems unavoidable at > this stage. > > For example, initially it would print: > > \: GPE=0x15, EC_CMD/EC_.... > > instead of something like: > > \_SB.PCI0.SBRG.EC0: GPE=0x15, EC_CMD/EC_.... Yes. This is intentional. :) It tells us from which node _REG evaluations are performed. OTOH, as long as we want to see the namespace path in the log for the EC device, it is unavoidable. Best regards Lv > > Kind regards, > Peter > > > Thanks and best regards > > Lv > > > > > > > > Other than that it looks good to me. > > > > > > (Aside, I think there is also an implicit assumption that > > > acpi_ec_submit_query never submits work during enumeration, I suspect a > > > possible use-after-free of ec->work is possible otherwise when > > > acpi_ec_free is called.) > > > > > > Peter > > > > > > > error: > > > > if (ret) > > > > - kfree(ec); > > > > - else > > > > - first_ec = boot_ec = ec; > > > > + acpi_ec_free(ec); > > > > return ret; > > > > } > > > > > > > > -- > > > > 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
