| Message ID | 20250506150748.3162-1-mans@mansr.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v2,1/2] tty: serial: 8250_omap: fix tx with dma | expand |
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@vger.kernel.org > Signed-off-by: Mans Rullgard <mans@mansr.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. > + > 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. thanks,
Jiri Slaby <jirislaby@kernel.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@vger.kernel.org >> Signed-off-by: Mans Rullgard <mans@mansr.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. >> + >> 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. You made this mess. Now fix it. I don't care how. It's wasted enough of my time already.
On 07. 05. 25, 9:49, Måns Rullgård wrote: > Jiri Slaby <jirislaby@kernel.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@vger.kernel.org >>> Signed-off-by: Mans Rullgard <mans@mansr.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. thanks,
Jiri Slaby <jirislaby@kernel.org> writes: > On 07. 05. 25, 9:49, Måns Rullgård wrote: >> Jiri Slaby <jirislaby@kernel.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@vger.kernel.org >>>> Signed-off-by: Mans Rullgard <mans@mansr.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.
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; @@ -1207,7 +1197,7 @@ static int omap_8250_tx_dma(struct uart_8250_port *p) ret = -EBUSY; goto err; } - if (dma->tx_size < 4) { + if (kfifo_len(&tport->xmit_fifo) < 4) { ret = -EINVAL; goto err; } @@ -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); + desc = dmaengine_prep_slave_sg(dma->txchan, &sg, 1, DMA_MEM_TO_DEV, DMA_PREP_INTERRUPT | DMA_CTRL_ACK); if (!desc) { @@ -1228,6 +1219,7 @@ static int omap_8250_tx_dma(struct uart_8250_port *p) goto err; } + dma->tx_size = sg_dma_len(&sg); dma->tx_running = 1; desc->callback = omap_8250_dma_tx_complete; @@ -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++; + } return ret; }
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@vger.kernel.org Signed-off-by: Mans Rullgard <mans@mansr.com> --- v2: split patch in two --- drivers/tty/serial/8250/8250_omap.c | 24 +++++++++--------------- 1 file changed, 9 insertions(+), 15 deletions(-)