[PATCH 0/3] adp5589 fixes

[PATCH 0/3] adp5589 fixes

[Guido Martínez]

Hi all,

Here are a couple of bug fixes for the adp5589-keys driver. The last two
are pretty serious, I think.

In particular the last one resulted in an interrupt storm and a
hung keypad when the FIFO filled completely, since the driver was
interpreting the event count of 16 as 0.

Thanks!

Guido Martínez (3):
  Input: adp5589-keys: handle i2c errors on the irq handler
  Input: adp5589-keys: fix pull mask setting
  Input: adp5589-keys: fix event count mask

 drivers/input/keyboard/adp5589-keys.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/3] Input: adp5589-keys: handle i2c errors on the irq handler

[Guido Martínez]

Bail out if reading the status register fails, otherwise we'll carry
on with a negative error code as if it were read from the keypad, and
falsely report events or errors.

Fixes: 9d2e173644bb ('Input: ADP5589 - new driver for I2C Keypad Decoder and I/O Expander')
Signed-off-by: Guido Martínez <guido@vanguardiasur.com.ar>
---
 drivers/input/keyboard/adp5589-keys.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/input/keyboard/adp5589-keys.c b/drivers/input/keyboard/adp5589-keys.c
index a452677..19edd2d 100644
--- a/drivers/input/keyboard/adp5589-keys.c
+++ b/drivers/input/keyboard/adp5589-keys.c
@@ -622,6 +622,9 @@ static irqreturn_t adp5589_irq(int irq, void *handle)
 
 	status = adp5589_read(client, ADP5589_5_INT_STATUS);
 
+	if (status < 0)
+		return IRQ_HANDLED;
+
 	if (status & OVRFLOW_INT)	/* Unlikely and should never happen */
 		dev_err(&client->dev, "Event Overflow Error\n");
 
-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/3] Input: adp5589-keys: handle i2c errors on the irq handler

[Dmitry Torokhov]

On Tue, Apr 21, 2015 at 04:35:04PM +0200, Michael Hennerich wrote:
> On 04/21/2015 04:21 PM, Guido Martínez wrote:
> >Bail out if reading the status register fails, otherwise we'll carry
> >on with a negative error code as if it were read from the keypad, and
> >falsely report events or errors.
> >
> >Fixes: 9d2e173644bb ('Input: ADP5589 - new driver for I2C Keypad Decoder and I/O Expander')
> >Signed-off-by: Guido Martínez <guido@vanguardiasur.com.ar>
> Acked-by: Michael Hennerich <michael.hennerich@analog.com>
> >---
> >  drivers/input/keyboard/adp5589-keys.c | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> >diff --git a/drivers/input/keyboard/adp5589-keys.c b/drivers/input/keyboard/adp5589-keys.c
> >index a452677..19edd2d 100644
> >--- a/drivers/input/keyboard/adp5589-keys.c
> >+++ b/drivers/input/keyboard/adp5589-keys.c
> >@@ -622,6 +622,9 @@ static irqreturn_t adp5589_irq(int irq, void *handle)
> >  	status = adp5589_read(client, ADP5589_5_INT_STATUS);
> >+	if (status < 0)
> >+		return IRQ_HANDLED;
> >+
> >  	if (status & OVRFLOW_INT)	/* Unlikely and should never happen */
> >  		dev_err(&client->dev, "Event Overflow Error\n");

Hmm, what about all other places where we read and ignore errors? Why
this place is more important than others?

Thanks.

-- 
Dmitry
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/3] Input: adp5589-keys: handle i2c errors on the irq handler

[Guido Martínez]

Dmitry,

On Wed, May 06, 2015 at 04:33:23PM -0700, Dmitry Torokhov wrote:
> On Tue, Apr 21, 2015 at 04:35:04PM +0200, Michael Hennerich wrote:
> > On 04/21/2015 04:21 PM, Guido Martínez wrote:
> > >Bail out if reading the status register fails, otherwise we'll carry
> > >on with a negative error code as if it were read from the keypad, and
> > >falsely report events or errors.
> > >
> > >Fixes: 9d2e173644bb ('Input: ADP5589 - new driver for I2C Keypad Decoder and I/O Expander')
> > >Signed-off-by: Guido Martínez <guido@vanguardiasur.com.ar>
> > Acked-by: Michael Hennerich <michael.hennerich@analog.com>
> > >---
> > >  drivers/input/keyboard/adp5589-keys.c | 3 +++
> > >  1 file changed, 3 insertions(+)
> > >
> > >diff --git a/drivers/input/keyboard/adp5589-keys.c b/drivers/input/keyboard/adp5589-keys.c
> > >index a452677..19edd2d 100644
> > >--- a/drivers/input/keyboard/adp5589-keys.c
> > >+++ b/drivers/input/keyboard/adp5589-keys.c
> > >@@ -622,6 +622,9 @@ static irqreturn_t adp5589_irq(int irq, void *handle)
> > >  	status = adp5589_read(client, ADP5589_5_INT_STATUS);
> > >+	if (status < 0)
> > >+		return IRQ_HANDLED;
> > >+
> > >  	if (status & OVRFLOW_INT)	/* Unlikely and should never happen */
> > >  		dev_err(&client->dev, "Event Overflow Error\n");
> 
> Hmm, what about all other places where we read and ignore errors? Why
> this place is more important than others?
Good point. It's just the error we were hitting on our system since the
i2c controller failed. It should be the most common one should i2c fail,
but I agree that all other ones should be consistent with this one.

I'll work some more on it and resend.

Thanks!

-- 
Guido Martínez, VanguardiaSur
www.vanguardiasur.com.ar
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/3] Input: adp5589-keys: fix pull mask setting

[Guido Martínez]

The pull mask is created by looping each row (column) and building an
8-bit integer with the configuration. It is written byte-by-byte, when
we reach the end of the rows (columns) or we're at the 3rd line (which
finishes the first byte, since each pin is 2bits on the mask).

However, this only works if we have at most 8 pins (2 bytes), which is
not the case for the ADP5589. So, write the byte at each boundary (every
4 rows/columns).

Fixes: 9d2e173644bb ('Input: ADP5589 - new driver for I2C Keypad Decoder and I/O Expander')
Signed-off-by: Guido Martínez <guido@vanguardiasur.com.ar>
---
 drivers/input/keyboard/adp5589-keys.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/input/keyboard/adp5589-keys.c b/drivers/input/keyboard/adp5589-keys.c
index 19edd2d..8709a7f 100644
--- a/drivers/input/keyboard/adp5589-keys.c
+++ b/drivers/input/keyboard/adp5589-keys.c
@@ -729,7 +729,7 @@ static int adp5589_setup(struct adp5589_kpad *kpad)
 
 		pull_mask |= val << (2 * (i & 0x3));
 
-		if (i == 3 || i == kpad->var->max_row_num) {
+		if (i % 4 == 3 || i == kpad->var->max_row_num) {
 			ret |= adp5589_write(client, reg(ADP5585_RPULL_CONFIG_A)
 					     + (i >> 2), pull_mask);
 			pull_mask = 0;
@@ -749,7 +749,7 @@ static int adp5589_setup(struct adp5589_kpad *kpad)
 
 		pull_mask |= val << (2 * (i & 0x3));
 
-		if (i == 3 || i == kpad->var->max_col_num) {
+		if (i % 4 == 3 || i == kpad->var->max_col_num) {
 			ret |= adp5589_write(client,
 					     reg(ADP5585_RPULL_CONFIG_C) +
 					     (i >> 2), pull_mask);
-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/3] Input: adp5589-keys: fix pull mask setting

[Michael Hennerich]

On 04/21/2015 04:21 PM, Guido Martínez wrote:
> The pull mask is created by looping each row (column) and building an
> 8-bit integer with the configuration. It is written byte-by-byte, when
> we reach the end of the rows (columns) or we're at the 3rd line (which
> finishes the first byte, since each pin is 2bits on the mask).
>
> However, this only works if we have at most 8 pins (2 bytes), which is
> not the case for the ADP5589. So, write the byte at each boundary (every
> 4 rows/columns).
>
> Fixes: 9d2e173644bb ('Input: ADP5589 - new driver for I2C Keypad Decoder and I/O Expander')
> Signed-off-by: Guido Martínez <guido@vanguardiasur.com.ar>
Acked-by: Michael Hennerich <michael.hennerich@analog.com>
> ---
>   drivers/input/keyboard/adp5589-keys.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/input/keyboard/adp5589-keys.c b/drivers/input/keyboard/adp5589-keys.c
> index 19edd2d..8709a7f 100644
> --- a/drivers/input/keyboard/adp5589-keys.c
> +++ b/drivers/input/keyboard/adp5589-keys.c
> @@ -729,7 +729,7 @@ static int adp5589_setup(struct adp5589_kpad *kpad)
>   
>   		pull_mask |= val << (2 * (i & 0x3));
>   
> -		if (i == 3 || i == kpad->var->max_row_num) {
> +		if (i % 4 == 3 || i == kpad->var->max_row_num) {
>   			ret |= adp5589_write(client, reg(ADP5585_RPULL_CONFIG_A)
>   					     + (i >> 2), pull_mask);
>   			pull_mask = 0;
> @@ -749,7 +749,7 @@ static int adp5589_setup(struct adp5589_kpad *kpad)
>   
>   		pull_mask |= val << (2 * (i & 0x3));
>   
> -		if (i == 3 || i == kpad->var->max_col_num) {
> +		if (i % 4 == 3 || i == kpad->var->max_col_num) {
>   			ret |= adp5589_write(client,
>   					     reg(ADP5585_RPULL_CONFIG_C) +
>   					     (i >> 2), pull_mask);


-- 
Greetings,
Michael

--
Analog Devices GmbH      Wilhelm-Wagenfeld-Str. 6      80807 Muenchen
Sitz der Gesellschaft: Muenchen; Registergericht: Muenchen HRB 40368;
Geschaeftsfuehrer:Dr.Carsten Suckrow, Thomas Wessel, William A. Martin,
Margaret Seif

--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/3] Input: adp5589-keys: fix pull mask setting

[Dmitry Torokhov]

On Tue, Apr 21, 2015 at 04:35:20PM +0200, Michael Hennerich wrote:
> On 04/21/2015 04:21 PM, Guido Martínez wrote:
> >The pull mask is created by looping each row (column) and building an
> >8-bit integer with the configuration. It is written byte-by-byte, when
> >we reach the end of the rows (columns) or we're at the 3rd line (which
> >finishes the first byte, since each pin is 2bits on the mask).
> >
> >However, this only works if we have at most 8 pins (2 bytes), which is
> >not the case for the ADP5589. So, write the byte at each boundary (every
> >4 rows/columns).
> >
> >Fixes: 9d2e173644bb ('Input: ADP5589 - new driver for I2C Keypad Decoder and I/O Expander')
> >Signed-off-by: Guido Martínez <guido@vanguardiasur.com.ar>
> Acked-by: Michael Hennerich <michael.hennerich@analog.com>

Applied, thank you.

> >---
> >  drivers/input/keyboard/adp5589-keys.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> >diff --git a/drivers/input/keyboard/adp5589-keys.c b/drivers/input/keyboard/adp5589-keys.c
> >index 19edd2d..8709a7f 100644
> >--- a/drivers/input/keyboard/adp5589-keys.c
> >+++ b/drivers/input/keyboard/adp5589-keys.c
> >@@ -729,7 +729,7 @@ static int adp5589_setup(struct adp5589_kpad *kpad)
> >  		pull_mask |= val << (2 * (i & 0x3));
> >-		if (i == 3 || i == kpad->var->max_row_num) {
> >+		if (i % 4 == 3 || i == kpad->var->max_row_num) {
> >  			ret |= adp5589_write(client, reg(ADP5585_RPULL_CONFIG_A)
> >  					     + (i >> 2), pull_mask);
> >  			pull_mask = 0;
> >@@ -749,7 +749,7 @@ static int adp5589_setup(struct adp5589_kpad *kpad)
> >  		pull_mask |= val << (2 * (i & 0x3));
> >-		if (i == 3 || i == kpad->var->max_col_num) {
> >+		if (i % 4 == 3 || i == kpad->var->max_col_num) {
> >  			ret |= adp5589_write(client,
> >  					     reg(ADP5585_RPULL_CONFIG_C) +
> >  					     (i >> 2), pull_mask);
> 
> 
> -- 
> Greetings,
> Michael
> 
> --
> Analog Devices GmbH      Wilhelm-Wagenfeld-Str. 6      80807 Muenchen
> Sitz der Gesellschaft: Muenchen; Registergericht: Muenchen HRB 40368;
> Geschaeftsfuehrer:Dr.Carsten Suckrow, Thomas Wessel, William A. Martin,
> Margaret Seif
> 

-- 
Dmitry
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 3/3] Input: adp5589-keys: fix event count mask

[Guido Martínez]

The event mask was specified as 0xF (4 bits) when in reality is 0x1F (5
bits) in order to be capable of representing all FIFO length values from
0 to 16.

This caused a problem: when the keypad reported 16 pending events the
driver took it as 0, and did nothing. This in turn caused the keypad to
re-issue the interrupt over and over again.

Fixes: 9d2e173644bb ('Input: ADP5589 - new driver for I2C Keypad Decoder and I/O Expander')
Signed-off-by: Guido Martínez <guido@vanguardiasur.com.ar>
---
 drivers/input/keyboard/adp5589-keys.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/input/keyboard/adp5589-keys.c b/drivers/input/keyboard/adp5589-keys.c
index 8709a7f..0419a7a 100644
--- a/drivers/input/keyboard/adp5589-keys.c
+++ b/drivers/input/keyboard/adp5589-keys.c
@@ -180,7 +180,7 @@
 #define LOGIC2_STAT	(1 << 7)	/* ADP5589 only */
 #define LOGIC1_STAT	(1 << 6)
 #define LOCK_STAT	(1 << 5)	/* ADP5589 only */
-#define KEC		0xF
+#define KEC		0x1F
 
 /* PIN_CONFIG_D Register */
 #define C4_EXTEND_CFG	(1 << 6)	/* RESET2 */
-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/3] Input: adp5589-keys: fix event count mask

[Michael Hennerich]

On 04/21/2015 04:21 PM, Guido Martínez wrote:
> The event mask was specified as 0xF (4 bits) when in reality is 0x1F (5
> bits) in order to be capable of representing all FIFO length values from
> 0 to 16.
>
> This caused a problem: when the keypad reported 16 pending events the
> driver took it as 0, and did nothing. This in turn caused the keypad to
> re-issue the interrupt over and over again.
>
> Fixes: 9d2e173644bb ('Input: ADP5589 - new driver for I2C Keypad Decoder and I/O Expander')
> Signed-off-by: Guido Martínez <guido@vanguardiasur.com.ar>
Acked-by: Michael Hennerich <michael.hennerich@analog.com>
> ---
>   drivers/input/keyboard/adp5589-keys.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/input/keyboard/adp5589-keys.c b/drivers/input/keyboard/adp5589-keys.c
> index 8709a7f..0419a7a 100644
> --- a/drivers/input/keyboard/adp5589-keys.c
> +++ b/drivers/input/keyboard/adp5589-keys.c
> @@ -180,7 +180,7 @@
>   #define LOGIC2_STAT	(1 << 7)	/* ADP5589 only */
>   #define LOGIC1_STAT	(1 << 6)
>   #define LOCK_STAT	(1 << 5)	/* ADP5589 only */
> -#define KEC		0xF
> +#define KEC		0x1F
>   
>   /* PIN_CONFIG_D Register */
>   #define C4_EXTEND_CFG	(1 << 6)	/* RESET2 */


-- 
Greetings,
Michael

--
Analog Devices GmbH      Wilhelm-Wagenfeld-Str. 6      80807 Muenchen
Sitz der Gesellschaft: Muenchen; Registergericht: Muenchen HRB 40368;
Geschaeftsfuehrer:Dr.Carsten Suckrow, Thomas Wessel, William A. Martin,
Margaret Seif

--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/3] Input: adp5589-keys: fix event count mask

[Dmitry Torokhov]

On Tue, Apr 21, 2015 at 04:36:44PM +0200, Michael Hennerich wrote:
> On 04/21/2015 04:21 PM, Guido Martínez wrote:
> >The event mask was specified as 0xF (4 bits) when in reality is 0x1F (5
> >bits) in order to be capable of representing all FIFO length values from
> >0 to 16.
> >
> >This caused a problem: when the keypad reported 16 pending events the
> >driver took it as 0, and did nothing. This in turn caused the keypad to
> >re-issue the interrupt over and over again.
> >
> >Fixes: 9d2e173644bb ('Input: ADP5589 - new driver for I2C Keypad Decoder and I/O Expander')
> >Signed-off-by: Guido Martínez <guido@vanguardiasur.com.ar>
> Acked-by: Michael Hennerich <michael.hennerich@analog.com>

Applied, thank you.

> >---
> >  drivers/input/keyboard/adp5589-keys.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> >diff --git a/drivers/input/keyboard/adp5589-keys.c b/drivers/input/keyboard/adp5589-keys.c
> >index 8709a7f..0419a7a 100644
> >--- a/drivers/input/keyboard/adp5589-keys.c
> >+++ b/drivers/input/keyboard/adp5589-keys.c
> >@@ -180,7 +180,7 @@
> >  #define LOGIC2_STAT	(1 << 7)	/* ADP5589 only */
> >  #define LOGIC1_STAT	(1 << 6)
> >  #define LOCK_STAT	(1 << 5)	/* ADP5589 only */
> >-#define KEC		0xF
> >+#define KEC		0x1F
> >  /* PIN_CONFIG_D Register */
> >  #define C4_EXTEND_CFG	(1 << 6)	/* RESET2 */
> 
> 
> -- 
> Greetings,
> Michael
> 
> --
> Analog Devices GmbH      Wilhelm-Wagenfeld-Str. 6      80807 Muenchen
> Sitz der Gesellschaft: Muenchen; Registergericht: Muenchen HRB 40368;
> Geschaeftsfuehrer:Dr.Carsten Suckrow, Thomas Wessel, William A. Martin,
> Margaret Seif
> 

-- 
Dmitry
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/3] Input: adp5589-keys: fix event count mask

[Ezequiel Garcia]

On 6 May 2015 at 20:36, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
> On Tue, Apr 21, 2015 at 04:36:44PM +0200, Michael Hennerich wrote:
>> On 04/21/2015 04:21 PM, Guido Martínez wrote:
>> >The event mask was specified as 0xF (4 bits) when in reality is 0x1F (5
>> >bits) in order to be capable of representing all FIFO length values from
>> >0 to 16.
>> >
>> >This caused a problem: when the keypad reported 16 pending events the
>> >driver took it as 0, and did nothing. This in turn caused the keypad to
>> >re-issue the interrupt over and over again.
>> >
>> >Fixes: 9d2e173644bb ('Input: ADP5589 - new driver for I2C Keypad Decoder and I/O Expander')
>> >Signed-off-by: Guido Martínez <guido@vanguardiasur.com.ar>
>> Acked-by: Michael Hennerich <michael.hennerich@analog.com>
>

Dmitry,

Going through this patchset and noticed the "Fixes" tag got dropped when
the last two fixes were pushed.

More importantly, the patches haven't been queued for -stable, despite it
fixes a serious issue.

Can you take care of sending them to -stable or should I do it?
Upstream commits are:
c615dcb6d13e Input: adp5589-keys - fix event count mask
195e610bf710 Input: adp5589-keys - fix pull mask setting

Thanks!
-- 
Ezequiel García, VanguardiaSur
www.vanguardiasur.com.ar
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/3] Input: adp5589-keys: fix event count mask

[Dmitry Torokhov]

Hi Ezequiel,

On Fri, Oct 2, 2015 at 2:28 PM, Ezequiel Garcia
<ezequiel@vanguardiasur.com.ar> wrote:
> On 6 May 2015 at 20:36, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
>> On Tue, Apr 21, 2015 at 04:36:44PM +0200, Michael Hennerich wrote:
>>> On 04/21/2015 04:21 PM, Guido Martínez wrote:
>>> >The event mask was specified as 0xF (4 bits) when in reality is 0x1F (5
>>> >bits) in order to be capable of representing all FIFO length values from
>>> >0 to 16.
>>> >
>>> >This caused a problem: when the keypad reported 16 pending events the
>>> >driver took it as 0, and did nothing. This in turn caused the keypad to
>>> >re-issue the interrupt over and over again.
>>> >
>>> >Fixes: 9d2e173644bb ('Input: ADP5589 - new driver for I2C Keypad Decoder and I/O Expander')
>>> >Signed-off-by: Guido Martínez <guido@vanguardiasur.com.ar>
>>> Acked-by: Michael Hennerich <michael.hennerich@analog.com>
>>
>
> Dmitry,
>
> Going through this patchset and noticed the "Fixes" tag got dropped when
> the last two fixes were pushed.

I believe "Fixes" tag is interesting when there was a patch that
changed behavior and broke things, not when there is a mistake that
was present since very beginning.

>
> More importantly, the patches haven't been queued for -stable, despite it
> fixes a serious issue.
>
> Can you take care of sending them to -stable or should I do it?
> Upstream commits are:
> c615dcb6d13e Input: adp5589-keys - fix event count mask
> 195e610bf710 Input: adp5589-keys - fix pull mask setting

Given that there were no complaints about this issues ever since the
driver has been accepted into main line (3.0 kernel) and that the
device is not very widely used I do not believe this has to go to
stable. Affected parties can cherry-pick from mainline.

Thanks.

-- 
Dmitry
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/3] Input: adp5589-keys: fix event count mask

[Ezequiel Garcia]

On 5 October 2015 at 21:29, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
> Hi Ezequiel,
>
> On Fri, Oct 2, 2015 at 2:28 PM, Ezequiel Garcia
> <ezequiel@vanguardiasur.com.ar> wrote:
>> On 6 May 2015 at 20:36, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
>>> On Tue, Apr 21, 2015 at 04:36:44PM +0200, Michael Hennerich wrote:
>>>> On 04/21/2015 04:21 PM, Guido Martínez wrote:
>>>> >The event mask was specified as 0xF (4 bits) when in reality is 0x1F (5
>>>> >bits) in order to be capable of representing all FIFO length values from
>>>> >0 to 16.
>>>> >
>>>> >This caused a problem: when the keypad reported 16 pending events the
>>>> >driver took it as 0, and did nothing. This in turn caused the keypad to
>>>> >re-issue the interrupt over and over again.
>>>> >
>>>> >Fixes: 9d2e173644bb ('Input: ADP5589 - new driver for I2C Keypad Decoder and I/O Expander')
>>>> >Signed-off-by: Guido Martínez <guido@vanguardiasur.com.ar>
>>>> Acked-by: Michael Hennerich <michael.hennerich@analog.com>
>>>
>>
>> Dmitry,
>>
>> Going through this patchset and noticed the "Fixes" tag got dropped when
>> the last two fixes were pushed.
>
> I believe "Fixes" tag is interesting when there was a patch that
> changed behavior and broke things, not when there is a mistake that
> was present since very beginning.
>

If anything else, the Fixes tag clearly states the commit fixes something
(if the commit log is not clear enough). And if the bug was there from the
very beginning, so be it. The tag is only clarifying that.

I'd say the more information about the commit, the better.

>>
>> More importantly, the patches haven't been queued for -stable, despite it
>> fixes a serious issue.
>>
>> Can you take care of sending them to -stable or should I do it?
>> Upstream commits are:
>> c615dcb6d13e Input: adp5589-keys - fix event count mask
>> 195e610bf710 Input: adp5589-keys - fix pull mask setting
>
> Given that there were no complaints about this issues ever since the
> driver has been accepted into main line (3.0 kernel) and that the
> device is not very widely used I do not believe this has to go to
> stable. Affected parties can cherry-pick from mainline.
>

Well, the event triggering the bug was quite marginal, so it's fairly
hard to hit
it in real life.

On the other side, the bug itself was a very subtle typo
in the mask value, and took a lot of effort to catch. I thought that effort
worth a simple backport to stable.

Having a bug in mainline, and deciding not to backport it to stable,
seems wrong to me, despite lack of (known) complaints and
despite how unused a driver may appear to be.

Anyway, it's your call. I'm happy with whatever you decide.
-- 
Ezequiel García, VanguardiaSur
www.vanguardiasur.com.ar
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/3] adp5589 fixes

[Guido Martínez]

Hi Dmitry,

On Tue, Apr 21, 2015 at 11:21:27AM -0300, Guido Martínez wrote:
> Guido Martínez (3):
>   Input: adp5589-keys: handle i2c errors on the irq handler
>   Input: adp5589-keys: fix pull mask setting
>   Input: adp5589-keys: fix event count mask
Did you have time to look at this patchset? Any comments on it?

Thank you!

-- 
Guido Martínez, VanguardiaSur
www.vanguardiasur.com.ar
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html