On Thu, Feb 13, 2014 at 5:22 AM, Peter Maydell <peter.mayd...@linaro.org> wrote:
> On 12 February 2014 03:36, Peter Crosthwaite
> <peter.crosthwa...@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.crosthwa...@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?
>
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
>
