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

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

>  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_at_aurel32.net                 http://www.aurel32.net