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

[Philippe Mathieu-Daudé]

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 <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?
>
> 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é <address@hidden>

> > > +        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