| 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: Tue, 06 Mar 2018 15:44:15 +0800
From: JeffyChen <jeffy.chen@...k-chips.com>
To: Dmitry Torokhov <dmitry.torokhov@...il.com>
CC: linux-kernel@...r.kernel.org, briannorris@...gle.com,
heiko@...ech.de, dtor@...gle.com, dianders@...gle.com,
Guenter Roeck <linux@...ck-us.net>,
Enric Balletbo i Serra <enric.balletbo@...labora.com>,
Thomas Gleixner <tglx@...utronix.de>,
Joseph Lo <josephl@...dia.com>,
stephen lu <lumotuwe@...il.com>,
Kate Stewart <kstewart@...uxfoundation.org>,
linux-input@...r.kernel.org,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
Philippe Ombredanne <pombredanne@...b.com>,
Arvind Yadav <arvind.yadav.cs@...il.com>
Subject: Re: [PATCH v3 1/3] Input: gpio-keys - add support for wakeup event
action
Hi Dmitry,
Thanks for your review.
On 03/06/2018 08:30 AM, Dmitry Torokhov wrote:
>> >+ switch (button->wakeup_event_action) {
>> >+ case EV_ACT_ASSERTED:
>> >+ bdata->wakeup_trigger_type = active_low ?
>> >+ IRQF_TRIGGER_FALLING : IRQF_TRIGGER_RISING;
> IRQ_TYPE_EDGE_FALLING : IRQ_TYPE_EDGE_RISING;
>
ok, will fix in next version
>> >+ break;
>> >+ case EV_ACT_DEASSERTED:
>> >+ bdata->wakeup_trigger_type = active_low ?
>> >+ IRQF_TRIGGER_RISING : IRQF_TRIGGER_FALLING;
>> >+ break;
> case EV_ACT_ANY:
ok, will fix in next version
>> >+ default:
>> >+ /*
>> >+ * For other cases, we are OK letting suspend/resume
>> >+ * not reconfigure the trigger type.
>> >+ */
>> >+ break;
>> >+ }
>> > } else {
>> > if (!button->irq) {
>> > dev_err(dev, "Found button without gpio or irq\n");
>> >@@ -586,6 +606,12 @@ static int gpio_keys_setup_key(struct platform_device *pdev,
>> >
>> > isr = gpio_keys_irq_isr;
>> > irqflags = 0;
>> >+
>> >+ /*
>> >+ * For IRQ buttons, the irq trigger type for press and release
>> >+ * are the same. So we don't need to reconfigure the trigger
>> >+ * type for wakeup.
> That is not entirely accurate. Interrupt triggers button press, which is
> followed by either immediate or delayed release. There is no interrupt
> for release.
ok, will fix the comment
>
>> >+ */
>> > }
>> >
>> > bdata->code = &ddata->keymap[idx];
>> >@@ -618,6 +644,8 @@ static int gpio_keys_setup_key(struct platform_device *pdev,
>> > return error;
>> > }
>> >
>> >+ bdata->irq_trigger_type = irq_get_trigger_type(bdata->irq);
> Why do we need to store the trigger type? It is always both edges for
> gpio-based keys and we do not support changing wakeup trigger for
> interrupt-based keys.
right, this is not needed.
>
>> >+
>> > return 0;
>> > }
>> >
>> >@@ -718,6 +746,9 @@ gpio_keys_get_devtree_pdata(struct device *dev)
>> > /* legacy name */
>> > fwnode_property_read_bool(child, "gpio-key,wakeup");
>> >
>> >+ fwnode_property_read_u32(child, "wakeup-event-action",
>> >+ &button->wakeup_event_action);
>> >+
>> > button->can_disable =
>> > fwnode_property_read_bool(child, "linux,can-disable");
>> >
>> >@@ -854,6 +885,10 @@ static int __maybe_unused gpio_keys_suspend(struct device *dev)
>> > if (device_may_wakeup(dev)) {
>> > for (i = 0; i < ddata->pdata->nbuttons; i++) {
>> > struct gpio_button_data *bdata = &ddata->data[i];
>> >+
>> >+ if (bdata->button->wakeup && bdata->wakeup_trigger_type)
>> >+ irq_set_irq_type(bdata->irq,
>> >+ bdata->wakeup_trigger_type);
> if (bdata->button->wakeup) {
> if (bdata->wakeup_trigger_type) {
> error = ...;
> }
>
> enable_irq_wake(bdata->irq);
> }
>
> Might need to be split into a helper; if you add error handling to
> enable_irq_wake() that woudl be great too.
ok, will do that.
>
>> > if (bdata->button->wakeup)
>> > enable_irq_wake(bdata->irq);
>> > bdata->suspended = true;
>> >@@ -878,6 +913,10 @@ static int __maybe_unused gpio_keys_resume(struct device *dev)
>> > if (device_may_wakeup(dev)) {
>> > for (i = 0; i < ddata->pdata->nbuttons; i++) {
>> > struct gpio_button_data *bdata = &ddata->data[i];
>> >+
>> >+ if (bdata->button->wakeup && bdata->wakeup_trigger_type)
>> >+ irq_set_irq_type(bdata->irq,
>> >+ bdata->irq_trigger_type);
> Just use IRQ_TYPE_EDGE_BOTH.
>
>> > if (bdata->button->wakeup)
>> > disable_irq_wake(bdata->irq);
>> > bdata->suspended = false;
>> >diff --git a/include/linux/gpio_keys.h b/include/linux/gpio_keys.h
>> >index d06bf77400f1..7160df54a6fe 100644
>> >--- a/include/linux/gpio_keys.h
>> >+++ b/include/linux/gpio_keys.h
>> >@@ -13,6 +13,7 @@ struct device;
>> > * @desc: label that will be attached to button's gpio
>> > * @type: input event type (%EV_KEY, %EV_SW, %EV_ABS)
>> > * @wakeup: configure the button as a wake-up source
>> >+ * @wakeup_event_action: event action to trigger wakeup
>> > * @debounce_interval: debounce ticks interval in msecs
>> > * @can_disable: %true indicates that userspace is allowed to
>> > * disable button via sysfs
>> >@@ -26,6 +27,7 @@ struct gpio_keys_button {
>> > const char *desc;
>> > unsigned int type;
>> > int wakeup;
>> >+ int wakeup_event_action;
>> > int debounce_interval;
>> > bool can_disable;
>> > int value;
>> >diff --git a/include/uapi/linux/input-event-codes.h b/include/uapi/linux/input-event-codes.h
>> >index 53fbae27b280..d7917b0bd438 100644
>> >--- a/include/uapi/linux/input-event-codes.h
>> >+++ b/include/uapi/linux/input-event-codes.h
>> >@@ -32,6 +32,15 @@
>> > #define INPUT_PROP_CNT (INPUT_PROP_MAX + 1)
>> >
>> > /*
>> >+ * Event action types
>> >+ */
>> >+#define EV_ACT_ANY 0x00 /* asserted or deasserted */
>> >+#define EV_ACT_ASSERTED 0x01 /* asserted */
>> >+#define EV_ACT_DEASSERTED 0x02 /* deasserted */
> These do not belong here: they are of no interest to userspace but
> simply a driver specific quirk. If you want to share symbolic names add
> include/dt-bindings/input/gpio-keys.h
ok
>
>
>> >+#define EV_ACT_MAX 0x02
>> >+#define EV_ACT_CNT (EV_ACT_MAX+1)
> These are not needed at all: you are not defining input bitmasks.
>
Powered by blists - more mailing lists