linux-kernel - Re: [PATCH v2 1/2] tty: serial: 8250

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <yw1xselglmdj.fsf@mansr.com>
Date: Wed, 07 May 2025 11:56:56 +0100
From: Måns Rullgård <mans@...sr.com>
To: Jiri Slaby <jirislaby@...nel.org>
Cc: gregkh@...uxfoundation.org, linux-kernel@...r.kernel.org,
 linux-serial@...r.kernel.org, linux-omap@...r.kernel.org,
 stable@...r.kernel.org
Subject: Re: [PATCH v2 1/2] tty: serial: 8250_omap: fix tx with dma

Jiri Slaby <jirislaby@...nel.org> writes:

> On 07. 05. 25, 9:49, Måns Rullgård wrote:
>> Jiri Slaby <jirislaby@...nel.org> writes:
>> 
>>> On 06. 05. 25, 17:07, Mans Rullgard wrote:
>>>> Commit 1788cf6a91d9 ("tty: serial: switch from circ_buf to kfifo")
>>>> introduced an error in the TX DMA handling for 8250_omap.
>>>> When the OMAP_DMA_TX_KICK flag is set, one byte is pulled from the
>>>> kfifo and emitted directly in order to start the DMA.  This is done
>>>> without updating DMA tx_size which leads to uart_xmit_advance() called
>>>> in the DMA complete callback advancing the kfifo by one too much.
>>>> In practice, transmitting N bytes has been seen to result in the last
>>>> N-1 bytes being sent repeatedly.
>>>> This change fixes the problem by moving all of the dma setup after
>>>> the OMAP_DMA_TX_KICK handling and using kfifo_len() instead of the
>>>> dma size for the 4-byte cutoff check. This slightly changes the
>>>> behaviour at buffer wraparound, but it still transmits the correct
>>>> bytes somehow. At the point kfifo_dma_out_prepare_mapped is called,
>>>> at least one byte is guaranteed to be in the fifo, so checking the
>>>> return value is not necessary.
>>>> Fixes: 1788cf6a91d9 ("tty: serial: switch from circ_buf to kfifo")
>>>> Cc: stable@...r.kernel.org
>>>> Signed-off-by: Mans Rullgard <mans@...sr.com>
>>>> ---
>>>> v2: split patch in two
>>>> ---
>>>>    drivers/tty/serial/8250/8250_omap.c | 24 +++++++++---------------
>>>>    1 file changed, 9 insertions(+), 15 deletions(-)
>>>> diff --git a/drivers/tty/serial/8250/8250_omap.c
>>>> b/drivers/tty/serial/8250/8250_omap.c
>>>> index f1aee915bc02..180466e09605 100644
>>>> --- a/drivers/tty/serial/8250/8250_omap.c
>>>> +++ b/drivers/tty/serial/8250/8250_omap.c
>>>> @@ -1173,16 +1173,6 @@ static int omap_8250_tx_dma(struct uart_8250_port *p)
>>>>    		return 0;
>>>>    	}
>>>>    -	sg_init_table(&sg, 1);
>>>> -	ret = kfifo_dma_out_prepare_mapped(&tport->xmit_fifo, &sg, 1,
>>>> -					   UART_XMIT_SIZE, dma->tx_addr);
>>>> -	if (ret != 1) {
>>>> -		serial8250_clear_THRI(p);
>>>> -		return 0;
>>>> -	}
>>>> -
>>>> -	dma->tx_size = sg_dma_len(&sg);
>>>> -
>>>>    	if (priv->habit & OMAP_DMA_TX_KICK) {
>>>>    		unsigned char c;
>>>>    		u8 tx_lvl;
>>> ...
>>>> @@ -1216,11 +1206,12 @@ static int omap_8250_tx_dma(struct uart_8250_port *p)
>>>>    			goto err;
>>>>    		}
>>>>    		skip_byte = c;
>>>> -		/* now we need to recompute due to kfifo_get */
>>>> -		kfifo_dma_out_prepare_mapped(&tport->xmit_fifo, &sg, 1,
>>>> -				UART_XMIT_SIZE, dma->tx_addr);
>>>>    	}
>>>>    +	sg_init_table(&sg, 1);
>>>> +	kfifo_dma_out_prepare_mapped(&tport->xmit_fifo, &sg, 1,
>>>> +				     UART_XMIT_SIZE, dma->tx_addr);
>>>
>>> This can fail (note the first call to this was checked). The latter
>>> (deliberately) not.
>> No, it can't.  The fifo has already been checked to contain something
>> right at the top of the function.  There is no other failure mode for
>> kfifo_dma_out_prepare_mapped.
>
> That it cannot fail now does not mean it cannot in the future. Simply do it
> properly and check the retval.
>
>>>> +
>>>>    	desc = dmaengine_prep_slave_sg(dma->txchan, &sg, 1, DMA_MEM_TO_DEV,
>>>>    			DMA_PREP_INTERRUPT | DMA_CTRL_ACK);
>>>>    	if (!desc) {
>>> ...
>>>> @@ -1248,8 +1240,10 @@ static int omap_8250_tx_dma(struct uart_8250_port *p)
>>>>    err:
>>>>    	dma->tx_err = 1;
>>>>    out_skip:
>>>> -	if (skip_byte >= 0)
>>>> +	if (skip_byte >= 0) {
>>>>    		serial_out(p, UART_TX, skip_byte);
>>>> +		p->port.icount.tx++;
>>>
>>> This is still unrelated.
>> No, it's not.  Your broken code called uart_xmit_advance with the full
>> amount due to the incorrect tx_size value.  This compensates for that.
>
> Then document it properly in the commit log.
>
>> You made this mess.
>
> I can only say that I am sorry for the breakage of this driver. This TX way
> with one byte via FIFO and the rest via DMA, and only if > 4 chars to be
> sent is indeed cumbersome and apparently uneasy to do right.
>
>> Now fix it.  I don't care how.  It's wasted enough
>> of my time already.
>
> How exactly does this help to get the code in shape? You apparently have the
> HW, you spent some time debugging that, you have patches, so you deserve the
> credits.

I don't care about credit, I just want it fixed, and I feel like that's
your responsibility since you broke it.  I've pointed out where the
problem is and provided a fix.  If that's not enough for you, then I
give up.  I'm not paid enough to play your guessing games.

-- 
Måns Rullgård