[PATCH] kvm tools, i8042: Fix device init failure

[PATCH] kvm tools, i8042: Fix device init failure

[Pekka Enberg]

This patch fixes the following device init failure:

  [    0.945942] usbcore: registered new interface driver sisusb
  [    0.947349] i8042: PNP: No PS/2 controller found. Probing ports directly.
  [    0.949033] i8042: [0] 20 -> i8042 (command)
  [    0.950370] i8042: [57]      -- i8042 (timeout)
  [    1.521143] i8042: Can't read CTR while initializing i8042
  [    1.522495] i8042: probe of i8042 failed with error -5

The kbd_out() function was taking 32 bits instead of 8 bits for 'outb'. This
caused kbd_write_command() to receive bogus 'val' which meant that
I8042_CMD_CTL_RCTR case in the switch statement was never executed.

Cc: Ingo Molnar <mingo@elte.hu>
Cc: Sasha Levin <levinsasha928@gmail.com>
Signed-off-by: Pekka Enberg <penberg@kernel.org>
---
 tools/kvm/hw/i8042.c |   48 +++++++++++++++++++++++++++++-------------------
 1 files changed, 29 insertions(+), 19 deletions(-)

diff --git a/tools/kvm/hw/i8042.c b/tools/kvm/hw/i8042.c
index 934ef4e..42ed4ba 100644
--- a/tools/kvm/hw/i8042.c
+++ b/tools/kvm/hw/i8042.c
@@ -140,10 +140,7 @@ static void kbd_queue(u8 c)
 	kbd_update_irq();
 }
 
-/*
- * This function is called when the OS issues a write to port 0x64
- */
-static void kbd_write_command(u32 val)
+static void kbd_write_command(u8 val)
 {
 	switch (val) {
 	case I8042_CMD_CTL_RCTR:
@@ -451,27 +448,40 @@ void kbd_handle_ptr(int buttonMask, int x, int y, rfbClientPtr cl)
  */
 static bool kbd_in(struct ioport *ioport, struct kvm *kvm, u16 port, void *data, int size, u32 count)
 {
-	u32 result;
-
-	if (port == I8042_COMMAND_REG) {
-		result = kbd_read_status();
-		ioport__write8(data, (char)result);
-	} else {
-		result = kbd_read_data();
-		ioport__write32(data, result);
+	switch (port) {
+	case I8042_COMMAND_REG: {
+		u8 value = kbd_read_status();
+		ioport__write8(data, value);
+		break;
+	}
+	case I8042_DATA_REG: {
+		u32 value = kbd_read_data();
+		ioport__write32(data, value);
+		break;
 	}
+	default:
+		break;
+	}
+
 	return true;
 }
 
-/*
- * Called when the OS attempts to read from a keyboard port (0x60 or 0x64)
- */
 static bool kbd_out(struct ioport *ioport, struct kvm *kvm, u16 port, void *data, int size, u32 count)
 {
-	if (port == I8042_COMMAND_REG)
-		kbd_write_command(*((u32 *)data));
-	else
-		kbd_write_data(*((u32 *)data));
+	switch (port) {
+	case I8042_COMMAND_REG: {
+		u8 value = ioport__read8(data);
+		kbd_write_command(value);
+		break;
+	}
+	case I8042_DATA_REG: {
+		u32 value = ioport__read32(data);
+		kbd_write_data(value);
+		break;
+	}
+	default:
+		break;
+	}
 
 	return true;
 }
-- 
1.7.0.4

Re: [PATCH] kvm tools, i8042: Fix device init failure

[Ingo Molnar]

* Pekka Enberg <penberg@kernel.org> wrote:

>  static bool kbd_in(struct ioport *ioport, struct kvm *kvm, u16 port, void *data, int size, u32 count)
>  {
> -	u32 result;
> -
> -	if (port == I8042_COMMAND_REG) {
> -		result = kbd_read_status();
> -		ioport__write8(data, (char)result);
> -	} else {
> -		result = kbd_read_data();
> -		ioport__write32(data, result);
> +	switch (port) {
> +	case I8042_COMMAND_REG: {
> +		u8 value = kbd_read_status();
> +		ioport__write8(data, value);
> +		break;
> +	}
> +	case I8042_DATA_REG: {
> +		u32 value = kbd_read_data();
> +		ioport__write32(data, value);
> +		break;
>  	}
> +	default:
> +		break;

should we BUG_ON() [or WARN_ON()] in that 'default' case?

> +	default:
> +		break;

ditto. This could have caught the bug straight away, right?

Thanks,

	Ingo

Re: [PATCH] kvm tools, i8042: Fix device init failure

[Pekka Enberg]

On Thu, Jun 2, 2011 at 4:40 PM, Ingo Molnar <mingo@elte.hu> wrote:
>> +     case I8042_COMMAND_REG: {
>> +             u8 value = kbd_read_status();
>> +             ioport__write8(data, value);
>> +             break;
>> +     }
>> +     case I8042_DATA_REG: {
>> +             u32 value = kbd_read_data();
>> +             ioport__write32(data, value);
>> +             break;
>>       }
>> +     default:
>> +             break;
>
> should we BUG_ON() [or WARN_ON()] in that 'default' case?
>
>> +     default:
>> +             break;
>
> ditto. This could have caught the bug straight away, right?

I changed them to return false. That WARN_ON should be in common code, I think.

                        Pekka

Re: [PATCH] kvm tools, i8042: Fix device init failure

[Sasha Levin]

On Thu, 2011-06-02 at 16:38 +0300, Pekka Enberg wrote:
> This patch fixes the following device init failure:
> 
>   [    0.945942] usbcore: registered new interface driver sisusb
>   [    0.947349] i8042: PNP: No PS/2 controller found. Probing ports directly.
>   [    0.949033] i8042: [0] 20 -> i8042 (command)
>   [    0.950370] i8042: [57]      -- i8042 (timeout)
>   [    1.521143] i8042: Can't read CTR while initializing i8042
>   [    1.522495] i8042: probe of i8042 failed with error -5
> 
> The kbd_out() function was taking 32 bits instead of 8 bits for 'outb'. This
> caused kbd_write_command() to receive bogus 'val' which meant that
> I8042_CMD_CTL_RCTR case in the switch statement was never executed.
> 
> Cc: Ingo Molnar <mingo@elte.hu>
> Cc: Sasha Levin <levinsasha928@gmail.com>
> Signed-off-by: Pekka Enberg <penberg@kernel.org>
> ---
>  tools/kvm/hw/i8042.c |   48 +++++++++++++++++++++++++++++-------------------
>  1 files changed, 29 insertions(+), 19 deletions(-)
> 
> diff --git a/tools/kvm/hw/i8042.c b/tools/kvm/hw/i8042.c
> index 934ef4e..42ed4ba 100644
> --- a/tools/kvm/hw/i8042.c
> +++ b/tools/kvm/hw/i8042.c
> @@ -140,10 +140,7 @@ static void kbd_queue(u8 c)
>  	kbd_update_irq();
>  }
>  
> -/*
> - * This function is called when the OS issues a write to port 0x64
> - */
> -static void kbd_write_command(u32 val)
> +static void kbd_write_command(u8 val)
>  {
>  	switch (val) {
>  	case I8042_CMD_CTL_RCTR:
> @@ -451,27 +448,40 @@ void kbd_handle_ptr(int buttonMask, int x, int y, rfbClientPtr cl)
>   */
>  static bool kbd_in(struct ioport *ioport, struct kvm *kvm, u16 port, void *data, int size, u32 count)
>  {
> -	u32 result;
> -
> -	if (port == I8042_COMMAND_REG) {
> -		result = kbd_read_status();
> -		ioport__write8(data, (char)result);
> -	} else {
> -		result = kbd_read_data();
> -		ioport__write32(data, result);
> +	switch (port) {
> +	case I8042_COMMAND_REG: {
> +		u8 value = kbd_read_status();
> +		ioport__write8(data, value);
> +		break;
> +	}
> +	case I8042_DATA_REG: {
> +		u32 value = kbd_read_data();
> +		ioport__write32(data, value);
> +		break;
>  	}
> +	default:
> +		break;
> +	}
> +
>  	return true;
>  }
>  
> -/*
> - * Called when the OS attempts to read from a keyboard port (0x60 or 0x64)
> - */
>  static bool kbd_out(struct ioport *ioport, struct kvm *kvm, u16 port, void *data, int size, u32 count)
>  {
> -	if (port == I8042_COMMAND_REG)
> -		kbd_write_command(*((u32 *)data));
> -	else
> -		kbd_write_data(*((u32 *)data));
> +	switch (port) {
> +	case I8042_COMMAND_REG: {
> +		u8 value = ioport__read8(data);
> +		kbd_write_command(value);
> +		break;
> +	}
> +	case I8042_DATA_REG: {
> +		u32 value = ioport__read32(data);
> +		kbd_write_data(value);
> +		break;
> +	}
> +	default:
> +		break;
> +	}
>  
>  	return true;
>  }

Tested it here, works great.
Looks like in my cause, instead of not detecting the keyboard it used to
not detect the mouse sometimes - that's also fixed now.

-- 

Sasha.