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

[Philippe Mathieu-Daudé]

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 <address@hidden>
> ---
>   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) {