diff mbox series

[v3,3/6] ps2: accept 'Set Key Make and Break' commands

Message ID 20191022205941.23152-4-svens@stackframe.org (mailing list archive)
State New, archived
Headers show
Series None | expand

Commit Message

Sven Schnelle Oct. 22, 2019, 8:59 p.m. UTC 
HP-UX sends both the 'Set key make and break (0xfc) and
'Set all key typematic make and break' (0xfa). QEMU response
with 'Resend' as it doesn't handle these commands. HP-UX than
reports an PS/2 max retransmission exceeded error. Add these
commands and just reply with ACK.

Signed-off-by: Sven Schnelle <svens@stackframe.org>
---
 hw/input/ps2.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

Comments

Philippe Mathieu-Daudé Oct. 23, 2019, 11:08 a.m. UTC | #1 
Hi Sven,

(Please Cc reviewers who previously commented your patch)

On 10/22/19 10:59 PM, Sven Schnelle wrote:
> HP-UX sends both the 'Set key make and break (0xfc) and
> 'Set all key typematic make and break' (0xfa). QEMU response
> with 'Resend' as it doesn't handle these commands. HP-UX than
> reports an PS/2 max retransmission exceeded error. Add these
> commands and just reply with ACK.
> 
> Signed-off-by: Sven Schnelle <svens@stackframe.org>
> ---
>   hw/input/ps2.c | 10 ++++++++++
>   1 file changed, 10 insertions(+)
> 
> diff --git a/hw/input/ps2.c b/hw/input/ps2.c
> index 67f92f6112..0b671b6339 100644
> --- a/hw/input/ps2.c
> +++ b/hw/input/ps2.c
> @@ -49,6 +49,8 @@
>   #define KBD_CMD_RESET_DISABLE	0xF5	/* reset and disable scanning */
>   #define KBD_CMD_RESET_ENABLE   	0xF6    /* reset and enable scanning */
>   #define KBD_CMD_RESET		0xFF	/* Reset */
> +#define KBD_CMD_SET_MAKE_BREAK  0xFC    /* Set Make and Break mode */
> +#define KBD_CMD_SET_TYPEMATIC   0xFA    /* Set Typematic Make and Break mode */
>   
>   /* Keyboard Replies */
>   #define KBD_REPLY_POR		0xAA	/* Power on reset */
> @@ -573,6 +575,7 @@ void ps2_write_keyboard(void *opaque, int val)
>           case KBD_CMD_SCANCODE:
>           case KBD_CMD_SET_LEDS:
>           case KBD_CMD_SET_RATE:
> +        case KBD_CMD_SET_MAKE_BREAK:

OK

>               s->common.write_cmd = val;
>               ps2_queue(&s->common, KBD_REPLY_ACK);
>               break;
> @@ -592,11 +595,18 @@ void ps2_write_keyboard(void *opaque, int val)
>                   KBD_REPLY_ACK,
>                   KBD_REPLY_POR);
>               break;
> +        case KBD_CMD_SET_TYPEMATIC:
> +            ps2_queue(&s->common, KBD_REPLY_ACK);

I'm not sure, can't this loop?

Can you fold it with the '0x00' case?

> +            break;
>           default:
>               ps2_queue(&s->common, KBD_REPLY_RESEND);
>               break;
>           }
>           break;
> +    case KBD_CMD_SET_MAKE_BREAK:

We can reorder this one in the same case with:

     case KBD_CMD_SET_LEDS:
     case KBD_CMD_SET_RATE:

> +        ps2_queue(&s->common, KBD_REPLY_ACK);
> +        s->common.write_cmd = -1;
> +        break;
>       case KBD_CMD_SCANCODE:
>           if (val == 0) {
>               if (s->common.queue.count <= PS2_QUEUE_SIZE - 2) {
>
Sven Schnelle Oct. 23, 2019, 12:08 p.m. UTC | #2 
Hi Philippe,

On Wed, Oct 23, 2019 at 01:08:35PM +0200, Philippe Mathieu-Daudé wrote:
> Hi Sven,
> 
> (Please Cc reviewers who previously commented your patch)
>
> On 10/22/19 10:59 PM, Sven Schnelle wrote:
> > HP-UX sends both the 'Set key make and break (0xfc) and
> > 'Set all key typematic make and break' (0xfa). QEMU response
> > with 'Resend' as it doesn't handle these commands. HP-UX than
> > reports an PS/2 max retransmission exceeded error. Add these
> > commands and just reply with ACK.
> > 
> > Signed-off-by: Sven Schnelle <svens@stackframe.org>
> > ---
> >   hw/input/ps2.c | 10 ++++++++++
> >   1 file changed, 10 insertions(+)
> > 
> > diff --git a/hw/input/ps2.c b/hw/input/ps2.c
> > index 67f92f6112..0b671b6339 100644
> > --- a/hw/input/ps2.c
> > +++ b/hw/input/ps2.c
> > @@ -49,6 +49,8 @@
> >   #define KBD_CMD_RESET_DISABLE	0xF5	/* reset and disable scanning */
> >   #define KBD_CMD_RESET_ENABLE   	0xF6    /* reset and enable scanning */
> >   #define KBD_CMD_RESET		0xFF	/* Reset */
> > +#define KBD_CMD_SET_MAKE_BREAK  0xFC    /* Set Make and Break mode */
> > +#define KBD_CMD_SET_TYPEMATIC   0xFA    /* Set Typematic Make and Break mode */
> >   /* Keyboard Replies */
> >   #define KBD_REPLY_POR		0xAA	/* Power on reset */
> > @@ -573,6 +575,7 @@ void ps2_write_keyboard(void *opaque, int val)
> >           case KBD_CMD_SCANCODE:
> >           case KBD_CMD_SET_LEDS:
> >           case KBD_CMD_SET_RATE:
> > +        case KBD_CMD_SET_MAKE_BREAK:
> 
> OK
> 
> >               s->common.write_cmd = val;
> >               ps2_queue(&s->common, KBD_REPLY_ACK);
> >               break;
> > @@ -592,11 +595,18 @@ void ps2_write_keyboard(void *opaque, int val)
> >                   KBD_REPLY_ACK,
> >                   KBD_REPLY_POR);
> >               break;
> > +        case KBD_CMD_SET_TYPEMATIC:
> > +            ps2_queue(&s->common, KBD_REPLY_ACK);
> 
> I'm not sure, can't this loop?

I don't see how?

> Can you fold it with the '0x00' case?

Ok.

> > +            break;
> >           default:
> >               ps2_queue(&s->common, KBD_REPLY_RESEND);
> >               break;
> >           }
> >           break;
> > +    case KBD_CMD_SET_MAKE_BREAK:
> 
> We can reorder this one in the same case with:
> 
>     case KBD_CMD_SET_LEDS:
>     case KBD_CMD_SET_RATE:

Ok.

> > +        ps2_queue(&s->common, KBD_REPLY_ACK);
> > +        s->common.write_cmd = -1;
> > +        break;
> >       case KBD_CMD_SCANCODE:
> >           if (val == 0) {
> >               if (s->common.queue.count <= PS2_QUEUE_SIZE - 2) {
> > 
> 

Regards
Sven
Philippe Mathieu-Daudé Oct. 23, 2019, 12:32 p.m. UTC | #3 
On 10/23/19 2:08 PM, Sven Schnelle wrote:
> Hi Philippe,
> 
> On Wed, Oct 23, 2019 at 01:08:35PM +0200, Philippe Mathieu-Daudé wrote:
>> Hi Sven,
>>
>> (Please Cc reviewers who previously commented your patch)
>>
>> On 10/22/19 10:59 PM, Sven Schnelle wrote:
>>> HP-UX sends both the 'Set key make and break (0xfc) and
>>> 'Set all key typematic make and break' (0xfa). QEMU response
>>> with 'Resend' as it doesn't handle these commands. HP-UX than
>>> reports an PS/2 max retransmission exceeded error. Add these
>>> commands and just reply with ACK.
>>>
>>> Signed-off-by: Sven Schnelle <svens@stackframe.org>
>>> ---
>>>    hw/input/ps2.c | 10 ++++++++++
>>>    1 file changed, 10 insertions(+)
>>>
>>> diff --git a/hw/input/ps2.c b/hw/input/ps2.c
>>> index 67f92f6112..0b671b6339 100644
>>> --- a/hw/input/ps2.c
>>> +++ b/hw/input/ps2.c
>>> @@ -49,6 +49,8 @@
>>>    #define KBD_CMD_RESET_DISABLE	0xF5	/* reset and disable scanning */
>>>    #define KBD_CMD_RESET_ENABLE   	0xF6    /* reset and enable scanning */
>>>    #define KBD_CMD_RESET		0xFF	/* Reset */
>>> +#define KBD_CMD_SET_MAKE_BREAK  0xFC    /* Set Make and Break mode */
>>> +#define KBD_CMD_SET_TYPEMATIC   0xFA    /* Set Typematic Make and Break mode */
>>>    /* Keyboard Replies */
>>>    #define KBD_REPLY_POR		0xAA	/* Power on reset */
>>> @@ -573,6 +575,7 @@ void ps2_write_keyboard(void *opaque, int val)
>>>            case KBD_CMD_SCANCODE:
>>>            case KBD_CMD_SET_LEDS:
>>>            case KBD_CMD_SET_RATE:
>>> +        case KBD_CMD_SET_MAKE_BREAK:
>>
>> OK
>>
>>>                s->common.write_cmd = val;
>>>                ps2_queue(&s->common, KBD_REPLY_ACK);
>>>                break;
>>> @@ -592,11 +595,18 @@ void ps2_write_keyboard(void *opaque, int val)
>>>                    KBD_REPLY_ACK,
>>>                    KBD_REPLY_POR);
>>>                break;
>>> +        case KBD_CMD_SET_TYPEMATIC:
>>> +            ps2_queue(&s->common, KBD_REPLY_ACK);
>>
>> I'm not sure, can't this loop?
> 
> I don't see how?

I misunderstood the two switch statements.

>> Can you fold it with the '0x00' case?
> 
> Ok.
> 
>>> +            break;
>>>            default:
>>>                ps2_queue(&s->common, KBD_REPLY_RESEND);
>>>                break;
>>>            }
>>>            break;
>>> +    case KBD_CMD_SET_MAKE_BREAK:
>>
>> We can reorder this one in the same case with:
>>
>>      case KBD_CMD_SET_LEDS:
>>      case KBD_CMD_SET_RATE:
> 
> Ok.

With these changes:
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

> 
>>> +        ps2_queue(&s->common, KBD_REPLY_ACK);
>>> +        s->common.write_cmd = -1;
>>> +        break;
>>>        case KBD_CMD_SCANCODE:
>>>            if (val == 0) {
>>>                if (s->common.queue.count <= PS2_QUEUE_SIZE - 2) {
>>>
>>
> 
> Regards
> Sven
>
diff mbox series

Patch

diff --git a/hw/input/ps2.c b/hw/input/ps2.c
index 67f92f6112..0b671b6339 100644
--- a/hw/input/ps2.c
+++ b/hw/input/ps2.c
@@ -49,6 +49,8 @@ 
 #define KBD_CMD_RESET_DISABLE	0xF5	/* reset and disable scanning */
 #define KBD_CMD_RESET_ENABLE   	0xF6    /* reset and enable scanning */
 #define KBD_CMD_RESET		0xFF	/* Reset */
+#define KBD_CMD_SET_MAKE_BREAK  0xFC    /* Set Make and Break mode */
+#define KBD_CMD_SET_TYPEMATIC   0xFA    /* Set Typematic Make and Break mode */
 
 /* Keyboard Replies */
 #define KBD_REPLY_POR		0xAA	/* Power on reset */
@@ -573,6 +575,7 @@  void ps2_write_keyboard(void *opaque, int val)
         case KBD_CMD_SCANCODE:
         case KBD_CMD_SET_LEDS:
         case KBD_CMD_SET_RATE:
+        case KBD_CMD_SET_MAKE_BREAK:
             s->common.write_cmd = val;
             ps2_queue(&s->common, KBD_REPLY_ACK);
             break;
@@ -592,11 +595,18 @@  void ps2_write_keyboard(void *opaque, int val)
                 KBD_REPLY_ACK,
                 KBD_REPLY_POR);
             break;
+        case KBD_CMD_SET_TYPEMATIC:
+            ps2_queue(&s->common, KBD_REPLY_ACK);
+            break;
         default:
             ps2_queue(&s->common, KBD_REPLY_RESEND);
             break;
         }
         break;
+    case KBD_CMD_SET_MAKE_BREAK:
+        ps2_queue(&s->common, KBD_REPLY_ACK);
+        s->common.write_cmd = -1;
+        break;
     case KBD_CMD_SCANCODE:
         if (val == 0) {
             if (s->common.queue.count <= PS2_QUEUE_SIZE - 2) {