Re: [Qemu-devel] [PATCH 3/6] serial: separate serial_xmit and serial_watch_cb

[Dr. David Alan Gilbert]

* Paolo Bonzini (address@hidden) wrote:
> serial_xmit starts transmission of whatever is in the FIFO or THR;
> serial_watch_cb is a wrapper around it.
> 
> Signed-off-by: Paolo Bonzini <address@hidden>
> ---
>  hw/char/serial.c | 21 +++++++++++++--------
>  1 file changed, 13 insertions(+), 8 deletions(-)
> 
> diff --git a/hw/char/serial.c b/hw/char/serial.c
> index 6f0a49e..4196a2e 100644
> --- a/hw/char/serial.c
> +++ b/hw/char/serial.c
> @@ -106,6 +106,7 @@ do {} while (0)
>  #endif
>  
>  static void serial_receive1(void *opaque, const uint8_t *buf, int size);
> +static void serial_xmit(SerialState *s);
>  
>  static inline void recv_fifo_put(SerialState *s, uint8_t chr)
>  {
> @@ -223,10 +224,16 @@ static void serial_update_msl(SerialState *s)
>      }
>  }
>  
> -static gboolean serial_xmit(GIOChannel *chan, GIOCondition cond, void 
> *opaque)
> +static gboolean serial_watch_cb(GIOChannel *chan, GIOCondition cond,
> +                                void *opaque)
>  {
>      SerialState *s = opaque;
> +    serial_xmit(s);
> +    return FALSE;
> +}

Yes, with a side question.
We register this with G_IO_OUT|G_IO_HUP  but don't check the cond, what's this
code supposed to do if it gets a HUP?

Since we didn't do anything with cond before either:

Reviewed-by: Dr. David Alan Gilbert <address@hidden>


>  
> +static void serial_xmit(SerialState *s)
> +{
>      do {
>          assert(!(s->lsr & UART_LSR_TEMT));
>          if (s->tsr_retry == 0) {
> @@ -254,9 +261,9 @@ static gboolean serial_xmit(GIOChannel *chan, 
> GIOCondition cond, void *opaque)
>          } else if (qemu_chr_fe_write(s->chr, &s->tsr, 1) != 1) {
>              if (s->tsr_retry < MAX_XMIT_RETRY &&
>                  qemu_chr_fe_add_watch(s->chr, G_IO_OUT|G_IO_HUP,
> -                                      serial_xmit, s) > 0) {
> +                                      serial_watch_cb, s) > 0) {
>                  s->tsr_retry++;
> -                return FALSE;
> +                return;
>              }
>              s->tsr_retry = 0;
>          } else {
> @@ -269,11 +276,8 @@ static gboolean serial_xmit(GIOChannel *chan, 
> GIOCondition cond, void *opaque)
>  
>      s->last_xmit_ts = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
>      s->lsr |= UART_LSR_TEMT;
> -
> -    return FALSE;
>  }
>  
> -
>  /* Setter for FCR.
>     is_load flag means, that value is set while loading VM state
>     and interrupt should not be invoked */
> @@ -331,7 +335,7 @@ static void serial_ioport_write(void *opaque, hwaddr 
> addr, uint64_t val,
>              s->lsr &= ~UART_LSR_TEMT;
>              serial_update_irq(s);
>              if (s->tsr_retry == 0) {
> -                serial_xmit(NULL, G_IO_OUT, s);
> +                serial_xmit(s);
>              }
>          }
>          break;
> @@ -647,7 +651,8 @@ static int serial_post_load(void *opaque, int version_id)
>          }
>  
>          assert(s->watch_tag == 0);
> -        s->watch_tag = qemu_chr_fe_add_watch(s->chr, G_IO_OUT|G_IO_HUP, 
> serial_xmit, s);
> +        s->watch_tag = qemu_chr_fe_add_watch(s->chr, G_IO_OUT|G_IO_HUP,
> +                                             serial_watch_cb, s);
>      } else {
>          /* tsr_retry == 0 implies LSR.TEMT = 1 (transmitter empty).  */
>          if (!(s->lsr & UART_LSR_TEMT)) {
> -- 
> 2.5.5
> 
> 
--
Dr. David Alan Gilbert / address@hidden / Manchester, UK