diff mbox series

[v2,3/5] msmouse: Use fifo8 instead of array

Message ID 20220908173120.16779-4-arwed.meyer@gmx.de
State New
Headers show
Series Make serial msmouse work | expand

Commit Message

Arwed Meyer Sept. 8, 2022, 5:31 p.m. UTC 
Make use of fifo8 functions instead of implementing own fifo code.
This makes the code more readable and reduces risk of bugs.

Signed-off-by: Arwed Meyer <arwed.meyer@gmx.de>
---
 chardev/msmouse.c | 43 +++++++++++++++++++++----------------------
 1 file changed, 21 insertions(+), 22 deletions(-)

--
2.34.1

Comments

Marc-André Lureau Sept. 9, 2022, 1:18 p.m. UTC | #1 
Hi

On Thu, Sep 8, 2022 at 9:38 PM Arwed Meyer <arwed.meyer@gmx.de> wrote:

> Make use of fifo8 functions instead of implementing own fifo code.
> This makes the code more readable and reduces risk of bugs.
>
> Signed-off-by: Arwed Meyer <arwed.meyer@gmx.de>
>

Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>


> ---
>  chardev/msmouse.c | 43 +++++++++++++++++++++----------------------
>  1 file changed, 21 insertions(+), 22 deletions(-)
>
> diff --git a/chardev/msmouse.c b/chardev/msmouse.c
> index 95fa488339..e9aa3eab55 100644
> --- a/chardev/msmouse.c
> +++ b/chardev/msmouse.c
> @@ -24,6 +24,7 @@
>
>  #include "qemu/osdep.h"
>  #include "qemu/module.h"
> +#include "qemu/fifo8.h"
>  #include "chardev/char.h"
>  #include "chardev/char-serial.h"
>  #include "ui/console.h"
> @@ -34,6 +35,12 @@
>  #define MSMOUSE_HI2(n)  (((n) & 0xc0) >> 6)
>  #define MSMOUSE_PWR(cm) (cm & (CHR_TIOCM_RTS | CHR_TIOCM_DTR))
>
> +/* Serial fifo size. */
> +#define MSMOUSE_BUF_SZ 64
>

Why bump to 64 bytes?


> +
> +/* Mouse ID: Send "M3" cause we behave like a 3 button logitech mouse. */
> +const uint8_t mouse_id[] = {'M', '3'};
> +
>  struct MouseChardev {
>      Chardev parent;
>
> @@ -42,8 +49,7 @@ struct MouseChardev {
>      int axis[INPUT_AXIS__MAX];
>      bool btns[INPUT_BUTTON__MAX];
>      bool btnc[INPUT_BUTTON__MAX];
> -    uint8_t outbuf[32];
> -    int outlen;
> +    Fifo8 outbuf;
>  };
>  typedef struct MouseChardev MouseChardev;
>
> @@ -54,21 +60,15 @@ DECLARE_INSTANCE_CHECKER(MouseChardev, MOUSE_CHARDEV,
>  static void msmouse_chr_accept_input(Chardev *chr)
>  {
>      MouseChardev *mouse = MOUSE_CHARDEV(chr);
> -    int len;
> +    uint32_t len_out, len;
>
> -    len = qemu_chr_be_can_write(chr);
> -    if (len > mouse->outlen) {
> -        len = mouse->outlen;
> -    }
> -    if (!len) {
> +    len_out = qemu_chr_be_can_write(chr);
> +    if (!len_out || fifo8_is_empty(&mouse->outbuf)) {
>          return;
>      }
> -
> -    qemu_chr_be_write(chr, mouse->outbuf, len);
> -    mouse->outlen -= len;
> -    if (mouse->outlen) {
> -        memmove(mouse->outbuf, mouse->outbuf + len, mouse->outlen);
> -    }
> +    len = MIN(fifo8_num_used(&mouse->outbuf), len_out);
> +    qemu_chr_be_write(chr, fifo8_pop_buf(&mouse->outbuf, len, &len_out),
> +            len_out);
>  }
>
>  static void msmouse_queue_event(MouseChardev *mouse)
> @@ -94,12 +94,11 @@ static void msmouse_queue_event(MouseChardev *mouse)
>          mouse->btnc[INPUT_BUTTON_MIDDLE]) {
>          bytes[3] |= (mouse->btns[INPUT_BUTTON_MIDDLE] ? 0x20 : 0x00);
>          mouse->btnc[INPUT_BUTTON_MIDDLE] = false;
> -        count = 4;
> +        count++;
>

a bit superfluous,  ok

     }
>
> -    if (mouse->outlen <= sizeof(mouse->outbuf) - count) {
> -        memcpy(mouse->outbuf + mouse->outlen, bytes, count);
> -        mouse->outlen += count;
> +    if (fifo8_num_free(&mouse->outbuf) >= count) {
> +        fifo8_push_all(&mouse->outbuf, bytes, count);
>      } else {
>          /* queue full -> drop event */
>      }
> @@ -172,9 +171,7 @@ static int msmouse_ioctl(Chardev *chr, int cmd, void
> *arg)
>                   * cause we behave like a 3 button logitech
>                   * mouse.
>                   */
> -                mouse->outbuf[0] = 'M';
> -                mouse->outbuf[1] = '3';
> -                mouse->outlen = 2;
> +                fifo8_push_all(&mouse->outbuf, mouse_id,
> sizeof(mouse_id));
>                  /* Start sending data to serial. */
>                  msmouse_chr_accept_input(chr);
>              }
> @@ -184,7 +181,7 @@ static int msmouse_ioctl(Chardev *chr, int cmd, void
> *arg)
>           * Reset mouse buffers on power down.
>           * Mouse won't send anything without power.
>           */
> -        mouse->outlen = 0;
> +        fifo8_reset(&mouse->outbuf);
>          memset(mouse->axis, 0, sizeof(mouse->axis));
>          memset(mouse->btns, false, sizeof(mouse->btns));
>          memset(mouse->btnc, false, sizeof(mouse->btns));
> @@ -204,6 +201,7 @@ static void char_msmouse_finalize(Object *obj)
>      MouseChardev *mouse = MOUSE_CHARDEV(obj);
>
>      qemu_input_handler_unregister(mouse->hs);
> +    fifo8_destroy(&mouse->outbuf);
>  }
>
>  static QemuInputHandler msmouse_handler = {
> @@ -224,6 +222,7 @@ static void msmouse_chr_open(Chardev *chr,
>      mouse->hs = qemu_input_handler_register((DeviceState *)mouse,
>                                              &msmouse_handler);
>      mouse->tiocm = 0;
> +    fifo8_create(&mouse->outbuf, MSMOUSE_BUF_SZ);
>  }
>
>  static void char_msmouse_class_init(ObjectClass *oc, void *data)
> --
> 2.34.1
>
>
>
Volker Rümelin Sept. 11, 2022, 6:12 a.m. UTC | #2 
Am 08.09.22 um 19:31 schrieb Arwed Meyer:

> @@ -54,21 +60,15 @@ DECLARE_INSTANCE_CHECKER(MouseChardev, MOUSE_CHARDEV,
>   static void msmouse_chr_accept_input(Chardev *chr)
>   {
>       MouseChardev *mouse = MOUSE_CHARDEV(chr);
> -    int len;
> +    uint32_t len_out, len;
>
> -    len = qemu_chr_be_can_write(chr);
> -    if (len > mouse->outlen) {
> -        len = mouse->outlen;
> -    }
> -    if (!len) {
> +    len_out = qemu_chr_be_can_write(chr);
> +    if (!len_out || fifo8_is_empty(&mouse->outbuf)) {
>           return;
>       }
> -
> -    qemu_chr_be_write(chr, mouse->outbuf, len);
> -    mouse->outlen -= len;
> -    if (mouse->outlen) {
> -        memmove(mouse->outbuf, mouse->outbuf + len, mouse->outlen);
> -    }
> +    len = MIN(fifo8_num_used(&mouse->outbuf), len_out);
> +    qemu_chr_be_write(chr, fifo8_pop_buf(&mouse->outbuf, len, &len_out),
> +            len_out);

Hi Arwed,

I think C function arguments are not evaluated in a defined order. It's 
not defined if the third argument of function qemu_chr_be_write() is 
len_out before or after the call to fifo8_pop_buf().

The fifo_pop_buf() function uses a ringbuffer. When the buffer wraps 
around at the end and the ringbuffer contains more than one byte you may 
need two fifo8_pop_buf() and qemu_chr_be_write() calls to write all 
bytes. The code you replace doesn't have that problem.

Some chardev frontends don't return the total number of bytes to write 
in qemu_chr_be_can_write(). They return the number of bytes that can be 
written with one qemu_chr_be_write() call. You need another 
qemu_chr_be_can_write() call after the qemu_chr_be_write() call to see 
if more bytes can be written.

The code in function gd_vc_send_chars() in ui/gtk.c could be used as a 
template to avoid the three issues above.

With best regards,
Volker

>   }
>
>   static void msmouse_queue_event(MouseChardev *mouse)
Arwed Meyer Sept. 11, 2022, 5:27 p.m. UTC | #3 
Hi,

Am 09.09.22 um 15:18 schrieb Marc-André Lureau:
> Hi
>
> On Thu, Sep 8, 2022 at 9:38 PM Arwed Meyer <arwed.meyer@gmx.de
> <mailto:arwed.meyer@gmx.de>> wrote:
>
>     Make use of fifo8 functions instead of implementing own fifo code.
>     This makes the code more readable and reduces risk of bugs.
>
>     Signed-off-by: Arwed Meyer <arwed.meyer@gmx.de
>     <mailto:arwed.meyer@gmx.de>>
>
> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com
> <mailto:marcandre.lureau@redhat.com>>
>
>     ---
>       chardev/msmouse.c | 43 +++++++++++++++++++++----------------------
>       1 file changed, 21 insertions(+), 22 deletions(-)
>
>     diff --git a/chardev/msmouse.c b/chardev/msmouse.c
>     index 95fa488339..e9aa3eab55 100644
>     --- a/chardev/msmouse.c
>     +++ b/chardev/msmouse.c
>     @@ -24,6 +24,7 @@
>
>       #include "qemu/osdep.h"
>       #include "qemu/module.h"
>     +#include "qemu/fifo8.h"
>       #include "chardev/char.h"
>       #include "chardev/char-serial.h"
>       #include "ui/console.h"
>     @@ -34,6 +35,12 @@
>       #define MSMOUSE_HI2(n)  (((n) & 0xc0) >> 6)
>       #define MSMOUSE_PWR(cm) (cm & (CHR_TIOCM_RTS | CHR_TIOCM_DTR))
>
>     +/* Serial fifo size. */
>     +#define MSMOUSE_BUF_SZ 64
>
>
> Why bump to 64 bytes?
That's to make some extra space for PnP data (see PATCH v2 4/5). Didn't
want to leave it at 32 (which seems rather random as well) just to
change it again in the next patch.
>
>     +
>     +/* Mouse ID: Send "M3" cause we behave like a 3 button logitech
>     mouse. */
>     +const uint8_t mouse_id[] = {'M', '3'};
>     +
>       struct MouseChardev {
>           Chardev parent;
>
>     @@ -42,8 +49,7 @@ struct MouseChardev {
>           int axis[INPUT_AXIS__MAX];
>           bool btns[INPUT_BUTTON__MAX];
>           bool btnc[INPUT_BUTTON__MAX];
>     -    uint8_t outbuf[32];
>     -    int outlen;
>     +    Fifo8 outbuf;
>       };
>       typedef struct MouseChardev MouseChardev;
>
>     @@ -54,21 +60,15 @@ DECLARE_INSTANCE_CHECKER(MouseChardev,
>     MOUSE_CHARDEV,
>       static void msmouse_chr_accept_input(Chardev *chr)
>       {
>           MouseChardev *mouse = MOUSE_CHARDEV(chr);
>     -    int len;
>     +    uint32_t len_out, len;
>
>     -    len = qemu_chr_be_can_write(chr);
>     -    if (len > mouse->outlen) {
>     -        len = mouse->outlen;
>     -    }
>     -    if (!len) {
>     +    len_out = qemu_chr_be_can_write(chr);
>     +    if (!len_out || fifo8_is_empty(&mouse->outbuf)) {
>               return;
>           }
>     -
>     -    qemu_chr_be_write(chr, mouse->outbuf, len);
>     -    mouse->outlen -= len;
>     -    if (mouse->outlen) {
>     -        memmove(mouse->outbuf, mouse->outbuf + len, mouse->outlen);
>     -    }
>     +    len = MIN(fifo8_num_used(&mouse->outbuf), len_out);
>     +    qemu_chr_be_write(chr, fifo8_pop_buf(&mouse->outbuf, len,
>     &len_out),
>     +            len_out);
>       }
>
>       static void msmouse_queue_event(MouseChardev *mouse)
>     @@ -94,12 +94,11 @@ static void msmouse_queue_event(MouseChardev *mouse)
>               mouse->btnc[INPUT_BUTTON_MIDDLE]) {
>               bytes[3] |= (mouse->btns[INPUT_BUTTON_MIDDLE] ? 0x20 : 0x00);
>               mouse->btnc[INPUT_BUTTON_MIDDLE] = false;
>     -        count = 4;
>     +        count++;
>
>
> a bit superfluous,  ok
Well... at least on x86 I think this may save a byte or two.
>
>           }
>
>     -    if (mouse->outlen <= sizeof(mouse->outbuf) - count) {
>     -        memcpy(mouse->outbuf + mouse->outlen, bytes, count);
>     -        mouse->outlen += count;
>     +    if (fifo8_num_free(&mouse->outbuf) >= count) {
>     +        fifo8_push_all(&mouse->outbuf, bytes, count);
>           } else {
>               /* queue full -> drop event */
>           }
>     @@ -172,9 +171,7 @@ static int msmouse_ioctl(Chardev *chr, int cmd,
>     void *arg)
>                        * cause we behave like a 3 button logitech
>                        * mouse.
>                        */
>     -                mouse->outbuf[0] = 'M';
>     -                mouse->outbuf[1] = '3';
>     -                mouse->outlen = 2;
>     +                fifo8_push_all(&mouse->outbuf, mouse_id,
>     sizeof(mouse_id));
>                       /* Start sending data to serial. */
>                       msmouse_chr_accept_input(chr);
>                   }
>     @@ -184,7 +181,7 @@ static int msmouse_ioctl(Chardev *chr, int cmd,
>     void *arg)
>                * Reset mouse buffers on power down.
>                * Mouse won't send anything without power.
>                */
>     -        mouse->outlen = 0;
>     +        fifo8_reset(&mouse->outbuf);
>               memset(mouse->axis, 0, sizeof(mouse->axis));
>               memset(mouse->btns, false, sizeof(mouse->btns));
>               memset(mouse->btnc, false, sizeof(mouse->btns));
>     @@ -204,6 +201,7 @@ static void char_msmouse_finalize(Object *obj)
>           MouseChardev *mouse = MOUSE_CHARDEV(obj);
>
>           qemu_input_handler_unregister(mouse->hs);
>     +    fifo8_destroy(&mouse->outbuf);
>       }
>
>       static QemuInputHandler msmouse_handler = {
>     @@ -224,6 +222,7 @@ static void msmouse_chr_open(Chardev *chr,
>           mouse->hs = qemu_input_handler_register((DeviceState *)mouse,
>                                                   &msmouse_handler);
>           mouse->tiocm = 0;
>     +    fifo8_create(&mouse->outbuf, MSMOUSE_BUF_SZ);
>       }
>
>       static void char_msmouse_class_init(ObjectClass *oc, void *data)
>     --
>     2.34.1
>
>
>
>
> --
> Marc-André Lureau
Arwed Meyer Sept. 11, 2022, 5:53 p.m. UTC | #4 
Am 11.09.22 um 08:12 schrieb Volker Rümelin:
> Am 08.09.22 um 19:31 schrieb Arwed Meyer:
>
>> @@ -54,21 +60,15 @@ DECLARE_INSTANCE_CHECKER(MouseChardev, MOUSE_CHARDEV,
>>   static void msmouse_chr_accept_input(Chardev *chr)
>>   {
>>       MouseChardev *mouse = MOUSE_CHARDEV(chr);
>> -    int len;
>> +    uint32_t len_out, len;
>>
>> -    len = qemu_chr_be_can_write(chr);
>> -    if (len > mouse->outlen) {
>> -        len = mouse->outlen;
>> -    }
>> -    if (!len) {
>> +    len_out = qemu_chr_be_can_write(chr);
>> +    if (!len_out || fifo8_is_empty(&mouse->outbuf)) {
>>           return;
>>       }
>> -
>> -    qemu_chr_be_write(chr, mouse->outbuf, len);
>> -    mouse->outlen -= len;
>> -    if (mouse->outlen) {
>> -        memmove(mouse->outbuf, mouse->outbuf + len, mouse->outlen);
>> -    }
>> +    len = MIN(fifo8_num_used(&mouse->outbuf), len_out);
>> +    qemu_chr_be_write(chr, fifo8_pop_buf(&mouse->outbuf, len, &len_out),
>> +            len_out);
>
> Hi Arwed,
>
> I think C function arguments are not evaluated in a defined order. It's
> not defined if the third argument of function qemu_chr_be_write() is
> len_out before or after the call to fifo8_pop_buf().
>
> The fifo_pop_buf() function uses a ringbuffer. When the buffer wraps
> around at the end and the ringbuffer contains more than one byte you may
> need two fifo8_pop_buf() and qemu_chr_be_write() calls to write all
> bytes. The code you replace doesn't have that problem.
>
> Some chardev frontends don't return the total number of bytes to write
> in qemu_chr_be_can_write(). They return the number of bytes that can be
> written with one qemu_chr_be_write() call. You need another
> qemu_chr_be_can_write() call after the qemu_chr_be_write() call to see
> if more bytes can be written.
>
> The code in function gd_vc_send_chars() in ui/gtk.c could be used as a
> template to avoid the three issues above.
>
> With best regards,
> Volker
>
>>   }
>>
>>   static void msmouse_queue_event(MouseChardev *mouse)
>

Hi Volker,

have to admit I was not completely sure about the order in which
function arguments get evaluated either. Thanks for the confirmation.
I'll change this to be safe.

I guess you probably won't see much difference between old and new code
in practice since serial I/O is slow anyway, but since I'll change the
patch anyway, I'll take the code from gd_vc_send_chars. Indeed looks better.
diff mbox series

Patch

diff --git a/chardev/msmouse.c b/chardev/msmouse.c
index 95fa488339..e9aa3eab55 100644
--- a/chardev/msmouse.c
+++ b/chardev/msmouse.c
@@ -24,6 +24,7 @@ 

 #include "qemu/osdep.h"
 #include "qemu/module.h"
+#include "qemu/fifo8.h"
 #include "chardev/char.h"
 #include "chardev/char-serial.h"
 #include "ui/console.h"
@@ -34,6 +35,12 @@ 
 #define MSMOUSE_HI2(n)  (((n) & 0xc0) >> 6)
 #define MSMOUSE_PWR(cm) (cm & (CHR_TIOCM_RTS | CHR_TIOCM_DTR))

+/* Serial fifo size. */
+#define MSMOUSE_BUF_SZ 64
+
+/* Mouse ID: Send "M3" cause we behave like a 3 button logitech mouse. */
+const uint8_t mouse_id[] = {'M', '3'};
+
 struct MouseChardev {
     Chardev parent;

@@ -42,8 +49,7 @@  struct MouseChardev {
     int axis[INPUT_AXIS__MAX];
     bool btns[INPUT_BUTTON__MAX];
     bool btnc[INPUT_BUTTON__MAX];
-    uint8_t outbuf[32];
-    int outlen;
+    Fifo8 outbuf;
 };
 typedef struct MouseChardev MouseChardev;

@@ -54,21 +60,15 @@  DECLARE_INSTANCE_CHECKER(MouseChardev, MOUSE_CHARDEV,
 static void msmouse_chr_accept_input(Chardev *chr)
 {
     MouseChardev *mouse = MOUSE_CHARDEV(chr);
-    int len;
+    uint32_t len_out, len;

-    len = qemu_chr_be_can_write(chr);
-    if (len > mouse->outlen) {
-        len = mouse->outlen;
-    }
-    if (!len) {
+    len_out = qemu_chr_be_can_write(chr);
+    if (!len_out || fifo8_is_empty(&mouse->outbuf)) {
         return;
     }
-
-    qemu_chr_be_write(chr, mouse->outbuf, len);
-    mouse->outlen -= len;
-    if (mouse->outlen) {
-        memmove(mouse->outbuf, mouse->outbuf + len, mouse->outlen);
-    }
+    len = MIN(fifo8_num_used(&mouse->outbuf), len_out);
+    qemu_chr_be_write(chr, fifo8_pop_buf(&mouse->outbuf, len, &len_out),
+            len_out);
 }

 static void msmouse_queue_event(MouseChardev *mouse)
@@ -94,12 +94,11 @@  static void msmouse_queue_event(MouseChardev *mouse)
         mouse->btnc[INPUT_BUTTON_MIDDLE]) {
         bytes[3] |= (mouse->btns[INPUT_BUTTON_MIDDLE] ? 0x20 : 0x00);
         mouse->btnc[INPUT_BUTTON_MIDDLE] = false;
-        count = 4;
+        count++;
     }

-    if (mouse->outlen <= sizeof(mouse->outbuf) - count) {
-        memcpy(mouse->outbuf + mouse->outlen, bytes, count);
-        mouse->outlen += count;
+    if (fifo8_num_free(&mouse->outbuf) >= count) {
+        fifo8_push_all(&mouse->outbuf, bytes, count);
     } else {
         /* queue full -> drop event */
     }
@@ -172,9 +171,7 @@  static int msmouse_ioctl(Chardev *chr, int cmd, void *arg)
                  * cause we behave like a 3 button logitech
                  * mouse.
                  */
-                mouse->outbuf[0] = 'M';
-                mouse->outbuf[1] = '3';
-                mouse->outlen = 2;
+                fifo8_push_all(&mouse->outbuf, mouse_id, sizeof(mouse_id));
                 /* Start sending data to serial. */
                 msmouse_chr_accept_input(chr);
             }
@@ -184,7 +181,7 @@  static int msmouse_ioctl(Chardev *chr, int cmd, void *arg)
          * Reset mouse buffers on power down.
          * Mouse won't send anything without power.
          */
-        mouse->outlen = 0;
+        fifo8_reset(&mouse->outbuf);
         memset(mouse->axis, 0, sizeof(mouse->axis));
         memset(mouse->btns, false, sizeof(mouse->btns));
         memset(mouse->btnc, false, sizeof(mouse->btns));
@@ -204,6 +201,7 @@  static void char_msmouse_finalize(Object *obj)
     MouseChardev *mouse = MOUSE_CHARDEV(obj);

     qemu_input_handler_unregister(mouse->hs);
+    fifo8_destroy(&mouse->outbuf);
 }

 static QemuInputHandler msmouse_handler = {
@@ -224,6 +222,7 @@  static void msmouse_chr_open(Chardev *chr,
     mouse->hs = qemu_input_handler_register((DeviceState *)mouse,
                                             &msmouse_handler);
     mouse->tiocm = 0;
+    fifo8_create(&mouse->outbuf, MSMOUSE_BUF_SZ);
 }

 static void char_msmouse_class_init(ObjectClass *oc, void *data)