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

[Peter Crosthwaite]

On Thu, Feb 13, 2014 at 5:22 AM, Peter Maydell <address@hidden> wrote:
> On 12 February 2014 03:36, Peter Crosthwaite
> <address@hidden> 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 <address@hidden>
>> ---
>>
>>  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?
>

Not sure. But this now matches char/serial.c which re-adds the watch
regardless of whether its an error or short return:

247     } else if (qemu_chr_fe_write(s->chr, &s->tsr, 1) != 1) {
248         if (s->tsr_retry >= 0 && s->tsr_retry < MAX_XMIT_RETRY &&
249             qemu_chr_fe_add_watch(s->chr, G_IO_OUT, serial_xmit, s) > 0) {
250             s->tsr_retry++;
251             return FALSE;
252         }
253         s->tsr_retry = 0;
254     } else {

With Don's fixing patch to that device model, the policy is that extra
unwanted (and potentially delayed) watch callbacks simply discard
themselves. This is already guarded in this device model by:

300     if (!s->tx_count) {
301         return FALSE;
302     }

> 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...
>

Yeh it's a bit of a mess. I'm watching new code for issues but there a
lot of legacy doing nasty things like busy waits or simply dropping
chars when the host gets too busy.

Regards,
Peter

> thanks
> -- PMM
>