linux-acpi.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/6] ACPI: EC: Updates related to initialization
@ 2020-02-27 22:19 Rafael J. Wysocki
  2020-02-27 22:21 ` [PATCH 1/6] ACPI: EC: Avoid printing confusing messages in acpi_ec_setup() Rafael J. Wysocki
                   ` (7 more replies)
  0 siblings, 8 replies; 24+ messages in thread
From: Rafael J. Wysocki @ 2020-02-27 22:19 UTC (permalink / raw)
  To: Linux ACPI; +Cc: LKML, Rafael J. Wysocki, Daniel Drake

Hi All,

The purpose of this series of update of the ACPI EC driver is to make its
initialization more straightforward.

They fix a couple of issues, clean up some things, remove redundant code etc.

Please refer to the changelogs of individual patches for details.

For easier access, the series is available in the git branch at

 git://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git \
 acpi-ec-work

on top of 5.6-rc3.

Thanks!




^ permalink raw reply	[flat|nested] 24+ messages in thread

* [PATCH 1/6] ACPI: EC: Avoid printing confusing messages in  acpi_ec_setup()
  2020-02-27 22:19 [PATCH 0/6] ACPI: EC: Updates related to initialization Rafael J. Wysocki
@ 2020-02-27 22:21 ` Rafael J. Wysocki
  2020-02-27 22:21 ` [PATCH 2/6] ACPI: EC: Avoid passing redundant argument to functions Rafael J. Wysocki
                   ` (6 subsequent siblings)
  7 siblings, 0 replies; 24+ messages in thread
From: Rafael J. Wysocki @ 2020-02-27 22:21 UTC (permalink / raw)
  To: Linux ACPI; +Cc: LKML, Rafael J. Wysocki, Daniel Drake

From: "Rafael J. Wysocki" <rafael.j.wysocki@intel.com>

It doesn't really make sense to pass ec->handle of the ECDT EC to
acpi_handle_info(), because it is set to ACPI_ROOT_OBJECT in
acpi_ec_ecdt_probe(), so rework acpi_ec_setup() to avoid using
acpi_handle_info() for printing messages.

First, notice that the "Used as first EC" message is not really
useful, because it is immediately followed by a more meaningful one
from either acpi_ec_ecdt_probe() or acpi_ec_dsdt_probe() (the latter
also includes the EC object path), so drop it altogether.

Second, use pr_info() for printing the EC configuration information.

While at it, make the code in question avoid printing invalid GPE or
IRQ numbers and make it print the GPE/IRQ information only when the
driver is ready to handle events.

Fixes: 72c77b7ea9ce ("ACPI / EC: Cleanup first_ec/boot_ec code")
Fixes: 406857f773b0 ("ACPI: EC: add support for hardware-reduced systems")
Cc: 5.5+ <stable@vger.kernel.org> # 5.5+
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/acpi/ec.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
index d1f1cf5d4bf0..2dc7cf2aeb21 100644
--- a/drivers/acpi/ec.c
+++ b/drivers/acpi/ec.c
@@ -1584,14 +1584,19 @@ static int acpi_ec_setup(struct acpi_ec *ec, struct acpi_device *device,
 		return ret;
 
 	/* First EC capable of handling transactions */
-	if (!first_ec) {
+	if (!first_ec)
 		first_ec = ec;
-		acpi_handle_info(first_ec->handle, "Used as first EC\n");
+
+	pr_info("EC_CMD/EC_SC=0x%lx, EC_DATA=0x%lx\n", ec->command_addr,
+		ec->data_addr);
+
+	if (test_bit(EC_FLAGS_EVENT_HANDLER_INSTALLED, &ec->flags)) {
+		if (ec->gpe >= 0)
+			pr_info("GPE=0x%x\n", ec->gpe);
+		else
+			pr_info("IRQ=%d\n", ec->irq);
 	}
 
-	acpi_handle_info(ec->handle,
-			 "GPE=0x%x, IRQ=%d, EC_CMD/EC_SC=0x%lx, EC_DATA=0x%lx\n",
-			 ec->gpe, ec->irq, ec->command_addr, ec->data_addr);
 	return ret;
 }
 
-- 
2.16.4






^ permalink raw reply related	[flat|nested] 24+ messages in thread

* [PATCH 2/6] ACPI: EC: Avoid passing redundant argument to functions
  2020-02-27 22:19 [PATCH 0/6] ACPI: EC: Updates related to initialization Rafael J. Wysocki
  2020-02-27 22:21 ` [PATCH 1/6] ACPI: EC: Avoid printing confusing messages in acpi_ec_setup() Rafael J. Wysocki
@ 2020-02-27 22:21 ` Rafael J. Wysocki
  2020-02-27 22:22 ` [PATCH 3/6] ACPI: EC: Drop AE_NOT_FOUND special case from ec_install_handlers() Rafael J. Wysocki
                   ` (5 subsequent siblings)
  7 siblings, 0 replies; 24+ messages in thread
From: Rafael J. Wysocki @ 2020-02-27 22:21 UTC (permalink / raw)
  To: Linux ACPI; +Cc: LKML, Rafael J. Wysocki, Daniel Drake

From: "Rafael J. Wysocki" <rafael.j.wysocki@intel.com>

After commit 406857f773b0 ("ACPI: EC: add support for hardware-reduced
systems") the handle_events argument passed to ec_install_handlers()
and acpi_ec_setup() is redundant, because it is always 'false' when
the device argument passed to them in NULL and it is always 'true'
otherwise, so the device argument can be tested against NULL instead
of testing the handle_events one.

Accordingly, modify ec_install_handlers() and acpi_ec_setup() to take
two arguments and reduce the number of checks in the former.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/acpi/ec.c | 21 ++++++++-------------
 1 file changed, 8 insertions(+), 13 deletions(-)

diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
index 2dc7cf2aeb21..3153e7684053 100644
--- a/drivers/acpi/ec.c
+++ b/drivers/acpi/ec.c
@@ -1476,8 +1476,7 @@ static int install_gpio_irq_event_handler(struct acpi_ec *ec,
  *       handler is not installed, which means "not able to handle
  *       transactions".
  */
-static int ec_install_handlers(struct acpi_ec *ec, struct acpi_device *device,
-			       bool handle_events)
+static int ec_install_handlers(struct acpi_ec *ec, struct acpi_device *device)
 {
 	acpi_status status;
 
@@ -1507,7 +1506,7 @@ static int ec_install_handlers(struct acpi_ec *ec, struct acpi_device *device,
 		set_bit(EC_FLAGS_EC_HANDLER_INSTALLED, &ec->flags);
 	}
 
-	if (!handle_events)
+	if (!device)
 		return 0;
 
 	if (!test_bit(EC_FLAGS_QUERY_METHODS_INSTALLED, &ec->flags)) {
@@ -1520,13 +1519,10 @@ static int ec_install_handlers(struct acpi_ec *ec, struct acpi_device *device,
 	if (!test_bit(EC_FLAGS_EVENT_HANDLER_INSTALLED, &ec->flags)) {
 		if (ec->gpe >= 0) {
 			install_gpe_event_handler(ec);
-		} else if (device) {
+		} else {
 			int ret = install_gpio_irq_event_handler(ec, device);
-
 			if (ret)
 				return ret;
-		} else { /* No GPE and no GpioInt? */
-			return -ENODEV;
 		}
 	}
 	/* EC is fully operational, allow queries */
@@ -1574,12 +1570,11 @@ static void ec_remove_handlers(struct acpi_ec *ec)
 	}
 }
 
-static int acpi_ec_setup(struct acpi_ec *ec, struct acpi_device *device,
-			 bool handle_events)
+static int acpi_ec_setup(struct acpi_ec *ec, struct acpi_device *device)
 {
 	int ret;
 
-	ret = ec_install_handlers(ec, device, handle_events);
+	ret = ec_install_handlers(ec, device);
 	if (ret)
 		return ret;
 
@@ -1660,7 +1655,7 @@ static int acpi_ec_add(struct acpi_device *device)
 		}
 	}
 
-	ret = acpi_ec_setup(ec, device, true);
+	ret = acpi_ec_setup(ec, device);
 	if (ret)
 		goto err_query;
 
@@ -1780,7 +1775,7 @@ void __init acpi_ec_dsdt_probe(void)
 	 * At this point, the GPE is not fully initialized, so do not to
 	 * handle the events.
 	 */
-	ret = acpi_ec_setup(ec, NULL, false);
+	ret = acpi_ec_setup(ec, NULL);
 	if (ret) {
 		acpi_ec_free(ec);
 		return;
@@ -1967,7 +1962,7 @@ void __init acpi_ec_ecdt_probe(void)
 	 * At this point, the namespace is not initialized, so do not find
 	 * the namespace objects, or handle the events.
 	 */
-	ret = acpi_ec_setup(ec, NULL, false);
+	ret = acpi_ec_setup(ec, NULL);
 	if (ret) {
 		acpi_ec_free(ec);
 		return;
-- 
2.16.4






^ permalink raw reply related	[flat|nested] 24+ messages in thread

* [PATCH 3/6] ACPI: EC: Drop AE_NOT_FOUND special case from ec_install_handlers()
  2020-02-27 22:19 [PATCH 0/6] ACPI: EC: Updates related to initialization Rafael J. Wysocki
  2020-02-27 22:21 ` [PATCH 1/6] ACPI: EC: Avoid printing confusing messages in acpi_ec_setup() Rafael J. Wysocki
  2020-02-27 22:21 ` [PATCH 2/6] ACPI: EC: Avoid passing redundant argument to functions Rafael J. Wysocki
@ 2020-02-27 22:22 ` Rafael J. Wysocki
  2020-02-27 22:23 ` [PATCH 4/6] ACPI: EC: Unify handling of event handler installation failures Rafael J. Wysocki
                   ` (4 subsequent siblings)
  7 siblings, 0 replies; 24+ messages in thread
From: Rafael J. Wysocki @ 2020-02-27 22:22 UTC (permalink / raw)
  To: Linux ACPI; +Cc: LKML, Rafael J. Wysocki, Daniel Drake

From: "Rafael J. Wysocki" <rafael.j.wysocki@intel.com>

If the status value returned by acpi_install_address_space_handler()
in ec_install_handlers() is AE_NOT_FOUND, it is treated in a special
way, apparently because it might mean a _REG method evaluation
failure (at least that is the case according to the comment in
there), but acpi_install_address_space_handler() does not take
_REG evaluation errors into account at all, so the AE_NOT_FOUND
special handling is confusing at best.

For this reason, change ec_install_handlers() to stop the EC and
return -ENODEV on all acpi_install_address_space_handler() errors.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/acpi/ec.c | 15 ++-------------
 1 file changed, 2 insertions(+), 13 deletions(-)

diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
index 3153e7684053..6f501d552e6e 100644
--- a/drivers/acpi/ec.c
+++ b/drivers/acpi/ec.c
@@ -1489,19 +1489,8 @@ static int ec_install_handlers(struct acpi_ec *ec, struct acpi_device *device)
 							    &acpi_ec_space_handler,
 							    NULL, ec);
 		if (ACPI_FAILURE(status)) {
-			if (status == AE_NOT_FOUND) {
-				/*
-				 * Maybe OS fails in evaluating the _REG
-				 * object. The AE_NOT_FOUND error will be
-				 * ignored and OS * continue to initialize
-				 * EC.
-				 */
-				pr_err("Fail in evaluating the _REG object"
-					" of EC device. Broken bios is suspected.\n");
-			} else {
-				acpi_ec_stop(ec, false);
-				return -ENODEV;
-			}
+			acpi_ec_stop(ec, false);
+			return -ENODEV;
 		}
 		set_bit(EC_FLAGS_EC_HANDLER_INSTALLED, &ec->flags);
 	}
-- 
2.16.4






^ permalink raw reply related	[flat|nested] 24+ messages in thread

* [PATCH 4/6] ACPI: EC: Unify handling of event handler installation failures
  2020-02-27 22:19 [PATCH 0/6] ACPI: EC: Updates related to initialization Rafael J. Wysocki
                   ` (2 preceding siblings ...)
  2020-02-27 22:22 ` [PATCH 3/6] ACPI: EC: Drop AE_NOT_FOUND special case from ec_install_handlers() Rafael J. Wysocki
@ 2020-02-27 22:23 ` Rafael J. Wysocki
  2020-02-27 22:23 ` [PATCH 5/6] ACPI: EC: Simplify acpi_ec_add() Rafael J. Wysocki
                   ` (3 subsequent siblings)
  7 siblings, 0 replies; 24+ messages in thread
From: Rafael J. Wysocki @ 2020-02-27 22:23 UTC (permalink / raw)
  To: Linux ACPI; +Cc: LKML, Rafael J. Wysocki, Daniel Drake

From: "Rafael J. Wysocki" <rafael.j.wysocki@intel.com>

Commit 406857f773b0 ("ACPI: EC: add support for hardware-reduced
systems") made ec_install_handlers() return an error on failures
to configure a GPIO IRQ for the EC, but that is inconsistent with
the handling of the GPE event handler installation failures even
though it is exactly the same issue and the driver can respond to
it in the same way in both cases (the EC can be actively polled
for events through its registers if the event handler installation
fails).

Moreover, it requires acpi_ec_add() to take that special case into
account and disagrees with the ec_install_handlers() header comment.

For this reason, consolidate the handling of event handler
installation failures in ec_install_handlers() so that they do not
prevent the EC from being used in any case.  On top of that, reduce
code duplication between install_gpe_event_handler() and
install_gpio_irq_event_handler() by moving some code from there
into ec_install_handlers() and simplify the error code path in
acpi_ec_add().

While at it, turn the ec_install_handlers() header comment into
a proper kerneldoc one and add some general control flow information
to it.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/acpi/ec.c | 96 +++++++++++++++++++++++++++----------------------------
 1 file changed, 48 insertions(+), 48 deletions(-)

diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
index 6f501d552e6e..3f6127e919ab 100644
--- a/drivers/acpi/ec.c
+++ b/drivers/acpi/ec.c
@@ -1427,54 +1427,53 @@ ec_parse_device(acpi_handle handle, u32 Level, void *context, void **retval)
 	return AE_CTRL_TERMINATE;
 }
 
-static void install_gpe_event_handler(struct acpi_ec *ec)
-{
-	acpi_status status =
-		acpi_install_gpe_raw_handler(NULL, ec->gpe,
-					     ACPI_GPE_EDGE_TRIGGERED,
-					     &acpi_ec_gpe_handler,
-					     ec);
-	if (ACPI_SUCCESS(status)) {
-		/* This is not fatal as we can poll EC events */
-		set_bit(EC_FLAGS_EVENT_HANDLER_INSTALLED, &ec->flags);
-		acpi_ec_leave_noirq(ec);
-		if (test_bit(EC_FLAGS_STARTED, &ec->flags) &&
-		    ec->reference_count >= 1)
-			acpi_ec_enable_gpe(ec, true);
-	}
+static bool install_gpe_event_handler(struct acpi_ec *ec)
+{
+	acpi_status status;
+
+	status = acpi_install_gpe_raw_handler(NULL, ec->gpe,
+					      ACPI_GPE_EDGE_TRIGGERED,
+					      &acpi_ec_gpe_handler, ec);
+	if (ACPI_FAILURE(status))
+		return false;
+
+	if (test_bit(EC_FLAGS_STARTED, &ec->flags) && ec->reference_count >= 1)
+		acpi_ec_enable_gpe(ec, true);
+
+	return true;
 }
 
-/* ACPI reduced hardware platforms use a GpioInt specified in _CRS. */
-static int install_gpio_irq_event_handler(struct acpi_ec *ec,
-					  struct acpi_device *device)
+static bool install_gpio_irq_event_handler(struct acpi_ec *ec,
+					   struct acpi_device *device)
 {
-	int irq = acpi_dev_gpio_irq_get(device, 0);
-	int ret;
+	int irq, ret;
 
+	/* ACPI reduced hardware platforms use a GpioInt specified in _CRS. */
+	irq = acpi_dev_gpio_irq_get(device, 0);
 	if (irq < 0)
-		return irq;
-
-	ret = request_irq(irq, acpi_ec_irq_handler, IRQF_SHARED,
-			  "ACPI EC", ec);
+		return false;
 
-	/*
-	 * Unlike the GPE case, we treat errors here as fatal, we'll only
-	 * implement GPIO polling if we find a case that needs it.
-	 */
+	ret = request_irq(irq, acpi_ec_irq_handler, IRQF_SHARED, "ACPI EC", ec);
 	if (ret < 0)
-		return ret;
+		return false;
 
 	ec->irq = irq;
-	set_bit(EC_FLAGS_EVENT_HANDLER_INSTALLED, &ec->flags);
-	acpi_ec_leave_noirq(ec);
 
-	return 0;
+	return true;
 }
 
-/*
- * Note: This function returns an error code only when the address space
- *       handler is not installed, which means "not able to handle
- *       transactions".
+/**
+ * ec_install_handlers - Install service callbacks and register query methods.
+ * @ec: Target EC.
+ * @device: ACPI device object corresponding to @ec.
+ *
+ * Install a handler for the EC address space type unless it has been installed
+ * already.  If @device is not NULL, also look for EC query methods in the
+ * namespace and register them, and install an event (either GPE or GPIO IRQ)
+ * handler for the EC, if possible.
+ *
+ * Return -ENODEV if the address space handler cannot be installed, which means
+ * "unable to handle transactions", or 0 (success) otherwise.
  */
 static int ec_install_handlers(struct acpi_ec *ec, struct acpi_device *device)
 {
@@ -1506,13 +1505,17 @@ static int ec_install_handlers(struct acpi_ec *ec, struct acpi_device *device)
 		set_bit(EC_FLAGS_QUERY_METHODS_INSTALLED, &ec->flags);
 	}
 	if (!test_bit(EC_FLAGS_EVENT_HANDLER_INSTALLED, &ec->flags)) {
-		if (ec->gpe >= 0) {
-			install_gpe_event_handler(ec);
-		} else {
-			int ret = install_gpio_irq_event_handler(ec, device);
-			if (ret)
-				return ret;
+		bool ready = ec->gpe >= 0 ?
+				install_gpe_event_handler(ec) :
+				install_gpio_irq_event_handler(ec, device);
+		if (ready) {
+			set_bit(EC_FLAGS_EVENT_HANDLER_INSTALLED, &ec->flags);
+			acpi_ec_leave_noirq(ec);
 		}
+		/*
+		 * Failures to install an event handler are not fatal, because
+		 * the EC can be polled for events.
+		 */
 	}
 	/* EC is fully operational, allow queries */
 	acpi_ec_enable_event(ec);
@@ -1625,7 +1628,7 @@ static int acpi_ec_add(struct acpi_device *device)
 		status = ec_parse_device(device->handle, 0, ec, NULL);
 		if (status != AE_CTRL_TERMINATE) {
 			ret = -EINVAL;
-			goto err_alloc;
+			goto err;
 		}
 
 		if (boot_ec && ec->command_addr == boot_ec->command_addr &&
@@ -1646,7 +1649,7 @@ static int acpi_ec_add(struct acpi_device *device)
 
 	ret = acpi_ec_setup(ec, device);
 	if (ret)
-		goto err_query;
+		goto err;
 
 	if (ec == boot_ec)
 		acpi_handle_info(boot_ec->handle,
@@ -1667,10 +1670,7 @@ static int acpi_ec_add(struct acpi_device *device)
 	acpi_handle_debug(ec->handle, "enumerated.\n");
 	return 0;
 
-err_query:
-	if (ec != boot_ec)
-		acpi_ec_remove_query_handlers(ec, true, 0);
-err_alloc:
+err:
 	if (ec != boot_ec)
 		acpi_ec_free(ec);
 	return ret;
-- 
2.16.4






^ permalink raw reply related	[flat|nested] 24+ messages in thread

* [PATCH 5/6] ACPI: EC: Simplify acpi_ec_add()
  2020-02-27 22:19 [PATCH 0/6] ACPI: EC: Updates related to initialization Rafael J. Wysocki
                   ` (3 preceding siblings ...)
  2020-02-27 22:23 ` [PATCH 4/6] ACPI: EC: Unify handling of event handler installation failures Rafael J. Wysocki
@ 2020-02-27 22:23 ` Rafael J. Wysocki
  2020-02-27 22:24 ` [PATCH 6/6] ACPI: EC: Use fast path in acpi_ec_add() for DSDT boot EC Rafael J. Wysocki
                   ` (2 subsequent siblings)
  7 siblings, 0 replies; 24+ messages in thread
From: Rafael J. Wysocki @ 2020-02-27 22:23 UTC (permalink / raw)
  To: Linux ACPI; +Cc: LKML, Rafael J. Wysocki, Daniel Drake

From: "Rafael J. Wysocki" <rafael.j.wysocki@intel.com>

First, notice that if the device ID in acpi_ec_add() is equal to
ACPI_ECDT_HID, boot_ec_is_ecdt must be set, because this means
that the device object passed to acpi_ec_add() comes from
acpi_ec_ecdt_start() which fails if boot_ec_is_ecdt is unset.
Accordingly, boot_ec_is_ecdt need not be set again in that case,
so drop that redundant update of it from the code.

Next, ec->handle must be a valid ACPI handle right before
returning 0 from acpi_ec_add(), because it either is the handle
of the device object passed to that function, or it is the boot EC
handle coming from acpi_ec_ecdt_start() which fails if it cannot
find a valid handle for the boot EC.  Moreover, the object with
that handle is regarded as a valid representation of the EC in all
cases, so there is no reason to avoid the _DEP list update walk if
that handle is the boot EC handle.  Accordingly, drop the dep_update
local variable from acpi_ec_add() and call acpi_walk_dep_device_list()
for ec->handle unconditionally before returning 0 from it.

Finally, the ec local variable in acpi_ec_add() need not be
initialized to NULL and the status local variable declaration
can be moved to the block in which it is used, so change the code
in accordance with these observations.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/acpi/ec.c | 17 ++++++++---------
 1 file changed, 8 insertions(+), 9 deletions(-)

diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
index 3f6127e919ab..4ed0c6155d3f 100644
--- a/drivers/acpi/ec.c
+++ b/drivers/acpi/ec.c
@@ -1608,19 +1608,18 @@ static bool acpi_ec_ecdt_get_handle(acpi_handle *phandle)
 
 static int acpi_ec_add(struct acpi_device *device)
 {
-	struct acpi_ec *ec = NULL;
-	bool dep_update = true;
-	acpi_status status;
+	struct acpi_ec *ec;
 	int ret;
 
 	strcpy(acpi_device_name(device), ACPI_EC_DEVICE_NAME);
 	strcpy(acpi_device_class(device), ACPI_EC_CLASS);
 
 	if (!strcmp(acpi_device_hid(device), ACPI_ECDT_HID)) {
-		boot_ec_is_ecdt = true;
+		/* Fast path: this device corresponds to the boot EC. */
 		ec = boot_ec;
-		dep_update = false;
 	} else {
+		acpi_status status;
+
 		ec = acpi_ec_alloc();
 		if (!ec)
 			return -ENOMEM;
@@ -1663,16 +1662,16 @@ 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);
 
-	if (dep_update) {
-		/* Reprobe devices depending on the EC */
-		acpi_walk_dep_device_list(ec->handle);
-	}
+	/* Reprobe devices depending on the EC */
+	acpi_walk_dep_device_list(ec->handle);
+
 	acpi_handle_debug(ec->handle, "enumerated.\n");
 	return 0;
 
 err:
 	if (ec != boot_ec)
 		acpi_ec_free(ec);
+
 	return ret;
 }
 
-- 
2.16.4






^ permalink raw reply related	[flat|nested] 24+ messages in thread

* [PATCH 6/6] ACPI: EC: Use fast path in acpi_ec_add() for DSDT boot EC
  2020-02-27 22:19 [PATCH 0/6] ACPI: EC: Updates related to initialization Rafael J. Wysocki
                   ` (4 preceding siblings ...)
  2020-02-27 22:23 ` [PATCH 5/6] ACPI: EC: Simplify acpi_ec_add() Rafael J. Wysocki
@ 2020-02-27 22:24 ` Rafael J. Wysocki
  2020-02-28  9:43 ` [PATCH 0/6] ACPI: EC: Updates related to initialization Daniel Drake
  2020-03-04 10:42 ` [PATCH v2 " Rafael J. Wysocki
  7 siblings, 0 replies; 24+ messages in thread
From: Rafael J. Wysocki @ 2020-02-27 22:24 UTC (permalink / raw)
  To: Linux ACPI; +Cc: LKML, Rafael J. Wysocki, Daniel Drake

From: "Rafael J. Wysocki" <rafael.j.wysocki@intel.com>

If the boot EC comes from the DSDT, its ACPI handle is equal to the
handle of a device object with the PNP0C09 device ID.  If that
device object is passed to acpi_ec_add(), it is not necessary to
allocate a new EC structure for it and parse it, because that has
been done already, so change the function to use the fast path in
that case.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/acpi/ec.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
index 4ed0c6155d3f..920a08c01ab6 100644
--- a/drivers/acpi/ec.c
+++ b/drivers/acpi/ec.c
@@ -1614,7 +1614,8 @@ static int acpi_ec_add(struct acpi_device *device)
 	strcpy(acpi_device_name(device), ACPI_EC_DEVICE_NAME);
 	strcpy(acpi_device_class(device), ACPI_EC_CLASS);
 
-	if (!strcmp(acpi_device_hid(device), ACPI_ECDT_HID)) {
+	if ((boot_ec && boot_ec->handle == device->handle) ||
+	    !strcmp(acpi_device_hid(device), ACPI_ECDT_HID)) {
 		/* Fast path: this device corresponds to the boot EC. */
 		ec = boot_ec;
 	} else {
-- 
2.16.4






^ permalink raw reply related	[flat|nested] 24+ messages in thread

* Re: [PATCH 0/6] ACPI: EC: Updates related to initialization
  2020-02-27 22:19 [PATCH 0/6] ACPI: EC: Updates related to initialization Rafael J. Wysocki
                   ` (5 preceding siblings ...)
  2020-02-27 22:24 ` [PATCH 6/6] ACPI: EC: Use fast path in acpi_ec_add() for DSDT boot EC Rafael J. Wysocki
@ 2020-02-28  9:43 ` Daniel Drake
  2020-03-02  5:53   ` Jian-Hong Pan
  2020-03-04 10:42 ` [PATCH v2 " Rafael J. Wysocki
  7 siblings, 1 reply; 24+ messages in thread
From: Daniel Drake @ 2020-02-28  9:43 UTC (permalink / raw)
  To: Rafael J. Wysocki, Jian-Hong Pan; +Cc: Linux ACPI, LKML, Rafael J. Wysocki

On Thu, Feb 27, 2020 at 10:25 PM Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> The purpose of this series of update of the ACPI EC driver is to make its
> initialization more straightforward.
>
> They fix a couple of issues, clean up some things, remove redundant code etc.
>
> Please refer to the changelogs of individual patches for details.
>
> For easier access, the series is available in the git branch at
>
>  git://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git \
>  acpi-ec-work
>
> on top of 5.6-rc3.

Jian-Hong, can you please test this on Asus UX434DA?
Check if the screen brightness hotkeys are still working after these changes.

Thanks
Daniel

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH 0/6] ACPI: EC: Updates related to initialization
  2020-02-28  9:43 ` [PATCH 0/6] ACPI: EC: Updates related to initialization Daniel Drake
@ 2020-03-02  5:53   ` Jian-Hong Pan
  2020-03-02  9:55     ` Rafael J. Wysocki
  2020-03-02 10:38     ` Rafael J. Wysocki
  0 siblings, 2 replies; 24+ messages in thread
From: Jian-Hong Pan @ 2020-03-02  5:53 UTC (permalink / raw)
  To: Rafael J. Wysocki; +Cc: Daniel Drake, Linux ACPI, LKML, Rafael J. Wysocki

Daniel Drake <drake@endlessm.com> 於 2020年2月28日 週五 下午5:43寫道:
>
> On Thu, Feb 27, 2020 at 10:25 PM Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> > The purpose of this series of update of the ACPI EC driver is to make its
> > initialization more straightforward.
> >
> > They fix a couple of issues, clean up some things, remove redundant code etc.
> >
> > Please refer to the changelogs of individual patches for details.
> >
> > For easier access, the series is available in the git branch at
> >
> >  git://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git \
> >  acpi-ec-work
> >
> > on top of 5.6-rc3.
>
> Jian-Hong, can you please test this on Asus UX434DA?
> Check if the screen brightness hotkeys are still working after these changes.

Hi Rafael,

Thanks for your patches, but we found an issue:
The laptops like ASUS UX434DA's screen brightness hotkeys work before
this patch series.  However, the hotkeys are failed with the patch
"ACPI: EC: Unify handling of event handler installation failures".

Jian-Hong Pan

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH 0/6] ACPI: EC: Updates related to initialization
  2020-03-02  5:53   ` Jian-Hong Pan
@ 2020-03-02  9:55     ` Rafael J. Wysocki
  2020-03-02 10:38     ` Rafael J. Wysocki
  1 sibling, 0 replies; 24+ messages in thread
From: Rafael J. Wysocki @ 2020-03-02  9:55 UTC (permalink / raw)
  To: Jian-Hong Pan
  Cc: Rafael J. Wysocki, Daniel Drake, Linux ACPI, LKML, Rafael J. Wysocki

On Mon, Mar 2, 2020 at 6:54 AM Jian-Hong Pan <jian-hong@endlessm.com> wrote:
>
> Daniel Drake <drake@endlessm.com> 於 2020年2月28日 週五 下午5:43寫道:
> >
> > On Thu, Feb 27, 2020 at 10:25 PM Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> > > The purpose of this series of update of the ACPI EC driver is to make its
> > > initialization more straightforward.
> > >
> > > They fix a couple of issues, clean up some things, remove redundant code etc.
> > >
> > > Please refer to the changelogs of individual patches for details.
> > >
> > > For easier access, the series is available in the git branch at
> > >
> > >  git://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git \
> > >  acpi-ec-work
> > >
> > > on top of 5.6-rc3.
> >
> > Jian-Hong, can you please test this on Asus UX434DA?
> > Check if the screen brightness hotkeys are still working after these changes.
>
> Hi Rafael,
>
> Thanks for your patches, but we found an issue:
> The laptops like ASUS UX434DA's screen brightness hotkeys work before
> this patch series.  However, the hotkeys are failed with the patch
> "ACPI: EC: Unify handling of event handler installation failures".

Thanks for checking!

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH 0/6] ACPI: EC: Updates related to initialization
  2020-03-02  5:53   ` Jian-Hong Pan
  2020-03-02  9:55     ` Rafael J. Wysocki
@ 2020-03-02 10:38     ` Rafael J. Wysocki
  2020-03-02 11:45       ` Rafael J. Wysocki
  1 sibling, 1 reply; 24+ messages in thread
From: Rafael J. Wysocki @ 2020-03-02 10:38 UTC (permalink / raw)
  To: Jian-Hong Pan
  Cc: Rafael J. Wysocki, Daniel Drake, Linux ACPI, LKML, Rafael J. Wysocki

On Mon, Mar 2, 2020 at 6:54 AM Jian-Hong Pan <jian-hong@endlessm.com> wrote:
>
> Daniel Drake <drake@endlessm.com> 於 2020年2月28日 週五 下午5:43寫道:
> >
> > On Thu, Feb 27, 2020 at 10:25 PM Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> > > The purpose of this series of update of the ACPI EC driver is to make its
> > > initialization more straightforward.
> > >
> > > They fix a couple of issues, clean up some things, remove redundant code etc.
> > >
> > > Please refer to the changelogs of individual patches for details.
> > >
> > > For easier access, the series is available in the git branch at
> > >
> > >  git://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git \
> > >  acpi-ec-work
> > >
> > > on top of 5.6-rc3.
> >
> > Jian-Hong, can you please test this on Asus UX434DA?
> > Check if the screen brightness hotkeys are still working after these changes.
>
> Hi Rafael,
>
> Thanks for your patches, but we found an issue:
> The laptops like ASUS UX434DA's screen brightness hotkeys work before
> this patch series.  However, the hotkeys are failed with the patch
> "ACPI: EC: Unify handling of event handler installation failures".

So I have modified the series to avoid the change that can possibly break this.

Can you please pull the new series from

 git://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git \
 acpi-ec-work

(same branch) and retest?

I'll post the updated patches later this week, but it would be good to
try them on now if possible.

Thanks!

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH 0/6] ACPI: EC: Updates related to initialization
  2020-03-02 10:38     ` Rafael J. Wysocki
@ 2020-03-02 11:45       ` Rafael J. Wysocki
  2020-03-03  7:28         ` Jian-Hong Pan
  0 siblings, 1 reply; 24+ messages in thread
From: Rafael J. Wysocki @ 2020-03-02 11:45 UTC (permalink / raw)
  To: Jian-Hong Pan
  Cc: Rafael J. Wysocki, Daniel Drake, Linux ACPI, LKML, Rafael J. Wysocki

On Mon, Mar 2, 2020 at 11:38 AM Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> On Mon, Mar 2, 2020 at 6:54 AM Jian-Hong Pan <jian-hong@endlessm.com> wrote:
> >
> > Daniel Drake <drake@endlessm.com> 於 2020年2月28日 週五 下午5:43寫道:
> > >
> > > On Thu, Feb 27, 2020 at 10:25 PM Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> > > > The purpose of this series of update of the ACPI EC driver is to make its
> > > > initialization more straightforward.
> > > >
> > > > They fix a couple of issues, clean up some things, remove redundant code etc.
> > > >
> > > > Please refer to the changelogs of individual patches for details.
> > > >
> > > > For easier access, the series is available in the git branch at
> > > >
> > > >  git://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git \
> > > >  acpi-ec-work
> > > >
> > > > on top of 5.6-rc3.
> > >
> > > Jian-Hong, can you please test this on Asus UX434DA?
> > > Check if the screen brightness hotkeys are still working after these changes.
> >
> > Hi Rafael,
> >
> > Thanks for your patches, but we found an issue:
> > The laptops like ASUS UX434DA's screen brightness hotkeys work before
> > this patch series.  However, the hotkeys are failed with the patch
> > "ACPI: EC: Unify handling of event handler installation failures".
>
> So I have modified the series to avoid the change that can possibly break this.
>
> Can you please pull the new series from
>
>  git://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git \
>  acpi-ec-work
>
> (same branch) and retest?

Note that the current top-most commit in that branch is

0957d98f50da ACPI: EC: Consolidate event handler installation code



>
> I'll post the updated patches later this week, but it would be good to
> try them on now if possible.
>
> Thanks!

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH 0/6] ACPI: EC: Updates related to initialization
  2020-03-02 11:45       ` Rafael J. Wysocki
@ 2020-03-03  7:28         ` Jian-Hong Pan
  2020-03-03  9:09           ` Rafael J. Wysocki
  0 siblings, 1 reply; 24+ messages in thread
From: Jian-Hong Pan @ 2020-03-03  7:28 UTC (permalink / raw)
  To: Rafael J. Wysocki
  Cc: Rafael J. Wysocki, Daniel Drake, Linux ACPI, LKML,
	Linux Upstreaming Team

Rafael J. Wysocki <rafael@kernel.org> 於 2020年3月2日 週一 下午7:45寫道:
>
> On Mon, Mar 2, 2020 at 11:38 AM Rafael J. Wysocki <rafael@kernel.org> wrote:
> >
> > On Mon, Mar 2, 2020 at 6:54 AM Jian-Hong Pan <jian-hong@endlessm.com> wrote:
> > >
> > > Daniel Drake <drake@endlessm.com> 於 2020年2月28日 週五 下午5:43寫道:
> > > >
> > > > On Thu, Feb 27, 2020 at 10:25 PM Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> > > > > The purpose of this series of update of the ACPI EC driver is to make its
> > > > > initialization more straightforward.
> > > > >
> > > > > They fix a couple of issues, clean up some things, remove redundant code etc.
> > > > >
> > > > > Please refer to the changelogs of individual patches for details.
> > > > >
> > > > > For easier access, the series is available in the git branch at
> > > > >
> > > > >  git://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git \
> > > > >  acpi-ec-work
> > > > >
> > > > > on top of 5.6-rc3.
> > > >
> > > > Jian-Hong, can you please test this on Asus UX434DA?
> > > > Check if the screen brightness hotkeys are still working after these changes.
> > >
> > > Hi Rafael,
> > >
> > > Thanks for your patches, but we found an issue:
> > > The laptops like ASUS UX434DA's screen brightness hotkeys work before
> > > this patch series.  However, the hotkeys are failed with the patch
> > > "ACPI: EC: Unify handling of event handler installation failures".
> >
> > So I have modified the series to avoid the change that can possibly break this.
> >
> > Can you please pull the new series from
> >
> >  git://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git \
> >  acpi-ec-work
> >
> > (same branch) and retest?
>
> Note that the current top-most commit in that branch is
>
> 0957d98f50da ACPI: EC: Consolidate event handler installation code

I tested the commits in acpi-ec-work branch whose last commit is
0957d98f50da ("ACPI: EC: Consolidate event handler installation
code").  The screen brightness hotkeys are still failed with
0957d98f50da ("ACPI: EC: Consolidate event handler installation
code").

I tweak and add some debug messages:

diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
index 85f1fe8e208a..3887f427283c 100644
--- a/drivers/acpi/ec.c
+++ b/drivers/acpi/ec.c
@@ -1443,23 +1443,27 @@ static bool install_gpe_event_handler(struct
acpi_ec *ec)
        return true;
 }

-static bool install_gpio_irq_event_handler(struct acpi_ec *ec,
+static int install_gpio_irq_event_handler(struct acpi_ec *ec,
                                           struct acpi_device *device)
 {
        int irq, ret;

        /* ACPI reduced hardware platforms use a GpioInt specified in _CRS. */
        irq = acpi_dev_gpio_irq_get(device, 0);
-       if (irq < 0)
-               return false;
+       if (irq < 0) {
+               pr_err("%s: acpi_dev_gpio_irq_get returns %d\n", __func__, irq);
+               return irq;
+       }

        ret = request_irq(irq, acpi_ec_irq_handler, IRQF_SHARED, "ACPI EC", ec);
-       if (ret < 0)
-               return false;
+       if (ret < 0) {
+               pr_err("%s: request_irq returns %d\n", __func__, ret);
+               return ret;
+       }

        ec->irq = irq;

-       return true;
+       return 0;
 }

 /**
@@ -1517,9 +1521,11 @@ static int ec_install_handlers(struct acpi_ec
*ec, struct acpi_device *device)
                         * fatal, because the EC can be polled for events.
                         */
                } else {
-                       ready = install_gpio_irq_event_handler(ec, device);
-                       if (!ready)
-                               return -ENXIO;
+                       pr_err("%s: install_gpio_irq_event_handler\n",
__func__);
+                       int ret = install_gpio_irq_event_handler(ec, device);
+                       if (ret)
+                               return ret;
+                       ready = true;
                }
                if (ready) {
                        set_bit(EC_FLAGS_EVENT_HANDLER_INSTALLED, &ec->flags);

The dmesg shows:

[    0.121117] ACPI: EC: ec_install_handlers: install_gpio_irq_event_handler
[    0.121133] ACPI: EC: install_gpio_irq_event_handler:
acpi_dev_gpio_irq_get returns -517

Originally, ec_install_handlers() will return the returned value from
install_gpio_irq_event_handler() from acpi_dev_gpio_irq_get(), which
is -EPROBE_DEFER, instead of -ENXIO.  However, ec_install_handlers()
returns -ENXIO directly if install_gpio_irq_event_handler() returns
false in patch ("ACPI: EC: Consolidate event handler installation
code").  Here needs some modification.

Jian-Hong Pan

^ permalink raw reply related	[flat|nested] 24+ messages in thread

* Re: [PATCH 0/6] ACPI: EC: Updates related to initialization
  2020-03-03  7:28         ` Jian-Hong Pan
@ 2020-03-03  9:09           ` Rafael J. Wysocki
  2020-03-03 22:23             ` Rafael J. Wysocki
  0 siblings, 1 reply; 24+ messages in thread
From: Rafael J. Wysocki @ 2020-03-03  9:09 UTC (permalink / raw)
  To: Jian-Hong Pan
  Cc: Rafael J. Wysocki, Rafael J. Wysocki, Daniel Drake, Linux ACPI,
	LKML, Linux Upstreaming Team

On Tue, Mar 3, 2020 at 8:29 AM Jian-Hong Pan <jian-hong@endlessm.com> wrote:
>
> Rafael J. Wysocki <rafael@kernel.org> 於 2020年3月2日 週一 下午7:45寫道:
> >
> > On Mon, Mar 2, 2020 at 11:38 AM Rafael J. Wysocki <rafael@kernel.org> wrote:
> > >
> > > On Mon, Mar 2, 2020 at 6:54 AM Jian-Hong Pan <jian-hong@endlessm.com> wrote:
> > > >
> > > > Daniel Drake <drake@endlessm.com> 於 2020年2月28日 週五 下午5:43寫道:
> > > > >
> > > > > On Thu, Feb 27, 2020 at 10:25 PM Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> > > > > > The purpose of this series of update of the ACPI EC driver is to make its
> > > > > > initialization more straightforward.
> > > > > >
> > > > > > They fix a couple of issues, clean up some things, remove redundant code etc.
> > > > > >
> > > > > > Please refer to the changelogs of individual patches for details.
> > > > > >
> > > > > > For easier access, the series is available in the git branch at
> > > > > >
> > > > > >  git://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git \
> > > > > >  acpi-ec-work
> > > > > >
> > > > > > on top of 5.6-rc3.
> > > > >
> > > > > Jian-Hong, can you please test this on Asus UX434DA?
> > > > > Check if the screen brightness hotkeys are still working after these changes.
> > > >
> > > > Hi Rafael,
> > > >
> > > > Thanks for your patches, but we found an issue:
> > > > The laptops like ASUS UX434DA's screen brightness hotkeys work before
> > > > this patch series.  However, the hotkeys are failed with the patch
> > > > "ACPI: EC: Unify handling of event handler installation failures".
> > >
> > > So I have modified the series to avoid the change that can possibly break this.
> > >
> > > Can you please pull the new series from
> > >
> > >  git://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git \
> > >  acpi-ec-work
> > >
> > > (same branch) and retest?
> >
> > Note that the current top-most commit in that branch is
> >
> > 0957d98f50da ACPI: EC: Consolidate event handler installation code
>
> I tested the commits in acpi-ec-work branch whose last commit is
> 0957d98f50da ("ACPI: EC: Consolidate event handler installation
> code").  The screen brightness hotkeys are still failed with
> 0957d98f50da ("ACPI: EC: Consolidate event handler installation
> code").
>
> I tweak and add some debug messages:
>
> diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
> index 85f1fe8e208a..3887f427283c 100644
> --- a/drivers/acpi/ec.c
> +++ b/drivers/acpi/ec.c
> @@ -1443,23 +1443,27 @@ static bool install_gpe_event_handler(struct
> acpi_ec *ec)
>         return true;
>  }
>
> -static bool install_gpio_irq_event_handler(struct acpi_ec *ec,
> +static int install_gpio_irq_event_handler(struct acpi_ec *ec,
>                                            struct acpi_device *device)
>  {
>         int irq, ret;
>
>         /* ACPI reduced hardware platforms use a GpioInt specified in _CRS. */
>         irq = acpi_dev_gpio_irq_get(device, 0);
> -       if (irq < 0)
> -               return false;
> +       if (irq < 0) {
> +               pr_err("%s: acpi_dev_gpio_irq_get returns %d\n", __func__, irq);
> +               return irq;
> +       }
>
>         ret = request_irq(irq, acpi_ec_irq_handler, IRQF_SHARED, "ACPI EC", ec);
> -       if (ret < 0)
> -               return false;
> +       if (ret < 0) {
> +               pr_err("%s: request_irq returns %d\n", __func__, ret);
> +               return ret;
> +       }
>
>         ec->irq = irq;
>
> -       return true;
> +       return 0;
>  }
>
>  /**
> @@ -1517,9 +1521,11 @@ static int ec_install_handlers(struct acpi_ec
> *ec, struct acpi_device *device)
>                          * fatal, because the EC can be polled for events.
>                          */
>                 } else {
> -                       ready = install_gpio_irq_event_handler(ec, device);
> -                       if (!ready)
> -                               return -ENXIO;
> +                       pr_err("%s: install_gpio_irq_event_handler\n",
> __func__);
> +                       int ret = install_gpio_irq_event_handler(ec, device);
> +                       if (ret)
> +                               return ret;
> +                       ready = true;
>                 }
>                 if (ready) {
>                         set_bit(EC_FLAGS_EVENT_HANDLER_INSTALLED, &ec->flags);
>
> The dmesg shows:
>
> [    0.121117] ACPI: EC: ec_install_handlers: install_gpio_irq_event_handler
> [    0.121133] ACPI: EC: install_gpio_irq_event_handler:
> acpi_dev_gpio_irq_get returns -517
>
> Originally, ec_install_handlers() will return the returned value from
> install_gpio_irq_event_handler() from acpi_dev_gpio_irq_get(), which
> is -EPROBE_DEFER, instead of -ENXIO.  However, ec_install_handlers()
> returns -ENXIO directly if install_gpio_irq_event_handler() returns
> false in patch ("ACPI: EC: Consolidate event handler installation
> code").  Here needs some modification.

Thanks, I forgot about the -EPROBE_DEFER case.

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH 0/6] ACPI: EC: Updates related to initialization
  2020-03-03  9:09           ` Rafael J. Wysocki
@ 2020-03-03 22:23             ` Rafael J. Wysocki
  2020-03-04  2:53               ` Jian-Hong Pan
  0 siblings, 1 reply; 24+ messages in thread
From: Rafael J. Wysocki @ 2020-03-03 22:23 UTC (permalink / raw)
  To: Jian-Hong Pan
  Cc: Rafael J. Wysocki, Rafael J. Wysocki, Daniel Drake, Linux ACPI,
	LKML, Linux Upstreaming Team

On Tue, Mar 3, 2020 at 10:09 AM Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> On Tue, Mar 3, 2020 at 8:29 AM Jian-Hong Pan <jian-hong@endlessm.com> wrote:
> >
> > Rafael J. Wysocki <rafael@kernel.org> 於 2020年3月2日 週一 下午7:45寫道:
> > >

[cut]

> >
> > Originally, ec_install_handlers() will return the returned value from
> > install_gpio_irq_event_handler() from acpi_dev_gpio_irq_get(), which
> > is -EPROBE_DEFER, instead of -ENXIO.  However, ec_install_handlers()
> > returns -ENXIO directly if install_gpio_irq_event_handler() returns
> > false in patch ("ACPI: EC: Consolidate event handler installation
> > code").  Here needs some modification.
>
> Thanks, I forgot about the -EPROBE_DEFER case.

The top-most commit in the git branch at

 git://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git \
 acpi-ec-work

has been updated to take that case into account (I think that it
should be spelled out explicitly or it will be very easy to overlook
in the future).

Please test this one if possible.

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH 0/6] ACPI: EC: Updates related to initialization
  2020-03-03 22:23             ` Rafael J. Wysocki
@ 2020-03-04  2:53               ` Jian-Hong Pan
  2020-03-04  9:13                 ` Rafael J. Wysocki
  0 siblings, 1 reply; 24+ messages in thread
From: Jian-Hong Pan @ 2020-03-04  2:53 UTC (permalink / raw)
  To: Rafael J. Wysocki
  Cc: Rafael J. Wysocki, Daniel Drake, Linux ACPI, LKML,
	Linux Upstreaming Team

Rafael J. Wysocki <rafael@kernel.org> 於 2020年3月4日 週三 上午6:23寫道:
>
> On Tue, Mar 3, 2020 at 10:09 AM Rafael J. Wysocki <rafael@kernel.org> wrote:
> >
> > On Tue, Mar 3, 2020 at 8:29 AM Jian-Hong Pan <jian-hong@endlessm.com> wrote:
> > >
> > > Rafael J. Wysocki <rafael@kernel.org> 於 2020年3月2日 週一 下午7:45寫道:
> > > >
>
> [cut]
>
> > >
> > > Originally, ec_install_handlers() will return the returned value from
> > > install_gpio_irq_event_handler() from acpi_dev_gpio_irq_get(), which
> > > is -EPROBE_DEFER, instead of -ENXIO.  However, ec_install_handlers()
> > > returns -ENXIO directly if install_gpio_irq_event_handler() returns
> > > false in patch ("ACPI: EC: Consolidate event handler installation
> > > code").  Here needs some modification.
> >
> > Thanks, I forgot about the -EPROBE_DEFER case.
>
> The top-most commit in the git branch at
>
>  git://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git \
>  acpi-ec-work
>
> has been updated to take that case into account (I think that it
> should be spelled out explicitly or it will be very easy to overlook
> in the future).
>
> Please test this one if possible.

Tested the commits on some laptops including ASUS UX434DA.  The
brightness hotkeys are working now.

Jian-Hong Pan

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH 0/6] ACPI: EC: Updates related to initialization
  2020-03-04  2:53               ` Jian-Hong Pan
@ 2020-03-04  9:13                 ` Rafael J. Wysocki
  0 siblings, 0 replies; 24+ messages in thread
From: Rafael J. Wysocki @ 2020-03-04  9:13 UTC (permalink / raw)
  To: Jian-Hong Pan
  Cc: Rafael J. Wysocki, Rafael J. Wysocki, Daniel Drake, Linux ACPI,
	LKML, Linux Upstreaming Team

On Wed, Mar 4, 2020 at 3:54 AM Jian-Hong Pan <jian-hong@endlessm.com> wrote:
>
> Rafael J. Wysocki <rafael@kernel.org> 於 2020年3月4日 週三 上午6:23寫道:
> >
> > On Tue, Mar 3, 2020 at 10:09 AM Rafael J. Wysocki <rafael@kernel.org> wrote:
> > >
> > > On Tue, Mar 3, 2020 at 8:29 AM Jian-Hong Pan <jian-hong@endlessm.com> wrote:
> > > >
> > > > Rafael J. Wysocki <rafael@kernel.org> 於 2020年3月2日 週一 下午7:45寫道:
> > > > >
> >
> > [cut]
> >
> > > >
> > > > Originally, ec_install_handlers() will return the returned value from
> > > > install_gpio_irq_event_handler() from acpi_dev_gpio_irq_get(), which
> > > > is -EPROBE_DEFER, instead of -ENXIO.  However, ec_install_handlers()
> > > > returns -ENXIO directly if install_gpio_irq_event_handler() returns
> > > > false in patch ("ACPI: EC: Consolidate event handler installation
> > > > code").  Here needs some modification.
> > >
> > > Thanks, I forgot about the -EPROBE_DEFER case.
> >
> > The top-most commit in the git branch at
> >
> >  git://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git \
> >  acpi-ec-work
> >
> > has been updated to take that case into account (I think that it
> > should be spelled out explicitly or it will be very easy to overlook
> > in the future).
> >
> > Please test this one if possible.
>
> Tested the commits on some laptops including ASUS UX434DA.  The
> brightness hotkeys are working now.

Thank you!

^ permalink raw reply	[flat|nested] 24+ messages in thread

* [PATCH v2 0/6] ACPI: EC: Updates related to initialization
  2020-02-27 22:19 [PATCH 0/6] ACPI: EC: Updates related to initialization Rafael J. Wysocki
                   ` (6 preceding siblings ...)
  2020-02-28  9:43 ` [PATCH 0/6] ACPI: EC: Updates related to initialization Daniel Drake
@ 2020-03-04 10:42 ` Rafael J. Wysocki
  2020-03-04 10:44   ` [PATCH v2 1/6] ACPI: EC: Avoid printing confusing messages in acpi_ec_setup() Rafael J. Wysocki
                     ` (5 more replies)
  7 siblings, 6 replies; 24+ messages in thread
From: Rafael J. Wysocki @ 2020-03-04 10:42 UTC (permalink / raw)
  To: Rafael J. Wysocki
  Cc: Linux ACPI, LKML, Rafael J. Wysocki, Daniel Drake, Jian-Hong Pan

Hi All,

On Thursday, February 27, 2020 11:19:19 PM CET Rafael J. Wysocki wrote:
> 
> The purpose of this series of update of the ACPI EC driver is to make its
> initialization more straightforward.
> 
> They fix a couple of issues, clean up some things, remove redundant code etc.
> 
> Please refer to the changelogs of individual patches for details.
> 
> For easier access, the series is available in the git branch at
> 
>  git://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git \
>  acpi-ec-work
> 
> on top of 5.6-rc3.

The above is still true, including the location of the git branch containing
the changes.

Since the original submission, the series has been rearranged to reduce
artificial dependencies between the patches and the last patch (previously
[4/6]) has been reworked to take deferred probing into account properly.

Thanks!




^ permalink raw reply	[flat|nested] 24+ messages in thread

* [PATCH v2 1/6] ACPI: EC: Avoid printing confusing messages in acpi_ec_setup()
  2020-03-04 10:42 ` [PATCH v2 " Rafael J. Wysocki
@ 2020-03-04 10:44   ` Rafael J. Wysocki
  2020-03-04 10:45   ` [PATCH v2 2/6] ACPI: EC: Avoid passing redundant argument to functions Rafael J. Wysocki
                     ` (4 subsequent siblings)
  5 siblings, 0 replies; 24+ messages in thread
From: Rafael J. Wysocki @ 2020-03-04 10:44 UTC (permalink / raw)
  To: Linux ACPI; +Cc: LKML, Rafael J. Wysocki, Daniel Drake, Jian-Hong Pan

From: "Rafael J. Wysocki" <rafael.j.wysocki@intel.com>

It doesn't really make sense to pass ec->handle of the ECDT EC to
acpi_handle_info(), because it is set to ACPI_ROOT_OBJECT in
acpi_ec_ecdt_probe(), so rework acpi_ec_setup() to avoid using
acpi_handle_info() for printing messages.

First, notice that the "Used as first EC" message is not really
useful, because it is immediately followed by a more meaningful one
from either acpi_ec_ecdt_probe() or acpi_ec_dsdt_probe() (the latter
also includes the EC object path), so drop it altogether.

Second, use pr_info() for printing the EC configuration information.

While at it, make the code in question avoid printing invalid GPE or
IRQ numbers and make it print the GPE/IRQ information only when the
driver is ready to handle events.

Fixes: 72c77b7ea9ce ("ACPI / EC: Cleanup first_ec/boot_ec code")
Fixes: 406857f773b0 ("ACPI: EC: add support for hardware-reduced systems")
Cc: 5.5+ <stable@vger.kernel.org> # 5.5+
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---

-> v2: No changes.

---
 drivers/acpi/ec.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
index d1f1cf5d4bf0..2dc7cf2aeb21 100644
--- a/drivers/acpi/ec.c
+++ b/drivers/acpi/ec.c
@@ -1584,14 +1584,19 @@ static int acpi_ec_setup(struct acpi_ec *ec, struct acpi_device *device,
 		return ret;
 
 	/* First EC capable of handling transactions */
-	if (!first_ec) {
+	if (!first_ec)
 		first_ec = ec;
-		acpi_handle_info(first_ec->handle, "Used as first EC\n");
+
+	pr_info("EC_CMD/EC_SC=0x%lx, EC_DATA=0x%lx\n", ec->command_addr,
+		ec->data_addr);
+
+	if (test_bit(EC_FLAGS_EVENT_HANDLER_INSTALLED, &ec->flags)) {
+		if (ec->gpe >= 0)
+			pr_info("GPE=0x%x\n", ec->gpe);
+		else
+			pr_info("IRQ=%d\n", ec->irq);
 	}
 
-	acpi_handle_info(ec->handle,
-			 "GPE=0x%x, IRQ=%d, EC_CMD/EC_SC=0x%lx, EC_DATA=0x%lx\n",
-			 ec->gpe, ec->irq, ec->command_addr, ec->data_addr);
 	return ret;
 }
 
-- 
2.16.4





^ permalink raw reply related	[flat|nested] 24+ messages in thread

* [PATCH v2 2/6] ACPI: EC: Avoid passing redundant argument to functions
  2020-03-04 10:42 ` [PATCH v2 " Rafael J. Wysocki
  2020-03-04 10:44   ` [PATCH v2 1/6] ACPI: EC: Avoid printing confusing messages in acpi_ec_setup() Rafael J. Wysocki
@ 2020-03-04 10:45   ` Rafael J. Wysocki
  2020-03-04 10:46   ` [PATCH v2 3/6] ACPI: EC: Drop AE_NOT_FOUND special case from ec_install_handlers() Rafael J. Wysocki
                     ` (3 subsequent siblings)
  5 siblings, 0 replies; 24+ messages in thread
From: Rafael J. Wysocki @ 2020-03-04 10:45 UTC (permalink / raw)
  To: Linux ACPI; +Cc: LKML, Rafael J. Wysocki, Daniel Drake, Jian-Hong Pan

From: "Rafael J. Wysocki" <rafael.j.wysocki@intel.com>

After commit 406857f773b0 ("ACPI: EC: add support for hardware-reduced
systems") the handle_events argument passed to ec_install_handlers()
and acpi_ec_setup() is redundant, because it is always 'false' when
the device argument passed to them in NULL and it is always 'true'
otherwise, so the device argument can be tested against NULL instead
of testing the handle_events one.

Accordingly, modify ec_install_handlers() and acpi_ec_setup() to take
two arguments and reduce the number of checks in the former.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---

-> v2: No changes.

---
 drivers/acpi/ec.c | 21 ++++++++-------------
 1 file changed, 8 insertions(+), 13 deletions(-)

diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
index 2dc7cf2aeb21..3153e7684053 100644
--- a/drivers/acpi/ec.c
+++ b/drivers/acpi/ec.c
@@ -1476,8 +1476,7 @@ static int install_gpio_irq_event_handler(struct acpi_ec *ec,
  *       handler is not installed, which means "not able to handle
  *       transactions".
  */
-static int ec_install_handlers(struct acpi_ec *ec, struct acpi_device *device,
-			       bool handle_events)
+static int ec_install_handlers(struct acpi_ec *ec, struct acpi_device *device)
 {
 	acpi_status status;
 
@@ -1507,7 +1506,7 @@ static int ec_install_handlers(struct acpi_ec *ec, struct acpi_device *device,
 		set_bit(EC_FLAGS_EC_HANDLER_INSTALLED, &ec->flags);
 	}
 
-	if (!handle_events)
+	if (!device)
 		return 0;
 
 	if (!test_bit(EC_FLAGS_QUERY_METHODS_INSTALLED, &ec->flags)) {
@@ -1520,13 +1519,10 @@ static int ec_install_handlers(struct acpi_ec *ec, struct acpi_device *device,
 	if (!test_bit(EC_FLAGS_EVENT_HANDLER_INSTALLED, &ec->flags)) {
 		if (ec->gpe >= 0) {
 			install_gpe_event_handler(ec);
-		} else if (device) {
+		} else {
 			int ret = install_gpio_irq_event_handler(ec, device);
-
 			if (ret)
 				return ret;
-		} else { /* No GPE and no GpioInt? */
-			return -ENODEV;
 		}
 	}
 	/* EC is fully operational, allow queries */
@@ -1574,12 +1570,11 @@ static void ec_remove_handlers(struct acpi_ec *ec)
 	}
 }
 
-static int acpi_ec_setup(struct acpi_ec *ec, struct acpi_device *device,
-			 bool handle_events)
+static int acpi_ec_setup(struct acpi_ec *ec, struct acpi_device *device)
 {
 	int ret;
 
-	ret = ec_install_handlers(ec, device, handle_events);
+	ret = ec_install_handlers(ec, device);
 	if (ret)
 		return ret;
 
@@ -1660,7 +1655,7 @@ static int acpi_ec_add(struct acpi_device *device)
 		}
 	}
 
-	ret = acpi_ec_setup(ec, device, true);
+	ret = acpi_ec_setup(ec, device);
 	if (ret)
 		goto err_query;
 
@@ -1780,7 +1775,7 @@ void __init acpi_ec_dsdt_probe(void)
 	 * At this point, the GPE is not fully initialized, so do not to
 	 * handle the events.
 	 */
-	ret = acpi_ec_setup(ec, NULL, false);
+	ret = acpi_ec_setup(ec, NULL);
 	if (ret) {
 		acpi_ec_free(ec);
 		return;
@@ -1967,7 +1962,7 @@ void __init acpi_ec_ecdt_probe(void)
 	 * At this point, the namespace is not initialized, so do not find
 	 * the namespace objects, or handle the events.
 	 */
-	ret = acpi_ec_setup(ec, NULL, false);
+	ret = acpi_ec_setup(ec, NULL);
 	if (ret) {
 		acpi_ec_free(ec);
 		return;
-- 
2.16.4






^ permalink raw reply related	[flat|nested] 24+ messages in thread

* [PATCH v2 3/6] ACPI: EC: Drop AE_NOT_FOUND special case from ec_install_handlers()
  2020-03-04 10:42 ` [PATCH v2 " Rafael J. Wysocki
  2020-03-04 10:44   ` [PATCH v2 1/6] ACPI: EC: Avoid printing confusing messages in acpi_ec_setup() Rafael J. Wysocki
  2020-03-04 10:45   ` [PATCH v2 2/6] ACPI: EC: Avoid passing redundant argument to functions Rafael J. Wysocki
@ 2020-03-04 10:46   ` Rafael J. Wysocki
  2020-03-04 10:48   ` [PATCH v2 4/6] ACPI: EC: Simplify acpi_ec_add() Rafael J. Wysocki
                     ` (2 subsequent siblings)
  5 siblings, 0 replies; 24+ messages in thread
From: Rafael J. Wysocki @ 2020-03-04 10:46 UTC (permalink / raw)
  To: Linux ACPI; +Cc: LKML, Rafael J. Wysocki, Daniel Drake, Jian-Hong Pan

From: "Rafael J. Wysocki" <rafael.j.wysocki@intel.com>

If the status value returned by acpi_install_address_space_handler()
in ec_install_handlers() is AE_NOT_FOUND, it is treated in a special
way, apparently because it might mean a _REG method evaluation
failure (at least that is the case according to the comment in
there), but acpi_install_address_space_handler() does not take
_REG evaluation errors into account at all, so the AE_NOT_FOUND
special handling is confusing at best.

For this reason, change ec_install_handlers() to stop the EC and
return -ENODEV on all acpi_install_address_space_handler() errors.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---

-> v2: No changes.

---
 drivers/acpi/ec.c | 15 ++-------------
 1 file changed, 2 insertions(+), 13 deletions(-)

diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
index 3153e7684053..6f501d552e6e 100644
--- a/drivers/acpi/ec.c
+++ b/drivers/acpi/ec.c
@@ -1489,19 +1489,8 @@ static int ec_install_handlers(struct acpi_ec *ec, struct acpi_device *device)
 							    &acpi_ec_space_handler,
 							    NULL, ec);
 		if (ACPI_FAILURE(status)) {
-			if (status == AE_NOT_FOUND) {
-				/*
-				 * Maybe OS fails in evaluating the _REG
-				 * object. The AE_NOT_FOUND error will be
-				 * ignored and OS * continue to initialize
-				 * EC.
-				 */
-				pr_err("Fail in evaluating the _REG object"
-					" of EC device. Broken bios is suspected.\n");
-			} else {
-				acpi_ec_stop(ec, false);
-				return -ENODEV;
-			}
+			acpi_ec_stop(ec, false);
+			return -ENODEV;
 		}
 		set_bit(EC_FLAGS_EC_HANDLER_INSTALLED, &ec->flags);
 	}
-- 
2.16.4






^ permalink raw reply related	[flat|nested] 24+ messages in thread

* [PATCH v2 4/6] ACPI: EC: Simplify acpi_ec_add()
  2020-03-04 10:42 ` [PATCH v2 " Rafael J. Wysocki
                     ` (2 preceding siblings ...)
  2020-03-04 10:46   ` [PATCH v2 3/6] ACPI: EC: Drop AE_NOT_FOUND special case from ec_install_handlers() Rafael J. Wysocki
@ 2020-03-04 10:48   ` Rafael J. Wysocki
  2020-03-04 10:49   ` [PATCH v2 5/6] ACPI: EC: Use fast path in acpi_ec_add() for DSDT boot EC Rafael J. Wysocki
  2020-03-04 10:52   ` [PATCH v2 6/6] ACPI: EC: Consolidate event handler installation code Rafael J. Wysocki
  5 siblings, 0 replies; 24+ messages in thread
From: Rafael J. Wysocki @ 2020-03-04 10:48 UTC (permalink / raw)
  To: Linux ACPI; +Cc: LKML, Rafael J. Wysocki, Daniel Drake, Jian-Hong Pan

From: "Rafael J. Wysocki" <rafael.j.wysocki@intel.com>

First, notice that if the device ID in acpi_ec_add() is equal to
ACPI_ECDT_HID, boot_ec_is_ecdt must be set, because this means
that the device object passed to acpi_ec_add() comes from
acpi_ec_ecdt_start() which fails if boot_ec_is_ecdt is unset.
Accordingly, boot_ec_is_ecdt need not be set again in that case,
so drop that redundant update of it from the code.

Next, ec->handle must be a valid ACPI handle right before
returning 0 from acpi_ec_add(), because it either is the handle
of the device object passed to that function, or it is the boot EC
handle coming from acpi_ec_ecdt_start() which fails if it cannot
find a valid handle for the boot EC.  Moreover, the object with
that handle is regarded as a valid representation of the EC in all
cases, so there is no reason to avoid the _DEP list update walk if
that handle is the boot EC handle.  Accordingly, drop the dep_update
local variable from acpi_ec_add() and call acpi_walk_dep_device_list()
for ec->handle unconditionally before returning 0 from it.

Finally, the ec local variable in acpi_ec_add() need not be
initialized to NULL and the status local variable declaration
can be moved to the block in which it is used, so change the code
in accordance with these observations.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---

-> v2: Reorder (previously [5/6]) and rebase.

---
 drivers/acpi/ec.c | 16 +++++++---------
 1 file changed, 7 insertions(+), 9 deletions(-)

diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
index 6f501d552e6e..116163add41b 100644
--- a/drivers/acpi/ec.c
+++ b/drivers/acpi/ec.c
@@ -1605,19 +1605,18 @@ static bool acpi_ec_ecdt_get_handle(acpi_handle *phandle)
 
 static int acpi_ec_add(struct acpi_device *device)
 {
-	struct acpi_ec *ec = NULL;
-	bool dep_update = true;
-	acpi_status status;
+	struct acpi_ec *ec;
 	int ret;
 
 	strcpy(acpi_device_name(device), ACPI_EC_DEVICE_NAME);
 	strcpy(acpi_device_class(device), ACPI_EC_CLASS);
 
 	if (!strcmp(acpi_device_hid(device), ACPI_ECDT_HID)) {
-		boot_ec_is_ecdt = true;
+		/* Fast path: this device corresponds to the boot EC. */
 		ec = boot_ec;
-		dep_update = false;
 	} else {
+		acpi_status status;
+
 		ec = acpi_ec_alloc();
 		if (!ec)
 			return -ENOMEM;
@@ -1660,10 +1659,9 @@ 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);
 
-	if (dep_update) {
-		/* Reprobe devices depending on the EC */
-		acpi_walk_dep_device_list(ec->handle);
-	}
+	/* Reprobe devices depending on the EC */
+	acpi_walk_dep_device_list(ec->handle);
+
 	acpi_handle_debug(ec->handle, "enumerated.\n");
 	return 0;
 
-- 
2.16.4





^ permalink raw reply related	[flat|nested] 24+ messages in thread

* [PATCH v2 5/6] ACPI: EC: Use fast path in acpi_ec_add() for DSDT boot EC
  2020-03-04 10:42 ` [PATCH v2 " Rafael J. Wysocki
                     ` (3 preceding siblings ...)
  2020-03-04 10:48   ` [PATCH v2 4/6] ACPI: EC: Simplify acpi_ec_add() Rafael J. Wysocki
@ 2020-03-04 10:49   ` Rafael J. Wysocki
  2020-03-04 10:52   ` [PATCH v2 6/6] ACPI: EC: Consolidate event handler installation code Rafael J. Wysocki
  5 siblings, 0 replies; 24+ messages in thread
From: Rafael J. Wysocki @ 2020-03-04 10:49 UTC (permalink / raw)
  To: Linux ACPI; +Cc: LKML, Rafael J. Wysocki, Daniel Drake, Jian-Hong Pan

From: "Rafael J. Wysocki" <rafael.j.wysocki@intel.com>

If the boot EC comes from the DSDT, its ACPI handle is equal to the
handle of a device object with the PNP0C09 device ID.  If that
device object is passed to acpi_ec_add(), it is not necessary to
allocate a new EC structure for it and parse it, because that has
been done already, so change the function to use the fast path in
that case.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---

-> v2: Reorder (previously [6/6]) and rebase.

---
 drivers/acpi/ec.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
index 116163add41b..355d6973cb70 100644
--- a/drivers/acpi/ec.c
+++ b/drivers/acpi/ec.c
@@ -1611,7 +1611,8 @@ static int acpi_ec_add(struct acpi_device *device)
 	strcpy(acpi_device_name(device), ACPI_EC_DEVICE_NAME);
 	strcpy(acpi_device_class(device), ACPI_EC_CLASS);
 
-	if (!strcmp(acpi_device_hid(device), ACPI_ECDT_HID)) {
+	if ((boot_ec && boot_ec->handle == device->handle) ||
+	    !strcmp(acpi_device_hid(device), ACPI_ECDT_HID)) {
 		/* Fast path: this device corresponds to the boot EC. */
 		ec = boot_ec;
 	} else {
-- 
2.16.4





^ permalink raw reply related	[flat|nested] 24+ messages in thread

* [PATCH v2 6/6] ACPI: EC: Consolidate event handler installation code
  2020-03-04 10:42 ` [PATCH v2 " Rafael J. Wysocki
                     ` (4 preceding siblings ...)
  2020-03-04 10:49   ` [PATCH v2 5/6] ACPI: EC: Use fast path in acpi_ec_add() for DSDT boot EC Rafael J. Wysocki
@ 2020-03-04 10:52   ` Rafael J. Wysocki
  5 siblings, 0 replies; 24+ messages in thread
From: Rafael J. Wysocki @ 2020-03-04 10:52 UTC (permalink / raw)
  To: Linux ACPI; +Cc: LKML, Rafael J. Wysocki, Daniel Drake, Jian-Hong Pan

From: "Rafael J. Wysocki" <rafael.j.wysocki@intel.com>

Commit 406857f773b0 ("ACPI: EC: add support for hardware-reduced
systems") made ec_install_handlers() return an error on failures
to configure a GPIO IRQ for the EC, but that is inconsistent with
the handling of the GPE event handler installation failures even
though it is exactly the same issue and the driver can respond to
it in the same way in both cases (the EC can be actively polled
for events through its registers if the event handler installation
fails).

Moreover, it requires acpi_ec_add() to take that special case into
account and disagrees with the ec_install_handlers() header comment.

For this reason, rework the event handler installation code in
ec_install_handlers() to explicitly take deferred probing (that
may be needed in the GPIO IRQ case) into account and to avoid
failing the EC initialization in any other case.

Among other things, reduce code duplication between
install_gpe_event_handler() and install_gpio_irq_event_handler() by
moving some code from there into ec_install_handlers() itself and
simplify the error code path in acpi_ec_add().

While at it, turn the ec_install_handlers() header comment into
a proper kerneldoc one and add some general control flow information
to it.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Tested-by: Jian-Hong Pan <jian-hong@endlessm.com>
---

-> v2:
   * Reorder (previously [4/6]) and rebase.
   * Take possible deferred GPIO IRQ acquisition into account.

---
 drivers/acpi/ec.c | 114 +++++++++++++++++++++++++++++-------------------------
 1 file changed, 61 insertions(+), 53 deletions(-)

diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
index 355d6973cb70..47baeec9190d 100644
--- a/drivers/acpi/ec.c
+++ b/drivers/acpi/ec.c
@@ -1427,54 +1427,43 @@ ec_parse_device(acpi_handle handle, u32 Level, void *context, void **retval)
 	return AE_CTRL_TERMINATE;
 }
 
-static void install_gpe_event_handler(struct acpi_ec *ec)
-{
-	acpi_status status =
-		acpi_install_gpe_raw_handler(NULL, ec->gpe,
-					     ACPI_GPE_EDGE_TRIGGERED,
-					     &acpi_ec_gpe_handler,
-					     ec);
-	if (ACPI_SUCCESS(status)) {
-		/* This is not fatal as we can poll EC events */
-		set_bit(EC_FLAGS_EVENT_HANDLER_INSTALLED, &ec->flags);
-		acpi_ec_leave_noirq(ec);
-		if (test_bit(EC_FLAGS_STARTED, &ec->flags) &&
-		    ec->reference_count >= 1)
-			acpi_ec_enable_gpe(ec, true);
-	}
-}
-
-/* ACPI reduced hardware platforms use a GpioInt specified in _CRS. */
-static int install_gpio_irq_event_handler(struct acpi_ec *ec,
-					  struct acpi_device *device)
+static bool install_gpe_event_handler(struct acpi_ec *ec)
 {
-	int irq = acpi_dev_gpio_irq_get(device, 0);
-	int ret;
-
-	if (irq < 0)
-		return irq;
+	acpi_status status;
 
-	ret = request_irq(irq, acpi_ec_irq_handler, IRQF_SHARED,
-			  "ACPI EC", ec);
+	status = acpi_install_gpe_raw_handler(NULL, ec->gpe,
+					      ACPI_GPE_EDGE_TRIGGERED,
+					      &acpi_ec_gpe_handler, ec);
+	if (ACPI_FAILURE(status))
+		return false;
 
-	/*
-	 * Unlike the GPE case, we treat errors here as fatal, we'll only
-	 * implement GPIO polling if we find a case that needs it.
-	 */
-	if (ret < 0)
-		return ret;
+	if (test_bit(EC_FLAGS_STARTED, &ec->flags) && ec->reference_count >= 1)
+		acpi_ec_enable_gpe(ec, true);
 
-	ec->irq = irq;
-	set_bit(EC_FLAGS_EVENT_HANDLER_INSTALLED, &ec->flags);
-	acpi_ec_leave_noirq(ec);
+	return true;
+}
 
-	return 0;
+static bool install_gpio_irq_event_handler(struct acpi_ec *ec)
+{
+	return request_irq(ec->irq, acpi_ec_irq_handler, IRQF_SHARED,
+			   "ACPI EC", ec) >= 0;
 }
 
-/*
- * Note: This function returns an error code only when the address space
- *       handler is not installed, which means "not able to handle
- *       transactions".
+/**
+ * ec_install_handlers - Install service callbacks and register query methods.
+ * @ec: Target EC.
+ * @device: ACPI device object corresponding to @ec.
+ *
+ * Install a handler for the EC address space type unless it has been installed
+ * already.  If @device is not NULL, also look for EC query methods in the
+ * namespace and register them, and install an event (either GPE or GPIO IRQ)
+ * handler for the EC, if possible.
+ *
+ * Return:
+ * -ENODEV if the address space handler cannot be installed, which means
+ *  "unable to handle transactions",
+ * -EPROBE_DEFER if GPIO IRQ acquisition needs to be deferred,
+ * or 0 (success) otherwise.
  */
 static int ec_install_handlers(struct acpi_ec *ec, struct acpi_device *device)
 {
@@ -1498,6 +1487,19 @@ static int ec_install_handlers(struct acpi_ec *ec, struct acpi_device *device)
 	if (!device)
 		return 0;
 
+	if (ec->gpe < 0) {
+		/* ACPI reduced hardware platforms use a GpioInt from _CRS. */
+		int irq = acpi_dev_gpio_irq_get(device, 0);
+		/*
+		 * Bail out right away for deferred probing or complete the
+		 * initialization regardless of any other errors.
+		 */
+		if (irq == -EPROBE_DEFER)
+			return -EPROBE_DEFER;
+		else if (irq >= 0)
+			ec->irq = irq;
+	}
+
 	if (!test_bit(EC_FLAGS_QUERY_METHODS_INSTALLED, &ec->flags)) {
 		/* Find and register all query methods */
 		acpi_walk_namespace(ACPI_TYPE_METHOD, ec->handle, 1,
@@ -1506,13 +1508,21 @@ static int ec_install_handlers(struct acpi_ec *ec, struct acpi_device *device)
 		set_bit(EC_FLAGS_QUERY_METHODS_INSTALLED, &ec->flags);
 	}
 	if (!test_bit(EC_FLAGS_EVENT_HANDLER_INSTALLED, &ec->flags)) {
-		if (ec->gpe >= 0) {
-			install_gpe_event_handler(ec);
-		} else {
-			int ret = install_gpio_irq_event_handler(ec, device);
-			if (ret)
-				return ret;
+		bool ready = false;
+
+		if (ec->gpe >= 0)
+			ready = install_gpe_event_handler(ec);
+		else if (ec->irq >= 0)
+			ready = install_gpio_irq_event_handler(ec);
+
+		if (ready) {
+			set_bit(EC_FLAGS_EVENT_HANDLER_INSTALLED, &ec->flags);
+			acpi_ec_leave_noirq(ec);
 		}
+		/*
+		 * Failures to install an event handler are not fatal, because
+		 * the EC can be polled for events.
+		 */
 	}
 	/* EC is fully operational, allow queries */
 	acpi_ec_enable_event(ec);
@@ -1625,7 +1635,7 @@ static int acpi_ec_add(struct acpi_device *device)
 		status = ec_parse_device(device->handle, 0, ec, NULL);
 		if (status != AE_CTRL_TERMINATE) {
 			ret = -EINVAL;
-			goto err_alloc;
+			goto err;
 		}
 
 		if (boot_ec && ec->command_addr == boot_ec->command_addr &&
@@ -1646,7 +1656,7 @@ static int acpi_ec_add(struct acpi_device *device)
 
 	ret = acpi_ec_setup(ec, device);
 	if (ret)
-		goto err_query;
+		goto err;
 
 	if (ec == boot_ec)
 		acpi_handle_info(boot_ec->handle,
@@ -1666,12 +1676,10 @@ static int acpi_ec_add(struct acpi_device *device)
 	acpi_handle_debug(ec->handle, "enumerated.\n");
 	return 0;
 
-err_query:
-	if (ec != boot_ec)
-		acpi_ec_remove_query_handlers(ec, true, 0);
-err_alloc:
+err:
 	if (ec != boot_ec)
 		acpi_ec_free(ec);
+
 	return ret;
 }
 
-- 
2.16.4





^ permalink raw reply related	[flat|nested] 24+ messages in thread

end of thread, other threads:[~2020-03-04 10:53 UTC | newest]

Thread overview: 24+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-02-27 22:19 [PATCH 0/6] ACPI: EC: Updates related to initialization Rafael J. Wysocki
2020-02-27 22:21 ` [PATCH 1/6] ACPI: EC: Avoid printing confusing messages in acpi_ec_setup() Rafael J. Wysocki
2020-02-27 22:21 ` [PATCH 2/6] ACPI: EC: Avoid passing redundant argument to functions Rafael J. Wysocki
2020-02-27 22:22 ` [PATCH 3/6] ACPI: EC: Drop AE_NOT_FOUND special case from ec_install_handlers() Rafael J. Wysocki
2020-02-27 22:23 ` [PATCH 4/6] ACPI: EC: Unify handling of event handler installation failures Rafael J. Wysocki
2020-02-27 22:23 ` [PATCH 5/6] ACPI: EC: Simplify acpi_ec_add() Rafael J. Wysocki
2020-02-27 22:24 ` [PATCH 6/6] ACPI: EC: Use fast path in acpi_ec_add() for DSDT boot EC Rafael J. Wysocki
2020-02-28  9:43 ` [PATCH 0/6] ACPI: EC: Updates related to initialization Daniel Drake
2020-03-02  5:53   ` Jian-Hong Pan
2020-03-02  9:55     ` Rafael J. Wysocki
2020-03-02 10:38     ` Rafael J. Wysocki
2020-03-02 11:45       ` Rafael J. Wysocki
2020-03-03  7:28         ` Jian-Hong Pan
2020-03-03  9:09           ` Rafael J. Wysocki
2020-03-03 22:23             ` Rafael J. Wysocki
2020-03-04  2:53               ` Jian-Hong Pan
2020-03-04  9:13                 ` Rafael J. Wysocki
2020-03-04 10:42 ` [PATCH v2 " Rafael J. Wysocki
2020-03-04 10:44   ` [PATCH v2 1/6] ACPI: EC: Avoid printing confusing messages in acpi_ec_setup() Rafael J. Wysocki
2020-03-04 10:45   ` [PATCH v2 2/6] ACPI: EC: Avoid passing redundant argument to functions Rafael J. Wysocki
2020-03-04 10:46   ` [PATCH v2 3/6] ACPI: EC: Drop AE_NOT_FOUND special case from ec_install_handlers() Rafael J. Wysocki
2020-03-04 10:48   ` [PATCH v2 4/6] ACPI: EC: Simplify acpi_ec_add() Rafael J. Wysocki
2020-03-04 10:49   ` [PATCH v2 5/6] ACPI: EC: Use fast path in acpi_ec_add() for DSDT boot EC Rafael J. Wysocki
2020-03-04 10:52   ` [PATCH v2 6/6] ACPI: EC: Consolidate event handler installation code Rafael J. Wysocki

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).