From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:52202) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SEkYO-0005sF-1V for qemu-devel@nongnu.org; Mon, 02 Apr 2012 12:56:29 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1SEkYJ-0006W5-61 for qemu-devel@nongnu.org; Mon, 02 Apr 2012 12:56:27 -0400 Received: from e37.co.us.ibm.com ([32.97.110.158]:35364) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SEkYI-0006VY-VM for qemu-devel@nongnu.org; Mon, 02 Apr 2012 12:56:23 -0400 Received: from /spool/local by e37.co.us.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Mon, 2 Apr 2012 10:56:18 -0600 Received: from d03relay05.boulder.ibm.com (d03relay05.boulder.ibm.com [9.17.195.107]) by d03dlp03.boulder.ibm.com (Postfix) with ESMTP id 7C1FE19D8052 for ; Mon, 2 Apr 2012 10:56:07 -0600 (MDT) Received: from d03av05.boulder.ibm.com (d03av05.boulder.ibm.com [9.17.195.85]) by d03relay05.boulder.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id q32Gu7dH131450 for ; Mon, 2 Apr 2012 10:56:11 -0600 Received: from d03av05.boulder.ibm.com (loopback [127.0.0.1]) by d03av05.boulder.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id q32Gu5Zo023837 for ; Mon, 2 Apr 2012 10:56:07 -0600 Message-ID: <4F79DA23.2010100@us.ibm.com> Date: Mon, 02 Apr 2012 11:56:03 -0500 From: Anthony Liguori MIME-Version: 1.0 References: <1333377579-7513-1-git-send-email-aliguori@us.ibm.com> <1333377579-7513-2-git-send-email-aliguori@us.ibm.com> In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 1/2] serial: fix retry logic List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Stefano Stabellini Cc: "qemu-devel@nongnu.org" On 04/02/2012 11:18 AM, Stefano Stabellini wrote: > On Mon, 2 Apr 2012, Anthony Liguori wrote: >> I'm not sure if the retry logic has ever worked when not using FIFO mode. I >> found this while writing a test case although code inspection confirms it is >> definitely broken. >> >> The TSR retry logic will never actually happen because it is guarded by an >> 'if (s->tsr_rety> 0)' but this is the only place that can ever make the >> variable greater than zero. That effectively makes the retry logic an 'if (0)'. >> >> I believe this is a typo and the intention was>= 0. > > I agree if you, I don't think there can be another explanation. Thanks for the confirmation. It's old code so I'm a bit surprised it hasn't been noticed yet :-) >> Once this is fixed though, >> I see double transmits with my test case. This is because in the non FIFO >> case, serial_xmit may get invoked while LSR.THRE is still high because the >> character was processed but the retransmit timer was still active. > > If that is the case then this problem must be independent from the tsr_retry > bug, considering that the code path you are changing is only taken when > tsr_retry<= 0, right? The double transmit is triggered by the xmit retry timer. That timer will never get armed if tsr_retry < 0. BTW, my test case requires a character device backend to return a short read. That's the only way to trigger this (as would be the case with serial device passthrough). > >> We can handle this by simply checking for LSR.THRE and returning early. >> It's >> possible that the FIFO paths also need some attention. > > The manual states: "In the FIFO mode this bit is set when the XMIT FIFO > is empty; it is cleared when at least 1 byte is written to the XMIT > FIFO", therefore I would return early if UART_LSR_THRE is set no matter > if we are in FIFO mode or not. I'll try to add FIFO mode to my test case and trigger the problem. There's a bit more going on in the FIFO paths so it's not clear to me yet if it's needed here. Regards, Anthony Liguori >