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