| Message ID | 1466432945-28682-3-git-send-email-pbonzini@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
* 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
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
* 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
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 */
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(-)