Skip to contentSkip to conversation

Fix some bugs in the i8402 driver when no controller is present

PATCH New Gabe Black 2 participants 7 messages Nov 8, 2011 – Nov 30, 2011 Input drivers

lore
Share & tools
lore

Gabe Black

author
#0 lore raw

PATCH 1/1 Fix some bugs in the i8402 driver when no controller is present

If no controller is present, the i8402 driver should return immediately and not attempt to operate on the missing hardware.

In kbd_input_empty, the status register is checked every millisecond to see whether the input buffer is empty, up to a timeout which is tracked by decrimenting a counter each time the check is performed. The decrement is performed with a postfix -- operator, and the value of the counter is checked in place. That means that when the counter reaches zero and the loop terminates, it will actually be decrimented one more time and become -1. That value is returned as the return value of the function. That would give the right answer if it wasn't for that extra decrement because a timeout would indicate that the buffer never became empty.

This change fixes both of those bugs.

Signed-off-by Gabe Black <[email protected]>
---
 drivers/input/i8042.c |   12 +++++++++++-
 1 files changed, 11 insertions(+), 1 deletions(-)
drivers/input/i8042.c +11 -1
index 58094c9..3bdfa7d 100644
@@ -315,6 +315,13 @@ static unsigned char ext_key_map [] =
     0x00    /* map end */
     };
 
+/******************************************************************************/
+
+static int kbd_controller_present (void)
+{
+    return in8(I8042_STATUS_REG) != 0xff;
+}
+
 /*******************************************************************************
  *
  * i8042_kbd_init - reset keyboard and init state flags

@@ -324,6 +331,9 @@ int i8042_kbd_init (void)
     int keymap, try;
     char *penv;
 
+    if (!kbd_controller_present())
+        return -1;
+
 #ifdef CONFIG_USE_CPCIDVI
     if ((penv = getenv ("console")) != NULL) {
 	    if (strncmp (penv, "serial", 7) == 0) {

@@ -629,7 +639,7 @@ static int kbd_input_empty (void)
     while ((in8 (I8042_STATUS_REG) & 0x02) && kbdTimeout--)
 	udelay(1000);
 
-    return kbdTimeout;
+    return kbdTimeout != -1;
 }
 
 /******************************************************************************/
Signature · 1 line
1.7.3.1
#1 lore raw

PATCH 1/1 Fix some bugs in the i8402 driver when no controller is present

Hi Gabe,

Quoted 13 lines · Gabe Black

If no controller is present, the i8402 driver should return immediately and not attempt to operate on the missing hardware.

In kbd_input_empty, the status register is checked every millisecond to see whether the input buffer is empty, up to a timeout which is tracked by decrimenting a counter each time the check is performed. The decrement is performed with a postfix -- operator, and the value of the counter is checked in place. That means that when the counter reaches zero and the loop terminates, it will actually be decrimented one more time and become -1. That value is returned as the return value of the function. That would give the right answer if it wasn't for that extra decrement because a timeout would indicate that the buffer never became empty.

Quoted 7 lines

This change fixes both of those bugs.

Signed-off-by Gabe Black <[email protected]>
---
 drivers/input/i8042.c |   12 +++++++++++-
 1 files changed, 11 insertions(+), 1 deletions(-)

total: 1 errors, 5 warnings, 30 lines checked

Patch is not checkpatch clean - Can you please fix checkpatch issues and change tag to 'x86:' style and resubmit

Thanks,

Graeme

author
#2 lore raw

PATCH 1/1 Fix some bugs in the i8402 driver when no controller is present

Quoted 27 lines · Graeme Russ

Hi Gabe,

Quoted 1 line · Gabe Black

If no controller is present, the i8402 driver should return immediately

and

Quoted 3 lines

not attempt to operate on the missing hardware.

In kbd_input_empty, the status register is checked every millisecond to

see

Quoted 6 lines

whether the input buffer is empty, up to a timeout which is tracked by decrimenting a counter each time the check is performed. The decrement is performed with a postfix -- operator, and the value of the counter is checked in place. That means that when the counter reaches zero and the loop terminates, it will actually be decrimented one more time and become -1. That value is returned as the return value of the function. That

would

Quoted 10 lines

give the right answer if it wasn't for that extra decrement because a timeout would indicate that the buffer never became empty.

This change fixes both of those bugs.

Signed-off-by Gabe Black <[email protected]>
---
 drivers/input/i8042.c |   12 +++++++++++-
 1 files changed, 11 insertions(+), 1 deletions(-)
Quoted 10 lines

total: 1 errors, 5 warnings, 30 lines checked

Patch is not checkpatch clean - Can you please fix checkpatch issues and change tag to 'x86:' style and resubmit

Thanks,

Graeme

checkpatch doesn't like it because I was consistent with the existing incorrect style in the file. There are three or four options for how to handle this.

1. Match the current style and ignore checkpatch (what I've already done).
2. Fix the style on just the lines I've modified.
3. Fix the style within a "local" area around my changes, for some definition of local, along with my changes.
4. Submit a separate cleanup patch which fixes the style first.

What is the standard way of approaching this sort of situation? I'm ok with option 1 if you are since I already have that ready to go, and I'd be fine with 4 if you'd like to clean up this file first like you've been cleaning up some others. 2 and 3 would make the style in that file inconsistent and harder to read, but if that's standard practice I'll do that.

Gabe

#3 lore raw

PATCH 1/1 Fix some bugs in the i8402 driver when no controller is present

Hi Gabe,

Quoted 53 lines · Gabe Black

On Sat, Nov 12, 2011 at 2:26 AM, Graeme Russ <[email protected] <mailto:[email protected]>> wrote:

Hi Gabe,

On 08/11/11 20:48, Gabe Black wrote:
> If no controller is present, the i8402 driver should return immediately and
> not attempt to operate on the missing hardware.
>
> In kbd_input_empty, the status register is checked every millisecond to see
> whether the input buffer is empty, up to a timeout which is tracked by > decrimenting a counter each time the check is performed. The decrement is > performed with a postfix -- operator, and the value of the counter is > checked in place. That means that when the counter reaches zero and the > loop terminates, it will actually be decrimented one more time and become > -1. That value is returned as the return value of the function. That would
> give the right answer if it wasn't for that extra decrement because a > timeout would indicate that the buffer never became empty.
>
> This change fixes both of those bugs.
>
> Signed-off-by: Gabe Black <[email protected] <mailto:[email protected]>>
> ---
> drivers/input/i8042.c | 12 +++++++++++-
> 1 files changed, 11 insertions(+), 1 deletions(-)
>

total: 1 errors, 5 warnings, 30 lines checked

Patch is not checkpatch clean - Can you please fix checkpatch issues and change tag to 'x86:' style and resubmit

Thanks,

Graeme

checkpatch doesn't like it because I was consistent with the existing incorrect style in the file. There are three or four options for how to handle this.

1. Match the current style and ignore checkpatch (what I've already done).
2. Fix the style on just the lines I've modified.
3. Fix the style within a "local" area around my changes, for some definition of local, along with my changes.
4. Submit a separate cleanup patch which fixes the style first.

Quoted 5 lines

What is the standard way of approaching this sort of situation? I'm ok with option 1 if you are since I already have that ready to go, and I'd be fine with 4 if you'd like to clean up this file first like you've been cleaning up some others. 2 and 3 would make the style in that file inconsistent and harder to read, but if that's standard practice I'll do that.

I think the standard we are now adopting (enforcing) is #4 - A two patch series consisting of a checkpatch cleanup of the file and checkpatch clean patch for the changes. Of course there are exceptions:

- The checkpatch cleanup would be too onerous (touches too many files, the patched file is too large, too close to release) and the bugfix is critical
- The bugfix covers too many files (again,
- The file is from another repository (typically Linux) and the patch has been applied to that repository (in which case, a commit reference is required in the patch summary)

I'm happy for Wolfgang to correct me

Regards,

Graeme

#4 lore raw

PATCH 1/1 Fix some bugs in the i8402 driver when no controller is present

Quoted 1 line · Graeme Russ

Hi Gabe,

[snip]

Quoted 4 lines

- The checkpatch cleanup would be too onerous (touches too many files, the patched file is too large, too close to release) and the bugfix is critical
- The bugfix covers too many files (again,

Oops - forgot to proof-read ;)

Regards,

Graeme

author
#5 lore raw

PATCH v2 1/1 x86: Fix some bugs in the i8402 driver when no controller is present

In reply to Graeme Russ

If no controller is present, the i8402 driver should return immediately and not attempt to operate on the missing hardware.

In kbd_input_empty, the status register is checked every millisecond to see whether the input buffer is empty, up to a timeout which is tracked by decrimenting a counter each time the check is performed. The decrement is performed with a postfix -- operator, and the value of the counter is checked in place. That means that when the counter reaches zero and the loop terminates, it will actually be decrimented one more time and become -1. That value is returned as the return value of the function. That would give the right answer if it wasn't for that extra decrement because a timeout would indicate that the buffer never became empty.

This change fixes both of those bugs.

Signed-off-by Gabe Black <[email protected]>

---
Changes in v2:
- Change summary tag style.
- Rebase onto checkpatch cleaned file.

 drivers/input/i8042.c |   12 +++++++++++-
 1 files changed, 11 insertions(+), 1 deletions(-)
drivers/input/i8042.c +11 -1
index 83b1bf4..c3bc536 100644
@@ -313,6 +313,13 @@ static unsigned char ext_key_map[] = {
 	0x00  /* map end */
 	};
 
+/******************************************************************************/
+
+static int kbd_controller_present(void)
+{
+	return in8(I8042_STATUS_REG) != 0xff;
+}
+
 /*******************************************************************************
  *
  * i8042_kbd_init - reset keyboard and init state flags

@@ -322,6 +329,9 @@ int i8042_kbd_init(void)
 	int keymap, try;
 	char *penv;
 
+	if (!kbd_controller_present())
+		return -1;
+
 #ifdef CONFIG_USE_CPCIDVI
 	penv = getenv("console");
 	if (penv != NULL) {

@@ -603,7 +613,7 @@ static int kbd_input_empty(void)
 	while ((in8(I8042_STATUS_REG) & 0x02) && kbdTimeout--)
 		udelay(1000);
 
-	return kbdTimeout;
+	return kbdTimeout != -1;
 }
 
 /******************************************************************************/
Signature · 1 line
1.7.3.1
#6 lore raw

PATCH v2 1/1 x86: Fix some bugs in the i8402 driver when no controller is present

Hi Gabe,

Quoted 15 lines · Gabe Black

If no controller is present, the i8402 driver should return immediately and not attempt to operate on the missing hardware.

In kbd_input_empty, the status register is checked every millisecond to see whether the input buffer is empty, up to a timeout which is tracked by decrimenting a counter each time the check is performed. The decrement is performed with a postfix -- operator, and the value of the counter is checked in place. That means that when the counter reaches zero and the loop terminates, it will actually be decrimented one more time and become -1. That value is returned as the return value of the function. That would give the right answer if it wasn't for that extra decrement because a timeout would indicate that the buffer never became empty.

This change fixes both of those bugs.

Quoted 8 lines
Signed-off-by Gabe Black <[email protected]>

---
Changes in v2:
- Change summary tag style.
- Rebase onto checkpatch cleaned file.

 drivers/input/i8042.c |   12 +++++++++++-
 1 files changed, 11 insertions(+), 1 deletions(-)

Applied to u-boot-x86/master

Thanks,

Graeme