Re: [PATCH 00/15] tty: serial: switch from circ_buf to kfifo

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

[-- Attachment #1: Type: text/plain, Size: 7053 bytes --]

On Sun, 16 Jun 2024, Ferry Toth wrote:

> Hi
> 
> adding Phil
> 
> Op 12-06-2024 om 15:13 schreef Ilpo Järvinen:
> > On Mon, 10 Jun 2024, Ferry Toth wrote:
> > > Op 07-06-2024 om 22:32 schreef Ferry Toth:
> > > > Op 22-04-2024 om 07:51 schreef Jiri Slaby:
> > > > > On 19. 04. 24, 17:12, Neil Armstrong wrote:
> > > > > > On 05/04/2024 08:08, Jiri Slaby (SUSE) wrote:
> > > > > > > This series switches tty serial layer to use kfifo instead of
> > > > > > > circ_buf.
> > > > > > > 
> > > > > > > The reasoning can be found in the switching patch in this series:
> > > > > > > """
> > > > > > > Switch from struct circ_buf to proper kfifo. kfifo provides much
> > > > > > > better
> > > > > > > API, esp. when wrap-around of the buffer needs to be taken into
> > > > > > > account.
> > > > > > > Look at pl011_dma_tx_refill() or cpm_uart_tx_pump() changes for
> > > > > > > example.
> > > > > > > 
> > > > > > > Kfifo API can also fill in scatter-gather DMA structures, so it
> > > > > > > easier
> > > > > > > for that use case too. Look at lpuart_dma_tx() for example. Note
> > > > > > > that
> > > > > > > not all drivers can be converted to that (like atmel_serial), they
> > > > > > > handle DMA specially.
> > > > > > > 
> > > > > > > Note that usb-serial uses kfifo for TX for ages.
> > > > > > > """
> > > > > Sadly, everyone had a chance to test the series:
> > > > >     https://lore.kernel.org/all/20240319095315.27624-1-jirislaby@kernel.org/
> > > > > for more than two weeks before I sent this version for inclusion. And
> > > > > then
> > > > > it took another 5 days till this series appeared in -next. But noone
> > > > > with
> > > > > this HW apparently cared enough back then. I'd wish they (you) didn't.
> > > > > Maybe next time, people will listen more carefully:
> > > > > ===
> > > > > This is Request for Testing as I cannot test all the changes
> > > > > (obviously). So please test your HW's serial properly.
> > > > > ===
> > > > > 
> > > > > > and should've been dropped immediately when the first regressions
> > > > > > were
> > > > > > reported.
> > > > > Provided the RFT was mostly ignored (anyone who tested that here, or I
> > > > > only wasted my time?), how exactly would dropping help me finding
> > > > > potential issues in the series? In the end, noone is running -next in
> > > > > production, so glitches are sort of expected, right? And I believe I
> > > > > smashed them quickly enough (despite I was sidetracked to handle the
> > > > > n_gsm
> > > > > issue). But I might be wrong, as usual.
> > > > I arrived at this party a bit late, sorry about that. No good excuses.
> > > > 
> > > > > So no, dropping is not helping moving forward, actions taken by e.g.
> > > > > Marek
> > > > > Szyprowski <m.szyprowski@samsung.com> do, IMNSHO.
> > > > Good news is I tested on Merrifield (Intel Edison) which is slow
> > > > (500MHz)
> > > > and has a HSU that can transmit up to 3.5Mb/s. It really normally needs
> > > > DMA
> > > > and just a single interrupt at the end of transmit and receive for which
> > > > I
> > > > my own patches locally. The bounce buffer I was using on transmit broke
> > > > due
> > > > to this patch, so I dropped that. Still, with the extra interrupts
> > > > caused by
> > > > the circ buffer wrapping around it seems to work well. Too late to add
> > > > my
> > > > Tested-by.
> > > > 
> > > > One question though: in 8250_dma.c serial8250_tx_dma() you mention "/*
> > > > kfifo
> > > > can do more than one sg, we don't (quite yet) */".
> > > > 
> > > > I see the opportunity to use 2 sg entries to get all the data out in one
> > > > dma
> > > > transfer, but there doesn't seem to be much documentation or examples on
> > > > how
> > > > to do that. It seems just increasing nents to 2 would do the trick?
> > > Currently I have this working on mrfld:
> 
> diff --git a/drivers/tty/serial/8250/8250_dma.c
> b/drivers/tty/serial/8250/8250_dma.c
> 
> index 8a353e3cc3dd..d215c494ee24 100644
> 
> --- a/drivers/tty/serial/8250/8250_dma.c
> 
> +++ b/drivers/tty/serial/8250/8250_dma.c
> 
> @@ -89,7 +89,9 @@ int serial8250_tx_dma(struct uart_8250_port *p)
> 
> struct tty_port *tport = &p->port.state->port;
> 
> struct dma_async_tx_descriptor *desc;
> 
> struct uart_port *up = &p->port;
> 
> - struct scatterlist sg;
> 
> + struct scatterlist *sg;
> 
> + struct scatterlist sgl[2];
> 
> + int i;
> 
> int ret;
> 
> if (dma->tx_running) {
> 
> @@ -110,18 +112,17 @@ int serial8250_tx_dma(struct uart_8250_port *p)
> 
> serial8250_do_prepare_tx_dma(p);
> 
> - sg_init_table(&sg, 1);
> 
> - /* kfifo can do more than one sg, we don't (quite yet) */
> 
> - ret = kfifo_dma_out_prepare_mapped(&tport->xmit_fifo, &sg, 1,
> 
> + sg_init_table(sgl, ARRAY_SIZE(sgl));
> 
> +
> 
> + ret = kfifo_dma_out_prepare_mapped(&tport->xmit_fifo, sgl, ARRAY_SIZE(sgl),
> 
> UART_XMIT_SIZE, dma->tx_addr);
> 
> - /* we already checked empty fifo above, so there should be something */
> 
> - if (WARN_ON_ONCE(ret != 1))
> 
> - return 0;
> 
> + dma->tx_size = 0;
> 
> - dma->tx_size = sg_dma_len(&sg);
> 
> + for_each_sg(sgl, sg, ret, i)
> 
> + dma->tx_size += sg_dma_len(sg);
> 
> - desc = dmaengine_prep_slave_sg(dma->txchan, &sg, 1,
> 
> + desc = dmaengine_prep_slave_sg(dma->txchan, sgl, ret,
> 
> DMA_MEM_TO_DEV,
> 
> DMA_PREP_INTERRUPT | DMA_CTRL_ACK);
> 
> if (!desc) {
> 
> > > Nevertheless I got this to work. Very nice. Thanks for this series.
> > > I am seeing only 2 interrupts (x2 as each interrupt happens twice), one
> > > for
> > > dma complete. The 2nd, not sure but likely, uart tx done.
> > > In any case the whole buffer is transferred without interchar gaps.
> > > 
> > > > So, what was the reason to "don't (quite yet)"?
> > > Before considering to send out a patch for this, are there any caveats
> > > that
> > > I'm overlooking?
> > Not exactly related to that quoted comment, but you should Cc the person
> > who added RNZ1 DMA a year or two back (in 8250_dw.c) because it required
> 
> RZN1
> 
> I think you are referring to aa63d786cea2 ("serial: 8250: dw: Add support for
> DMA flow controlling devices") by
> 
> Phil Edworthy<phil.edworthy@renesas.com>?

The change was submitted by Miquel, I've added him into receipients as 
well.

> > writing Tx length into some custom register. I don't know the meaning of
> > that HW specific register so it would be good to get confirmation the HW
> I see dw8250_prepare_tx_dma() has RZN1_UART_xDMACR_BLK_SZ(dma->tx_size)
> > is okay if it gets more than 1 sg entry (at worst, a HW-specific limit
> > on nents might need to be imposed).
> > 
> And is there a way to get the maximum nents supported? I thought
> kfifo_dma_out_prepare_mapped() would return a safe number.

This is about writing a value into RZN1_UART_*DMACR which seems to be 
outside of dma subsystem's influence so I'd expect dma side does not know 
about it.

-- 
 i.