Message ID | 20220908173120.16779-4-arwed.meyer@gmx.de |
---|---|
State | New |
Headers | show |
Series | Make serial msmouse work | expand |
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 > > >
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, 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
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 --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)
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