Re: [Qemu-devel] [PATCH] ps2: set ps/2 output buffer size as the same as kernel

[Gonglei (Arei)]

> Subject: Re: [PATCH] ps2: set ps/2 output buffer size as the same as kernel
> 
> > @@ -137,7 +139,7 @@ void ps2_queue(void *opaque, int b)
> >      PS2State *s = (PS2State *)opaque;
> >      PS2Queue *q = &s->queue;
> >
> > -    if (q->count >= PS2_QUEUE_SIZE)
> > +    if (q->count >= PS2_QUEUE_SIZE - 1)
> 
> Why?
> 
Because when the queue buffer is 16 bytes (full), the i8042_controller_check()
will report the error in linux kernel code. And the testing results show 
decreasing 1 is necessary.
 
> >      if (!(s->mouse_status & MOUSE_STATUS_REMOTE) &&
> > -        (s->common.queue.count < (PS2_QUEUE_SIZE - 16))) {
> > +        (s->common.queue.count < PS2_QUEUE_SIZE)) {
> 
> To me this looks like an attempt to make sure the queue has enough space
> for the whole mouse message.  Message size is 3 or 4 bytes (depending on
> mode), so I think we should make that "... < (PS2_QUEUE_SIZE-4)".
> 
TBH, I don't understand the reason, and I haven't found any patches 
about " PS2_QUEUE_SIZE - 16". But I quite agree with you, and I will test it.
  
> >          for(;;) {
> >              /* if not remote, send event. Multiple events are sent if
> >                 too big deltas */
> 
> ... and move the check into the loop.  Or, maybe even better, into the
> ps2_mouse_send_packet() function.
> 
OK.
> > +    for (i = 0; i < size; i++) {
> > +        /* move the queue elements to the temporary buffer */
> > +        tmp_data[i] = q->data[q->rptr];
> > +        if (++q->rptr == 256) {
> > +            q->rptr = 0;
> > +        }
> > +    }
> > +    /* move the queue elements to the start of data array */
> > +    if (size > 0) {
> > +        memcpy(q->data, tmp_data, size);
> > +    }
> 
> You can move the loop into the "if (size > 0) { ... }" section.
> 
Agreed.

> >  static const VMStateDescription vmstate_ps2_mouse = {
> >      .name = "ps2mouse",
> >      .version_id = 2,
> >      .minimum_version_id = 2,
> >      .minimum_version_id_old = 2,
> > +    .post_load = ps2_mouse_post_load,
> 
> You have to call ps2_common_post_load in pre_save too.
> 
OK. I will rework the patch and test it. Thanks, Gerd.


Best regards,
-Gonglei