From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:51779) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Y0UN4-0001hh-84 for qemu-devel@nongnu.org; Mon, 15 Dec 2014 07:03:30 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Y0UMz-00079T-QR for qemu-devel@nongnu.org; Mon, 15 Dec 2014 07:03:26 -0500 Received: from mail-wg0-x22b.google.com ([2a00:1450:400c:c00::22b]:55224) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Y0UMz-00079F-JG for qemu-devel@nongnu.org; Mon, 15 Dec 2014 07:03:21 -0500 Received: by mail-wg0-f43.google.com with SMTP id l18so14491122wgh.2 for ; Mon, 15 Dec 2014 04:03:21 -0800 (PST) Sender: Paolo Bonzini Message-ID: <548ECE05.3050009@redhat.com> Date: Mon, 15 Dec 2014 13:03:17 +0100 From: Paolo Bonzini MIME-Version: 1.0 References: <1418388243-1886-1-git-send-email-pbonzini@redhat.com> <1418388243-1886-3-git-send-email-pbonzini@redhat.com> <20141215114018.GC5502@work-vm> In-Reply-To: <20141215114018.GC5502@work-vm> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 8bit 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: "Dr. David Alan Gilbert" Cc: imammedo@redhat.com, andrey@xdel.ru, qemu-devel@nongnu.org, dslutz@verizon.com, batuzovk@ispras.ru 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 - 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