From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:48648) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WPdEM-0005z6-Gm for qemu-devel@nongnu.org; Mon, 17 Mar 2014 15:29:56 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WPdEG-0000WJ-Dr for qemu-devel@nongnu.org; Mon, 17 Mar 2014 15:29:50 -0400 Received: from omzsmtpe04.verizonbusiness.com ([199.249.25.207]:8532) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WPdEG-0000Vt-8V for qemu-devel@nongnu.org; Mon, 17 Mar 2014 15:29:44 -0400 From: Don Slutz Message-ID: <53274D26.4020106@terremark.com> Date: Mon, 17 Mar 2014 15:29:42 -0400 MIME-Version: 1.0 References: <1393469285-18091-1-git-send-email-dslutz@verizon.com> In-Reply-To: Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v2 1/1] char/serial: Fix emptyness handling List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Peter Crosthwaite , Don Slutz Cc: "qemu-devel@nongnu.org Developers" On 03/16/14 22:21, Peter Crosthwaite wrote: > On Thu, Feb 27, 2014 at 12:48 PM, Don Slutz wrote: >> The commit 88c1ee73d3231c74ff90bcfc084a7589670ec244 >> char/serial: Fix emptyness check >> >> Still causes extra NULL byte(s) to be sent. >> >> So if the fifo is empty, do not send an extra NULL byte. >> Do full state change on fifo8_is_empty. >> >> Signed-off-by: Don Slutz >> --- >> v1 to v2: Do all the state changes that would have been done sending the NULL byte. >> >> hw/char/serial.c | 9 +++++++-- >> 1 file changed, 7 insertions(+), 2 deletions(-) >> >> diff --git a/hw/char/serial.c b/hw/char/serial.c >> index 6d3b5af..7af3c1b 100644 >> --- a/hw/char/serial.c >> +++ b/hw/char/serial.c >> @@ -225,8 +225,13 @@ static gboolean serial_xmit(GIOChannel *chan, GIOCondition cond, void *opaque) >> >> if (s->tsr_retry <= 0) { >> if (s->fcr & UART_FCR_FE) { >> - s->tsr = fifo8_is_empty(&s->xmit_fifo) ? >> - 0 : fifo8_pop(&s->xmit_fifo); >> + if (fifo8_is_empty(&s->xmit_fifo)) { >> + s->lsr |= UART_LSR_THRE | UART_LSR_TEMT; >> + s->thr_ipending = 1; >> + serial_update_irq(s); >> + return FALSE; >> + } > This is copy paste of the UART_LSR_THRE handler code below. The > implementation is now somewhat inconsistent with non-fifo mode with > the mixed stage handling of empty fifo/hold-reg. Perhaps you could: > > if (s->fcr & UART_FCR_FE && fifo8_is_empty(&s->xmit_fifo)) { > s->lsr |= UART_LSR_THRE; > } > > after a a successful transmit? Then let the existing check of > UART_LSR_THRE catch for your irq updates etc. If I am understanding correctly, you would like v1 of this patch (02/19/2014) which just does a "return FALSE;" in this case. At the posting of this version, I was still unsure of the steps that can get you into this case. Since then I have come up with: If I have the right steps leading to this case, you need to have at least 2 ( maybe more) xmit overruns just before xmit starts working. At which time there are more calls outstanding via qemu_chr_fe_add_watch() then there are bytes in the FIFO when all the timing lines up. So when the last byte is successfully transmitted, at least 1 more call will be done to serial_xmit with an empty FIFO. So now v1 looks to be the better fix. With no input, I decided that doing what the code would have done on a successful transmit of an extra NULL was better. >> + s->tsr = fifo8_pop(&s->xmit_fifo); >> if (!s->xmit_fifo.num) { >> s->lsr |= UART_LSR_THRE; >> } > I think this is this now dead code. I think (!s->xmit_fifo.num) is the > same condition as fifo8_is_empty(). This is not dead code. And yes (!s->xmit_fifo.num) is the same condition. However, the pop 1 line before can change the state to empty. I am happy to also use fifo8_is_empty() here if a v3 is needed. Just going for the minimal change. I can also make it a separate patch if wanted. > > Sorry about the delayed response. Thanks for the patch. No problem. -Don Slutz > Regards, > Peter > >> -- >> 1.8.4 >> >>