* Paolo Bonzini (pbonz...@redhat.com) 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 dsl...@verizon.com'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 / dgilb...@redhat.com / Manchester, UK
