* [PATCH] Input: matrix_keypad - fix keypad does not response
@ 2018-02-03 12:00 Zhang Bo
2018-02-03 13:44 ` Marcus Folkesson
0 siblings, 1 reply; 4+ messages in thread
From: Zhang Bo @ 2018-02-03 12:00 UTC (permalink / raw)
To: dmitry.torokhov
Cc: DRivshin, robh, linux-input, linux-kernel, zbsdta, zhang.bo19
From: zhangbo <zbsdta@126.com>
If matrix_keypad_stop() is calling and the keypad interrupt is triggered,
disable_row_irqs() is called by both matrix_keypad_interrupt() and
matrix_keypad_stop() at the same time. then disable_row_irqs() is called
twice, and the device enter suspend state before keypad->work is executed.
At this condition the device will start keypad and enable irq once after
resume. and then irqs are disabled yet because irqs are disabled twice and
only enable once.
Signed-off-by: zhangbo <zbsdta@126.com>
---
drivers/input/keyboard/matrix_keypad.c | 18 ++++++++++++------
1 file changed, 12 insertions(+), 6 deletions(-)
diff --git a/drivers/input/keyboard/matrix_keypad.c b/drivers/input/keyboard/matrix_keypad.c
index 1f316d66e6f7..13fe51824637 100644
--- a/drivers/input/keyboard/matrix_keypad.c
+++ b/drivers/input/keyboard/matrix_keypad.c
@@ -169,7 +169,8 @@ static void matrix_keypad_scan(struct work_struct *work)
/* Enable IRQs again */
spin_lock_irq(&keypad->lock);
keypad->scan_pending = false;
- enable_row_irqs(keypad);
+ if (keypad->stopped == false)
+ enable_row_irqs(keypad);
spin_unlock_irq(&keypad->lock);
}
@@ -202,14 +203,16 @@ static int matrix_keypad_start(struct input_dev *dev)
{
struct matrix_keypad *keypad = input_get_drvdata(dev);
+ spin_lock_irq(&keypad->lock);
keypad->stopped = false;
- mb();
/*
* Schedule an immediate key scan to capture current key state;
* columns will be activated and IRQs be enabled after the scan.
*/
- schedule_delayed_work(&keypad->work, 0);
+ if (keypad->scan_pending == false)
+ schedule_delayed_work(&keypad->work, 0);
+ spin_unlock_irq(&keypad->lock);
return 0;
}
@@ -218,14 +221,17 @@ static void matrix_keypad_stop(struct input_dev *dev)
{
struct matrix_keypad *keypad = input_get_drvdata(dev);
+ spin_lock_irq(&keypad->lock);
keypad->stopped = true;
- mb();
- flush_work(&keypad->work.work);
/*
* matrix_keypad_scan() will leave IRQs enabled;
* we should disable them now.
*/
- disable_row_irqs(keypad);
+ if (keypad->scan_pending == false)
+ disable_row_irqs(keypad);
+ spin_unlock_irq(&keypad->lock);
+
+ flush_work(&keypad->work.work);
}
#ifdef CONFIG_PM_SLEEP
--
2.14.3
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [PATCH] Input: matrix_keypad - fix keypad does not response
2018-02-03 12:00 [PATCH] Input: matrix_keypad - fix keypad does not response Zhang Bo
@ 2018-02-03 13:44 ` Marcus Folkesson
2018-02-03 16:24 ` Zhang Bo
2018-02-03 16:31 ` Zhang Bo
0 siblings, 2 replies; 4+ messages in thread
From: Marcus Folkesson @ 2018-02-03 13:44 UTC (permalink / raw)
To: Zhang Bo
Cc: dmitry.torokhov, DRivshin, robh, linux-input, linux-kernel,
zhang.bo19
[-- Attachment #1: Type: text/plain, Size: 2810 bytes --]
Hi!
On Sat, Feb 03, 2018 at 08:00:46PM +0800, Zhang Bo wrote:
> From: zhangbo <zbsdta@126.com>
>
> If matrix_keypad_stop() is calling and the keypad interrupt is triggered,
> disable_row_irqs() is called by both matrix_keypad_interrupt() and
> matrix_keypad_stop() at the same time. then disable_row_irqs() is called
> twice, and the device enter suspend state before keypad->work is executed.
> At this condition the device will start keypad and enable irq once after
> resume. and then irqs are disabled yet because irqs are disabled twice and
> only enable once.
>
> Signed-off-by: zhangbo <zbsdta@126.com>
Please use your full real name in the signed-off-by tag.
> ---
> drivers/input/keyboard/matrix_keypad.c | 18 ++++++++++++------
> 1 file changed, 12 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/input/keyboard/matrix_keypad.c b/drivers/input/keyboard/matrix_keypad.c
> index 1f316d66e6f7..13fe51824637 100644
> --- a/drivers/input/keyboard/matrix_keypad.c
> +++ b/drivers/input/keyboard/matrix_keypad.c
> @@ -169,7 +169,8 @@ static void matrix_keypad_scan(struct work_struct *work)
> /* Enable IRQs again */
> spin_lock_irq(&keypad->lock);
> keypad->scan_pending = false;
> - enable_row_irqs(keypad);
> + if (keypad->stopped == false)
> + enable_row_irqs(keypad);
> spin_unlock_irq(&keypad->lock);
> }
>
> @@ -202,14 +203,16 @@ static int matrix_keypad_start(struct input_dev *dev)
> {
> struct matrix_keypad *keypad = input_get_drvdata(dev);
>
> + spin_lock_irq(&keypad->lock);
> keypad->stopped = false;
> - mb();
>
> /*
> * Schedule an immediate key scan to capture current key state;
> * columns will be activated and IRQs be enabled after the scan.
> */
> - schedule_delayed_work(&keypad->work, 0);
> + if (keypad->scan_pending == false)
> + schedule_delayed_work(&keypad->work, 0);
> + spin_unlock_irq(&keypad->lock);
>
> return 0;
> }
> @@ -218,14 +221,17 @@ static void matrix_keypad_stop(struct input_dev *dev)
> {
> struct matrix_keypad *keypad = input_get_drvdata(dev);
>
> + spin_lock_irq(&keypad->lock);
> keypad->stopped = true;
> - mb();
> - flush_work(&keypad->work.work);
> /*
> * matrix_keypad_scan() will leave IRQs enabled;
> * we should disable them now.
> */
> - disable_row_irqs(keypad);
> + if (keypad->scan_pending == false)
> + disable_row_irqs(keypad);
> + spin_unlock_irq(&keypad->lock);
> +
> + flush_work(&keypad->work.work);
> }
Hum, I think we should use spin_lock_irqsave/spin_lock_irqrestore
instead to be on the safe side.
I don't see how we could guarantee that irqs is allways enabled when
calling spin_lock_irq().
>
> #ifdef CONFIG_PM_SLEEP
> --
> 2.14.3
Best regards
Marcus Folkesson
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 4+ messages in thread* Re:Re: [PATCH] Input: matrix_keypad - fix keypad does not response
2018-02-03 13:44 ` Marcus Folkesson
@ 2018-02-03 16:24 ` Zhang Bo
2018-02-03 16:31 ` Zhang Bo
1 sibling, 0 replies; 4+ messages in thread
From: Zhang Bo @ 2018-02-03 16:24 UTC (permalink / raw)
To: Marcus Folkesson
Cc: dmitry.torokhov, DRivshin, robh, linux-input, linux-kernel,
zhang.bo19
At 2018-02-03 21:44:50, "Marcus Folkesson" <marcus.folkesson@gmail.com> wrote:
>> diff --git a/drivers/input/keyboard/matrix_keypad.c b/drivers/input/keyboard/matrix_keypad.c
>> index 1f316d66e6f7..13fe51824637 100644
>> --- a/drivers/input/keyboard/matrix_keypad.c
>> +++ b/drivers/input/keyboard/matrix_keypad.c
>> @@ -169,7 +169,8 @@ static void matrix_keypad_scan(struct work_struct *work)
>> /* Enable IRQs again */
>> spin_lock_irq(&keypad->lock);
>> keypad->scan_pending = false;
>> - enable_row_irqs(keypad);
>> + if (keypad->stopped == false)
>> + enable_row_irqs(keypad);
>> spin_unlock_irq(&keypad->lock);
>> }
>>
>> @@ -202,14 +203,16 @@ static int matrix_keypad_start(struct input_dev *dev)
>> {
>> struct matrix_keypad *keypad = input_get_drvdata(dev);
>>
>> + spin_lock_irq(&keypad->lock);
>> keypad->stopped = false;
>> - mb();
>>
>> /*
>> * Schedule an immediate key scan to capture current key state;
>> * columns will be activated and IRQs be enabled after the scan.
>> */
>> - schedule_delayed_work(&keypad->work, 0);
>> + if (keypad->scan_pending == false)
>> + schedule_delayed_work(&keypad->work, 0);
>> + spin_unlock_irq(&keypad->lock);
>>
>> return 0;
>> }
>> @@ -218,14 +221,17 @@ static void matrix_keypad_stop(struct input_dev *dev)
>> {
>> struct matrix_keypad *keypad = input_get_drvdata(dev);
>>
>> + spin_lock_irq(&keypad->lock);
>> keypad->stopped = true;
>> - mb();
>> - flush_work(&keypad->work.work);
>> /*
>> * matrix_keypad_scan() will leave IRQs enabled;
>> * we should disable them now.
>> */
>> - disable_row_irqs(keypad);
>> + if (keypad->scan_pending == false)
>> + disable_row_irqs(keypad);
>> + spin_unlock_irq(&keypad->lock);
>> +
>> + flush_work(&keypad->work.work);
>> }
>
>
>Hum, I think we should use spin_lock_irqsave/spin_lock_irqrestore
>instead to be on the safe side.
>I don't see how we could guarantee that irqs is allways enabled when
>calling spin_lock_irq().
spin_lock_irq and spin_unlock_irq are called in matrix_keypad_stop and matrix_keypad_start
which are in suspend and resume, they run in process context, matrix_keypad_scan runs in
worker which is in process context also.
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] Input: matrix_keypad - fix keypad does not response
2018-02-03 13:44 ` Marcus Folkesson
2018-02-03 16:24 ` Zhang Bo
@ 2018-02-03 16:31 ` Zhang Bo
1 sibling, 0 replies; 4+ messages in thread
From: Zhang Bo @ 2018-02-03 16:31 UTC (permalink / raw)
To: Marcus Folkesson
Cc: dmitry.torokhov, DRivshin, robh, linux-input, linux-kernel,
zhang.bo19
At 2018-02-03 21:44:50, "Marcus Folkesson" <marcus.folkesson@gmail.com> wrote:
>> diff --git a/drivers/input/keyboard/matrix_keypad.c b/drivers/input/keyboard/matrix_keypad.c
>> index 1f316d66e6f7..13fe51824637 100644
>> --- a/drivers/input/keyboard/matrix_keypad.c
>> +++ b/drivers/input/keyboard/matrix_keypad.c
>> @@ -169,7 +169,8 @@ static void matrix_keypad_scan(struct work_struct *work)
>> /* Enable IRQs again */
>> spin_lock_irq(&keypad->lock);
>> keypad->scan_pending = false;
>> - enable_row_irqs(keypad);
>> + if (keypad->stopped == false)
>> + enable_row_irqs(keypad);
>> spin_unlock_irq(&keypad->lock);
>> }
>>
>> @@ -202,14 +203,16 @@ static int matrix_keypad_start(struct input_dev *dev)
>> {
>> struct matrix_keypad *keypad = input_get_drvdata(dev);
>>
>> + spin_lock_irq(&keypad->lock);
>> keypad->stopped = false;
>> - mb();
>>
>> /*
>> * Schedule an immediate key scan to capture current key state;
>> * columns will be activated and IRQs be enabled after the scan.
>> */
>> - schedule_delayed_work(&keypad->work, 0);
>> + if (keypad->scan_pending == false)
>> + schedule_delayed_work(&keypad->work, 0);
>> + spin_unlock_irq(&keypad->lock);
>>
>> return 0;
>> }
>> @@ -218,14 +221,17 @@ static void matrix_keypad_stop(struct input_dev *dev)
>> {
>> struct matrix_keypad *keypad = input_get_drvdata(dev);
>>
>> + spin_lock_irq(&keypad->lock);
>> keypad->stopped = true;
>> - mb();
>> - flush_work(&keypad->work.work);
>> /*
>> * matrix_keypad_scan() will leave IRQs enabled;
>> * we should disable them now.
>> */
>> - disable_row_irqs(keypad);
>> + if (keypad->scan_pending == false)
>> + disable_row_irqs(keypad);
>> + spin_unlock_irq(&keypad->lock);
>> +
>> + flush_work(&keypad->work.work);
>> }
>
>
>Hum, I think we should use spin_lock_irqsave/spin_lock_irqrestore
>instead to be on the safe side.
>I don't see how we could guarantee that irqs is allways enabled when
>calling spin_lock_irq().
spin_lock_irq and spin_unlock_irq are called in matrix_keypad_stop and matrix_keypad_start
which are in suspend and resume, they run in process context, matrix_keypad_scan runs in
worker which is in process context also.
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2018-02-03 16:31 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-02-03 12:00 [PATCH] Input: matrix_keypad - fix keypad does not response Zhang Bo
2018-02-03 13:44 ` Marcus Folkesson
2018-02-03 16:24 ` Zhang Bo
2018-02-03 16:31 ` Zhang Bo
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).