* [Qemu-devel] [PATCH 0/6] serial: flow control fixes
@ 2016-06-20 14:28 Paolo Bonzini
2016-06-20 14:29 ` [Qemu-devel] [PATCH 1/6] serial: make tsr_retry unsigned Paolo Bonzini
` (5 more replies)
0 siblings, 6 replies; 17+ messages in thread
From: Paolo Bonzini @ 2016-06-20 14:28 UTC (permalink / raw)
To: qemu-devel; +Cc: dgilbert, bcketchum
The main fixes here are in patch 2 and patch 6: watches are lost
after migration, and not removed on reset.
The rest are cleanups; patch 5 fixes the qemu_chr_fe_add_watch
API, which botches its return value pretty badly.
Paolo
Paolo Bonzini (6):
serial: make tsr_retry unsigned
serial: reinstate watch after migration
serial: separate serial_xmit and serial_watch_cb
serial: simplify tsr_retry reset
char: change qemu_chr_fe_add_watch to return unsigned
serial: remove watch on reset
hw/char/cadence_uart.c | 5 +++-
hw/char/serial.c | 59 +++++++++++++++++++++++++++++++++++-------------
include/hw/char/serial.h | 3 ++-
include/sysemu/char.h | 16 +++++++++++--
qemu-char.c | 8 +++----
5 files changed, 67 insertions(+), 24 deletions(-)
--
2.5.5
^ permalink raw reply [flat|nested] 17+ messages in thread
* [Qemu-devel] [PATCH 1/6] serial: make tsr_retry unsigned
2016-06-20 14:28 [Qemu-devel] [PATCH 0/6] serial: flow control fixes Paolo Bonzini
@ 2016-06-20 14:29 ` Paolo Bonzini
2016-06-22 14:44 ` Dr. David Alan Gilbert
2016-06-20 14:29 ` [Qemu-devel] [PATCH 2/6] serial: reinstate watch after migration Paolo Bonzini
` (4 subsequent siblings)
5 siblings, 1 reply; 17+ messages in thread
From: Paolo Bonzini @ 2016-06-20 14:29 UTC (permalink / raw)
To: qemu-devel; +Cc: dgilbert, bcketchum
It can never become negative; reflect this in the type of the field
and simplify the conditions.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
hw/char/serial.c | 12 ++++++++----
include/hw/char/serial.h | 2 +-
2 files changed, 9 insertions(+), 5 deletions(-)
diff --git a/hw/char/serial.c b/hw/char/serial.c
index 6d815b5..e65e9e0 100644
--- a/hw/char/serial.c
+++ b/hw/char/serial.c
@@ -229,7 +229,7 @@ static gboolean serial_xmit(GIOChannel *chan, GIOCondition cond, void *opaque)
do {
assert(!(s->lsr & UART_LSR_TEMT));
- if (s->tsr_retry <= 0) {
+ if (s->tsr_retry == 0) {
assert(!(s->lsr & UART_LSR_THRE));
if (s->fcr & UART_FCR_FE) {
@@ -252,7 +252,7 @@ static gboolean serial_xmit(GIOChannel *chan, GIOCondition cond, void *opaque)
/* in loopback mode, say that we just received a char */
serial_receive1(s, &s->tsr, 1);
} else if (qemu_chr_fe_write(s->chr, &s->tsr, 1) != 1) {
- if (s->tsr_retry >= 0 && s->tsr_retry < MAX_XMIT_RETRY &&
+ if (s->tsr_retry < MAX_XMIT_RETRY &&
qemu_chr_fe_add_watch(s->chr, G_IO_OUT|G_IO_HUP,
serial_xmit, s) > 0) {
s->tsr_retry++;
@@ -330,7 +330,7 @@ static void serial_ioport_write(void *opaque, hwaddr addr, uint64_t val,
s->lsr &= ~UART_LSR_THRE;
s->lsr &= ~UART_LSR_TEMT;
serial_update_irq(s);
- if (s->tsr_retry <= 0) {
+ if (s->tsr_retry == 0) {
serial_xmit(NULL, G_IO_OUT, s);
}
}
@@ -639,6 +639,10 @@ static int serial_post_load(void *opaque, int version_id)
if (s->thr_ipending == -1) {
s->thr_ipending = ((s->iir & UART_IIR_ID) == UART_IIR_THRI);
}
+ if (s->tsr_retry > MAX_XMIT_RETRY) {
+ s->tsr_retry = MAX_XMIT_RETRY;
+ }
+
s->last_break_enable = (s->lcr >> 6) & 1;
/* Initialize fcr via setter to perform essential side-effects */
serial_write_fcr(s, s->fcr_vmstate);
@@ -685,7 +689,7 @@ static const VMStateDescription vmstate_serial_tsr = {
.minimum_version_id = 1,
.needed = serial_tsr_needed,
.fields = (VMStateField[]) {
- VMSTATE_INT32(tsr_retry, SerialState),
+ VMSTATE_UINT32(tsr_retry, SerialState),
VMSTATE_UINT8(thr, SerialState),
VMSTATE_UINT8(tsr, SerialState),
VMSTATE_END_OF_LIST()
diff --git a/include/hw/char/serial.h b/include/hw/char/serial.h
index 15beb6b..ef90615 100644
--- a/include/hw/char/serial.h
+++ b/include/hw/char/serial.h
@@ -55,7 +55,7 @@ struct SerialState {
int last_break_enable;
int it_shift;
int baudbase;
- int tsr_retry;
+ unsigned tsr_retry;
uint32_t wakeup;
/* Time when the last byte was successfully sent out of the tsr */
--
2.5.5
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [Qemu-devel] [PATCH 2/6] serial: reinstate watch after migration
2016-06-20 14:28 [Qemu-devel] [PATCH 0/6] serial: flow control fixes Paolo Bonzini
2016-06-20 14:29 ` [Qemu-devel] [PATCH 1/6] serial: make tsr_retry unsigned Paolo Bonzini
@ 2016-06-20 14:29 ` Paolo Bonzini
2016-06-22 15:05 ` Dr. David Alan Gilbert
2016-06-20 14:29 ` [Qemu-devel] [PATCH 3/6] serial: separate serial_xmit and serial_watch_cb Paolo Bonzini
` (3 subsequent siblings)
5 siblings, 1 reply; 17+ messages in thread
From: Paolo Bonzini @ 2016-06-20 14:29 UTC (permalink / raw)
To: qemu-devel; +Cc: dgilbert, bcketchum
Otherwise, a serial port can get stuck if it is migrated while flow control
is in effect.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
hw/char/serial.c | 16 ++++++++++++++--
include/hw/char/serial.h | 1 +
2 files changed, 15 insertions(+), 2 deletions(-)
diff --git a/hw/char/serial.c b/hw/char/serial.c
index e65e9e0..6f0a49e 100644
--- a/hw/char/serial.c
+++ b/hw/char/serial.c
@@ -639,8 +639,20 @@ static int serial_post_load(void *opaque, int version_id)
if (s->thr_ipending == -1) {
s->thr_ipending = ((s->iir & UART_IIR_ID) == UART_IIR_THRI);
}
- if (s->tsr_retry > MAX_XMIT_RETRY) {
- s->tsr_retry = MAX_XMIT_RETRY;
+
+ if (s->tsr_retry > 0) {
+ /* tsr_retry > 0 implies LSR.TEMT = 0 (transmitter not empty). */
+ if (s->lsr & UART_LSR_TEMT) {
+ return -1;
+ }
+
+ assert(s->watch_tag == 0);
+ s->watch_tag = qemu_chr_fe_add_watch(s->chr, G_IO_OUT|G_IO_HUP, serial_xmit, s);
+ } else {
+ /* tsr_retry == 0 implies LSR.TEMT = 1 (transmitter empty). */
+ if (!(s->lsr & UART_LSR_TEMT)) {
+ return -1;
+ }
}
s->last_break_enable = (s->lcr >> 6) & 1;
diff --git a/include/hw/char/serial.h b/include/hw/char/serial.h
index ef90615..2ab835c 100644
--- a/include/hw/char/serial.h
+++ b/include/hw/char/serial.h
@@ -56,6 +56,7 @@ struct SerialState {
int it_shift;
int baudbase;
unsigned tsr_retry;
+ unsigned watch_tag;
uint32_t wakeup;
/* Time when the last byte was successfully sent out of the tsr */
--
2.5.5
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [Qemu-devel] [PATCH 3/6] serial: separate serial_xmit and serial_watch_cb
2016-06-20 14:28 [Qemu-devel] [PATCH 0/6] serial: flow control fixes Paolo Bonzini
2016-06-20 14:29 ` [Qemu-devel] [PATCH 1/6] serial: make tsr_retry unsigned Paolo Bonzini
2016-06-20 14:29 ` [Qemu-devel] [PATCH 2/6] serial: reinstate watch after migration Paolo Bonzini
@ 2016-06-20 14:29 ` Paolo Bonzini
2016-06-22 15:09 ` Dr. David Alan Gilbert
2016-06-20 14:29 ` [Qemu-devel] [PATCH 4/6] serial: simplify tsr_retry reset Paolo Bonzini
` (2 subsequent siblings)
5 siblings, 1 reply; 17+ messages in thread
From: Paolo Bonzini @ 2016-06-20 14:29 UTC (permalink / raw)
To: qemu-devel; +Cc: dgilbert, bcketchum
serial_xmit starts transmission of whatever is in the FIFO or THR;
serial_watch_cb is a wrapper around it.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
hw/char/serial.c | 21 +++++++++++++--------
1 file changed, 13 insertions(+), 8 deletions(-)
diff --git a/hw/char/serial.c b/hw/char/serial.c
index 6f0a49e..4196a2e 100644
--- a/hw/char/serial.c
+++ b/hw/char/serial.c
@@ -106,6 +106,7 @@ do {} while (0)
#endif
static void serial_receive1(void *opaque, const uint8_t *buf, int size);
+static void serial_xmit(SerialState *s);
static inline void recv_fifo_put(SerialState *s, uint8_t chr)
{
@@ -223,10 +224,16 @@ static void serial_update_msl(SerialState *s)
}
}
-static gboolean serial_xmit(GIOChannel *chan, GIOCondition cond, void *opaque)
+static gboolean serial_watch_cb(GIOChannel *chan, GIOCondition cond,
+ void *opaque)
{
SerialState *s = opaque;
+ serial_xmit(s);
+ return FALSE;
+}
+static void serial_xmit(SerialState *s)
+{
do {
assert(!(s->lsr & UART_LSR_TEMT));
if (s->tsr_retry == 0) {
@@ -254,9 +261,9 @@ static gboolean serial_xmit(GIOChannel *chan, GIOCondition cond, void *opaque)
} else if (qemu_chr_fe_write(s->chr, &s->tsr, 1) != 1) {
if (s->tsr_retry < MAX_XMIT_RETRY &&
qemu_chr_fe_add_watch(s->chr, G_IO_OUT|G_IO_HUP,
- serial_xmit, s) > 0) {
+ serial_watch_cb, s) > 0) {
s->tsr_retry++;
- return FALSE;
+ return;
}
s->tsr_retry = 0;
} else {
@@ -269,11 +276,8 @@ static gboolean serial_xmit(GIOChannel *chan, GIOCondition cond, void *opaque)
s->last_xmit_ts = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
s->lsr |= UART_LSR_TEMT;
-
- return FALSE;
}
-
/* Setter for FCR.
is_load flag means, that value is set while loading VM state
and interrupt should not be invoked */
@@ -331,7 +335,7 @@ static void serial_ioport_write(void *opaque, hwaddr addr, uint64_t val,
s->lsr &= ~UART_LSR_TEMT;
serial_update_irq(s);
if (s->tsr_retry == 0) {
- serial_xmit(NULL, G_IO_OUT, s);
+ serial_xmit(s);
}
}
break;
@@ -647,7 +651,8 @@ static int serial_post_load(void *opaque, int version_id)
}
assert(s->watch_tag == 0);
- s->watch_tag = qemu_chr_fe_add_watch(s->chr, G_IO_OUT|G_IO_HUP, serial_xmit, s);
+ s->watch_tag = qemu_chr_fe_add_watch(s->chr, G_IO_OUT|G_IO_HUP,
+ serial_watch_cb, s);
} else {
/* tsr_retry == 0 implies LSR.TEMT = 1 (transmitter empty). */
if (!(s->lsr & UART_LSR_TEMT)) {
--
2.5.5
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [Qemu-devel] [PATCH 4/6] serial: simplify tsr_retry reset
2016-06-20 14:28 [Qemu-devel] [PATCH 0/6] serial: flow control fixes Paolo Bonzini
` (2 preceding siblings ...)
2016-06-20 14:29 ` [Qemu-devel] [PATCH 3/6] serial: separate serial_xmit and serial_watch_cb Paolo Bonzini
@ 2016-06-20 14:29 ` Paolo Bonzini
2016-06-22 15:12 ` Dr. David Alan Gilbert
2016-06-20 14:29 ` [Qemu-devel] [PATCH 5/6] char: change qemu_chr_fe_add_watch to return unsigned Paolo Bonzini
2016-06-20 14:29 ` [Qemu-devel] [PATCH 6/6] serial: remove watch on reset Paolo Bonzini
5 siblings, 1 reply; 17+ messages in thread
From: Paolo Bonzini @ 2016-06-20 14:29 UTC (permalink / raw)
To: qemu-devel; +Cc: dgilbert, bcketchum
Move common code outside the if, and reset tsr_retry even in loopback mode.
Right now it cannot become non-zero, but it will be possible as soon as
we start respecting the baud rate.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
hw/char/serial.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/hw/char/serial.c b/hw/char/serial.c
index 4196a2e..d232473 100644
--- a/hw/char/serial.c
+++ b/hw/char/serial.c
@@ -265,10 +265,8 @@ static void serial_xmit(SerialState *s)
s->tsr_retry++;
return;
}
- s->tsr_retry = 0;
- } else {
- s->tsr_retry = 0;
}
+ s->tsr_retry = 0;
/* Transmit another byte if it is already available. It is only
possible when FIFO is enabled and not empty. */
--
2.5.5
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [Qemu-devel] [PATCH 5/6] char: change qemu_chr_fe_add_watch to return unsigned
2016-06-20 14:28 [Qemu-devel] [PATCH 0/6] serial: flow control fixes Paolo Bonzini
` (3 preceding siblings ...)
2016-06-20 14:29 ` [Qemu-devel] [PATCH 4/6] serial: simplify tsr_retry reset Paolo Bonzini
@ 2016-06-20 14:29 ` Paolo Bonzini
2016-06-22 15:22 ` Dr. David Alan Gilbert
2016-06-20 14:29 ` [Qemu-devel] [PATCH 6/6] serial: remove watch on reset Paolo Bonzini
5 siblings, 1 reply; 17+ messages in thread
From: Paolo Bonzini @ 2016-06-20 14:29 UTC (permalink / raw)
To: qemu-devel; +Cc: dgilbert, bcketchum
g_source_attach can return any value between 1 and UINT_MAX if you let
QEMU run long enough. However, qemu_chr_fe_add_watch can also return
a negative errno value when the device is disconnected or does not
support chr_add_watch. Change it to return zero to avoid overloading
these values.
Fix the cadence_uart which asserts in this case (easily obtained with
"-serial pty").
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
hw/char/cadence_uart.c | 5 ++++-
include/sysemu/char.h | 16 ++++++++++++++--
qemu-char.c | 8 ++++----
3 files changed, 22 insertions(+), 7 deletions(-)
diff --git a/hw/char/cadence_uart.c b/hw/char/cadence_uart.c
index c856fc3..488a570 100644
--- a/hw/char/cadence_uart.c
+++ b/hw/char/cadence_uart.c
@@ -294,7 +294,10 @@ static gboolean cadence_uart_xmit(GIOChannel *chan, GIOCondition cond,
if (s->tx_count) {
int r = qemu_chr_fe_add_watch(s->chr, G_IO_OUT|G_IO_HUP,
cadence_uart_xmit, s);
- assert(r);
+ if (!r) {
+ s->tx_count = 0;
+ return FALSE;
+ }
}
uart_update_status(s);
diff --git a/include/sysemu/char.h b/include/sysemu/char.h
index 1eb2d0f..07434a0 100644
--- a/include/sysemu/char.h
+++ b/include/sysemu/char.h
@@ -221,8 +221,20 @@ void qemu_chr_fe_event(CharDriverState *s, int event);
void qemu_chr_fe_printf(CharDriverState *s, const char *fmt, ...)
GCC_FMT_ATTR(2, 3);
-int qemu_chr_fe_add_watch(CharDriverState *s, GIOCondition cond,
- GIOFunc func, void *user_data);
+/**
+ * @qemu_chr_fe_add_watch:
+ *
+ * If the backend is connected, create and add a #GSource that fires
+ * when the given condition (typically G_IO_OUT|G_IO_HUP or G_IO_HUP)
+ * is active; return the #GSource's tag. If it is disconnected,
+ * return 0.
+ *
+ * @cond the condition to poll for
+ * @func the function to call when the condition happens
+ * @user_data the opaque pointer to pass to @func
+ */
+unsigned qemu_chr_fe_add_watch(CharDriverState *s, GIOCondition cond,
+ GIOFunc func, void *user_data);
/**
* @qemu_chr_fe_write:
diff --git a/qemu-char.c b/qemu-char.c
index 84f49ac..39b2ccd 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -3966,19 +3966,19 @@ void qemu_chr_fe_event(struct CharDriverState *chr, int event)
}
}
-int qemu_chr_fe_add_watch(CharDriverState *s, GIOCondition cond,
- GIOFunc func, void *user_data)
+unsigned qemu_chr_fe_add_watch(CharDriverState *s, GIOCondition cond,
+ GIOFunc func, void *user_data)
{
GSource *src;
guint tag;
if (s->chr_add_watch == NULL) {
- return -ENOSYS;
+ return 0;
}
src = s->chr_add_watch(s, cond);
if (!src) {
- return -EINVAL;
+ return 0;
}
g_source_set_callback(src, (GSourceFunc)func, user_data, NULL);
--
2.5.5
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [Qemu-devel] [PATCH 6/6] serial: remove watch on reset
2016-06-20 14:28 [Qemu-devel] [PATCH 0/6] serial: flow control fixes Paolo Bonzini
` (4 preceding siblings ...)
2016-06-20 14:29 ` [Qemu-devel] [PATCH 5/6] char: change qemu_chr_fe_add_watch to return unsigned Paolo Bonzini
@ 2016-06-20 14:29 ` Paolo Bonzini
2016-06-22 14:04 ` Bret Ketchum
2016-06-22 15:38 ` Dr. David Alan Gilbert
5 siblings, 2 replies; 17+ messages in thread
From: Paolo Bonzini @ 2016-06-20 14:29 UTC (permalink / raw)
To: qemu-devel; +Cc: dgilbert, bcketchum
Otherwise, this can cause serial_xmit to be entered with LSR.TEMT=0,
which is invalid and causes an assertion failure.
Reported-by: Bret Ketchum <bcketchum@gmail.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
hw/char/serial.c | 16 ++++++++++++----
1 file changed, 12 insertions(+), 4 deletions(-)
diff --git a/hw/char/serial.c b/hw/char/serial.c
index d232473..7c196e2 100644
--- a/hw/char/serial.c
+++ b/hw/char/serial.c
@@ -228,6 +228,7 @@ static gboolean serial_watch_cb(GIOChannel *chan, GIOCondition cond,
void *opaque)
{
SerialState *s = opaque;
+ s->watch_tag = 0;
serial_xmit(s);
return FALSE;
}
@@ -258,10 +259,12 @@ static void serial_xmit(SerialState *s)
if (s->mcr & UART_MCR_LOOP) {
/* in loopback mode, say that we just received a char */
serial_receive1(s, &s->tsr, 1);
- } else if (qemu_chr_fe_write(s->chr, &s->tsr, 1) != 1) {
- if (s->tsr_retry < MAX_XMIT_RETRY &&
- qemu_chr_fe_add_watch(s->chr, G_IO_OUT|G_IO_HUP,
- serial_watch_cb, s) > 0) {
+ } else if (qemu_chr_fe_write(s->chr, &s->tsr, 1) != 1 &&
+ s->tsr_retry < MAX_XMIT_RETRY) {
+ assert(s->watch_tag == 0);
+ s->watch_tag = qemu_chr_fe_add_watch(s->chr, G_IO_OUT|G_IO_HUP,
+ serial_watch_cb, s);
+ if (s->watch_tag > 0) {
s->tsr_retry++;
return;
}
@@ -834,6 +837,11 @@ static void serial_reset(void *opaque)
{
SerialState *s = opaque;
+ if (s->watch_tag > 0) {
+ g_source_remove(s->watch_tag);
+ s->watch_tag = 0;
+ }
+
s->rbr = 0;
s->ier = 0;
s->iir = UART_IIR_NO_INT;
--
2.5.5
^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [PATCH 6/6] serial: remove watch on reset
2016-06-20 14:29 ` [Qemu-devel] [PATCH 6/6] serial: remove watch on reset Paolo Bonzini
@ 2016-06-22 14:04 ` Bret Ketchum
2016-06-22 15:38 ` Dr. David Alan Gilbert
1 sibling, 0 replies; 17+ messages in thread
From: Bret Ketchum @ 2016-06-22 14:04 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel, dgilbert
Tested-by: Bret Ketchum <bcketchum@gmail.com>
On Mon, Jun 20, 2016 at 9:29 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Otherwise, this can cause serial_xmit to be entered with LSR.TEMT=0,
> which is invalid and causes an assertion failure.
>
> Reported-by: Bret Ketchum <bcketchum@gmail.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> hw/char/serial.c | 16 ++++++++++++----
> 1 file changed, 12 insertions(+), 4 deletions(-)
>
> diff --git a/hw/char/serial.c b/hw/char/serial.c
> index d232473..7c196e2 100644
> --- a/hw/char/serial.c
> +++ b/hw/char/serial.c
> @@ -228,6 +228,7 @@ static gboolean serial_watch_cb(GIOChannel *chan, GIOCondition cond,
> void *opaque)
> {
> SerialState *s = opaque;
> + s->watch_tag = 0;
> serial_xmit(s);
> return FALSE;
> }
> @@ -258,10 +259,12 @@ static void serial_xmit(SerialState *s)
> if (s->mcr & UART_MCR_LOOP) {
> /* in loopback mode, say that we just received a char */
> serial_receive1(s, &s->tsr, 1);
> - } else if (qemu_chr_fe_write(s->chr, &s->tsr, 1) != 1) {
> - if (s->tsr_retry < MAX_XMIT_RETRY &&
> - qemu_chr_fe_add_watch(s->chr, G_IO_OUT|G_IO_HUP,
> - serial_watch_cb, s) > 0) {
> + } else if (qemu_chr_fe_write(s->chr, &s->tsr, 1) != 1 &&
> + s->tsr_retry < MAX_XMIT_RETRY) {
> + assert(s->watch_tag == 0);
> + s->watch_tag = qemu_chr_fe_add_watch(s->chr, G_IO_OUT|G_IO_HUP,
> + serial_watch_cb, s);
> + if (s->watch_tag > 0) {
> s->tsr_retry++;
> return;
> }
> @@ -834,6 +837,11 @@ static void serial_reset(void *opaque)
> {
> SerialState *s = opaque;
>
> + if (s->watch_tag > 0) {
> + g_source_remove(s->watch_tag);
> + s->watch_tag = 0;
> + }
> +
> s->rbr = 0;
> s->ier = 0;
> s->iir = UART_IIR_NO_INT;
> --
> 2.5.5
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [PATCH 1/6] serial: make tsr_retry unsigned
2016-06-20 14:29 ` [Qemu-devel] [PATCH 1/6] serial: make tsr_retry unsigned Paolo Bonzini
@ 2016-06-22 14:44 ` Dr. David Alan Gilbert
0 siblings, 0 replies; 17+ messages in thread
From: Dr. David Alan Gilbert @ 2016-06-22 14:44 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel, bcketchum
* Paolo Bonzini (pbonzini@redhat.com) wrote:
> It can never become negative; reflect this in the type of the field
> and simplify the conditions.
OK, yes, the one case that could make it go -ve was removed in fcfb4d6
somewhere after 1.4.0.
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> hw/char/serial.c | 12 ++++++++----
> include/hw/char/serial.h | 2 +-
> 2 files changed, 9 insertions(+), 5 deletions(-)
>
> diff --git a/hw/char/serial.c b/hw/char/serial.c
> index 6d815b5..e65e9e0 100644
> --- a/hw/char/serial.c
> +++ b/hw/char/serial.c
> @@ -229,7 +229,7 @@ static gboolean serial_xmit(GIOChannel *chan, GIOCondition cond, void *opaque)
>
> do {
> assert(!(s->lsr & UART_LSR_TEMT));
> - if (s->tsr_retry <= 0) {
> + if (s->tsr_retry == 0) {
> assert(!(s->lsr & UART_LSR_THRE));
>
> if (s->fcr & UART_FCR_FE) {
> @@ -252,7 +252,7 @@ static gboolean serial_xmit(GIOChannel *chan, GIOCondition cond, void *opaque)
> /* in loopback mode, say that we just received a char */
> serial_receive1(s, &s->tsr, 1);
> } else if (qemu_chr_fe_write(s->chr, &s->tsr, 1) != 1) {
> - if (s->tsr_retry >= 0 && s->tsr_retry < MAX_XMIT_RETRY &&
> + if (s->tsr_retry < MAX_XMIT_RETRY &&
> qemu_chr_fe_add_watch(s->chr, G_IO_OUT|G_IO_HUP,
> serial_xmit, s) > 0) {
> s->tsr_retry++;
> @@ -330,7 +330,7 @@ static void serial_ioport_write(void *opaque, hwaddr addr, uint64_t val,
> s->lsr &= ~UART_LSR_THRE;
> s->lsr &= ~UART_LSR_TEMT;
> serial_update_irq(s);
> - if (s->tsr_retry <= 0) {
> + if (s->tsr_retry == 0) {
> serial_xmit(NULL, G_IO_OUT, s);
> }
> }
> @@ -639,6 +639,10 @@ static int serial_post_load(void *opaque, int version_id)
> if (s->thr_ipending == -1) {
> s->thr_ipending = ((s->iir & UART_IIR_ID) == UART_IIR_THRI);
> }
> + if (s->tsr_retry > MAX_XMIT_RETRY) {
> + s->tsr_retry = MAX_XMIT_RETRY;
> + }
Good that makes it safe even after receiving the case that could be -1.
Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> s->last_break_enable = (s->lcr >> 6) & 1;
> /* Initialize fcr via setter to perform essential side-effects */
> serial_write_fcr(s, s->fcr_vmstate);
> @@ -685,7 +689,7 @@ static const VMStateDescription vmstate_serial_tsr = {
> .minimum_version_id = 1,
> .needed = serial_tsr_needed,
> .fields = (VMStateField[]) {
> - VMSTATE_INT32(tsr_retry, SerialState),
> + VMSTATE_UINT32(tsr_retry, SerialState),
> VMSTATE_UINT8(thr, SerialState),
> VMSTATE_UINT8(tsr, SerialState),
> VMSTATE_END_OF_LIST()
> diff --git a/include/hw/char/serial.h b/include/hw/char/serial.h
> index 15beb6b..ef90615 100644
> --- a/include/hw/char/serial.h
> +++ b/include/hw/char/serial.h
> @@ -55,7 +55,7 @@ struct SerialState {
> int last_break_enable;
> int it_shift;
> int baudbase;
> - int tsr_retry;
> + unsigned tsr_retry;
> uint32_t wakeup;
>
> /* Time when the last byte was successfully sent out of the tsr */
> --
> 2.5.5
>
>
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [PATCH 2/6] serial: reinstate watch after migration
2016-06-20 14:29 ` [Qemu-devel] [PATCH 2/6] serial: reinstate watch after migration Paolo Bonzini
@ 2016-06-22 15:05 ` Dr. David Alan Gilbert
2016-06-22 15:11 ` Paolo Bonzini
0 siblings, 1 reply; 17+ messages in thread
From: Dr. David Alan Gilbert @ 2016-06-22 15:05 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel, bcketchum
* Paolo Bonzini (pbonzini@redhat.com) wrote:
> Otherwise, a serial port can get stuck if it is migrated while flow control
> is in effect.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> hw/char/serial.c | 16 ++++++++++++++--
> include/hw/char/serial.h | 1 +
> 2 files changed, 15 insertions(+), 2 deletions(-)
>
> diff --git a/hw/char/serial.c b/hw/char/serial.c
> index e65e9e0..6f0a49e 100644
> --- a/hw/char/serial.c
> +++ b/hw/char/serial.c
> @@ -639,8 +639,20 @@ static int serial_post_load(void *opaque, int version_id)
> if (s->thr_ipending == -1) {
> s->thr_ipending = ((s->iir & UART_IIR_ID) == UART_IIR_THRI);
> }
> - if (s->tsr_retry > MAX_XMIT_RETRY) {
> - s->tsr_retry = MAX_XMIT_RETRY;
Why remove the check you just added?
> +
> + if (s->tsr_retry > 0) {
> + /* tsr_retry > 0 implies LSR.TEMT = 0 (transmitter not empty). */
> + if (s->lsr & UART_LSR_TEMT) {
> + return -1;
> + }
> +
> + assert(s->watch_tag == 0);
> + s->watch_tag = qemu_chr_fe_add_watch(s->chr, G_IO_OUT|G_IO_HUP, serial_xmit, s);
> + } else {
> + /* tsr_retry == 0 implies LSR.TEMT = 1 (transmitter empty). */
> + if (!(s->lsr & UART_LSR_TEMT)) {
> + return -1;
If you're failing migration (-1) then please error_report to say why
so when someone hits it I can immediately see why.
Dave
> + }
> }
>
> s->last_break_enable = (s->lcr >> 6) & 1;
> diff --git a/include/hw/char/serial.h b/include/hw/char/serial.h
> index ef90615..2ab835c 100644
> --- a/include/hw/char/serial.h
> +++ b/include/hw/char/serial.h
> @@ -56,6 +56,7 @@ struct SerialState {
> int it_shift;
> int baudbase;
> unsigned tsr_retry;
> + unsigned watch_tag;
> uint32_t wakeup;
>
> /* Time when the last byte was successfully sent out of the tsr */
> --
> 2.5.5
>
>
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [PATCH 3/6] serial: separate serial_xmit and serial_watch_cb
2016-06-20 14:29 ` [Qemu-devel] [PATCH 3/6] serial: separate serial_xmit and serial_watch_cb Paolo Bonzini
@ 2016-06-22 15:09 ` Dr. David Alan Gilbert
2016-06-22 15:14 ` Paolo Bonzini
0 siblings, 1 reply; 17+ messages in thread
From: Dr. David Alan Gilbert @ 2016-06-22 15:09 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel, bcketchum
* Paolo Bonzini (pbonzini@redhat.com) wrote:
> serial_xmit starts transmission of whatever is in the FIFO or THR;
> serial_watch_cb is a wrapper around it.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> hw/char/serial.c | 21 +++++++++++++--------
> 1 file changed, 13 insertions(+), 8 deletions(-)
>
> diff --git a/hw/char/serial.c b/hw/char/serial.c
> index 6f0a49e..4196a2e 100644
> --- a/hw/char/serial.c
> +++ b/hw/char/serial.c
> @@ -106,6 +106,7 @@ do {} while (0)
> #endif
>
> static void serial_receive1(void *opaque, const uint8_t *buf, int size);
> +static void serial_xmit(SerialState *s);
>
> static inline void recv_fifo_put(SerialState *s, uint8_t chr)
> {
> @@ -223,10 +224,16 @@ static void serial_update_msl(SerialState *s)
> }
> }
>
> -static gboolean serial_xmit(GIOChannel *chan, GIOCondition cond, void *opaque)
> +static gboolean serial_watch_cb(GIOChannel *chan, GIOCondition cond,
> + void *opaque)
> {
> SerialState *s = opaque;
> + serial_xmit(s);
> + return FALSE;
> +}
Yes, with a side question.
We register this with G_IO_OUT|G_IO_HUP but don't check the cond, what's this
code supposed to do if it gets a HUP?
Since we didn't do anything with cond before either:
Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
>
> +static void serial_xmit(SerialState *s)
> +{
> do {
> assert(!(s->lsr & UART_LSR_TEMT));
> if (s->tsr_retry == 0) {
> @@ -254,9 +261,9 @@ static gboolean serial_xmit(GIOChannel *chan, GIOCondition cond, void *opaque)
> } else if (qemu_chr_fe_write(s->chr, &s->tsr, 1) != 1) {
> if (s->tsr_retry < MAX_XMIT_RETRY &&
> qemu_chr_fe_add_watch(s->chr, G_IO_OUT|G_IO_HUP,
> - serial_xmit, s) > 0) {
> + serial_watch_cb, s) > 0) {
> s->tsr_retry++;
> - return FALSE;
> + return;
> }
> s->tsr_retry = 0;
> } else {
> @@ -269,11 +276,8 @@ static gboolean serial_xmit(GIOChannel *chan, GIOCondition cond, void *opaque)
>
> s->last_xmit_ts = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
> s->lsr |= UART_LSR_TEMT;
> -
> - return FALSE;
> }
>
> -
> /* Setter for FCR.
> is_load flag means, that value is set while loading VM state
> and interrupt should not be invoked */
> @@ -331,7 +335,7 @@ static void serial_ioport_write(void *opaque, hwaddr addr, uint64_t val,
> s->lsr &= ~UART_LSR_TEMT;
> serial_update_irq(s);
> if (s->tsr_retry == 0) {
> - serial_xmit(NULL, G_IO_OUT, s);
> + serial_xmit(s);
> }
> }
> break;
> @@ -647,7 +651,8 @@ static int serial_post_load(void *opaque, int version_id)
> }
>
> assert(s->watch_tag == 0);
> - s->watch_tag = qemu_chr_fe_add_watch(s->chr, G_IO_OUT|G_IO_HUP, serial_xmit, s);
> + s->watch_tag = qemu_chr_fe_add_watch(s->chr, G_IO_OUT|G_IO_HUP,
> + serial_watch_cb, s);
> } else {
> /* tsr_retry == 0 implies LSR.TEMT = 1 (transmitter empty). */
> if (!(s->lsr & UART_LSR_TEMT)) {
> --
> 2.5.5
>
>
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [PATCH 2/6] serial: reinstate watch after migration
2016-06-22 15:05 ` Dr. David Alan Gilbert
@ 2016-06-22 15:11 ` Paolo Bonzini
2016-06-22 15:41 ` Dr. David Alan Gilbert
0 siblings, 1 reply; 17+ messages in thread
From: Paolo Bonzini @ 2016-06-22 15:11 UTC (permalink / raw)
To: Dr. David Alan Gilbert; +Cc: qemu-devel, bcketchum
On 22/06/2016 17:05, Dr. David Alan Gilbert wrote:
> * Paolo Bonzini (pbonzini@redhat.com) wrote:
>> Otherwise, a serial port can get stuck if it is migrated while flow control
>> is in effect.
>>
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>> ---
>> hw/char/serial.c | 16 ++++++++++++++--
>> include/hw/char/serial.h | 1 +
>> 2 files changed, 15 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/char/serial.c b/hw/char/serial.c
>> index e65e9e0..6f0a49e 100644
>> --- a/hw/char/serial.c
>> +++ b/hw/char/serial.c
>> @@ -639,8 +639,20 @@ static int serial_post_load(void *opaque, int version_id)
>> if (s->thr_ipending == -1) {
>> s->thr_ipending = ((s->iir & UART_IIR_ID) == UART_IIR_THRI);
>> }
>> - if (s->tsr_retry > MAX_XMIT_RETRY) {
>> - s->tsr_retry = MAX_XMIT_RETRY;
>
> Why remove the check you just added?
Because I'm dumb. :)
>> +
>> + if (s->tsr_retry > 0) {
>> + /* tsr_retry > 0 implies LSR.TEMT = 0 (transmitter not empty). */
>> + if (s->lsr & UART_LSR_TEMT) {
>> + return -1;
>> + }
>> +
>> + assert(s->watch_tag == 0);
>> + s->watch_tag = qemu_chr_fe_add_watch(s->chr, G_IO_OUT|G_IO_HUP, serial_xmit, s);
>> + } else {
>> + /* tsr_retry == 0 implies LSR.TEMT = 1 (transmitter empty). */
>> + if (!(s->lsr & UART_LSR_TEMT)) {
>> + return -1;
>
> If you're failing migration (-1) then please error_report to say why
> so when someone hits it I can immediately see why.
Ok, something like
error_report("inconsistent state in serial device");
?
Paolo
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [PATCH 4/6] serial: simplify tsr_retry reset
2016-06-20 14:29 ` [Qemu-devel] [PATCH 4/6] serial: simplify tsr_retry reset Paolo Bonzini
@ 2016-06-22 15:12 ` Dr. David Alan Gilbert
0 siblings, 0 replies; 17+ messages in thread
From: Dr. David Alan Gilbert @ 2016-06-22 15:12 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel, bcketchum
* Paolo Bonzini (pbonzini@redhat.com) wrote:
> Move common code outside the if, and reset tsr_retry even in loopback mode.
> Right now it cannot become non-zero, but it will be possible as soon as
> we start respecting the baud rate.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> ---
> hw/char/serial.c | 4 +---
> 1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/hw/char/serial.c b/hw/char/serial.c
> index 4196a2e..d232473 100644
> --- a/hw/char/serial.c
> +++ b/hw/char/serial.c
> @@ -265,10 +265,8 @@ static void serial_xmit(SerialState *s)
> s->tsr_retry++;
> return;
> }
> - s->tsr_retry = 0;
> - } else {
> - s->tsr_retry = 0;
> }
> + s->tsr_retry = 0;
>
> /* Transmit another byte if it is already available. It is only
> possible when FIFO is enabled and not empty. */
> --
> 2.5.5
>
>
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [PATCH 3/6] serial: separate serial_xmit and serial_watch_cb
2016-06-22 15:09 ` Dr. David Alan Gilbert
@ 2016-06-22 15:14 ` Paolo Bonzini
0 siblings, 0 replies; 17+ messages in thread
From: Paolo Bonzini @ 2016-06-22 15:14 UTC (permalink / raw)
To: Dr. David Alan Gilbert; +Cc: qemu-devel, bcketchum
On 22/06/2016 17:09, Dr. David Alan Gilbert wrote:
> > -static gboolean serial_xmit(GIOChannel *chan, GIOCondition cond, void *opaque)
> > +static gboolean serial_watch_cb(GIOChannel *chan, GIOCondition cond,
> > + void *opaque)
> > {
> > SerialState *s = opaque;
> > + serial_xmit(s);
> > + return FALSE;
> > +}
>
> Yes, with a side question.
> We register this with G_IO_OUT|G_IO_HUP but don't check the cond, what's this
> code supposed to do if it gets a HUP?
It will attempt again to write, which will fail (see pty_chr_write);
then attempt to add a watch again, which this time should return 0 (see
pty_chr_add_watch). The outcome is that tsr_retry is reset to 0.
Thanks,
Paolo
> Since we didn't do anything with cond before either:
>
> Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
>
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [PATCH 5/6] char: change qemu_chr_fe_add_watch to return unsigned
2016-06-20 14:29 ` [Qemu-devel] [PATCH 5/6] char: change qemu_chr_fe_add_watch to return unsigned Paolo Bonzini
@ 2016-06-22 15:22 ` Dr. David Alan Gilbert
0 siblings, 0 replies; 17+ messages in thread
From: Dr. David Alan Gilbert @ 2016-06-22 15:22 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel, bcketchum
* Paolo Bonzini (pbonzini@redhat.com) wrote:
> g_source_attach can return any value between 1 and UINT_MAX if you let
> QEMU run long enough. However, qemu_chr_fe_add_watch can also return
> a negative errno value when the device is disconnected or does not
> support chr_add_watch. Change it to return zero to avoid overloading
> these values.
>
> Fix the cadence_uart which asserts in this case (easily obtained with
> "-serial pty").
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> hw/char/cadence_uart.c | 5 ++++-
> include/sysemu/char.h | 16 ++++++++++++++--
> qemu-char.c | 8 ++++----
> 3 files changed, 22 insertions(+), 7 deletions(-)
>
> diff --git a/hw/char/cadence_uart.c b/hw/char/cadence_uart.c
> index c856fc3..488a570 100644
> --- a/hw/char/cadence_uart.c
> +++ b/hw/char/cadence_uart.c
> @@ -294,7 +294,10 @@ static gboolean cadence_uart_xmit(GIOChannel *chan, GIOCondition cond,
> if (s->tx_count) {
> int r = qemu_chr_fe_add_watch(s->chr, G_IO_OUT|G_IO_HUP,
> cadence_uart_xmit, s);
> - assert(r);
> + if (!r) {
> + s->tx_count = 0;
> + return FALSE;
> + }
> }
>
> uart_update_status(s);
> diff --git a/include/sysemu/char.h b/include/sysemu/char.h
> index 1eb2d0f..07434a0 100644
> --- a/include/sysemu/char.h
> +++ b/include/sysemu/char.h
> @@ -221,8 +221,20 @@ void qemu_chr_fe_event(CharDriverState *s, int event);
> void qemu_chr_fe_printf(CharDriverState *s, const char *fmt, ...)
> GCC_FMT_ATTR(2, 3);
>
> -int qemu_chr_fe_add_watch(CharDriverState *s, GIOCondition cond,
> - GIOFunc func, void *user_data);
> +/**
> + * @qemu_chr_fe_add_watch:
> + *
> + * If the backend is connected, create and add a #GSource that fires
> + * when the given condition (typically G_IO_OUT|G_IO_HUP or G_IO_HUP)
> + * is active; return the #GSource's tag. If it is disconnected,
> + * return 0.
> + *
> + * @cond the condition to poll for
> + * @func the function to call when the condition happens
> + * @user_data the opaque pointer to pass to @func
> + */
> +unsigned qemu_chr_fe_add_watch(CharDriverState *s, GIOCondition cond,
> + GIOFunc func, void *user_data);
can we make this a guint? hw/char/virtio-console.c and monitor.c already
use guint as the type for a watch and that matches the glib definition?
(Although not net/vhost-user.c which still has int, but that should probably
be fixed anyway).
But other than that;
Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
Dave
>
> /**
> * @qemu_chr_fe_write:
> diff --git a/qemu-char.c b/qemu-char.c
> index 84f49ac..39b2ccd 100644
> --- a/qemu-char.c
> +++ b/qemu-char.c
> @@ -3966,19 +3966,19 @@ void qemu_chr_fe_event(struct CharDriverState *chr, int event)
> }
> }
>
> -int qemu_chr_fe_add_watch(CharDriverState *s, GIOCondition cond,
> - GIOFunc func, void *user_data)
> +unsigned qemu_chr_fe_add_watch(CharDriverState *s, GIOCondition cond,
> + GIOFunc func, void *user_data)
> {
> GSource *src;
> guint tag;
>
> if (s->chr_add_watch == NULL) {
> - return -ENOSYS;
> + return 0;
> }
>
> src = s->chr_add_watch(s, cond);
> if (!src) {
> - return -EINVAL;
> + return 0;
> }
>
> g_source_set_callback(src, (GSourceFunc)func, user_data, NULL);
> --
> 2.5.5
>
>
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [PATCH 6/6] serial: remove watch on reset
2016-06-20 14:29 ` [Qemu-devel] [PATCH 6/6] serial: remove watch on reset Paolo Bonzini
2016-06-22 14:04 ` Bret Ketchum
@ 2016-06-22 15:38 ` Dr. David Alan Gilbert
1 sibling, 0 replies; 17+ messages in thread
From: Dr. David Alan Gilbert @ 2016-06-22 15:38 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel, bcketchum
* Paolo Bonzini (pbonzini@redhat.com) wrote:
> Otherwise, this can cause serial_xmit to be entered with LSR.TEMT=0,
> which is invalid and causes an assertion failure.
Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> Reported-by: Bret Ketchum <bcketchum@gmail.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> hw/char/serial.c | 16 ++++++++++++----
> 1 file changed, 12 insertions(+), 4 deletions(-)
>
> diff --git a/hw/char/serial.c b/hw/char/serial.c
> index d232473..7c196e2 100644
> --- a/hw/char/serial.c
> +++ b/hw/char/serial.c
> @@ -228,6 +228,7 @@ static gboolean serial_watch_cb(GIOChannel *chan, GIOCondition cond,
> void *opaque)
> {
> SerialState *s = opaque;
> + s->watch_tag = 0;
> serial_xmit(s);
> return FALSE;
> }
> @@ -258,10 +259,12 @@ static void serial_xmit(SerialState *s)
> if (s->mcr & UART_MCR_LOOP) {
> /* in loopback mode, say that we just received a char */
> serial_receive1(s, &s->tsr, 1);
> - } else if (qemu_chr_fe_write(s->chr, &s->tsr, 1) != 1) {
> - if (s->tsr_retry < MAX_XMIT_RETRY &&
> - qemu_chr_fe_add_watch(s->chr, G_IO_OUT|G_IO_HUP,
> - serial_watch_cb, s) > 0) {
> + } else if (qemu_chr_fe_write(s->chr, &s->tsr, 1) != 1 &&
> + s->tsr_retry < MAX_XMIT_RETRY) {
> + assert(s->watch_tag == 0);
> + s->watch_tag = qemu_chr_fe_add_watch(s->chr, G_IO_OUT|G_IO_HUP,
> + serial_watch_cb, s);
> + if (s->watch_tag > 0) {
> s->tsr_retry++;
> return;
> }
> @@ -834,6 +837,11 @@ static void serial_reset(void *opaque)
> {
> SerialState *s = opaque;
>
> + if (s->watch_tag > 0) {
> + g_source_remove(s->watch_tag);
> + s->watch_tag = 0;
> + }
> +
> s->rbr = 0;
> s->ier = 0;
> s->iir = UART_IIR_NO_INT;
> --
> 2.5.5
>
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [PATCH 2/6] serial: reinstate watch after migration
2016-06-22 15:11 ` Paolo Bonzini
@ 2016-06-22 15:41 ` Dr. David Alan Gilbert
0 siblings, 0 replies; 17+ messages in thread
From: Dr. David Alan Gilbert @ 2016-06-22 15:41 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel, bcketchum
* Paolo Bonzini (pbonzini@redhat.com) wrote:
>
>
> On 22/06/2016 17:05, Dr. David Alan Gilbert wrote:
> > * Paolo Bonzini (pbonzini@redhat.com) wrote:
> >> Otherwise, a serial port can get stuck if it is migrated while flow control
> >> is in effect.
> >>
> >> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> >> ---
> >> hw/char/serial.c | 16 ++++++++++++++--
> >> include/hw/char/serial.h | 1 +
> >> 2 files changed, 15 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/hw/char/serial.c b/hw/char/serial.c
> >> index e65e9e0..6f0a49e 100644
> >> --- a/hw/char/serial.c
> >> +++ b/hw/char/serial.c
> >> @@ -639,8 +639,20 @@ static int serial_post_load(void *opaque, int version_id)
> >> if (s->thr_ipending == -1) {
> >> s->thr_ipending = ((s->iir & UART_IIR_ID) == UART_IIR_THRI);
> >> }
> >> - if (s->tsr_retry > MAX_XMIT_RETRY) {
> >> - s->tsr_retry = MAX_XMIT_RETRY;
> >
> > Why remove the check you just added?
>
> Because I'm dumb. :)
>
> >> +
> >> + if (s->tsr_retry > 0) {
> >> + /* tsr_retry > 0 implies LSR.TEMT = 0 (transmitter not empty). */
> >> + if (s->lsr & UART_LSR_TEMT) {
> >> + return -1;
> >> + }
> >> +
> >> + assert(s->watch_tag == 0);
> >> + s->watch_tag = qemu_chr_fe_add_watch(s->chr, G_IO_OUT|G_IO_HUP, serial_xmit, s);
> >> + } else {
> >> + /* tsr_retry == 0 implies LSR.TEMT = 1 (transmitter empty). */
> >> + if (!(s->lsr & UART_LSR_TEMT)) {
> >> + return -1;
> >
> > If you're failing migration (-1) then please error_report to say why
> > so when someone hits it I can immediately see why.
>
> Ok, something like
>
> error_report("inconsistent state in serial device");
>
> ?
Yeh, feel free to add tsr_retry and lsr to the message as well so that if you're
looking at it you can figure out what happened.
Dave
>
> Paolo
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2016-06-22 15:41 UTC | newest]
Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-06-20 14:28 [Qemu-devel] [PATCH 0/6] serial: flow control fixes Paolo Bonzini
2016-06-20 14:29 ` [Qemu-devel] [PATCH 1/6] serial: make tsr_retry unsigned Paolo Bonzini
2016-06-22 14:44 ` Dr. David Alan Gilbert
2016-06-20 14:29 ` [Qemu-devel] [PATCH 2/6] serial: reinstate watch after migration Paolo Bonzini
2016-06-22 15:05 ` Dr. David Alan Gilbert
2016-06-22 15:11 ` Paolo Bonzini
2016-06-22 15:41 ` Dr. David Alan Gilbert
2016-06-20 14:29 ` [Qemu-devel] [PATCH 3/6] serial: separate serial_xmit and serial_watch_cb Paolo Bonzini
2016-06-22 15:09 ` Dr. David Alan Gilbert
2016-06-22 15:14 ` Paolo Bonzini
2016-06-20 14:29 ` [Qemu-devel] [PATCH 4/6] serial: simplify tsr_retry reset Paolo Bonzini
2016-06-22 15:12 ` Dr. David Alan Gilbert
2016-06-20 14:29 ` [Qemu-devel] [PATCH 5/6] char: change qemu_chr_fe_add_watch to return unsigned Paolo Bonzini
2016-06-22 15:22 ` Dr. David Alan Gilbert
2016-06-20 14:29 ` [Qemu-devel] [PATCH 6/6] serial: remove watch on reset Paolo Bonzini
2016-06-22 14:04 ` Bret Ketchum
2016-06-22 15:38 ` Dr. David Alan Gilbert
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.