| Message ID | 1307021895-20434-1-git-send-email-penberg@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
* 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 -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
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 -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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; }
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(-)