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

[Dr. David Alan Gilbert]

* Paolo Bonzini (address@hidden) wrote:
> 
> 
> 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

Are you sure about TEMT? My reading of serial_ioport_write is that if
!FCR_FE then TEMT isn't cleared.

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

Ah yes, that changed things around a lot.

Dave

> 
> Paolo
--
Dr. David Alan Gilbert / address@hidden / Manchester, UK