[PATCH 0/4] ACPI / EC: Fix ECDT and boot_ec support

[PATCH 0/4] ACPI / EC: Fix ECDT and boot_ec support

[Lv Zheng]

Linux uses ECDT only in boot stage (before the namespace is fully
initialized), but there are cases reported that Windows allows ECDT to be
used for OSPM runtime, responding EC events by evaluating _Qxx methods
under the device node indicated in the ECDT.

This patchset changes Linux ECDT support to follow Windows behavior and
also fixes related boot_ec support.

Lv Zheng (4):
  ACPI / EC: Cleanup first_ec/boot_ec code
  ACPI / EC: Fix a memory leakage issue in acpi_ec_add()
  ACPI / EC: Fix a gap that ECDT EC cannot handle EC events
  ACPI / EC: Fix issues related to boot_ec

 drivers/acpi/ec.c       |  240 +++++++++++++++++++++++++++++++++++++----------
 drivers/acpi/internal.h |    1 +
 drivers/acpi/scan.c     |    1 +
 3 files changed, 194 insertions(+), 48 deletions(-)

-- 
1.7.10

[PATCH 1/4] ACPI / EC: Cleanup first_ec/boot_ec code

[Lv Zheng]

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@fedoraproject.org>
Suggested-by: Peter Wu <peter@lekensteyn.nl>
Cc: Peter Wu <peter@lekensteyn.nl>
Cc: Luya Tshimbalanga <luya@fedoraproject.org>
Signed-off-by: Lv Zheng <lv.zheng@intel.com>
---
 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);
 error:
 	if (ret)
-		kfree(ec);
-	else
-		first_ec = boot_ec = ec;
+		acpi_ec_free(ec);
 	return ret;
 }
 
-- 
1.7.10

Re: [PATCH 1/4] ACPI / EC: Cleanup first_ec/boot_ec code

[Peter Wu]

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@fedoraproject.org>
> Suggested-by: Peter Wu <peter@lekensteyn.nl>
> Cc: Peter Wu <peter@lekensteyn.nl>
> Cc: Luya Tshimbalanga <luya@fedoraproject.org>
> Signed-off-by: Lv Zheng <lv.zheng@intel.com>

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.

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
> 

RE: [PATCH 1/4] ACPI / EC: Cleanup first_ec/boot_ec code

[Zheng, Lv]

Hi, Peter

> From: Peter Wu [mailto:peter@lekensteyn.nl]
> 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@fedoraproject.org>
> > Suggested-by: Peter Wu <peter@lekensteyn.nl>
> > Cc: Peter Wu <peter@lekensteyn.nl>
> > Cc: Luya Tshimbalanga <luya@fedoraproject.org>
> > Signed-off-by: Lv Zheng <lv.zheng@intel.com>
> 
> 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.

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
> >

Re: [PATCH 1/4] ACPI / EC: Cleanup first_ec/boot_ec code

[Peter Wu]

On Mon, Sep 05, 2016 at 05:57:47AM +0000, Zheng, Lv wrote:
> Hi, Peter
> 
> > From: Peter Wu [mailto:peter@lekensteyn.nl]
> > 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@fedoraproject.org>
> > > Suggested-by: Peter Wu <peter@lekensteyn.nl>
> > > Cc: Peter Wu <peter@lekensteyn.nl>
> > > Cc: Luya Tshimbalanga <luya@fedoraproject.org>
> > > Signed-off-by: Lv Zheng <lv.zheng@intel.com>
> > 
> > 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_....

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
> > >

RE: [PATCH 1/4] ACPI / EC: Cleanup first_ec/boot_ec code

[Zheng, Lv]

Hi, Peter

> From: Peter Wu [mailto:peter@lekensteyn.nl]
> 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@lekensteyn.nl]
> > > 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@fedoraproject.org>
> > > > Suggested-by: Peter Wu <peter@lekensteyn.nl>
> > > > Cc: Peter Wu <peter@lekensteyn.nl>
> > > > Cc: Luya Tshimbalanga <luya@fedoraproject.org>
> > > > Signed-off-by: Lv Zheng <lv.zheng@intel.com>
> > >
> > > 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
> > > >

[PATCH 2/4] ACPI / EC: Fix a memory leakage issue in acpi_ec_add()

[Lv Zheng]

When the handler installation failed, there was no code to free the
allocated EC device. This patch fixes this memory leakage issue.

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: Peter Wu <peter@lekensteyn.nl>
Cc: Luya Tshimbalanga <luya@fedoraproject.org>
Signed-off-by: Lv Zheng <lv.zheng@intel.com>
---
 drivers/acpi/ec.c |   14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
index 4a5f3ab..4b4c0cb 100644
--- a/drivers/acpi/ec.c
+++ b/drivers/acpi/ec.c
@@ -1438,22 +1438,25 @@ static int acpi_ec_add(struct acpi_device *device)
 		return -ENOMEM;
 	if (ec_parse_device(device->handle, 0, ec, NULL) !=
 		AE_CTRL_TERMINATE) {
-			acpi_ec_free(ec);
-			return -EINVAL;
+			ret = -EINVAL;
+			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);
+	if (ret)
+		goto error;
+
 	device->driver_data = ec;
 
 	ret = !!request_region(ec->data_addr, 1, "EC data");
 	WARN(!ret, "Could not request EC data io port 0x%lx", ec->data_addr);
 	ret = !!request_region(ec->command_addr, 1, "EC cmd");
 	WARN(!ret, "Could not request EC cmd io port 0x%lx", ec->command_addr);
-
-	ret = acpi_config_boot_ec(ec, false);
+	ret = 0;
 
 	/* Reprobe devices depending on the EC */
 	acpi_walk_dep_device_list(ec->handle);
@@ -1464,6 +1467,9 @@ static int acpi_ec_add(struct acpi_device *device)
 	/* Clear stale _Q events if hardware might require that */
 	if (EC_FLAGS_CLEAR_ON_RESUME)
 		acpi_ec_clear(ec);
+error:
+	if (ret)
+		acpi_ec_free(ec);
 	return ret;
 }
 
-- 
1.7.10

Re: [PATCH 2/4] ACPI / EC: Fix a memory leakage issue in acpi_ec_add()

[Peter Wu]

On Fri, Sep 02, 2016 at 03:46:38PM +0800, Lv Zheng wrote:
> When the handler installation failed, there was no code to free the
> allocated EC device. This patch fixes this memory leakage issue.
> 
> 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: Peter Wu <peter@lekensteyn.nl>
> Cc: Luya Tshimbalanga <luya@fedoraproject.org>
> Signed-off-by: Lv Zheng <lv.zheng@intel.com>
> ---
>  drivers/acpi/ec.c |   14 ++++++++++----
>  1 file changed, 10 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
> index 4a5f3ab..4b4c0cb 100644
> --- a/drivers/acpi/ec.c
> +++ b/drivers/acpi/ec.c
> @@ -1438,22 +1438,25 @@ static int acpi_ec_add(struct acpi_device *device)
>  		return -ENOMEM;
>  	if (ec_parse_device(device->handle, 0, ec, NULL) !=
>  		AE_CTRL_TERMINATE) {
> -			acpi_ec_free(ec);
> -			return -EINVAL;
> +			ret = -EINVAL;
> +			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);

I think you should call acpi_ec_remove_query_handlers too if at this
point acpi_config_boot_ec fails.

Peter

>  
> +	ret = acpi_config_boot_ec(ec, false);
> +	if (ret)
> +		goto error;
> +
>  	device->driver_data = ec;
>  
>  	ret = !!request_region(ec->data_addr, 1, "EC data");
>  	WARN(!ret, "Could not request EC data io port 0x%lx", ec->data_addr);
>  	ret = !!request_region(ec->command_addr, 1, "EC cmd");
>  	WARN(!ret, "Could not request EC cmd io port 0x%lx", ec->command_addr);
> -
> -	ret = acpi_config_boot_ec(ec, false);
> +	ret = 0;
>  
>  	/* Reprobe devices depending on the EC */
>  	acpi_walk_dep_device_list(ec->handle);
> @@ -1464,6 +1467,9 @@ static int acpi_ec_add(struct acpi_device *device)
>  	/* Clear stale _Q events if hardware might require that */
>  	if (EC_FLAGS_CLEAR_ON_RESUME)
>  		acpi_ec_clear(ec);
> +error:
> +	if (ret)
> +		acpi_ec_free(ec);
>  	return ret;
>  }
>  
> -- 
> 1.7.10
> 

[PATCH 3/4] ACPI / EC: Fix a gap that ECDT EC cannot handle EC events

[Lv Zheng]

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);
 		}
 	}
 
@@ -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();
 
 	acpi_scan_initialized = true;
 
-- 
1.7.10

Re: [PATCH 3/4] ACPI / EC: Fix a gap that ECDT EC cannot handle EC events

[Peter Wu]

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
> 

RE: [PATCH 3/4] ACPI / EC: Fix a gap that ECDT EC cannot handle EC events

[Zheng, Lv]

Hi, Peter

> From: Peter Wu [mailto:peter@lekensteyn.nl]
> 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@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?

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@lekensteyn.nl>

Thanks for the review.

Best regards
Lv

> 
> >
> >  	acpi_scan_initialized = true;
> >
> > --
> > 1.7.10
> >

[PATCH 4/4] ACPI / EC: Fix issues related to boot_ec

[Lv Zheng]

There are issues related to the boot_ec:
1. If acpi_ec_remove() is invoked, boot_ec will also be freed, this is not
   expected as the boot_ec could be enumerated via ECDT.
2. Address space handler installation/unstallation lead to unexpected _REG
   evaluations.
This patch adds acpi_is_boot_ec() check to be used to fix the above issues.
However, since acpi_ec_remove() actually won't be invoked, this patch
doesn't handle the reference counting of "struct acpi_ec", it only ensures
the correctness of the boot_ec destruction during the boot.

Link: https://bugzilla.kernel.org/show_bug.cgi?id=153511
Reported-by: <jw.hendy@gmail.com>
Cc: <jw.hendy@gmail.com>
Signed-off-by: Lv Zheng <lv.zheng@intel.com>
---
 drivers/acpi/ec.c |   63 +++++++++++++++++++++++++++++++++++++++++------------
 1 file changed, 49 insertions(+), 14 deletions(-)

diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
index 847e665..ab784a6 100644
--- a/drivers/acpi/ec.c
+++ b/drivers/acpi/ec.c
@@ -1461,6 +1461,37 @@ static int acpi_config_boot_ec(struct acpi_ec *ec, acpi_handle handle,
 	return ret;
 }
 
+static bool acpi_ec_ecdt_get_handle(acpi_handle *phandle)
+{
+	struct acpi_table_ecdt *ecdt_ptr;
+	acpi_status status;
+	acpi_handle handle;
+
+	status = acpi_get_table(ACPI_SIG_ECDT, 1,
+				(struct acpi_table_header **)&ecdt_ptr);
+	if (ACPI_FAILURE(status))
+		return false;
+
+	status = acpi_get_handle(NULL, ecdt_ptr->id, &handle);
+	if (ACPI_FAILURE(status))
+		return false;
+
+	*phandle = handle;
+	return true;
+}
+
+static bool acpi_is_boot_ec(struct acpi_ec *ec)
+{
+	if (!boot_ec)
+		return false;
+	if (ec->handle == boot_ec->handle &&
+	    ec->gpe == boot_ec->gpe &&
+	    ec->command_addr == boot_ec->command_addr &&
+	    ec->data_addr == boot_ec->data_addr)
+		return true;
+	return false;
+}
+
 static int acpi_ec_add(struct acpi_device *device)
 {
 	struct acpi_ec *ec = NULL;
@@ -1478,7 +1509,14 @@ static int acpi_ec_add(struct acpi_device *device)
 			goto error;
 	}
 
-	ret = acpi_config_boot_ec(ec, device->handle, true, false);
+	if (acpi_is_boot_ec(ec)) {
+		boot_ec_is_ecdt = false;
+		acpi_handle_debug(ec->handle, "duplicated.\n");
+		acpi_ec_free(ec);
+		ec = boot_ec;
+		ret = acpi_config_boot_ec(ec, ec->handle, true, false);
+	} else
+		ret = acpi_ec_setup(ec, true);
 	if (ret)
 		goto error;
 
@@ -1496,9 +1534,12 @@ static int acpi_ec_add(struct acpi_device *device)
 	/* Clear stale _Q events if hardware might require that */
 	if (EC_FLAGS_CLEAR_ON_RESUME)
 		acpi_ec_clear(ec);
+	acpi_handle_debug(ec->handle, "enumerated.\n");
 error:
-	if (ret)
-		acpi_ec_free(ec);
+	if (ret) {
+		if (ec != boot_ec)
+			acpi_ec_free(ec);
+	}
 	return ret;
 }
 
@@ -1510,11 +1551,13 @@ static int acpi_ec_remove(struct acpi_device *device)
 		return -EINVAL;
 
 	ec = acpi_driver_data(device);
-	ec_remove_handlers(ec);
 	release_region(ec->data_addr, 1);
 	release_region(ec->command_addr, 1);
 	device->driver_data = NULL;
-	acpi_ec_free(ec);
+	if (ec != boot_ec) {
+		ec_remove_handlers(ec);
+		acpi_ec_free(ec);
+	}
 	return 0;
 }
 
@@ -1586,8 +1629,6 @@ error:
  */
 int __init acpi_ec_ecdt_start(void)
 {
-	struct acpi_table_ecdt *ecdt_ptr;
-	acpi_status status;
 	acpi_handle handle;
 
 	if (!boot_ec)
@@ -1599,17 +1640,11 @@ int __init acpi_ec_ecdt_start(void)
 	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))
+	if (!acpi_ec_ecdt_get_handle(&handle))
 		return -ENODEV;
 	return acpi_config_boot_ec(boot_ec, handle, true, true);
 }
-- 
1.7.10

[PATCH v2 0/4] ACPI / EC: Fix ECDT and boot_ec support

[Lv Zheng]

Linux uses ECDT only in boot stage (before the namespace is fully
initialized), but there are cases reported that Windows allows ECDT to be
used for OSPM runtime, responding EC events by evaluating _Qxx methods
under the device node indicated in the ECDT.

This patchset changes Linux ECDT support to follow Windows behavior and
also fixes related boot_ec support.

v2:
 Rebased on top of recent linux-pm.git/linux-next.
 Addressed 1 comment from Peter Wu.
 Update patch SOBs.

Lv Zheng (4):
  ACPI / EC: Cleanup first_ec/boot_ec code
  ACPI / EC: Fix a memory leakage issue in acpi_ec_add()
  ACPI / EC: Fix a gap that ECDT EC cannot handle EC events
  ACPI / EC: Fix issues related to boot_ec

 drivers/acpi/ec.c       |  240 +++++++++++++++++++++++++++++++++++++----------
 drivers/acpi/internal.h |    1 +
 drivers/acpi/scan.c     |    1 +
 3 files changed, 195 insertions(+), 47 deletions(-)

-- 
1.7.10

[PATCH v2 1/4] ACPI / EC: Cleanup first_ec/boot_ec code

[Lv Zheng]

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@fedoraproject.org>
Tested-by: Jonh Henderson <jw.hendy@gmail.com>
Signed-off-by: Lv Zheng <lv.zheng@intel.com>
Cc: Peter Wu <peter@lekensteyn.nl>
---
 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 1925589..9eab651 100644
--- a/drivers/acpi/ec.c
+++ b/drivers/acpi/ec.c
@@ -184,6 +184,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 */
@@ -1304,7 +1305,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);
 
@@ -1365,6 +1375,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;
@@ -1440,21 +1455,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)
@@ -1470,7 +1513,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;
 	}
 
@@ -1478,8 +1521,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");
@@ -1487,10 +1528,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);
@@ -1513,9 +1551,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;
 }
 
@@ -1567,13 +1603,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;
 }
 
@@ -1671,7 +1704,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;
@@ -1681,12 +1713,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);
 error:
 	if (ret)
-		kfree(ec);
-	else
-		first_ec = boot_ec = ec;
+		acpi_ec_free(ec);
 	return ret;
 }
 
-- 
1.7.10

[PATCH v2 2/4] ACPI / EC: Fix a memory leakage issue in acpi_ec_add()

[Lv Zheng]

When the handler installation failed, there was no code to free the
allocated EC device. This patch fixes this memory leakage issue.

Link: https://bugzilla.kernel.org/show_bug.cgi?id=115021
Reported-and-tested-by: Luya Tshimbalanga <luya@fedoraproject.org>
Tested-by: Jonh Henderson <jw.hendy@gmail.com>
Signed-off-by: Lv Zheng <lv.zheng@intel.com>
Cc: Peter Wu <peter@lekensteyn.nl>
---
 drivers/acpi/ec.c |   16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
index 9eab651..50895ff 100644
--- a/drivers/acpi/ec.c
+++ b/drivers/acpi/ec.c
@@ -1513,14 +1513,18 @@ static int acpi_ec_add(struct acpi_device *device)
 		return -ENOMEM;
 	if (ec_parse_device(device->handle, 0, ec, NULL) !=
 		AE_CTRL_TERMINATE) {
-			acpi_ec_free(ec);
-			return -EINVAL;
+			ret = -EINVAL;
+			goto err_alloc;
 	}
 
 	/* 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);
+	if (ret)
+		goto err_query;
+
 	device->driver_data = ec;
 
 	ret = !!request_region(ec->data_addr, 1, "EC data");
@@ -1528,13 +1532,17 @@ 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);
 
-	ret = acpi_config_boot_ec(ec, false);
-
 	/* Reprobe devices depending on the EC */
 	acpi_walk_dep_device_list(ec->handle);
 
 	/* EC is fully operational, allow queries */
 	acpi_ec_enable_event(ec);
+	return 0;
+
+err_query:
+	acpi_ec_remove_query_handlers(ec, true, 0);
+err_alloc:
+	acpi_ec_free(ec);
 	return ret;
 }
 
-- 
1.7.10

[PATCH v2 3/4] ACPI / EC: Fix a gap that ECDT EC cannot handle EC events

[Lv Zheng]

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>
Tested-by: Jonh Henderson <jw.hendy@gmail.com>
Reviewed-by: Peter Wu <peter@lekensteyn.nl>
Signed-off-by: Lv Zheng <lv.zheng@intel.com>
Cc: Peter Wu <peter@lekensteyn.nl>
---
 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 50895ff..2ae9194 100644
--- a/drivers/acpi/ec.c
+++ b/drivers/acpi/ec.c
@@ -109,6 +109,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
@@ -1380,7 +1381,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;
 
@@ -1409,6 +1410,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,
@@ -1419,6 +1430,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 */
+			acpi_ec_enable_event(ec);
 		}
 	}
 
@@ -1453,13 +1467,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;
 
@@ -1475,18 +1493,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;
 
@@ -1494,9 +1527,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;
 }
 
@@ -1517,11 +1553,7 @@ static int acpi_ec_add(struct acpi_device *device)
 			goto err_alloc;
 	}
 
-	/* 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 err_query;
 
@@ -1534,9 +1566,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 */
-	acpi_ec_enable_event(ec);
 	return 0;
 
 err_query:
@@ -1555,7 +1584,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;
@@ -1601,9 +1629,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);
@@ -1611,13 +1638,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
@@ -1720,8 +1789,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 358b012..3797155 100644
--- a/drivers/acpi/internal.h
+++ b/drivers/acpi/internal.h
@@ -186,6 +186,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);
 int acpi_ec_add_query_handler(struct acpi_ec *ec, u8 query_bit,
diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
index e878fc7..f8f9f4e 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();
 
 	acpi_scan_initialized = true;
 
-- 
1.7.10

[PATCH v2 4/4] ACPI / EC: Fix issues related to boot_ec

[Lv Zheng]

There are issues related to the boot_ec:
1. If acpi_ec_remove() is invoked, boot_ec will also be freed, this is not
   expected as the boot_ec could be enumerated via ECDT.
2. Address space handler installation/unstallation lead to unexpected _REG
   evaluations.
This patch adds acpi_is_boot_ec() check to be used to fix the above issues.
However, since acpi_ec_remove() actually won't be invoked, this patch
doesn't handle the reference counting of "struct acpi_ec", it only ensures
the correctness of the boot_ec destruction during the boot.

Link: https://bugzilla.kernel.org/show_bug.cgi?id=153511
Reported-and-tested-by: Jonh Henderson <jw.hendy@gmail.com>
Signed-off-by: Lv Zheng <lv.zheng@intel.com>
Cc: Peter Wu <peter@lekensteyn.nl>
---
 drivers/acpi/ec.c |   63 +++++++++++++++++++++++++++++++++++++++++------------
 1 file changed, 49 insertions(+), 14 deletions(-)

diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
index 2ae9194..6805310 100644
--- a/drivers/acpi/ec.c
+++ b/drivers/acpi/ec.c
@@ -1536,6 +1536,37 @@ static int acpi_config_boot_ec(struct acpi_ec *ec, acpi_handle handle,
 	return ret;
 }
 
+static bool acpi_ec_ecdt_get_handle(acpi_handle *phandle)
+{
+	struct acpi_table_ecdt *ecdt_ptr;
+	acpi_status status;
+	acpi_handle handle;
+
+	status = acpi_get_table(ACPI_SIG_ECDT, 1,
+				(struct acpi_table_header **)&ecdt_ptr);
+	if (ACPI_FAILURE(status))
+		return false;
+
+	status = acpi_get_handle(NULL, ecdt_ptr->id, &handle);
+	if (ACPI_FAILURE(status))
+		return false;
+
+	*phandle = handle;
+	return true;
+}
+
+static bool acpi_is_boot_ec(struct acpi_ec *ec)
+{
+	if (!boot_ec)
+		return false;
+	if (ec->handle == boot_ec->handle &&
+	    ec->gpe == boot_ec->gpe &&
+	    ec->command_addr == boot_ec->command_addr &&
+	    ec->data_addr == boot_ec->data_addr)
+		return true;
+	return false;
+}
+
 static int acpi_ec_add(struct acpi_device *device)
 {
 	struct acpi_ec *ec = NULL;
@@ -1553,7 +1584,14 @@ static int acpi_ec_add(struct acpi_device *device)
 			goto err_alloc;
 	}
 
-	ret = acpi_config_boot_ec(ec, device->handle, true, false);
+	if (acpi_is_boot_ec(ec)) {
+		boot_ec_is_ecdt = false;
+		acpi_handle_debug(ec->handle, "duplicated.\n");
+		acpi_ec_free(ec);
+		ec = boot_ec;
+		ret = acpi_config_boot_ec(ec, ec->handle, true, false);
+	} else
+		ret = acpi_ec_setup(ec, true);
 	if (ret)
 		goto err_query;
 
@@ -1566,12 +1604,15 @@ static int acpi_ec_add(struct acpi_device *device)
 
 	/* Reprobe devices depending on the EC */
 	acpi_walk_dep_device_list(ec->handle);
+	acpi_handle_debug(ec->handle, "enumerated.\n");
 	return 0;
 
 err_query:
-	acpi_ec_remove_query_handlers(ec, true, 0);
+	if (ec != boot_ec)
+		acpi_ec_remove_query_handlers(ec, true, 0);
 err_alloc:
-	acpi_ec_free(ec);
+	if (ec != boot_ec)
+		acpi_ec_free(ec);
 	return ret;
 }
 
@@ -1583,11 +1624,13 @@ static int acpi_ec_remove(struct acpi_device *device)
 		return -EINVAL;
 
 	ec = acpi_driver_data(device);
-	ec_remove_handlers(ec);
 	release_region(ec->data_addr, 1);
 	release_region(ec->command_addr, 1);
 	device->driver_data = NULL;
-	acpi_ec_free(ec);
+	if (ec != boot_ec) {
+		ec_remove_handlers(ec);
+		acpi_ec_free(ec);
+	}
 	return 0;
 }
 
@@ -1659,8 +1702,6 @@ error:
  */
 int __init acpi_ec_ecdt_start(void)
 {
-	struct acpi_table_ecdt *ecdt_ptr;
-	acpi_status status;
 	acpi_handle handle;
 
 	if (!boot_ec)
@@ -1672,17 +1713,11 @@ int __init acpi_ec_ecdt_start(void)
 	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))
+	if (!acpi_ec_ecdt_get_handle(&handle))
 		return -ENODEV;
 	return acpi_config_boot_ec(boot_ec, handle, true, true);
 }
-- 
1.7.10

Re: [PATCH v2 0/4] ACPI / EC: Fix ECDT and boot_ec support

[Rafael J. Wysocki]

On Wednesday, September 07, 2016 04:49:59 PM Lv Zheng wrote:
> Linux uses ECDT only in boot stage (before the namespace is fully
> initialized), but there are cases reported that Windows allows ECDT to be
> used for OSPM runtime, responding EC events by evaluating _Qxx methods
> under the device node indicated in the ECDT.
> 
> This patchset changes Linux ECDT support to follow Windows behavior and
> also fixes related boot_ec support.
> 
> v2:
>  Rebased on top of recent linux-pm.git/linux-next.
>  Addressed 1 comment from Peter Wu.
>  Update patch SOBs.
> 
> Lv Zheng (4):
>   ACPI / EC: Cleanup first_ec/boot_ec code
>   ACPI / EC: Fix a memory leakage issue in acpi_ec_add()
>   ACPI / EC: Fix a gap that ECDT EC cannot handle EC events
>   ACPI / EC: Fix issues related to boot_ec
> 
>  drivers/acpi/ec.c       |  240 +++++++++++++++++++++++++++++++++++++----------
>  drivers/acpi/internal.h |    1 +
>  drivers/acpi/scan.c     |    1 +
>  3 files changed, 195 insertions(+), 47 deletions(-)

Whole series applied.

Thanks,
Rafael