[Qemu-devel] hw/serial.c: Xmit fifo never used

[Qemu-devel] hw/serial.c: Xmit fifo never used

[Frank Mehnert]

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

Hi,

the xmit fifo of the serial device is never used. If qemu_chr_write()
fails (interface currently not able to send characters) then the
transmit_timer should be engaged to try to send the current character
from the fifo again after some time. The code is

    } else if (qemu_chr_write(s->chr, &s->tsr, 1) != 1) {
        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;
        }
        ...
    }

The problem is that this path is never used as tsr_retry is never > 0
initially. So if qemu_chr_write() fails, we never try again but drop
the character.

I assume the correct condition would be '>= 0', that is

        ...
        if ((s->tsr_retry >= 0) && (s->tsr_retry <= MAX_XMIT_RETRY)) {
            s->tsr_retry++;
        ...

Kind regards,

Frank
-- 
Dr.-Ing. Frank Mehnert

Sitz der Gesellschaft:
Sun Microsystems GmbH, Sonnenallee 1, 85551 Kirchheim-Heimstetten
Amtsgericht München: HRB 161028
Geschäftsführer: Jürgen Kunz

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

[Qemu-devel] Re: hw/serial.c: Xmit fifo never used

[Jan Kiszka]

Frank Mehnert wrote:
> Hi,
> 
> the xmit fifo of the serial device is never used. If qemu_chr_write()
> fails (interface currently not able to send characters) then the
> transmit_timer should be engaged to try to send the current character
> from the fifo again after some time. The code is
> 
>     } else if (qemu_chr_write(s->chr, &s->tsr, 1) != 1) {
>         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;
>         }
>         ...
>     }
> 
> The problem is that this path is never used as tsr_retry is never > 0
> initially. So if qemu_chr_write() fails, we never try again but drop
> the character.
> 
> I assume the correct condition would be '>= 0', that is
> 
>         ...
>         if ((s->tsr_retry >= 0) && (s->tsr_retry <= MAX_XMIT_RETRY)) {
>             s->tsr_retry++;
>         ...

Makes sense, patch welcome.

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Re: [Qemu-devel] Re: hw/serial.c: Xmit fifo never used

[Frank Mehnert]

[-- Attachment #1.1: Type: text/plain, Size: 536 bytes --]

On Wednesday 26 May 2010, Jan Kiszka wrote:
> Frank Mehnert wrote:
> > I assume the correct condition would be '>= 0', that is
> >
> >         ...
> >         if ((s->tsr_retry >= 0) && (s->tsr_retry <= MAX_XMIT_RETRY)) {
> >             s->tsr_retry++;
> >         ...
>
> Makes sense, patch welcome.

Attached.

Kind regards,

Frank
-- 
Dr.-Ing. Frank Mehnert

Sitz der Gesellschaft:
Sun Microsystems GmbH, Sonnenallee 1, 85551 Kirchheim-Heimstetten
Amtsgericht München: HRB 161028
Geschäftsführer: Jürgen Kunz

[-- Attachment #1.2: patch_hw_serial --]
[-- Type: text/x-diff, Size: 608 bytes --]

diff --git a/hw/serial.c b/hw/serial.c
index 9102edb..0b1550b 100644
--- a/hw/serial.c
+++ b/hw/serial.c
@@ -327,7 +327,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_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;

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

[Qemu-devel] Re: hw/serial.c: Xmit fifo never used

[Jan Kiszka]

Frank Mehnert wrote:
> On Wednesday 26 May 2010, Jan Kiszka wrote:
>> Frank Mehnert wrote:
>>> I assume the correct condition would be '>= 0', that is
>>>
>>>         ...
>>>         if ((s->tsr_retry >= 0) && (s->tsr_retry <= MAX_XMIT_RETRY)) {
>>>             s->tsr_retry++;
>>>         ...
>> Makes sense, patch welcome.
> 
> Attached.

Signed-off, subject, and description please. And I would keep Stefano as
the original author of this feature in CC. A cross-check by him might be
good as we enable a code path that was apparently dead so far.

Thanks,
Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Re: [Qemu-devel] Re: hw/serial.c: Xmit fifo never used

[Stefano Stabellini]

On Wed, 26 May 2010, Jan Kiszka wrote:
> Frank Mehnert wrote:
> > On Wednesday 26 May 2010, Jan Kiszka wrote:
> >> Frank Mehnert wrote:
> >>> I assume the correct condition would be '>= 0', that is
> >>>
> >>>         ...
> >>>         if ((s->tsr_retry >= 0) && (s->tsr_retry <= MAX_XMIT_RETRY)) {
> >>>             s->tsr_retry++;
> >>>         ...
> >> Makes sense, patch welcome.
> > 
> > Attached.
> 
> Signed-off, subject, and description please. And I would keep Stefano as
> the original author of this feature in CC. A cross-check by him might be
> good as we enable a code path that was apparently dead so far.
> 

I think the patch is correct.

Re: [Qemu-devel] Re: hw/serial.c: Xmit fifo never used

[Frank Mehnert]

On Wednesday 26 May 2010, Stefano Stabellini wrote:
> I think the patch is correct.

serial: fixed bug which prevented the use of the xmit fifo

Signed-off-by: Frank Mehnert <frank.mehnert@sun.com>
---
 hw/serial.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/hw/serial.c b/hw/serial.c
index 9102edb..0b1550b 100644
--- a/hw/serial.c
+++ b/hw/serial.c
@@ -327,7 +327,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_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;
--
1.5.6.5



Hope this is now the correct format.

Kind regards,

Frank
-- 
Dr.-Ing. Frank Mehnert

Sitz der Gesellschaft:
Sun Microsystems GmbH, Sonnenallee 1, 85551 Kirchheim-Heimstetten
Amtsgericht München: HRB 161028
Geschäftsführer: Jürgen Kunz

Re: [Qemu-devel] Re: hw/serial.c: Xmit fifo never used

[Jan Kiszka]

Frank Mehnert wrote:
> On Wednesday 26 May 2010, Stefano Stabellini wrote:
>> I think the patch is correct.
> 
> serial: fixed bug which prevented the use of the xmit fifo
> 
> Signed-off-by: Frank Mehnert <frank.mehnert@sun.com>
> ---
>  hw/serial.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/hw/serial.c b/hw/serial.c
> index 9102edb..0b1550b 100644
> --- a/hw/serial.c
> +++ b/hw/serial.c
> @@ -327,7 +327,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_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;
> --
> 1.5.6.5
> 
> 
> 
> Hope this is now the correct format.

Really close: Without "[PATCH]" prefix in the subject line there is the
risk that no maintainer will find your patch. Moreover, your mail client
wrapped long lines.

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Re: [Qemu-devel] hw/serial.c: Xmit fifo never used

[Paul Brook]

> The problem is that this path is never used as tsr_retry is never > 0
> initially. So if qemu_chr_write() fails, we never try again but drop

qemu_chr_write is a blocking interface. It should only fail if an 
unrecoverable error occurs. In that case there's noting useful we can do, and 
no reason to retry.

Paul