| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Thu, 5 Jan 2017 14:43:03 +0000
From: Lee Jones <lee.jones@...aro.org>
To: Thierry Escande <thierry.escande@...labora.com>,
"Rafael J. Wysocki" <rjw@...ysocki.net>
Cc: Benson Leung <bleung@...omium.org>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 3/3] mfd: cros_ec: Add ACPI GPE handler for LID0 devices
Rafael,
On Thu, 05 Jan 2017, Thierry Escande wrote:
> Hi Lee,
>
> On 05/01/2017 08:54, Lee Jones wrote:
> > On Wed, 04 Jan 2017, Thierry Escande wrote:
> >
> > > Hi Lee,
> > >
> > > On 04/01/2017 10:06, Lee Jones wrote:
> > > > On Fri, 16 Dec 2016, Thierry Escande wrote:
> > > >
> > > > > From: Archana Patni <archana.patni@...el.com>
> > > > >
> > > > > This patch installs an ACPI GPE handler for LID0 ACPI device to indicate
> > > > > ACPI core that this GPE should stay enabled for lid to work in suspend
> > > > > to idle path.
> > > > >
> > > > > Signed-off-by: Archana Patni <archana.patni@...el.com>
> > > > > Signed-off-by: Thierry Escande <thierry.escande@...labora.com>
> > > > > ---
> > > > > drivers/mfd/cros_ec.c | 97 +++++++++++++++++++++++++++++++++++++++++++++++++--
> > > > > 1 file changed, 95 insertions(+), 2 deletions(-)
> > > > >
> > > > > diff --git a/drivers/mfd/cros_ec.c b/drivers/mfd/cros_ec.c
> > > > > index b8a5080..628718e 100644
> > > > > --- a/drivers/mfd/cros_ec.c
> > > > > +++ b/drivers/mfd/cros_ec.c
> > > > > @@ -17,6 +17,9 @@
> > > > > * battery charging and regulator control, firmware update.
> > > > > */
> > > > >
> > > > > +#ifdef CONFIG_ACPI
> > > > > +#include <linux/acpi.h>
> > > > > +#endif
> > > >
> > > > Please don't place #ifery in C files.
> > > >
> > > > > #include <linux/of_platform.h>
> > > > > #include <linux/interrupt.h>
> > > > > #include <linux/slab.h>
> > > > > @@ -29,6 +32,73 @@
> > > > > #define CROS_EC_DEV_EC_INDEX 0
> > > > > #define CROS_EC_DEV_PD_INDEX 1
> > > > >
> > > > > +#ifdef CONFIG_ACPI
> > > >
> > > > Remove this.
> > > >
> > > > > +#define ACPI_LID_DEVICE "LID0"
> > > > > +
> > > > > +static int ec_wake_gpe = -EINVAL;
> > > > > +
> > > > > +/*
> > > > > + * This handler indicates to ACPI core that this GPE should stay enabled for
> > > > > + * lid to work in suspend to idle path.
> > > > > + */
> > > > > +static u32 cros_ec_gpe_handler(acpi_handle gpe_device, u32 gpe_number,
> > > > > + void *data)
> > > > > +{
> > > > > + return ACPI_INTERRUPT_HANDLED | ACPI_REENABLE_GPE;
> > > > > +}
> > > > > +
> > > > > +/*
> > > > > + * Get ACPI GPE for LID0 device.
> > > > > + */
> > > > > +static int cros_ec_get_ec_wake_gpe(struct device *dev)
> > > > > +{
> > > If it's ok for you, I'll keep one #ifdef CONFIG_ACPI around the body of this
> > > function. Otherwise it won't compile if CONFIG_ACPI is not set.
> >
> > Can you try placing:
> >
> > if (IS_ENABLED(CONFIG_ACPI))
> >
> > ... before the call to cros_ec_get_ec_wake_gpe() and see if the
> > compiler will do-the-right-thing please?
>
> With CONFIG_ACPI not defined, the acpi_dev structure is not defined as well
> as the acpi_bus_get_device() API. So no, the compiler is not smart enough
> even if the function body is skipped by the test:
> if (!IS_ENABLED(CONFIG_ACPI))
> return -ENODEV;
Is there any way we can make ACPI play nicely if it's not enabled?
I'm thinking stubs. Like we do for most other things.
> > > > > + struct acpi_device *cros_acpi_dev;
> > > > > + struct acpi_device *adev;
> > > > > + acpi_handle handle;
> > > > > + acpi_status status;
> > > > > + int ret;
> > > > > +
> > > > > + cros_acpi_dev = ACPI_COMPANION(dev);
> > > > > +
> > > > > + if (!cros_acpi_dev || !cros_acpi_dev->parent ||
> > > > > + !cros_acpi_dev->parent->handle)
> > > > > + return -EINVAL;
> > > > > +
> > > > > + status = acpi_get_handle(cros_acpi_dev->parent->handle, ACPI_LID_DEVICE,
> > > > > + &handle);
> > > > > + if (ACPI_FAILURE(status))
> > > > > + return -EINVAL;
> > > > > +
> > > > > + ret = acpi_bus_get_device(handle, &adev);
> > > > > + if (ret)
> > > > > + return ret;
> > > > > +
> > > > > + return adev->wakeup.gpe_number;
> > > > > +}
> > > > > +
> > > > > +static int cros_ec_install_handler(struct device *dev)
> > > > > +{
> > > > > + acpi_status status;
> > > > > +
> > > > > + ec_wake_gpe = cros_ec_get_ec_wake_gpe(dev);
> > > > > +
> > > > > + if (ec_wake_gpe < 0)
> > > > > + return ec_wake_gpe;
> > > > > +
> > > > > + status = acpi_install_gpe_handler(NULL, ec_wake_gpe,
> > > > > + ACPI_GPE_EDGE_TRIGGERED,
> > > > > + &cros_ec_gpe_handler, NULL);
> > > > > + if (ACPI_FAILURE(status))
> > > > > + return -ENODEV;
> > > > > +
> > > > > + dev_info(dev, "Initialized, GPE = 0x%x\n", ec_wake_gpe);
> > > > > +
> > > > > + return 0;
> > > > > +}
> > > > > +
> > > > > +#endif
> > > > > +
> > > > > static struct cros_ec_platform ec_p = {
> > > > > .ec_name = CROS_EC_DEV_NAME,
> > > > > .cmd_offset = EC_CMD_PASSTHRU_OFFSET(CROS_EC_DEV_EC_INDEX),
> > > > > @@ -166,6 +236,10 @@ int cros_ec_register(struct cros_ec_device *ec_dev)
> > > > >
> > > > > dev_info(dev, "Chrome EC device registered\n");
> > > > >
> > > > > +#ifdef CONFIG_ACPI
> > > > > + cros_ec_install_handler(dev);
> > > > > +#endif
> > > >
> > > > Here, just do:
> > > >
> > > > if (IS_ENABLED(CONFIG_ACPI))
> > > > cros_ec_install_handler(dev);
> > > >
> > > > And let the compiler take care of the rest.
> > > >
> > > > > return 0;
> > > > >
> > > > > fail_mfd:
> > > > > @@ -179,6 +253,17 @@ int cros_ec_remove(struct cros_ec_device *ec_dev)
> > > > > {
> > > > > mfd_remove_devices(ec_dev->dev);
> > > > >
> > > > > +#ifdef CONFIG_ACPI
> > > > > + if (ec_wake_gpe >= 0) {
> > > >
> > > > if (IS_ENABLED(CONFIG_ACPI) && ec_wake_gpe >= 0) {
> > > >
> > > > > + acpi_status status;
> > > > > +
> > > > > + status = acpi_remove_gpe_handler(NULL, ec_wake_gpe,
> > > > > + &cros_ec_gpe_handler);
> > > > > + if (ACPI_FAILURE(status))
> > > > > + pr_err("failed to remove gpe handler\n");
> > > > > + }
> > > > > +#endif
> > > > > +
> > > > > return 0;
> > > > > }
> > > > > EXPORT_SYMBOL(cros_ec_remove);
> > > > > @@ -190,8 +275,16 @@ int cros_ec_suspend(struct cros_ec_device *ec_dev)
> > > > > int ret;
> > > > > u8 sleep_event;
> > > > >
> > > > > - sleep_event = pm_suspend_via_firmware() ? HOST_SLEEP_EVENT_S3_RESUME :
> > > > > - HOST_SLEEP_EVENT_S0IX_RESUME;
> > > > > + if (!pm_suspend_via_firmware()) {
> > > > > + sleep_event = HOST_SLEEP_EVENT_S0IX_SUSPEND;
> > > > > +#ifdef CONFIG_ACPI
> > > > > + /* Clearing the GPE status for any pending event */
> > > > > + if (ec_wake_gpe >= 0)
> > > >
> > > > As above.
> > > >
> > > > > + acpi_clear_gpe(NULL, ec_wake_gpe);
> > > > > +#endif
> > > > > + } else {
> > > > > + sleep_event = HOST_SLEEP_EVENT_S3_SUSPEND;
> > > > > + }
> > > > >
> > > > > ret = cros_ec_sleep_event(ec_dev, sleep_event);
> > > > > if (ret < 0)
> > > >
> >
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
Powered by blists - more mailing lists