[Qemu-devel] [PATCH v1 0/3] Serial cleanup

[Qemu-devel] [PATCH v1 0/3] Serial cleanup

[peter.crosthwaite]

From: Peter Crosthwaite <peter.crosthwaite@xilinx.com>

Some cosmetics, refactored to use util/fifo8 for the FIFO8, then
factored out some common code.

Tested as working on petalogix-ml605 machine model + Linux (has
coverage of serial fifo usage).


Peter Crosthwaite (3):
  char/serial: cosmetic fixes.
  char/serial: Use generic Fifo8
  char/serial: serial_ioport_write: Factor out common code

 hw/char/serial.c         | 128 +++++++++++++++++++----------------------------
 include/hw/char/serial.h |  15 ++----
 2 files changed, 56 insertions(+), 87 deletions(-)

-- 
1.8.3.rc1.44.gb387c77.dirty

[Qemu-devel] [PATCH v1 1/3] char/serial: cosmetic fixes.

[peter.crosthwaite]

From: Peter Crosthwaite <peter.crosthwaite@xilinx.com>

Some cosmetic fixes to char/serial fixing some checkpatch errors.

Cc: qemu-trivial@nongnu.org

Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
---
Needed for the next patch to pass checkpatch. Done as sep patch to not
obscure that patch.

 hw/char/serial.c | 30 +++++++++++++++++++-----------
 1 file changed, 19 insertions(+), 11 deletions(-)

diff --git a/hw/char/serial.c b/hw/char/serial.c
index 66b6348..bd6813e 100644
--- a/hw/char/serial.c
+++ b/hw/char/serial.c
@@ -263,8 +263,9 @@ static gboolean serial_xmit(GIOChannel *chan, GIOCondition cond, void *opaque)
     if (s->tsr_retry <= 0) {
         if (s->fcr & UART_FCR_FE) {
             s->tsr = fifo_get(s,XMIT_FIFO);
-            if (!s->xmit_fifo.count)
+            if (!s->xmit_fifo.count) {
                 s->lsr |= UART_LSR_THRE;
+            }
         } else if ((s->lsr & UART_LSR_THRE)) {
             return FALSE;
         } else {
@@ -461,10 +462,11 @@ static uint64_t serial_ioport_read(void *opaque, hwaddr addr, unsigned size)
         } else {
             if(s->fcr & UART_FCR_FE) {
                 ret = fifo_get(s,RECV_FIFO);
-                if (s->recv_fifo.count == 0)
+                if (s->recv_fifo.count == 0) {
                     s->lsr &= ~(UART_LSR_DR | UART_LSR_BI);
-                else
+                } else {
                     qemu_mod_timer(s->fifo_timeout_timer, qemu_get_clock_ns (vm_clock) + s->char_transmit_time * 4);
+                }
                 s->timeout_ipending = 0;
             } else {
                 ret = s->rbr;
@@ -534,15 +536,21 @@ static uint64_t serial_ioport_read(void *opaque, hwaddr addr, unsigned size)
 static int serial_can_receive(SerialState *s)
 {
     if(s->fcr & UART_FCR_FE) {
-        if(s->recv_fifo.count < UART_FIFO_LENGTH)
-        /* Advertise (fifo.itl - fifo.count) bytes when count < ITL, and 1 if above. If UART_FIFO_LENGTH - fifo.count is
-        advertised the effect will be to almost always fill the fifo completely before the guest has a chance to respond,
-        effectively overriding the ITL that the guest has set. */
-             return (s->recv_fifo.count <= s->recv_fifo.itl) ? s->recv_fifo.itl - s->recv_fifo.count : 1;
-        else
-             return 0;
+        if (s->recv_fifo.count < UART_FIFO_LENGTH) {
+            /*
+             * Advertise (fifo.itl - fifo.count) bytes when count < ITL, and 1
+             * if above. If UART_FIFO_LENGTH - fifo.count is advertised the
+             * effect will be to almost always fill the fifo completely before
+             * the guest has a chance to respond, effectively overriding the ITL
+             * that the guest has set.
+             */
+            return (s->recv_fifo.count <= s->recv_fifo.itl) ?
+                        s->recv_fifo.itl - s->recv_fifo.count : 1;
+        } else {
+            return 0;
+        }
     } else {
-    return !(s->lsr & UART_LSR_DR);
+        return !(s->lsr & UART_LSR_DR);
     }
 }
 
-- 
1.8.3.rc1.44.gb387c77.dirty

[Qemu-devel] [PATCH v1 2/3] char/serial: Use generic Fifo8

[peter.crosthwaite]

From: Peter Crosthwaite <peter.crosthwaite@xilinx.com>

Use the generic Fifo8 helper provided by QEMU, rather than re-implement
privately.

Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
---

 hw/char/serial.c         | 98 +++++++++++++++++-------------------------------
 include/hw/char/serial.h | 15 +++-----
 2 files changed, 39 insertions(+), 74 deletions(-)

diff --git a/hw/char/serial.c b/hw/char/serial.c
index bd6813e..0a2b6c9 100644
--- a/hw/char/serial.c
+++ b/hw/char/serial.c
@@ -92,8 +92,6 @@
 #define UART_FCR_RFR        0x02    /* RCVR Fifo Reset */
 #define UART_FCR_FE         0x01    /* FIFO Enable */
 
-#define XMIT_FIFO           0
-#define RECV_FIFO           1
 #define MAX_XMIT_RETRY      4
 
 #ifdef DEBUG_SERIAL
@@ -106,50 +104,14 @@ do {} while (0)
 
 static void serial_receive1(void *opaque, const uint8_t *buf, int size);
 
-static void fifo_clear(SerialState *s, int fifo)
+static inline void recv_fifo_put(SerialState *s, uint8_t chr)
 {
-    SerialFIFO *f = (fifo) ? &s->recv_fifo : &s->xmit_fifo;
-    memset(f->data, 0, UART_FIFO_LENGTH);
-    f->count = 0;
-    f->head = 0;
-    f->tail = 0;
-}
-
-static int fifo_put(SerialState *s, int fifo, uint8_t chr)
-{
-    SerialFIFO *f = (fifo) ? &s->recv_fifo : &s->xmit_fifo;
-
     /* Receive overruns do not overwrite FIFO contents. */
-    if (fifo == XMIT_FIFO || f->count < UART_FIFO_LENGTH) {
-
-        f->data[f->head++] = chr;
-
-        if (f->head == UART_FIFO_LENGTH)
-            f->head = 0;
-    }
-
-    if (f->count < UART_FIFO_LENGTH)
-        f->count++;
-    else if (fifo == RECV_FIFO)
+    if (!fifo8_is_full(&s->recv_fifo)) {
+        fifo8_push(&s->recv_fifo, chr);
+    } else {
         s->lsr |= UART_LSR_OE;
-
-    return 1;
-}
-
-static uint8_t fifo_get(SerialState *s, int fifo)
-{
-    SerialFIFO *f = (fifo) ? &s->recv_fifo : &s->xmit_fifo;
-    uint8_t c;
-
-    if(f->count == 0)
-        return 0;
-
-    c = f->data[f->tail++];
-    if (f->tail == UART_FIFO_LENGTH)
-        f->tail = 0;
-    f->count--;
-
-    return c;
+    }
 }
 
 static void serial_update_irq(SerialState *s)
@@ -165,7 +127,7 @@ static void serial_update_irq(SerialState *s)
         tmp_iir = UART_IIR_CTI;
     } 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)) {
+                s->recv_fifo.num >= s->recv_fifo_itl)) {
         tmp_iir = UART_IIR_RDI;
     } else if ((s->ier & UART_IER_THRI) && s->thr_ipending) {
         tmp_iir = UART_IIR_THRI;
@@ -262,8 +224,9 @@ static gboolean serial_xmit(GIOChannel *chan, GIOCondition cond, void *opaque)
 
     if (s->tsr_retry <= 0) {
         if (s->fcr & UART_FCR_FE) {
-            s->tsr = fifo_get(s,XMIT_FIFO);
-            if (!s->xmit_fifo.count) {
+            s->tsr = fifo8_is_full(&s->xmit_fifo) ?
+                        0 : fifo8_pop(&s->xmit_fifo);
+            if (!s->xmit_fifo.num) {
                 s->lsr |= UART_LSR_THRE;
             }
         } else if ((s->lsr & UART_LSR_THRE)) {
@@ -317,7 +280,11 @@ static void serial_ioport_write(void *opaque, hwaddr addr, uint64_t val,
         } else {
             s->thr = (uint8_t) val;
             if(s->fcr & UART_FCR_FE) {
-                fifo_put(s, XMIT_FIFO, s->thr);
+                /* xmit overruns overwrite data, so make space if needed */
+                if (fifo8_is_full(&s->xmit_fifo)) {
+                    fifo8_pop(&s->xmit_fifo);
+                }
+                fifo8_push(&s->xmit_fifo, s->thr);
                 s->thr_ipending = 0;
                 s->lsr &= ~UART_LSR_TEMT;
                 s->lsr &= ~UART_LSR_THRE;
@@ -368,28 +335,28 @@ static void serial_ioport_write(void *opaque, hwaddr addr, uint64_t val,
         if (val & UART_FCR_RFR) {
             qemu_del_timer(s->fifo_timeout_timer);
             s->timeout_ipending=0;
-            fifo_clear(s,RECV_FIFO);
+            fifo8_reset(&s->recv_fifo);
         }
 
         if (val & UART_FCR_XFR) {
-            fifo_clear(s,XMIT_FIFO);
+            fifo8_reset(&s->xmit_fifo);
         }
 
         if (val & UART_FCR_FE) {
             s->iir |= UART_IIR_FE;
-            /* Set RECV_FIFO trigger Level */
+            /* Set recv_fifo trigger Level */
             switch (val & 0xC0) {
             case UART_FCR_ITL_1:
-                s->recv_fifo.itl = 1;
+                s->recv_fifo_itl = 1;
                 break;
             case UART_FCR_ITL_2:
-                s->recv_fifo.itl = 4;
+                s->recv_fifo_itl = 4;
                 break;
             case UART_FCR_ITL_3:
-                s->recv_fifo.itl = 8;
+                s->recv_fifo_itl = 8;
                 break;
             case UART_FCR_ITL_4:
-                s->recv_fifo.itl = 14;
+                s->recv_fifo_itl = 14;
                 break;
             }
         } else
@@ -461,8 +428,9 @@ static uint64_t serial_ioport_read(void *opaque, hwaddr addr, unsigned size)
             ret = s->divider & 0xff;
         } else {
             if(s->fcr & UART_FCR_FE) {
-                ret = fifo_get(s,RECV_FIFO);
-                if (s->recv_fifo.count == 0) {
+                ret = fifo8_is_full(&s->recv_fifo) ?
+                            0 : fifo8_pop(&s->recv_fifo);
+                if (s->recv_fifo.num == 0) {
                     s->lsr &= ~(UART_LSR_DR | UART_LSR_BI);
                 } else {
                     qemu_mod_timer(s->fifo_timeout_timer, qemu_get_clock_ns (vm_clock) + s->char_transmit_time * 4);
@@ -536,7 +504,7 @@ static uint64_t serial_ioport_read(void *opaque, hwaddr addr, unsigned size)
 static int serial_can_receive(SerialState *s)
 {
     if(s->fcr & UART_FCR_FE) {
-        if (s->recv_fifo.count < UART_FIFO_LENGTH) {
+        if (s->recv_fifo.num < UART_FIFO_LENGTH) {
             /*
              * Advertise (fifo.itl - fifo.count) bytes when count < ITL, and 1
              * if above. If UART_FIFO_LENGTH - fifo.count is advertised the
@@ -544,8 +512,8 @@ static int serial_can_receive(SerialState *s)
              * the guest has a chance to respond, effectively overriding the ITL
              * that the guest has set.
              */
-            return (s->recv_fifo.count <= s->recv_fifo.itl) ?
-                        s->recv_fifo.itl - s->recv_fifo.count : 1;
+            return (s->recv_fifo.num <= s->recv_fifo_itl) ?
+                        s->recv_fifo_itl - s->recv_fifo.num : 1;
         } else {
             return 0;
         }
@@ -558,7 +526,7 @@ static void serial_receive_break(SerialState *s)
 {
     s->rbr = 0;
     /* When the LSR_DR is set a null byte is pushed into the fifo */
-    fifo_put(s, RECV_FIFO, '\0');
+    recv_fifo_put(s, '\0');
     s->lsr |= UART_LSR_BI | UART_LSR_DR;
     serial_update_irq(s);
 }
@@ -566,7 +534,7 @@ static void serial_receive_break(SerialState *s)
 /* There's data in recv_fifo and s->rbr has not been read for 4 char transmit times */
 static void fifo_timeout_int (void *opaque) {
     SerialState *s = opaque;
-    if (s->recv_fifo.count) {
+    if (s->recv_fifo.num) {
         s->timeout_ipending = 1;
         serial_update_irq(s);
     }
@@ -588,7 +556,7 @@ static void serial_receive1(void *opaque, const uint8_t *buf, int size)
     if(s->fcr & UART_FCR_FE) {
         int i;
         for (i = 0; i < size; i++) {
-            fifo_put(s, RECV_FIFO, buf[i]);
+            recv_fifo_put(s, buf[i]);
         }
         s->lsr |= UART_LSR_DR;
         /* call the timeout receive callback in 4 char transmit time */
@@ -668,8 +636,8 @@ static void serial_reset(void *opaque)
     s->char_transmit_time = (get_ticks_per_sec() / 9600) * 10;
     s->poll_msl = 0;
 
-    fifo_clear(s,RECV_FIFO);
-    fifo_clear(s,XMIT_FIFO);
+    fifo8_reset(&s->recv_fifo);
+    fifo8_reset(&s->xmit_fifo);
 
     s->last_xmit_ts = qemu_get_clock_ns(vm_clock);
 
@@ -692,6 +660,8 @@ void serial_init_core(SerialState *s)
 
     qemu_chr_add_handlers(s->chr, serial_can_receive1, serial_receive1,
                           serial_event, s);
+    fifo8_create(&s->recv_fifo, UART_FIFO_LENGTH);
+    fifo8_create(&s->xmit_fifo, UART_FIFO_LENGTH);
 }
 
 void serial_exit_core(SerialState *s)
diff --git a/include/hw/char/serial.h b/include/hw/char/serial.h
index bca79f1..9ab81f6 100644
--- a/include/hw/char/serial.h
+++ b/include/hw/char/serial.h
@@ -28,17 +28,10 @@
 #include "hw/hw.h"
 #include "sysemu/sysemu.h"
 #include "exec/memory.h"
+#include "qemu/fifo8.h"
 
 #define UART_FIFO_LENGTH    16      /* 16550A Fifo Length */
 
-typedef struct SerialFIFO {
-    uint8_t data[UART_FIFO_LENGTH];
-    uint8_t count;
-    uint8_t itl;                        /* Interrupt Trigger Level */
-    uint8_t tail;
-    uint8_t head;
-} SerialFIFO;
-
 struct SerialState {
     uint16_t divider;
     uint8_t rbr; /* receive register */
@@ -67,8 +60,10 @@ struct SerialState {
 
     /* Time when the last byte was successfully sent out of the tsr */
     uint64_t last_xmit_ts;
-    SerialFIFO recv_fifo;
-    SerialFIFO xmit_fifo;
+    Fifo8 recv_fifo;
+    Fifo8 xmit_fifo;
+    /* Interrupt trigger level for recv_fifo */
+    uint8_t recv_fifo_itl;
 
     struct QEMUTimer *fifo_timeout_timer;
     int timeout_ipending;           /* timeout interrupt pending state */
-- 
1.8.3.rc1.44.gb387c77.dirty

[Qemu-devel] [PATCH v1 3/3] char/serial: serial_ioport_write: Factor out common code

[peter.crosthwaite]

From: Peter Crosthwaite <peter.crosthwaite@xilinx.com>

These three lines are common to both FIFO and regular mode. Just factor
them out to outside the if rather than replicate the same lines inside
both if and else.

Cc: qemu-trivial@nongnu.org

Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
---

 hw/char/serial.c | 10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)

diff --git a/hw/char/serial.c b/hw/char/serial.c
index 0a2b6c9..017610e 100644
--- a/hw/char/serial.c
+++ b/hw/char/serial.c
@@ -285,15 +285,11 @@ static void serial_ioport_write(void *opaque, hwaddr addr, uint64_t val,
                     fifo8_pop(&s->xmit_fifo);
                 }
                 fifo8_push(&s->xmit_fifo, s->thr);
-                s->thr_ipending = 0;
                 s->lsr &= ~UART_LSR_TEMT;
-                s->lsr &= ~UART_LSR_THRE;
-                serial_update_irq(s);
-            } else {
-                s->thr_ipending = 0;
-                s->lsr &= ~UART_LSR_THRE;
-                serial_update_irq(s);
             }
+            s->thr_ipending = 0;
+            s->lsr &= ~UART_LSR_THRE;
+            serial_update_irq(s);
             serial_xmit(NULL, G_IO_OUT, s);
         }
         break;
-- 
1.8.3.rc1.44.gb387c77.dirty

Re: [Qemu-devel] [PATCH v1 0/3] Serial cleanup

[Peter Crosthwaite]

Ping!

Any objections to this one going in? perhaps even via trivial queue?

Regards,
Peter

On Mon, Jun 3, 2013 at 3:11 PM,  <peter.crosthwaite@xilinx.com> wrote:
> From: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
>
> Some cosmetics, refactored to use util/fifo8 for the FIFO8, then
> factored out some common code.
>
> Tested as working on petalogix-ml605 machine model + Linux (has
> coverage of serial fifo usage).
>
>
> Peter Crosthwaite (3):
>   char/serial: cosmetic fixes.
>   char/serial: Use generic Fifo8
>   char/serial: serial_ioport_write: Factor out common code
>
>  hw/char/serial.c         | 128 +++++++++++++++++++----------------------------
>  include/hw/char/serial.h |  15 ++----
>  2 files changed, 56 insertions(+), 87 deletions(-)
>
> --
> 1.8.3.rc1.44.gb387c77.dirty
>

Re: [Qemu-devel] [PATCH v1 0/3] Serial cleanup

[Andreas Färber]

Am 10.06.2013 12:23, schrieb Peter Crosthwaite:
> Ping!
> 
> Any objections to this one going in? perhaps even via trivial queue?

No strong objection, but you are using an unusual 12-char indentation in
some places that you may want to check.

Otherwise the cosmetic cleanup looks fine to me.

Cheers,
Andreas

> On Mon, Jun 3, 2013 at 3:11 PM,  <peter.crosthwaite@xilinx.com> wrote:
>> From: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
>>
>> Some cosmetics, refactored to use util/fifo8 for the FIFO8, then
>> factored out some common code.
>>
>> Tested as working on petalogix-ml605 machine model + Linux (has
>> coverage of serial fifo usage).
>>
>>
>> Peter Crosthwaite (3):
>>   char/serial: cosmetic fixes.
>>   char/serial: Use generic Fifo8
>>   char/serial: serial_ioport_write: Factor out common code
>>
>>  hw/char/serial.c         | 128 +++++++++++++++++++----------------------------
>>  include/hw/char/serial.h |  15 ++----
>>  2 files changed, 56 insertions(+), 87 deletions(-)
>>
>> --
>> 1.8.3.rc1.44.gb387c77.dirty
>>
> 


-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH v1 0/3] Serial cleanup

[Peter Crosthwaite]

Hi Andreas,

On Mon, Jun 10, 2013 at 9:49 PM, Andreas Färber <afaerber@suse.de> wrote:
> Am 10.06.2013 12:23, schrieb Peter Crosthwaite:
>> Ping!
>>
>> Any objections to this one going in? perhaps even via trivial queue?
>
> No strong objection, but you are using an unusual 12-char indentation in
> some places that you may want to check.
>

I use that indentation when a ? : operator continues to the next line.
What indentation scheme should be used in this instance?

Regards,
Peter

> Otherwise the cosmetic cleanup looks fine to me.
>
> Cheers,
> Andreas
>
>> On Mon, Jun 3, 2013 at 3:11 PM,  <peter.crosthwaite@xilinx.com> wrote:
>>> From: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
>>>
>>> Some cosmetics, refactored to use util/fifo8 for the FIFO8, then
>>> factored out some common code.
>>>
>>> Tested as working on petalogix-ml605 machine model + Linux (has
>>> coverage of serial fifo usage).
>>>
>>>
>>> Peter Crosthwaite (3):
>>>   char/serial: cosmetic fixes.
>>>   char/serial: Use generic Fifo8
>>>   char/serial: serial_ioport_write: Factor out common code
>>>
>>>  hw/char/serial.c         | 128 +++++++++++++++++++----------------------------
>>>  include/hw/char/serial.h |  15 ++----
>>>  2 files changed, 56 insertions(+), 87 deletions(-)
>>>
>>> --
>>> 1.8.3.rc1.44.gb387c77.dirty
>>>
>>
>
>
> --
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
>

Re: [Qemu-devel] [PATCH v1 1/3] char/serial: cosmetic fixes.

[Andreas Färber]

Hi Peter,

Am 03.06.2013 07:12, schrieb peter.crosthwaite@xilinx.com:
> From: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
> 
> Some cosmetic fixes to char/serial fixing some checkpatch errors.
> 
> Cc: qemu-trivial@nongnu.org
> 
> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
> ---
> Needed for the next patch to pass checkpatch. Done as sep patch to not
> obscure that patch.
> 
>  hw/char/serial.c | 30 +++++++++++++++++++-----------
>  1 file changed, 19 insertions(+), 11 deletions(-)
> 
> diff --git a/hw/char/serial.c b/hw/char/serial.c
> index 66b6348..bd6813e 100644
> --- a/hw/char/serial.c
> +++ b/hw/char/serial.c
> @@ -263,8 +263,9 @@ static gboolean serial_xmit(GIOChannel *chan, GIOCondition cond, void *opaque)
>      if (s->tsr_retry <= 0) {
>          if (s->fcr & UART_FCR_FE) {
>              s->tsr = fifo_get(s,XMIT_FIFO);
> -            if (!s->xmit_fifo.count)
> +            if (!s->xmit_fifo.count) {
>                  s->lsr |= UART_LSR_THRE;
> +            }
>          } else if ((s->lsr & UART_LSR_THRE)) {
>              return FALSE;
>          } else {
> @@ -461,10 +462,11 @@ static uint64_t serial_ioport_read(void *opaque, hwaddr addr, unsigned size)
>          } else {
>              if(s->fcr & UART_FCR_FE) {
>                  ret = fifo_get(s,RECV_FIFO);
> -                if (s->recv_fifo.count == 0)
> +                if (s->recv_fifo.count == 0) {
>                      s->lsr &= ~(UART_LSR_DR | UART_LSR_BI);
> -                else
> +                } else {
>                      qemu_mod_timer(s->fifo_timeout_timer, qemu_get_clock_ns (vm_clock) + s->char_transmit_time * 4);

Wanna rebreak this one too in case you respin/pull?

> +                }
>                  s->timeout_ipending = 0;
>              } else {
>                  ret = s->rbr;
> @@ -534,15 +536,21 @@ static uint64_t serial_ioport_read(void *opaque, hwaddr addr, unsigned size)
>  static int serial_can_receive(SerialState *s)
>  {
>      if(s->fcr & UART_FCR_FE) {
> -        if(s->recv_fifo.count < UART_FIFO_LENGTH)
> -        /* Advertise (fifo.itl - fifo.count) bytes when count < ITL, and 1 if above. If UART_FIFO_LENGTH - fifo.count is
> -        advertised the effect will be to almost always fill the fifo completely before the guest has a chance to respond,
> -        effectively overriding the ITL that the guest has set. */
> -             return (s->recv_fifo.count <= s->recv_fifo.itl) ? s->recv_fifo.itl - s->recv_fifo.count : 1;
> -        else
> -             return 0;
> +        if (s->recv_fifo.count < UART_FIFO_LENGTH) {
> +            /*
> +             * Advertise (fifo.itl - fifo.count) bytes when count < ITL, and 1
> +             * if above. If UART_FIFO_LENGTH - fifo.count is advertised the
> +             * effect will be to almost always fill the fifo completely before
> +             * the guest has a chance to respond, effectively overriding the ITL
> +             * that the guest has set.
> +             */
> +            return (s->recv_fifo.count <= s->recv_fifo.itl) ?
> +                        s->recv_fifo.itl - s->recv_fifo.count : 1;

Here I stumbled over the indentation being 5 chars from '(' or 4 chars
within, but the latter doesn't make sense since it's terminated before.
I would've expected 4 chars from block or aligned below '(' or
4-char-indented from there. But I'm not sure if there are any clear
recommendations, so since it's apparently not using tabs (my initial
suspicion), no objection.

Cheers,
Andreas

> +        } else {
> +            return 0;
> +        }
>      } else {
> -    return !(s->lsr & UART_LSR_DR);
> +        return !(s->lsr & UART_LSR_DR);
>      }
>  }
>  
> 


-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH v1 3/3] char/serial: serial_ioport_write: Factor out common code

[Andreas Färber]

Am 03.06.2013 07:14, schrieb peter.crosthwaite@xilinx.com:
> From: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
> 
> These three lines are common to both FIFO and regular mode. Just factor
> them out to outside the if rather than replicate the same lines inside
> both if and else.
> 
> Cc: qemu-trivial@nongnu.org
> 
> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>

Reviewed-by: Andreas Färber <afaerber@suse.de>

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [Qemu-trivial] [PATCH v1 1/3] char/serial: cosmetic fixes.

[Michael Tokarev]

03.06.2013 09:12, peter.crosthwaite@xilinx.com wrote:
> From: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
> 
> Some cosmetic fixes to char/serial fixing some checkpatch errors.

These are all cosmetic fixes indeed.  I weren't sure we're
applying stylistic changes by its own.  But apparently
people don't object so I don't see why not, either, except
that I don't really want to open a can of worms with other
stylistic patches to come ;)

Applied all 3 to the trivial patches queue.

Thank you!

/mjt

Re: [Qemu-devel] [Qemu-trivial] [PATCH v1 1/3] char/serial: cosmetic fixes.

[Michael Tokarev]

10.06.2013 19:17, Michael Tokarev пишет:
> 03.06.2013 09:12, peter.crosthwaite@xilinx.com wrote:
>> From: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
>>
>> Some cosmetic fixes to char/serial fixing some checkpatch errors.
> 
> These are all cosmetic fixes indeed.  I weren't sure we're
> applying stylistic changes by its own.  But apparently
> people don't object so I don't see why not, either, except
> that I don't really want to open a can of worms with other
> stylistic patches to come ;)
> 
> Applied all 3 to the trivial patches queue.

Actually I didn't apply these.  Since patch 2/3 weren't sent to
-trivial, and 3/3 does not apply without it, I'm not sure I want
to apply either of these.  Maybe all 3 should be sent to the same
tree?  Should I pick the 2/3 too, which wasn't submitted to
-trivial?

Thank you!

/mjt

Re: [Qemu-devel] [Qemu-trivial] [PATCH v1 1/3] char/serial: cosmetic fixes.

[Peter Crosthwaite]

Hi Michael,

On Tue, Jun 11, 2013 at 1:29 AM, Michael Tokarev <mjt@tls.msk.ru> wrote:
> 10.06.2013 19:17, Michael Tokarev пишет:
>> 03.06.2013 09:12, peter.crosthwaite@xilinx.com wrote:
>>> From: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
>>>
>>> Some cosmetic fixes to char/serial fixing some checkpatch errors.
>>
>> These are all cosmetic fixes indeed.  I weren't sure we're
>> applying stylistic changes by its own.  But apparently
>> people don't object so I don't see why not, either, except
>> that I don't really want to open a can of worms with other
>> stylistic patches to come ;)
>>
>> Applied all 3 to the trivial patches queue.
>
> Actually I didn't apply these.  Since patch 2/3 weren't sent to
> -trivial, and 3/3 does not apply without it, I'm not sure I want
> to apply either of these.  Maybe all 3 should be sent to the same
> tree?  Should I pick the 2/3 too, which wasn't submitted to
> -trivial?
>

Im happy for you to take all three as they have had a week of list
time and 2/3 is cleanup only with no functional change. It's
borderline as to whether its trivial.

Regards,
Peter

> Thank you!
>
> /mjt
>
>

Re: [Qemu-devel] [Qemu-trivial] [PATCH v1 0/3] Serial cleanup

[Michael Tokarev]

10.06.2013 14:23, Peter Crosthwaite wrote:
> Ping!
> 
> Any objections to this one going in? perhaps even via trivial queue?

Actually applied all 3 (including 2/3 which weren't submitted to -trivial --
I had some fun verifying it :) to the trivial-patches queue.

Thanks!

/mjt