Re: [Qemu-devel] [PATCH] Revert "serial: fix retry logic"

[Michael Tokarev <mjt@tls.msk.ru>]

Ping?

/mjt

On 19.09.2012 12:08, Michael Tokarev wrote:
> This reverts commit 67c5322d7000fd105a926eec44bc1765b7d70bdd:
> 
>     I'm not sure if the retry logic has ever worked when not using FIFO mode.  I
>     found this while writing a test case although code inspection confirms it is
>     definitely broken.
> 
>     The TSR retry logic will never actually happen because it is guarded by an
>     'if (s->tsr_rety > 0)' but this is the only place that can ever make the
>     variable greater than zero.  That effectively makes the retry logic an 'if (0)
> 
>     I believe this is a typo and the intention was >= 0.  Once this is fixed thoug
>     I see double transmits with my test case.  This is because in the non FIFO
>     case, serial_xmit may get invoked while LSR.THRE is still high because the
>     character was processed but the retransmit timer was still active.
> 
>     We can handle this by simply checking for LSR.THRE and returning early.  It's
>     possible that the FIFO paths also need some attention.
> 
>     Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
>     Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
> 
> Even if the previous logic was never worked, new logic breaks stuff -
> namely,
> 
>  qemu -enable-kvm -nographic -kernel /boot/vmlinuz-$(uname -r) -append console=ttyS0 -serial pty
> 
> the above command will cause the virtual machine to stuck at startup
> using 100% CPU till one connects to the pty and sends any char to it.
> 
> Note this is rather typical invocation for various headless virtual
> machines by libvirt.
> 
> So revert this change for now, till a better solution will be found.
> 
> Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>
> ---
>  hw/serial.c |    4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/hw/serial.c b/hw/serial.c
> index a421d1e..df54de2 100644
> --- a/hw/serial.c
> +++ b/hw/serial.c
> @@ -327,8 +327,6 @@ static void serial_xmit(void *opaque)
>              s->tsr = fifo_get(s,XMIT_FIFO);
>              if (!s->xmit_fifo.count)
>                  s->lsr |= UART_LSR_THRE;
> -        } else if ((s->lsr & UART_LSR_THRE)) {
> -            return;
>          } else {
>              s->tsr = s->thr;
>              s->lsr |= UART_LSR_THRE;
> @@ -340,7 +338,7 @@ static void serial_xmit(void *opaque)
>          /* in loopback mode, say that we just received a char */
>          serial_receive1(s, &s->tsr, 1);
>      } else if (qemu_chr_fe_write(s->chr, &s->tsr, 1) != 1) {
> -        if ((s->tsr_retry >= 0) && (s->tsr_retry <= MAX_XMIT_RETRY)) {
> +        if ((s->tsr_retry > 0) && (s->tsr_retry <= MAX_XMIT_RETRY)) {
>              s->tsr_retry++;
>              qemu_mod_timer(s->transmit_timer,  new_xmit_ts + s->char_transmit_time);
>              return;