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

[Michael Tokarev]

Ping^2 ?

/mjt

27.10.2012 12:31, Michael Tokarev wrote:
> Ping?
> 
> 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 <address@hidden>
>>     Signed-off-by: Anthony Liguori <address@hidden>
>>
>> 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 <address@hidden>
>> ---
>>  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;
> 
>