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

[Jan Kiszka <jan.kiszka@web.de>]

[-- Attachment #1: Type: text/plain, Size: 3865 bytes --]

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:
> 
> Index: qemu/hw/serial.c
> @@ -196,12 +195,10 @@ static void serial_update_irq(SerialStat
>           * this is not in the specification but is observed on existing
>           * hardware.  */
>          tmp_iir = UART_IIR_CTI;
> -    } else if ((s->ier & UART_IER_RDI) && (s->lsr & UART_LSR_DR)) {
> -        if (!(s->fcr & UART_FCR_FE)) {
> -           tmp_iir = UART_IIR_RDI;
> -        } else if (s->recv_fifo.count >= s->recv_fifo.itl) {
> -           tmp_iir = UART_IIR_RDI;
> -        }
> +    } else if ((s->ier & UART_IER_RDI) && (s->lsr & UART_LSR_DR) &&
> +               (!(s->fcr & UART_FCR_FE) ||
> +                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)) {
> 
>  Signed-off-by: Juergen Lock <nox@jelal.kn-bremen.de>

Yep, that does make sense!

Acked-by: Jan Kiszka <jan.kiszka@web.de>

But I also but Stefano on CC as he introduced the logic above.

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]