Date: Thu, 24 Sep 2009 18:20:00 +0200 From: Aurelien Jarno <aurelien@aurel32.net> To: Juergen Lock <nox@jelal.kn-bremen.de> Cc: Olivier =?iso-8859-15?Q?Cochard-Labb=E9?= <olivier@cochard.me>, freebsd-current@FreeBSD.org, Jan Kiszka <jan.kiszka@web.de>, qemu-devel@nongnu.org Subject: Re: [Qemu-devel] Re: qemu serial: lost tx irqs (affectig FreeBSD's new uart(4) driver) Message-ID: <20090924162000.GG770@volta.aurel32.net> In-Reply-To: <200909231847.n8NIlNij097002@triton8.kn-bremen.de> References: <20090911213508.GA97446@triton8.kn-bremen.de> <4AAB938B.9080004@web.de> <20090912165222.GA38048@triton8.kn-bremen.de> <200909231847.n8NIlNij097002@triton8.kn-bremen.de>
next in thread | previous in thread | raw e-mail | index | archive | help
On Wed, Sep 23, 2009 at 08:47:23PM +0200, Juergen Lock wrote:
> In article <20090916190142.GC770@volta.aurel32.net> you write:
> >On Sat, Sep 12, 2009 at 06:52:22PM +0200, Juergen Lock wrote:
> >> On Sat, Sep 12, 2009 at 02:26:51PM +0200, Jan Kiszka wrote:
> >> > Juergen Lock wrote:
> >> > > Hi!
> >> > >
> >> > > I got a report of FreeBSD guest's new uart(4) driver misbehaving in
> >> > > qemu again(?) (output stopping for no apparent reason), and now found
> >> > > out the problem is tx irqs (UART_IIR_THRI) are getting lost because
> >> > > serial_update_irq() checks for the rx condtion,
> >> > > ... if ((s->ier & UART_IER_RDI) && (s->lsr & UART_LSR_DR))
> >> > > first before checking for the tx irq condition,
> >> > > ... if ((s->ier & UART_IER_THRI) && s->thr_ipending)
> >> > > which at least in this case (FreeBSD 8 guest after doing
> >> > > set console="comconsole"
> >> > > at the loader prompt or when simply echo'ing text to /dev/ttyu0
> >> > > or typing to the serial port from cu(1) on a `regular' vga console)
> >> > > causes the second condition (.. && s->thr_ipending) to be never
> >> > > reached anymore, or only after a very long delay. Moving that
> >> > > condition up so it is checked first like this,
> >> > >
> >> > > Index: qemu/hw/serial.c
> >> > > @@ -189,7 +188,9 @@ static void serial_update_irq(SerialStat
> >> > > {
> >> > > uint8_t tmp_iir = UART_IIR_NO_INT;
> >> > >
> >> > > - if ((s->ier & UART_IER_RLSI) && (s->lsr & UART_LSR_INT_ANY)) {
> >> > > + if ((s->ier & UART_IER_THRI) && s->thr_ipending) {
> >> > > + tmp_iir = UART_IIR_THRI;
> >> > > + } else if ((s->ier & UART_IER_RLSI) && (s->lsr & UART_LSR_INT_ANY)) {
> >> > > tmp_iir = UART_IIR_RLSI;
> >> > > } else if ((s->ier & UART_IER_RDI) && s->timeout_ipending) {
> >> > > /* Note that(s->ier & UART_IER_RDI) can mask this interrupt,
> >> > > @@ -202,8 +203,6 @@ static void serial_update_irq(SerialStat
> >> > > } else if (s->recv_fifo.count >= s->recv_fifo.itl) {
> >> > > tmp_iir = UART_IIR_RDI;
> >> > > }
> >> > > - } else if ((s->ier & UART_IER_THRI) && s->thr_ipending) {
> >> > > - tmp_iir = UART_IIR_THRI;
> >> > > } else if ((s->ier & UART_IER_MSI) && (s->msr & UART_MSR_ANY_DELTA)) {
> >> > > tmp_iir = UART_IIR_MSI;
> >> > > }
> >> > >
> >> > > ...fixes the issue for me, but I'm not 100% sure if this might cause
> >> > > rx irqs to come (too?) late when a guest keeps sending while its
> >> > > receiving at the same time. Anyone care to comment? :)
> >> >
> >> > The reordering violates the 16550A spec in that RX event overrules TX in
> >> > the IRQ status register. Maybe something else is wrong but it's not the
> >> > ordering in serial_update_irq.
> >>
> >> Well one problem seems to be the rx condition,
> >> ... if ((s->ier & UART_IER_RDI) && (s->lsr & UART_LSR_DR))
> >> is not enough to trigger an irq, yet still causes the following
> >> conditions not to be checked anymore at all. And ideed, fixing that
> >> seems to get my FreeBSD 8 guest back to working order as well:
> >
> >Applied. In the future, could you please make sure to send patches with
> >a correct unified headers?
>
> Alright, if thats is what you guys prefer... (I just didn't want to
> break the thread.)
Don't need to break the thread, just use
| --- a/hw/serial.c
| +++ a/hw/serial.c
instead of simply
| Index: qemu/hw/serial.c
ie the output of diff -u
> Anyway, I guess this is also material for the stable branch(es)?
> (I just saw 0.11.0 has already been tagged but not announced yet, and
> another patch merged to the same branch after that, maybe the tag can
> still be slided if this is possible with git?)
>
Pushed to the branch. For the details about the release, I let Anthony
handling that. Worst case scenario, it will be in 0.11.1.
--
Aurelien Jarno GPG: 1024D/F1BCDB73
aurelien@aurel32.net http://www.aurel32.net
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20090924162000.GG770>