Patchwork Revert "serial: fix retry logic"

login
register
mail settings
Submitter Michael Tokarev
Date Sept. 19, 2012, 8:08 a.m.
Message ID <1348042111-16143-1-git-send-email-mjt@msgid.tls.msk.ru>
Download mbox | patch
Permalink /patch/184945/
State New
Headers show

Comments

Michael Tokarev - Sept. 19, 2012, 8:08 a.m.
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(-)
Michael Tokarev - Oct. 27, 2012, 8:31 a.m.
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;
Michael Tokarev - Nov. 12, 2012, 3:13 p.m.
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 <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;
> 
>
Michael Tokarev - Jan. 22, 2013, 11:01 a.m.
Ping^3?

This issue is still present in qemu 1.3 and current git (1.4-tobe) versions,
and the said commit is still revertable, and reverting it still fixes the
problem...

I wonder why only debian users suffer from this problem ;)

Thanks,

/mjt

12.11.2012 19:13, Michael Tokarev wrote:
> 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 <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;
>>
>>
>
>
Andreas Färber - Jan. 24, 2013, 10:03 a.m.
Am 22.01.2013 12:01, schrieb Michael Tokarev:
> Ping^3?
> 
> This issue is still present in qemu 1.3 and current git (1.4-tobe)
> versions,
> and the said commit is still revertable, and reverting it still fixes the
> problem...
> 
> I wonder why only debian users suffer from this problem ;)

It was reported for openSUSE as well [1], but Anthony promised me it
would get reverted for 1.3... I admit, I simply assumed it to be fixed
with v1.3.0.

Regards,
Andreas

[1] https://bugzilla.novell.com/show_bug.cgi?id=779727

> 
> Thanks,
> 
> /mjt
> 
> 12.11.2012 19:13, Michael Tokarev wrote:
>> 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 <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;
>>>
>>>
>>
>>
> 
>

Patch

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;