Re: [Qemu-devel] Re: qemu serial: lost tx irqs (affectig FreeBSD's new uart(4) driver)

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_at_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
> > >> > > _at__at_ -189,7 +188,9 _at__at_ 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,
> > >> > > _at__at_ -202,8 +203,6 _at__at_ 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