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(-)
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)
--
2.34.1Am 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)
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.
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
>
>
>
--
Marc-André Lureau
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
© 2016 - 2026 Red Hat, Inc.