Date: Thu, 24 Sep 2009 23:26:59 +0200 From: Juergen Lock <nox@jelal.kn-bremen.de> To: Aurelien Jarno <aurelien@aurel32.net> Cc: Olivier =?iso-8859-1?Q?Cochard-Labb=E9?= <olivier@cochard.me>, freebsd-current@FreeBSD.org, Jan Kiszka <jan.kiszka@web.de>, Juergen Lock <nox@jelal.kn-bremen.de>, qemu-devel@nongnu.org Subject: Re: [Qemu-devel] Re: qemu serial: lost tx irqs (affectig FreeBSD's new uart(4) driver) Message-ID: <20090924212659.GA3248@triton8.kn-bremen.de> In-Reply-To: <20090924162000.GG770@volta.aurel32.net> References: <20090911213508.GA97446@triton8.kn-bremen.de> <4AAB938B.9080004@web.de> <20090912165222.GA38048@triton8.kn-bremen.de> <200909231847.n8NIlNij097002@triton8.kn-bremen.de> <20090924162000.GG770@volta.aurel32.net>
next in thread | previous in thread | raw e-mail | index | archive | help
On Thu, Sep 24, 2009 at 06:20:00PM +0200, Aurelien Jarno wrote:
> 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
>
Oh ok, then I completely misunderstood. (And I had no idea this makes
a difference... :) Anyway, I'll try to remember.
> > 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.
>
Yeah, looks like it...
Thanx,
Juergen
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20090924212659.GA3248>