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!
From: "Rafael J. Wysocki" <[email protected]>
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 <[email protected]>
---
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
From: "Rafael J. Wysocki" <[email protected]>
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 <[email protected]>
---
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
From: "Rafael J. Wysocki" <[email protected]>
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 <[email protected]>
---
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
On Thu, Feb 27, 2020 at 10:25 PM Rafael J. Wysocki <[email protected]> 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
Daniel Drake <[email protected]> 於 2020年2月28日 週五 下午5:43寫道：
>
> On Thu, Feb 27, 2020 at 10:25 PM Rafael J. Wysocki <[email protected]> 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
On Mon, Mar 2, 2020 at 6:54 AM Jian-Hong Pan <[email protected]> wrote:
>
> Daniel Drake <[email protected]> 於 2020年2月28日 週五 下午5:43寫道：
> >
> > On Thu, Feb 27, 2020 at 10:25 PM Rafael J. Wysocki <[email protected]> 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!
On Mon, Mar 2, 2020 at 6:54 AM Jian-Hong Pan <[email protected]> wrote:
>
> Daniel Drake <[email protected]> 於 2020年2月28日 週五 下午5:43寫道：
> >
> > On Thu, Feb 27, 2020 at 10:25 PM Rafael J. Wysocki <[email protected]> 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!
On Mon, Mar 2, 2020 at 11:38 AM Rafael J. Wysocki <[email protected]> wrote:
>
> On Mon, Mar 2, 2020 at 6:54 AM Jian-Hong Pan <[email protected]> wrote:
> >
> > Daniel Drake <[email protected]> 於 2020年2月28日 週五 下午5:43寫道：
> > >
> > > On Thu, Feb 27, 2020 at 10:25 PM Rafael J. Wysocki <[email protected]> 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!
Rafael J. Wysocki <[email protected]> 於 2020年3月2日 週一 下午7:45寫道：
>
> On Mon, Mar 2, 2020 at 11:38 AM Rafael J. Wysocki <[email protected]> wrote:
> >
> > On Mon, Mar 2, 2020 at 6:54 AM Jian-Hong Pan <[email protected]> wrote:
> > >
> > > Daniel Drake <[email protected]> 於 2020年2月28日 週五 下午5:43寫道：
> > > >
> > > > On Thu, Feb 27, 2020 at 10:25 PM Rafael J. Wysocki <[email protected]> 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
On Tue, Mar 3, 2020 at 8:29 AM Jian-Hong Pan <[email protected]> wrote:
>
> Rafael J. Wysocki <[email protected]> 於 2020年3月2日 週一 下午7:45寫道：
> >
> > On Mon, Mar 2, 2020 at 11:38 AM Rafael J. Wysocki <[email protected]> wrote:
> > >
> > > On Mon, Mar 2, 2020 at 6:54 AM Jian-Hong Pan <[email protected]> wrote:
> > > >
> > > > Daniel Drake <[email protected]> 於 2020年2月28日 週五 下午5:43寫道：
> > > > >
> > > > > On Thu, Feb 27, 2020 at 10:25 PM Rafael J. Wysocki <[email protected]> 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.
On Tue, Mar 3, 2020 at 10:09 AM Rafael J. Wysocki <[email protected]> wrote:
>
> On Tue, Mar 3, 2020 at 8:29 AM Jian-Hong Pan <[email protected]> wrote:
> >
> > Rafael J. Wysocki <[email protected]> 於 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.
Rafael J. Wysocki <[email protected]> 於 2020年3月4日 週三 上午6:23寫道：
>
> On Tue, Mar 3, 2020 at 10:09 AM Rafael J. Wysocki <[email protected]> wrote:
> >
> > On Tue, Mar 3, 2020 at 8:29 AM Jian-Hong Pan <[email protected]> wrote:
> > >
> > > Rafael J. Wysocki <[email protected]> 於 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
On Wed, Mar 4, 2020 at 3:54 AM Jian-Hong Pan <[email protected]> wrote:
>
> Rafael J. Wysocki <[email protected]> 於 2020年3月4日 週三 上午6:23寫道：
> >
> > On Tue, Mar 3, 2020 at 10:09 AM Rafael J. Wysocki <[email protected]> wrote:
> > >
> > > On Tue, Mar 3, 2020 at 8:29 AM Jian-Hong Pan <[email protected]> wrote:
> > > >
> > > > Rafael J. Wysocki <[email protected]> 於 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!
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!
From: "Rafael J. Wysocki" <[email protected]>
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 <[email protected]>
---
-> 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
From: "Rafael J. Wysocki" <[email protected]>
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 <[email protected]>
---
-> 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
From: "Rafael J. Wysocki" <[email protected]>
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 <[email protected]>
---
-> 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