Re: [Qemu-devel] [PATCH v3 2/4] serial: clean up THRE/TEMT handling

[Paolo Bonzini]

On 15/12/2014 12:40, Dr. David Alan Gilbert wrote:
>> >      do {
>> > +        assert(!(s->lsr & UART_LSR_TEMT));
>> > +        assert(!(s->lsr & UART_LSR_THRE));
>> > +
>> >          if (s->tsr_retry <= 0) {
>> >              if (s->fcr & UART_FCR_FE) {
>> > -                if (fifo8_is_empty(&s->xmit_fifo)) {
>> > -                    return FALSE;
>> > -                }
>> > +                assert(!fifo8_is_empty(&s->xmit_fifo));
> That's undoing address@hidden's 
> 
> dffacd46 - Fix emptyness checking
> 
> See, http://permalink.gmane.org/gmane.comp.emulators.qemu/262412
> I don't think your assumptions are safe because of that qemu_chr_fe_add_watch.

I think it's safe because:

- serial_xmit is called from outside only after resetting TEMT and THRE
and pushing a character on the FIFO

- serial_xmit iterates a second time over do...while() only if the FIFO
is not empty (both before and after this patch; this patch only changes
the condition that is used)

- if qemu_chr_fe_add_watch is called, the next call will have tsr_retry
>= 1 and thus the "if" would be skipped.

Note that in the middle we had commit f702e62 (serial: change retry
logic to avoid concurrency, 2014-07-11) that fixed some messy behavior
of qemu_chr_fe_add_watch.  The commit message talks about multiple calls
to qemu_chr_fe_add_watch triggering s->tsr_retry >= MAX_XMIT_RETRY but
this is not the only possible failure.  If you have multiple calls, the
subsequent ones will see s->tsr_retry == 0 and will find (s->lsr &
UART_LSR_THRE) != 0 on entry.  But this should really never happen.

(Thanks for making me think more about it. :))

Paolo