From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:46027) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Y0Xkm-00074Y-7X for qemu-devel@nongnu.org; Mon, 15 Dec 2014 10:40:12 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Y0Xkh-0004aX-SG for qemu-devel@nongnu.org; Mon, 15 Dec 2014 10:40:08 -0500 Received: from mx1.redhat.com ([209.132.183.28]:42070) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Y0Xkh-0004YA-KY for qemu-devel@nongnu.org; Mon, 15 Dec 2014 10:40:03 -0500 Date: Mon, 15 Dec 2014 15:21:26 +0000 From: "Dr. David Alan Gilbert" Message-ID: <20141215152125.GG5502@work-vm> References: <1418388243-1886-1-git-send-email-pbonzini@redhat.com> <1418388243-1886-3-git-send-email-pbonzini@redhat.com> <20141215114018.GC5502@work-vm> <548ECE05.3050009@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <548ECE05.3050009@redhat.com> Subject: Re: [Qemu-devel] [PATCH v3 2/4] serial: clean up THRE/TEMT handling List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Paolo Bonzini Cc: imammedo@redhat.com, andrey@xdel.ru, qemu-devel@nongnu.org, dslutz@verizon.com, batuzovk@ispras.ru * Paolo Bonzini (pbonzini@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 dslutz@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 / dgilbert@redhat.com / Manchester, UK