| Message ID | 20150517160052.6B12849ED81@webmail.sinamail.sina.com.cn |
|---|---|
| State | New |
| Headers | show |
On 05/17/2015 10:00 AM, penghao122@sina.com wrote: > Subject: [PATCH] keyboard: handle ps2 typing buffer overrun This line is redundant. Also, you have tried to send this patch more than once. Please be sure to include a proper 'v3' or 'v4' in the subject line, as appropriate. > Starting a linux guest with ps2 keyboard, if you type many times during leaving > grub and into linux kernel,then you can't use keyboard after linux initialization finished. > Specally when you setup linux guest from iso file,you will type in grub. s/Specally/Specifically,/ s/file,you/file, you/ > During grub,the work method of ps2 keyboard is like this: > First, ps2 keyboard driver send command KBD_CCMD_KBD_ENABLE. > Second, if there is a keyboard input, then ps2 keyboard driver read data. > Third, ps2 keyboard driver send command KBD_CCMD_KBD_ENABLE again. > After leaving grub and before finishing linux kernel ps2 driver initialization, > if you type many times, the input data keep saving in ps2 queue of qemu. > Before linux kernel initialize ps2 keyboard,linux call i8042_controller_check, s/initialize/initializes/ s/keyboard,linux call/keyboard, linux calls/ > I8042_STR_OBF. Then i8042_flush() will return I8042_BUFFER_SIZE. > Signed-off-by: Hao Peng <penghao122@sina.com> Usually a blank line between the main commit body and the trailing tags like Signed-off-by. > --- > hw/input/pckbd.c | 11 +++++++++-- > hw/input/ps2.c | 7 +++++++ > include/hw/input/ps2.h | 1 + > 3 files changed, 17 insertions(+), 2 deletions(-) > diff --git a/hw/input/pckbd.c b/hw/input/pckbd.c > index 9b9a7d7..1253b04 100644 > --- a/hw/input/pckbd.c > +++ b/hw/input/pckbd.c > @@ -207,6 +207,8 @@ static uint64_t kbd_read_status(void *opaque, hwaddr addr, > KBDState *s = opaque; > int val; > val = s->status; > + if(s->write_cmd == KBD_CCMD_KBD_ENABLE) > + val &= ~KBD_STAT_OBF; Missing {}, and please use space after keywords like 'if'. Please run ./scripts/checkpatch.pl on your submission. > DPRINTF("kbd: read status=0x%02x\n", val); > return val; > } > @@ -251,9 +253,10 @@ static void kbd_write_command(void *opaque, hwaddr addr, > else > val = KBD_CCMD_NO_OP; > } > - > + s->write_cmd = 0; > switch(val) { Pre-existing, but as long as you are touching this code, it is worth putting a space after the keyword 'switch'. > @@ -364,7 +368,10 @@ static void kbd_write_data(void *opaque, hwaddr addr, > default: > break; > } > - s->write_cmd = 0; > + if(s->write_cmd == KBD_CCMD_WRITE_MODE && s->mode == 0x61) > + s->write_cmd = KBD_CCMD_KBD_ENABLE; > + else > + s->write_cmd = 0; More missing {}, and another missing space after 'if'.
On 05/18/2015 06:54 AM, Eric Blake wrote: > On 05/17/2015 10:00 AM, penghao122@sina.com wrote: >> Subject: [PATCH] keyboard: handle ps2 typing buffer overrun > > This line is redundant. > > Also, you have tried to send this patch more than once. Please be sure > to include a proper 'v3' or 'v4' in the subject line, as appropriate. > >> Starting a linux guest with ps2 keyboard, if you type many times during leaving >> grub and into linux kernel,then you can't use keyboard after linux initialization finished. >> Specally when you setup linux guest from iso file,you will type in grub. > > s/Specally/Specifically,/ > s/file,you/file, you/ In fact, I called these out on your v2 submission. https://lists.gnu.org/archive/html/qemu-devel/2015-05/msg00938.html Sending a v3 without addressing points made in v2 is not good - it wastes everyone's time, and makes it more likely that your patch will be intentionally overlooked.
Okay, while the patch has been criticised by Eric already, mostly due to the commit message and stylistic errors in the code, but what about the code changed in this patch, are the changes needed, correct? I remember a discussion on a related topic, http://thread.gmane.org/gmane.comp.emulators.qemu/292614 , and for example this reply by Gerd (Cc'd): http://thread.gmane.org/gmane.comp.emulators.qemu/292614/focus=292921 Maybe the patch below can help? Thanks, /mjt [Quoting original message in full] 17.05.2015 19:00, penghao122@sina.com wrote: > Subject: [PATCH] keyboard: handle ps2 typing buffer overrun > > Starting a linux guest with ps2 keyboard, if you type many times during leaving > grub and into linux kernel,then you can't use keyboard after linux initialization finished. > Specally when you setup linux guest from iso file,you will type in grub. > During grub,the work method of ps2 keyboard is like this: > First, ps2 keyboard driver send command KBD_CCMD_KBD_ENABLE. > Second, if there is a keyboard input, then ps2 keyboard driver read data. > Third, ps2 keyboard driver send command KBD_CCMD_KBD_ENABLE again. > > After leaving grub and before finishing linux kernel ps2 driver initialization, > if you type many times, the input data keep saving in ps2 queue of qemu. > Before linux kernel initialize ps2 keyboard,linux call i8042_controller_check, > if i8042_controller_check return fail, then ps2 keyboard driver will never initialize. > (i8042.c in kernel 2.6.32 ) > static int i8042_controller_check(void) > { > if (i8042_flush() == I8042_BUFFER_SIZE) > return -ENODEV; > return 0; > } > static int i8042_flush(void) > { > ... > while (((str = i8042_read_status()) & I8042_STR_OBF) && (i < I8042_BUFFER_SIZE)) { > udelay(50); > data = i8042_read_data(); > i++; > } > return i; > } > During calling i8042_flush it is full in ps2 queue of qemu. ps_read_data will execute > kbd_update_irq(s->update_arg, q->count != 0). Because q->count!=0, kbd_update_irq can set > I8042_STR_OBF. Then i8042_flush() will return I8042_BUFFER_SIZE. > > Signed-off-by: Hao Peng <penghao122@sina.com <mailto:penghao122@sina.com>> > --- > hw/input/pckbd.c | 11 +++++++++-- > hw/input/ps2.c | 7 +++++++ > include/hw/input/ps2.h | 1 + > 3 files changed, 17 insertions(+), 2 deletions(-) > > diff --git a/hw/input/pckbd.c b/hw/input/pckbd.c > index 9b9a7d7..1253b04 100644 > --- a/hw/input/pckbd.c > +++ b/hw/input/pckbd.c > @@ -207,6 +207,8 @@ static uint64_t kbd_read_status(void *opaque, hwaddr addr, > KBDState *s = opaque; > int val; > val = s->status; > + if(s->write_cmd == KBD_CCMD_KBD_ENABLE) > + val &= ~KBD_STAT_OBF; > DPRINTF("kbd: read status=0x%02x\n", val); > return val; > } > @@ -251,9 +253,10 @@ static void kbd_write_command(void *opaque, hwaddr addr, > else > val = KBD_CCMD_NO_OP; > } > - > + s->write_cmd = 0; > switch(val) { > case KBD_CCMD_READ_MODE: > + ps2_clear_queue(s->kbd); > kbd_queue(s, s->mode, 0); > break; > case KBD_CCMD_WRITE_MODE: > @@ -284,6 +287,7 @@ static void kbd_write_command(void *opaque, hwaddr addr, > kbd_update_irq(s); > break; > case KBD_CCMD_KBD_ENABLE: > + s->write_cmd = KBD_CCMD_KBD_ENABLE; > s->mode &= ~KBD_MODE_DISABLE_KBD; > kbd_update_irq(s); > break; > @@ -364,7 +368,10 @@ static void kbd_write_data(void *opaque, hwaddr addr, > default: > break; > } > - s->write_cmd = 0; > + if(s->write_cmd == KBD_CCMD_WRITE_MODE && s->mode == 0x61) > + s->write_cmd = KBD_CCMD_KBD_ENABLE; > + else > + s->write_cmd = 0; > } > > static void kbd_reset(void *opaque) > diff --git a/hw/input/ps2.c b/hw/input/ps2.c > index 4baeea2..b7c72bb 100644 > --- a/hw/input/ps2.c > +++ b/hw/input/ps2.c > @@ -151,6 +151,13 @@ void ps2_queue(void *opaque, int b) > s->update_irq(s->update_arg, 1); > } > > +void ps2_clear_queue(void *opaque) > +{ > + PS2State *s = (PS2State *)opaque; > + PS2Queue *q = &s->queue; > + q->wptr = q->rptr = q->count = 0; > +} > + > /* > keycode is expressed as follow: > bit 7 - 0 key pressed, 1 = key released > diff --git a/include/hw/input/ps2.h b/include/hw/input/ps2.h > index 7c45ce7..7bd9158 100644 > --- a/include/hw/input/ps2.h > +++ b/include/hw/input/ps2.h > @@ -32,6 +32,7 @@ void ps2_write_mouse(void *, int val); > void ps2_write_keyboard(void *, int val); > uint32_t ps2_read_data(void *); > void ps2_queue(void *, int b); > +void ps2_clear_queue(void *opaque); > void ps2_keyboard_set_translation(void *opaque, int mode); > void ps2_mouse_fake_event(void *opaque); > > --
diff --git a/hw/input/pckbd.c b/hw/input/pckbd.c index 9b9a7d7..1253b04 100644 --- a/hw/input/pckbd.c +++ b/hw/input/pckbd.c @@ -207,6 +207,8 @@ static uint64_t kbd_read_status(void *opaque, hwaddr addr, KBDState *s = opaque; int val; val = s->status; + if(s->write_cmd == KBD_CCMD_KBD_ENABLE) + val &= ~KBD_STAT_OBF; DPRINTF("kbd: read status=0x%02x\n", val); return val; } @@ -251,9 +253,10 @@ static void kbd_write_command(void *opaque, hwaddr addr, else val = KBD_CCMD_NO_OP; } - + s->write_cmd = 0; switch(val) { case KBD_CCMD_READ_MODE: + ps2_clear_queue(s->kbd); kbd_queue(s, s->mode, 0); break; case KBD_CCMD_WRITE_MODE: @@ -284,6 +287,7 @@ static void kbd_write_command(void *opaque, hwaddr addr, kbd_update_irq(s); break; case KBD_CCMD_KBD_ENABLE: + s->write_cmd = KBD_CCMD_KBD_ENABLE; s->mode &= ~KBD_MODE_DISABLE_KBD; kbd_update_irq(s); break; @@ -364,7 +368,10 @@ static void kbd_write_data(void *opaque, hwaddr addr, default: break; } - s->write_cmd = 0; + if(s->write_cmd == KBD_CCMD_WRITE_MODE && s->mode == 0x61) + s->write_cmd = KBD_CCMD_KBD_ENABLE; + else + s->write_cmd = 0; } static void kbd_reset(void *opaque) diff --git a/hw/input/ps2.c b/hw/input/ps2.c index 4baeea2..b7c72bb 100644 --- a/hw/input/ps2.c +++ b/hw/input/ps2.c @@ -151,6 +151,13 @@ void ps2_queue(void *opaque, int b) s->update_irq(s->update_arg, 1); } +void ps2_clear_queue(void *opaque) +{ + PS2State *s = (PS2State *)opaque; + PS2Queue *q = &s->queue; + q->wptr = q->rptr = q->count = 0; +} + /* keycode is expressed as follow: bit 7 - 0 key pressed, 1 = key released diff --git a/include/hw/input/ps2.h b/include/hw/input/ps2.h index 7c45ce7..7bd9158 100644 --- a/include/hw/input/ps2.h +++ b/include/hw/input/ps2.h @@ -32,6 +32,7 @@ void ps2_write_mouse(void *, int val); void ps2_write_keyboard(void *, int val); uint32_t ps2_read_data(void *); void ps2_queue(void *, int b); +void ps2_clear_queue(void *opaque); void ps2_keyboard_set_translation(void *opaque, int mode); void ps2_mouse_fake_event(void *opaque);
Subject: [PATCH] keyboard: handle ps2 typing buffer overrun Starting a linux guest with ps2 keyboard, if you type many times during leaving grub and into linux kernel,then you can't use keyboard after linux initialization finished. Specally when you setup linux guest from iso file,you will type in grub. During grub,the work method of ps2 keyboard is like this: First, ps2 keyboard driver send command KBD_CCMD_KBD_ENABLE. Second, if there is a keyboard input, then ps2 keyboard driver read data. Third, ps2 keyboard driver send command KBD_CCMD_KBD_ENABLE again. After leaving grub and before finishing linux kernel ps2 driver initialization, if you type many times, the input data keep saving in ps2 queue of qemu. Before linux kernel initialize ps2 keyboard,linux call i8042_controller_check, if i8042_controller_check return fail, then ps2 keyboard driver will never initialize. (i8042.c in kernel 2.6.32 ) static int i8042_controller_check(void) { if (i8042_flush() == I8042_BUFFER_SIZE) return -ENODEV; return 0; } static int i8042_flush(void) { ... while (((str = i8042_read_status()) & I8042_STR_OBF) && (i < I8042_BUFFER_SIZE)) { udelay(50); data = i8042_read_data(); i++; } return i; } During calling i8042_flush it is full in ps2 queue of qemu. ps_read_data will execute kbd_update_irq(s->update_arg, q->count != 0). Because q->count!=0, kbd_update_irq can set I8042_STR_OBF. Then i8042_flush() will return I8042_BUFFER_SIZE. Signed-off-by: Hao Peng <penghao122@sina.com> --- hw/input/pckbd.c | 11 +++++++++-- hw/input/ps2.c | 7 +++++++ include/hw/input/ps2.h | 1 + 3 files changed, 17 insertions(+), 2 deletions(-)