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

[Juergen Lock]

In article <address@hidden> 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.)

 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?)

 Thanx,
        Juergen