[Qemu-devel] [PATCH target-arm v1 0/2] More Cadence UART fixes

[Qemu-devel] [PATCH target-arm v1 0/2] More Cadence UART fixes

[Peter Crosthwaite]

Hi Peter,

Two fixes to Cadence UART. First is a bug in the recently refactored
TX code path around error handling.

Second is the long known crashing-cadence-UART bug when there in no
backing serial device (i.e. boot a kernel that talks to serial without
adding -serial args).

Chris recently reported the second issue to me offline.

Chris,

Please test to see if this a resolution to your issue.

Regards,
Peter


Peter Crosthwaite (2):
  char/cadence_uart: Handle qemu_chr_fe_write errors
  char/cadence_uart: Add NULL guards against chr

 hw/char/cadence_uart.c | 21 +++++++++++++++------
 1 file changed, 15 insertions(+), 6 deletions(-)

-- 
1.8.5.4

[Qemu-devel] [PATCH target-arm v1 1/2] char/cadence_uart: Handle qemu_chr_fe_write errors

[Peter Crosthwaite]

By just ignoring them and trying again later. This handles the
EGAIN case properly (the previous implementation was only dealing
with short returns and not errors).

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

 hw/char/cadence_uart.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/hw/char/cadence_uart.c b/hw/char/cadence_uart.c
index 1012f1a..1985047 100644
--- a/hw/char/cadence_uart.c
+++ b/hw/char/cadence_uart.c
@@ -302,8 +302,11 @@ static gboolean cadence_uart_xmit(GIOChannel *chan, GIOCondition cond,
     }
 
     ret = qemu_chr_fe_write(s->chr, s->tx_fifo, s->tx_count);
-    s->tx_count -= ret;
-    memmove(s->tx_fifo, s->tx_fifo + ret, s->tx_count);
+
+    if (ret >= 0) {
+        s->tx_count -= ret;
+        memmove(s->tx_fifo, s->tx_fifo + ret, s->tx_count);
+    }
 
     if (s->tx_count) {
         int r = qemu_chr_fe_add_watch(s->chr, G_IO_OUT, cadence_uart_xmit, s);
-- 
1.8.5.4

Re: [Qemu-devel] [PATCH target-arm v1 1/2] char/cadence_uart: Handle qemu_chr_fe_write errors

[Peter Maydell]

On 12 February 2014 03:36, Peter Crosthwaite
<peter.crosthwaite@xilinx.com> wrote:
> By just ignoring them and trying again later. This handles the
> EGAIN case properly (the previous implementation was only dealing
> with short returns and not errors).
>
> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
> ---
>
>  hw/char/cadence_uart.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/hw/char/cadence_uart.c b/hw/char/cadence_uart.c
> index 1012f1a..1985047 100644
> --- a/hw/char/cadence_uart.c
> +++ b/hw/char/cadence_uart.c
> @@ -302,8 +302,11 @@ static gboolean cadence_uart_xmit(GIOChannel *chan, GIOCondition cond,
>      }
>
>      ret = qemu_chr_fe_write(s->chr, s->tx_fifo, s->tx_count);
> -    s->tx_count -= ret;
> -    memmove(s->tx_fifo, s->tx_fifo + ret, s->tx_count);
> +
> +    if (ret >= 0) {
> +        s->tx_count -= ret;
> +        memmove(s->tx_fifo, s->tx_fifo + ret, s->tx_count);
> +    }
>
>      if (s->tx_count) {
>          int r = qemu_chr_fe_add_watch(s->chr, G_IO_OUT, cadence_uart_xmit, s);

If we got EAGAIN do we really need to re-add the watch and recompute
the UART status, or could we just return early?

Incidentally, this looks like the third or fourth patch fixing use of
qemu_chr_fe_write() in a UART model; perhaps we should audit
the others...

thanks
-- PMM

[Qemu-devel] [PATCH target-arm v1 2/2] char/cadence_uart: Add NULL guards against chr

[Peter Crosthwaite]

It's possible and valid for users of this device model to instantiate
it without a backing chr device. To avoid crashes, guard all uses of
the backing chr device against NULL.

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

 hw/char/cadence_uart.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/hw/char/cadence_uart.c b/hw/char/cadence_uart.c
index 1985047..10abb4d 100644
--- a/hw/char/cadence_uart.c
+++ b/hw/char/cadence_uart.c
@@ -175,8 +175,10 @@ static void uart_send_breaks(UartState *s)
 {
     int break_enabled = 1;
 
-    qemu_chr_fe_ioctl(s->chr, CHR_IOCTL_SERIAL_SET_BREAK,
-                               &break_enabled);
+    if (s->chr) {
+        qemu_chr_fe_ioctl(s->chr, CHR_IOCTL_SERIAL_SET_BREAK,
+                          &break_enabled);
+    }
 }
 
 static void uart_parameters_setup(UartState *s)
@@ -227,7 +229,9 @@ static void uart_parameters_setup(UartState *s)
 
     packet_size += ssp.data_bits + ssp.stop_bits;
     s->char_tx_time = (get_ticks_per_sec() / ssp.speed) * packet_size;
-    qemu_chr_fe_ioctl(s->chr, CHR_IOCTL_SERIAL_SET_PARAMS, &ssp);
+    if (s->chr) {
+        qemu_chr_fe_ioctl(s->chr, CHR_IOCTL_SERIAL_SET_PARAMS, &ssp);
+    }
 }
 
 static int uart_can_receive(void *opaque)
@@ -377,7 +381,9 @@ static void uart_read_rx_fifo(UartState *s, uint32_t *c)
         *c = s->rx_fifo[rx_rpos];
         s->rx_count--;
 
-        qemu_chr_accept_input(s->chr);
+        if (s->chr) {
+            qemu_chr_accept_input(s->chr);
+        }
     } else {
         *c = 0;
     }
-- 
1.8.5.4

Re: [Qemu-devel] [PATCH target-arm v1 2/2] char/cadence_uart: Add NULL guards against chr

[Peter Maydell]

On 12 February 2014 03:37, Peter Crosthwaite
<peter.crosthwaite@xilinx.com> wrote:
> It's possible and valid for users of this device model to instantiate
> it without a backing chr device. To avoid crashes, guard all uses of
> the backing chr device against NULL.
>
> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

I guess the other way to approach this would be to require the
backing device and just set it up to be a "throw away output,
never provide input" dummy if there isn't a real one. No idea
if that's feasible though so NULL-guards seems reasonable.

thanks
-- PMM