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: Mon, 13 Feb 2017 13:42:11 +0100 From: Thierry Escande <thierry.escande@...labora.com> To: Lee Jones <lee.jones@...aro.org> Cc: Benson Leung <bleung@...omium.org>, linux-kernel@...r.kernel.org Subject: Re: [PATCH 3/3] cros_ec: Don't signal wake event for non-wake host events Hi Lee, On 07/02/2017 17:12, Lee Jones wrote: > On Wed, 25 Jan 2017, Thierry Escande wrote: > >> From: Shawn Nematbakhsh <shawnn@...omium.org> >> >> The subset of wake-enabled host events is defined by the EC, but the EC >> may still send non-wake host events if we're in the process of >> suspending. Get the mask of wake-enabled host events from the EC and >> filter out non-wake events to prevent spurious aborted suspend >> attempts. >> >> Signed-off-by: Shawn Nematbakhsh <shawnn@...omium.org> >> Signed-off-by: Thierry Escande <thierry.escande@...labora.com> >> --- >> drivers/mfd/cros_ec.c | 14 ++++++-- >> drivers/platform/chrome/cros_ec_proto.c | 60 +++++++++++++++++++++++++++++++++ >> include/linux/mfd/cros_ec.h | 12 +++++++ >> 3 files changed, 84 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/mfd/cros_ec.c b/drivers/mfd/cros_ec.c >> index abd8342..510dfbb 100644 >> --- a/drivers/mfd/cros_ec.c >> +++ b/drivers/mfd/cros_ec.c >> @@ -53,12 +53,22 @@ static const struct mfd_cell ec_pd_cell = { >> static irqreturn_t ec_irq_thread(int irq, void *data) >> { >> struct cros_ec_device *ec_dev = data; >> + u8 wake_event = 1; > > Bool, true? Ok. > >> + u32 host_event; >> int ret; >> >> - if (device_may_wakeup(ec_dev->dev)) >> + ret = cros_ec_get_next_event(ec_dev); > > What does (ret == 0) mean? The get_next_event() API called from cros_ec_get_next_event() only sets event_data in the ec_dev structure if the command has actually been sent. So (ret == 0) is not treated as an error and is silently ignored. In this case, the interrupt is treated as a wake event. > > ... and is is possible for (ret < 0)? > >> + if (ret > 0 && ec_dev->mkbp_event_supported) { > > cros_ec_get_host_event() checks for (ec_dev->mkbp_event_supported) > anyway, so you can drop it here no? Sure but we only need to call cros_ec_get_host_event() if mkbp_event_supported is true. Anyway, I'll move the host_event check with cros_ec_get_host_event() into cros_ec_get_next_event() to avoid this double check. > >> + /* Don't signal wake event for non-wake host events */ >> + host_event = cros_ec_get_host_event(ec_dev); >> + if (host_event && !(host_event & ec_dev->host_event_wake_mask)) >> + wake_event = 0; > > false Will do. Thanks, Thierry > >> + } >> + >> + if (wake_event && device_may_wakeup(ec_dev->dev)) >> pm_wakeup_event(ec_dev->dev, 0); >> >> - ret = cros_ec_get_next_event(ec_dev); >> if (ret > 0) >> blocking_notifier_call_chain(&ec_dev->event_notifier, >> 0, ec_dev); >> diff --git a/drivers/platform/chrome/cros_ec_proto.c b/drivers/platform/chrome/cros_ec_proto.c >> index 256249b..a216a32 100644 >> --- a/drivers/platform/chrome/cros_ec_proto.c >> +++ b/drivers/platform/chrome/cros_ec_proto.c >> @@ -150,6 +150,40 @@ int cros_ec_check_result(struct cros_ec_device *ec_dev, >> } >> EXPORT_SYMBOL(cros_ec_check_result); >> >> +/* >> + * cros_ec_get_host_event_wake_mask >> + * >> + * Get the mask of host events that cause wake from suspend. >> + * >> + * @ec_dev: EC device to call >> + * @msg: message structure to use >> + * @mask: result when function returns >=0. >> + * >> + * LOCKING: >> + * the caller has ec_dev->lock mutex, or the caller knows there is >> + * no other command in progress. >> + */ >> +static int cros_ec_get_host_event_wake_mask(struct cros_ec_device *ec_dev, >> + struct cros_ec_command *msg, >> + uint32_t *mask) >> +{ >> + struct ec_response_host_event_mask *r; >> + int ret; >> + >> + msg->command = EC_CMD_HOST_EVENT_GET_WAKE_MASK; >> + msg->version = 0; >> + msg->outsize = 0; >> + msg->insize = sizeof(*r); >> + >> + ret = send_command(ec_dev, msg); >> + if (ret > 0) { >> + r = (struct ec_response_host_event_mask *)msg->data; >> + *mask = r->mask; >> + } >> + >> + return ret; >> +} >> + >> static int cros_ec_host_command_proto_query(struct cros_ec_device *ec_dev, >> int devidx, >> struct cros_ec_command *msg) >> @@ -387,6 +421,15 @@ int cros_ec_query_all(struct cros_ec_device *ec_dev) >> else >> ec_dev->mkbp_event_supported = 1; >> >> + /* >> + * Get host event wake mask, assume all events are wake events >> + * if unavailable. >> + */ >> + ret = cros_ec_get_host_event_wake_mask(ec_dev, proto_msg, >> + &ec_dev->host_event_wake_mask); >> + if (ret < 0) >> + ec_dev->host_event_wake_mask = U32_MAX; >> + >> ret = 0; >> >> exit: >> @@ -507,3 +550,20 @@ int cros_ec_get_next_event(struct cros_ec_device *ec_dev) >> return get_keyboard_state_event(ec_dev); >> } >> EXPORT_SYMBOL(cros_ec_get_next_event); >> + >> +u32 cros_ec_get_host_event(struct cros_ec_device *ec_dev) >> +{ >> + if (WARN_ON(!ec_dev->mkbp_event_supported)) >> + return 0; >> + >> + if (ec_dev->event_data.event_type != EC_MKBP_EVENT_HOST_EVENT) >> + return 0; >> + >> + if (ec_dev->event_size != sizeof(u32)) { >> + dev_warn(ec_dev->dev, "Invalid host event size\n"); >> + return 0; >> + } >> + >> + return get_unaligned_le32(&ec_dev->event_data.data.host_event); >> +} >> +EXPORT_SYMBOL(cros_ec_get_host_event); >> diff --git a/include/linux/mfd/cros_ec.h b/include/linux/mfd/cros_ec.h >> index f62043a..8f37c4e 100644 >> --- a/include/linux/mfd/cros_ec.h >> +++ b/include/linux/mfd/cros_ec.h >> @@ -146,6 +146,7 @@ struct cros_ec_device { >> >> struct ec_response_get_next_event event_data; >> int event_size; >> + u32 host_event_wake_mask; >> }; >> >> /** >> @@ -297,6 +298,17 @@ int cros_ec_query_all(struct cros_ec_device *ec_dev); >> */ >> int cros_ec_get_next_event(struct cros_ec_device *ec_dev); >> >> +/** >> + * cros_ec_get_host_event - Return a mask of event set by the EC. >> + * >> + * Once cros_ec_get_next_event() has been called, if the event source is >> + * a host event, this function returns the precise event that triggered >> + * the interrupt. >> + * >> + * This function is a helper to know which events are raised. >> + */ >> +u32 cros_ec_get_host_event(struct cros_ec_device *ec_dev); >> + >> /* sysfs stuff */ >> extern struct attribute_group cros_ec_attr_group; >> extern struct attribute_group cros_ec_lightbar_attr_group; >
Powered by blists - more mailing lists