[PATCH] Input: gpio-keys - fix un-responsive keys issue on hibernate exit

[PATCH] Input: gpio-keys - fix un-responsive keys issue on hibernate exit

[Kamal Wadhwa]

GPIO IRQ setting may get reset during hibernate mode, as device
is completely powered off. This can cause the GPIO keys to become
un-responsive after hibernate-exit.

To fix this problem, re-request IRQs in restore() callback, in the
hibernate exit flow.

Also, to keep the software in-sync with actual IRQ state in hardware,
disable and free the IRQs before entering hibernate(in freeze()
callback). Note that without this extra step, the IRQ re-request in
restore() may not work properly.

Besides this issue scenario, another usecase where this change
may be useful is - where these IRQs need to be handled by a low-power
co-processor during hibernate state. In this case too, these IRQs
need to be freed/re-requested during entry/exit transitions for
hibernate mode. so that co-processer can handle them, while main
processor is in hibernate.

Signed-off-by: Kamal Wadhwa <quic_kamalw@quicinc.com>
---
 drivers/input/keyboard/gpio_keys.c | 67 +++++++++++++++++++++++++++++++++++++-
 1 file changed, 66 insertions(+), 1 deletion(-)

diff --git a/drivers/input/keyboard/gpio_keys.c b/drivers/input/keyboard/gpio_keys.c
index 380fe8dab3b06379f9744e8190a4afcc0aee20b4..8c123e1b14ae34d3d15380eeb96b4a522732573c 100644
--- a/drivers/input/keyboard/gpio_keys.c
+++ b/drivers/input/keyboard/gpio_keys.c
@@ -1041,6 +1041,66 @@ gpio_keys_disable_wakeup(struct gpio_keys_drvdata *ddata)
 	}
 }
 
+static int gpio_keys_freeze(struct device *dev)
+{
+	struct gpio_keys_drvdata *ddata = dev_get_drvdata(dev);
+	struct gpio_button_data *bdata;
+	int i;
+
+	for (i = 0; i < ddata->pdata->nbuttons; i++) {
+		bdata = &ddata->data[i];
+
+		if (!bdata->irq)
+			continue;
+
+		mutex_lock(&ddata->disable_lock);
+		gpio_keys_disable_button(bdata);
+		mutex_unlock(&ddata->disable_lock);
+
+		devm_free_irq(dev, bdata->irq, bdata);
+	}
+
+	return 0;
+}
+
+static int gpio_keys_restore(struct device *dev)
+{
+	struct gpio_keys_drvdata *ddata = dev_get_drvdata(dev);
+	struct gpio_button_data *bdata;
+	int error = 0, i;
+	irq_handler_t isr;
+	unsigned long irqflags;
+	const char *desc;
+
+	for (i = 0; i < ddata->pdata->nbuttons; i++) {
+		bdata = &ddata->data[i];
+		desc = bdata->button->desc ? bdata->button->desc : "gpio_keys";
+		if (!bdata->irq)
+			continue;
+
+		if (bdata->gpiod) {
+			isr = gpio_keys_gpio_isr;
+			irqflags = IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING;
+		} else {
+			isr = gpio_keys_irq_isr;
+			irqflags = 0;
+		}
+
+		if (!bdata->button->can_disable)
+			irqflags = IRQF_SHARED;
+
+		error = devm_request_any_context_irq(dev, bdata->irq,
+						     isr, irqflags, desc, bdata);
+		if (error < 0) {
+			dev_err(dev, "Unable to claim irq %d; error %d\n",
+				bdata->irq, error);
+			return error;
+		}
+	}
+
+	return 0;
+}
+
 static int gpio_keys_suspend(struct device *dev)
 {
 	struct gpio_keys_drvdata *ddata = dev_get_drvdata(dev);
@@ -1083,7 +1143,12 @@ static int gpio_keys_resume(struct device *dev)
 	return 0;
 }
 
-static DEFINE_SIMPLE_DEV_PM_OPS(gpio_keys_pm_ops, gpio_keys_suspend, gpio_keys_resume);
+static const struct dev_pm_ops gpio_keys_pm_ops = {
+	.freeze = gpio_keys_freeze,
+	.restore = gpio_keys_restore,
+	.suspend = gpio_keys_suspend,
+	.resume = gpio_keys_resume,
+};
 
 static void gpio_keys_shutdown(struct platform_device *pdev)
 {

---
base-commit: f2493655d2d3d5c6958ed996b043c821c23ae8d3
change-id: 20241018-unresponsive-gpio-keys-hibernate-31dfd03bf089

Best regards,
-- 
Kamal Wadhwa <quic_kamalw@quicinc.com>

Re: [PATCH] Input: gpio-keys - fix un-responsive keys issue on hibernate exit

[Dmitry Torokhov]

Hi Kamal,

On Fri, Oct 18, 2024 at 06:22:35PM +0530, Kamal Wadhwa wrote:
> GPIO IRQ setting may get reset during hibernate mode, as device
> is completely powered off. This can cause the GPIO keys to become
> un-responsive after hibernate-exit.
> 
> To fix this problem, re-request IRQs in restore() callback, in the
> hibernate exit flow.

No, absolutely not. GPIO state and configuration is owned by GPIO
controller and it should be the entity tasked with restoring the
configuration after hibernation. Individual GPIO consumers need not do
that. Same goes for coprocessor and what's not.

Also, as a side statement: whenever you call devm API outside of probe
and remove path you are likely introduce bugs, because that interferes
with the original order of acquisition of the resources so order of
releasing them during unbinding of the device from the driver will
likely be wrong.

Thanks.

-- 
Dmitry