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

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

[Jiri Slaby (SUSE)]

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.
"""

Cc: Al Cooper <alcooperx@gmail.com>
Cc: Alexander Shiyan <shc_work@mail.ru>
Cc: Alexandre Belloni <alexandre.belloni@bootlin.com>
Cc: Alexandre Torgue <alexandre.torgue@foss.st.com>
Cc: Alim Akhtar <alim.akhtar@samsung.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: "Aneesh Kumar K.V" <aneesh.kumar@kernel.org>
Cc: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
Cc: Baolin Wang <baolin.wang@linux.alibaba.com>
Cc: Baruch Siach <baruch@tkos.co.il>
Cc: Bjorn Andersson <andersson@kernel.org>
Cc: Claudiu Beznea <claudiu.beznea@tuxon.dev>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Fabio Estevam <festevam@gmail.com>
Cc: Hammer Hsieh <hammerh0314@gmail.com>
Cc: "Christian König" <christian.koenig@amd.com>
Cc: Christophe Leroy <christophe.leroy@csgroup.eu>
Cc: Chunyan Zhang <zhang.lyra@gmail.com>
Cc: Jerome Brunet <jbrunet@baylibre.com>
Cc: Jonathan Hunter <jonathanh@nvidia.com>
Cc: Kevin Hilman <khilman@baylibre.com>
Cc: Konrad Dybcio <konrad.dybcio@linaro.org>
Cc: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Cc: Kumaravel Thiagarajan <kumaravel.thiagarajan@microchip.com>
Cc: Laxman Dewangan <ldewangan@nvidia.com>
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-arm-msm@vger.kernel.org
Cc: "Maciej W. Rozycki" <macro@orcam.me.uk>
Cc: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
Cc: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
Cc: Matthias Brugger <matthias.bgg@gmail.com>
Cc: Maxime Coquelin <mcoquelin.stm32@gmail.com>
Cc: Michael Ellerman <mpe@ellerman.id.au>
Cc: Michal Simek <michal.simek@amd.com>
Cc: "Naveen N. Rao" <naveen.n.rao@linux.ibm.com>
Cc: Neil Armstrong <neil.armstrong@linaro.org>
Cc: Nicolas Ferre <nicolas.ferre@microchip.com>
Cc: Nicholas Piggin <npiggin@gmail.com>
Cc: Orson Zhai <orsonzhai@gmail.com>
Cc: "Pali Rohár" <pali@kernel.org>
Cc: Patrice Chotard <patrice.chotard@foss.st.com>
Cc: Peter Korsgaard <jacmet@sunsite.dk>
Cc: Richard Genoud <richard.genoud@gmail.com>
Cc: Russell King <linux@armlinux.org.uk>
Cc: Sascha Hauer <s.hauer@pengutronix.de>
Cc: Shawn Guo <shawnguo@kernel.org>
Cc: Stefani Seibold <stefani@seibold.net>
Cc: Sumit Semwal <sumit.semwal@linaro.org>
Cc: Taichi Sugaya <sugaya.taichi@socionext.com>
Cc: Takao Orito <orito.takao@socionext.com>
Cc: Tharun Kumar P <tharunkumar.pasumarthi@microchip.com>
Cc: Thierry Reding <thierry.reding@gmail.com>
Cc: Timur Tabi <timur@kernel.org>
Cc: Vineet Gupta <vgupta@kernel.org>

Jiri Slaby (SUSE) (15):
  kfifo: drop __kfifo_dma_out_finish_r()
  kfifo: introduce and use kfifo_skip_count()
  kfifo: add kfifo_out_linear{,_ptr}()
  kfifo: remove support for physically non-contiguous memory
  kfifo: rename l to len_to_end in setup_sgl()
  kfifo: pass offset to setup_sgl_buf() instead of a pointer
  kfifo: add kfifo_dma_out_prepare_mapped()
  kfifo: fix typos in kernel-doc
  tty: 8250_dma: use dmaengine_prep_slave_sg()
  tty: 8250_omap: use dmaengine_prep_slave_sg()
  tty: msm_serial: use dmaengine_prep_slave_sg()
  tty: serial: switch from circ_buf to kfifo
  tty: atmel_serial: use single DMA mapping for TX
  tty: atmel_serial: define macro for RX size
  tty: atmel_serial: use single DMA mapping for RX

 drivers/tty/serial/8250/8250_bcm7271.c  |  14 +--
 drivers/tty/serial/8250/8250_core.c     |   3 +-
 drivers/tty/serial/8250/8250_dma.c      |  31 +++--
 drivers/tty/serial/8250/8250_exar.c     |   5 +-
 drivers/tty/serial/8250/8250_mtk.c      |   2 +-
 drivers/tty/serial/8250/8250_omap.c     |  48 +++++---
 drivers/tty/serial/8250/8250_pci1xxxx.c |  50 ++++----
 drivers/tty/serial/8250/8250_port.c     |  22 ++--
 drivers/tty/serial/amba-pl011.c         |  46 +++-----
 drivers/tty/serial/ar933x_uart.c        |  15 ++-
 drivers/tty/serial/arc_uart.c           |   8 +-
 drivers/tty/serial/atmel_serial.c       | 150 +++++++++++-------------
 drivers/tty/serial/clps711x.c           |  12 +-
 drivers/tty/serial/cpm_uart.c           |  20 ++--
 drivers/tty/serial/digicolor-usart.c    |  12 +-
 drivers/tty/serial/dz.c                 |  13 +-
 drivers/tty/serial/fsl_linflexuart.c    |  17 +--
 drivers/tty/serial/fsl_lpuart.c         |  39 +++---
 drivers/tty/serial/icom.c               |  25 +---
 drivers/tty/serial/imx.c                |  54 ++++-----
 drivers/tty/serial/ip22zilog.c          |  26 ++--
 drivers/tty/serial/jsm/jsm_cls.c        |  29 ++---
 drivers/tty/serial/jsm/jsm_neo.c        |  38 ++----
 drivers/tty/serial/max3100.c            |  14 +--
 drivers/tty/serial/max310x.c            |  35 +++---
 drivers/tty/serial/men_z135_uart.c      |  26 ++--
 drivers/tty/serial/meson_uart.c         |  11 +-
 drivers/tty/serial/milbeaut_usio.c      |  15 +--
 drivers/tty/serial/msm_serial.c         | 114 +++++++++---------
 drivers/tty/serial/mvebu-uart.c         |   8 +-
 drivers/tty/serial/mxs-auart.c          |  23 +---
 drivers/tty/serial/pch_uart.c           |  21 ++--
 drivers/tty/serial/pic32_uart.c         |  15 ++-
 drivers/tty/serial/pmac_zilog.c         |  24 ++--
 drivers/tty/serial/qcom_geni_serial.c   |  36 +++---
 drivers/tty/serial/rda-uart.c           |  17 +--
 drivers/tty/serial/samsung_tty.c        |  54 +++++----
 drivers/tty/serial/sb1250-duart.c       |  13 +-
 drivers/tty/serial/sc16is7xx.c          |  40 +++----
 drivers/tty/serial/sccnxp.c             |  16 ++-
 drivers/tty/serial/serial-tegra.c       |  43 ++++---
 drivers/tty/serial/serial_core.c        |  56 ++++-----
 drivers/tty/serial/serial_port.c        |   2 +-
 drivers/tty/serial/sh-sci.c             |  51 ++++----
 drivers/tty/serial/sprd_serial.c        |  20 ++--
 drivers/tty/serial/st-asc.c             |   4 +-
 drivers/tty/serial/stm32-usart.c        |  52 ++++----
 drivers/tty/serial/sunhv.c              |  35 +++---
 drivers/tty/serial/sunplus-uart.c       |  16 +--
 drivers/tty/serial/sunsab.c             |  30 ++---
 drivers/tty/serial/sunsu.c              |  15 +--
 drivers/tty/serial/sunzilog.c           |  27 ++---
 drivers/tty/serial/tegra-tcu.c          |  10 +-
 drivers/tty/serial/timbuart.c           |  17 ++-
 drivers/tty/serial/uartlite.c           |  13 +-
 drivers/tty/serial/ucc_uart.c           |  20 ++--
 drivers/tty/serial/xilinx_uartps.c      |  20 ++--
 drivers/tty/serial/zs.c                 |  13 +-
 include/linux/kfifo.h                   | 143 ++++++++++++++++------
 include/linux/serial_core.h             |  49 +++++---
 lib/kfifo.c                             | 107 +++++++++--------
 61 files changed, 944 insertions(+), 960 deletions(-)

-- 
2.44.0

[PATCH 11/15] tty: msm_serial: use dmaengine_prep_slave_sg()

[Jiri Slaby (SUSE)]

This is a preparatory for the serial-to-kfifo switch. kfifo understands
only scatter-gatter approach, so switch to that.

No functional change intended, it's just dmaengine_prep_slave_single()
inline expanded.

And in this case, switch from dma_map_single() to dma_map_sg() too. This
needs struct msm_dma changes. I split the rx and tx parts into an union.
TX is now struct scatterlist, RX remains the old good phys-virt-count
triple.

Signed-off-by: Jiri Slaby (SUSE) <jirislaby@kernel.org>
Cc: Bjorn Andersson <andersson@kernel.org>
Cc: Konrad Dybcio <konrad.dybcio@linaro.org>
Cc: linux-arm-msm@vger.kernel.org
---
 drivers/tty/serial/msm_serial.c | 86 +++++++++++++++++++--------------
 1 file changed, 49 insertions(+), 37 deletions(-)

diff --git a/drivers/tty/serial/msm_serial.c b/drivers/tty/serial/msm_serial.c
index d27c4c8c84e1..7bf30e632313 100644
--- a/drivers/tty/serial/msm_serial.c
+++ b/drivers/tty/serial/msm_serial.c
@@ -161,11 +161,16 @@ enum {
 struct msm_dma {
 	struct dma_chan		*chan;
 	enum dma_data_direction dir;
-	dma_addr_t		phys;
-	unsigned char		*virt;
+	union {
+		struct {
+			dma_addr_t		phys;
+			unsigned char		*virt;
+			unsigned int		count;
+		} rx;
+		struct scatterlist tx_sg;
+	};
 	dma_cookie_t		cookie;
 	u32			enable_bit;
-	unsigned int		count;
 	struct dma_async_tx_descriptor	*desc;
 };
 
@@ -249,8 +254,12 @@ static void msm_stop_dma(struct uart_port *port, struct msm_dma *dma)
 	unsigned int mapped;
 	u32 val;
 
-	mapped = dma->count;
-	dma->count = 0;
+	if (dma->dir == DMA_TO_DEVICE) {
+		mapped = sg_dma_len(&dma->tx_sg);
+	} else {
+		mapped = dma->rx.count;
+		dma->rx.count = 0;
+	}
 
 	dmaengine_terminate_all(dma->chan);
 
@@ -265,8 +274,13 @@ static void msm_stop_dma(struct uart_port *port, struct msm_dma *dma)
 	val &= ~dma->enable_bit;
 	msm_write(port, val, UARTDM_DMEN);
 
-	if (mapped)
-		dma_unmap_single(dev, dma->phys, mapped, dma->dir);
+	if (mapped) {
+		if (dma->dir == DMA_TO_DEVICE) {
+			dma_unmap_sg(dev, &dma->tx_sg, 1, dma->dir);
+			sg_init_table(&dma->tx_sg, 1);
+		} else
+			dma_unmap_single(dev, dma->rx.phys, mapped, dma->dir);
+	}
 }
 
 static void msm_release_dma(struct msm_port *msm_port)
@@ -285,7 +299,7 @@ static void msm_release_dma(struct msm_port *msm_port)
 	if (dma->chan) {
 		msm_stop_dma(&msm_port->uart, dma);
 		dma_release_channel(dma->chan);
-		kfree(dma->virt);
+		kfree(dma->rx.virt);
 	}
 
 	memset(dma, 0, sizeof(*dma));
@@ -357,8 +371,8 @@ static void msm_request_rx_dma(struct msm_port *msm_port, resource_size_t base)
 
 	of_property_read_u32(dev->of_node, "qcom,rx-crci", &crci);
 
-	dma->virt = kzalloc(UARTDM_RX_SIZE, GFP_KERNEL);
-	if (!dma->virt)
+	dma->rx.virt = kzalloc(UARTDM_RX_SIZE, GFP_KERNEL);
+	if (!dma->rx.virt)
 		goto rel_rx;
 
 	memset(&conf, 0, sizeof(conf));
@@ -385,7 +399,7 @@ static void msm_request_rx_dma(struct msm_port *msm_port, resource_size_t base)
 
 	return;
 err:
-	kfree(dma->virt);
+	kfree(dma->rx.virt);
 rel_rx:
 	dma_release_channel(dma->chan);
 no_rx:
@@ -420,7 +434,7 @@ static void msm_start_tx(struct uart_port *port)
 	struct msm_dma *dma = &msm_port->tx_dma;
 
 	/* Already started in DMA mode */
-	if (dma->count)
+	if (sg_dma_len(&dma->tx_sg))
 		return;
 
 	msm_port->imr |= MSM_UART_IMR_TXLEV;
@@ -448,12 +462,12 @@ static void msm_complete_tx_dma(void *args)
 	uart_port_lock_irqsave(port, &flags);
 
 	/* Already stopped */
-	if (!dma->count)
+	if (!sg_dma_len(&dma->tx_sg))
 		goto done;
 
 	dmaengine_tx_status(dma->chan, dma->cookie, &state);
 
-	dma_unmap_single(port->dev, dma->phys, dma->count, dma->dir);
+	dma_unmap_sg(port->dev, &dma->tx_sg, 1, dma->dir);
 
 	val = msm_read(port, UARTDM_DMEN);
 	val &= ~dma->enable_bit;
@@ -464,9 +478,9 @@ static void msm_complete_tx_dma(void *args)
 		msm_write(port, MSM_UART_CR_TX_ENABLE, MSM_UART_CR);
 	}
 
-	count = dma->count - state.residue;
+	count = sg_dma_len(&dma->tx_sg) - state.residue;
 	uart_xmit_advance(port, count);
-	dma->count = 0;
+	sg_init_table(&dma->tx_sg, 1);
 
 	/* Restore "Tx FIFO below watermark" interrupt */
 	msm_port->imr |= MSM_UART_IMR_TXLEV;
@@ -485,19 +499,18 @@ static int msm_handle_tx_dma(struct msm_port *msm_port, unsigned int count)
 	struct circ_buf *xmit = &msm_port->uart.state->xmit;
 	struct uart_port *port = &msm_port->uart;
 	struct msm_dma *dma = &msm_port->tx_dma;
-	void *cpu_addr;
 	int ret;
 	u32 val;
 
-	cpu_addr = &xmit->buf[xmit->tail];
+	sg_init_table(&dma->tx_sg, 1);
+	sg_set_buf(&dma->tx_sg, &xmit->buf[xmit->tail], count);
 
-	dma->phys = dma_map_single(port->dev, cpu_addr, count, dma->dir);
-	ret = dma_mapping_error(port->dev, dma->phys);
+	ret = dma_map_sg(port->dev, &dma->tx_sg, 1, dma->dir);
 	if (ret)
 		return ret;
 
-	dma->desc = dmaengine_prep_slave_single(dma->chan, dma->phys,
-						count, DMA_MEM_TO_DEV,
+	dma->desc = dmaengine_prep_slave_sg(dma->chan, &dma->tx_sg, 1,
+						DMA_MEM_TO_DEV,
 						DMA_PREP_INTERRUPT |
 						DMA_PREP_FENCE);
 	if (!dma->desc) {
@@ -520,8 +533,6 @@ static int msm_handle_tx_dma(struct msm_port *msm_port, unsigned int count)
 	msm_port->imr &= ~MSM_UART_IMR_TXLEV;
 	msm_write(port, msm_port->imr, MSM_UART_IMR);
 
-	dma->count = count;
-
 	val = msm_read(port, UARTDM_DMEN);
 	val |= dma->enable_bit;
 
@@ -536,7 +547,8 @@ static int msm_handle_tx_dma(struct msm_port *msm_port, unsigned int count)
 	dma_async_issue_pending(dma->chan);
 	return 0;
 unmap:
-	dma_unmap_single(port->dev, dma->phys, count, dma->dir);
+	dma_unmap_sg(port->dev, &dma->tx_sg, 1, dma->dir);
+	sg_init_table(&dma->tx_sg, 1);
 	return ret;
 }
 
@@ -553,7 +565,7 @@ static void msm_complete_rx_dma(void *args)
 	uart_port_lock_irqsave(port, &flags);
 
 	/* Already stopped */
-	if (!dma->count)
+	if (!dma->rx.count)
 		goto done;
 
 	val = msm_read(port, UARTDM_DMEN);
@@ -570,14 +582,14 @@ static void msm_complete_rx_dma(void *args)
 
 	port->icount.rx += count;
 
-	dma->count = 0;
+	dma->rx.count = 0;
 
-	dma_unmap_single(port->dev, dma->phys, UARTDM_RX_SIZE, dma->dir);
+	dma_unmap_single(port->dev, dma->rx.phys, UARTDM_RX_SIZE, dma->dir);
 
 	for (i = 0; i < count; i++) {
 		char flag = TTY_NORMAL;
 
-		if (msm_port->break_detected && dma->virt[i] == 0) {
+		if (msm_port->break_detected && dma->rx.virt[i] == 0) {
 			port->icount.brk++;
 			flag = TTY_BREAK;
 			msm_port->break_detected = false;
@@ -588,9 +600,9 @@ static void msm_complete_rx_dma(void *args)
 		if (!(port->read_status_mask & MSM_UART_SR_RX_BREAK))
 			flag = TTY_NORMAL;
 
-		sysrq = uart_prepare_sysrq_char(port, dma->virt[i]);
+		sysrq = uart_prepare_sysrq_char(port, dma->rx.virt[i]);
 		if (!sysrq)
-			tty_insert_flip_char(tport, dma->virt[i], flag);
+			tty_insert_flip_char(tport, dma->rx.virt[i], flag);
 	}
 
 	msm_start_rx_dma(msm_port);
@@ -614,13 +626,13 @@ static void msm_start_rx_dma(struct msm_port *msm_port)
 	if (!dma->chan)
 		return;
 
-	dma->phys = dma_map_single(uart->dev, dma->virt,
+	dma->rx.phys = dma_map_single(uart->dev, dma->rx.virt,
 				   UARTDM_RX_SIZE, dma->dir);
-	ret = dma_mapping_error(uart->dev, dma->phys);
+	ret = dma_mapping_error(uart->dev, dma->rx.phys);
 	if (ret)
 		goto sw_mode;
 
-	dma->desc = dmaengine_prep_slave_single(dma->chan, dma->phys,
+	dma->desc = dmaengine_prep_slave_single(dma->chan, dma->rx.phys,
 						UARTDM_RX_SIZE, DMA_DEV_TO_MEM,
 						DMA_PREP_INTERRUPT);
 	if (!dma->desc)
@@ -648,7 +660,7 @@ static void msm_start_rx_dma(struct msm_port *msm_port)
 
 	msm_write(uart, msm_port->imr, MSM_UART_IMR);
 
-	dma->count = UARTDM_RX_SIZE;
+	dma->rx.count = UARTDM_RX_SIZE;
 
 	dma_async_issue_pending(dma->chan);
 
@@ -668,7 +680,7 @@ static void msm_start_rx_dma(struct msm_port *msm_port)
 
 	return;
 unmap:
-	dma_unmap_single(uart->dev, dma->phys, UARTDM_RX_SIZE, dma->dir);
+	dma_unmap_single(uart->dev, dma->rx.phys, UARTDM_RX_SIZE, dma->dir);
 
 sw_mode:
 	/*
@@ -955,7 +967,7 @@ static irqreturn_t msm_uart_irq(int irq, void *dev_id)
 	}
 
 	if (misr & (MSM_UART_IMR_RXLEV | MSM_UART_IMR_RXSTALE)) {
-		if (dma->count) {
+		if (dma->rx.count) {
 			val = MSM_UART_CR_CMD_STALE_EVENT_DISABLE;
 			msm_write(port, val, MSM_UART_CR);
 			val = MSM_UART_CR_CMD_RESET_STALE_INT;
-- 
2.44.0

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

[Marek Szyprowski]

Dear All,

On 05.04.2024 08:08, Jiri Slaby (SUSE) wrote:
> 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.
>
> omap needed a bit more care as it needs to put a char into FIFO to start
> the DMA transfer when OMAP_DMA_TX_KICK is set. In that case, we have to
> do kfifo_dma_out_prepare twice: once to find out the tx_size (to find
> out if it is worths to do DMA at all -- size >= 4), the second time for
> the actual transfer.
>
> All traces of circ_buf are removed from serial_core.h (and its struct
> uart_state).
>
> Signed-off-by: Jiri Slaby (SUSE) <jirislaby@kernel.org>
> ...

This patch landed in linux-next as commit 1788cf6a91d9 ("tty: serial: 
switch from circ_buf to kfifo"). Unfortunately it breaks UART operation 
on thr Amlogic Meson based boards (drivers/tty/serial/meson_uart.c 
driver) and Qualcomm RB5 board (drivers/tty/serial/qcom_geni_serial.c). 
Once the init process is started, a complete garbage is printed to the 
serial console. Here is an example how it looks:

[    8.763154] Run /sbin/init as init process
NT [   12.429776] platform cpufreq-dt: deferred probe pending: (reason 
unknown)
[   12.434259] platform regulator-vddcpu: deferred probe pending: 
pwm-regulator: Failed to get PWM
[[6if;9]Uigmkfl-tl ocretbo nrnee .[[6if;9]Uigmkfl-tl ocretbo nrnee .
[[6if;9]Uigmkfl-tl ocretbo nrnee .
[[6if;9]Uigmkfl-tl ocretbo nrnee .
[[6if;9]Uigmkfl-tl ocretbo nrnee .
[[6if;9]Uigmkfl-tl ocretbo nrnee .
[[6if;9]Uigmkfl-tl ocretbo nrnee .
[.. trightlgeet ipthr ytm-dv[[6if;9]Uigmkfl-tl ocretbo nrnee .
[.. trightlgeet ipthr ytm-dv5c[[6if;9]Uigmkfl-tl ocretbo nrnee .
[.. trightlgeet ipthr ytm-dv5c7[Gmo [94m8[2h[0.
..]Snhszn h nta opu vns(usses..[[6if;9]Uigmkfl-tl ocretbo nrnee .
[.. trightlgeet ipthr ytm-dv5c7[Gmo [94m8[2h[0.
..]Snhszn h nta opu vns(usses..[[6if;9]Uigmkfl-tl ocretbo nrnee .
[.. trightlgeet ipthr ytm-dv5c7[Gmo [94m8[2h[0.
..]Snhszn h nta opu vns(usses..[[6if;9]Uigmkfl-tl ocretbo nrnee .
[.. trightlgeet ipthr ytm-dv5c7[Gmo [94m8[2h[0.
..]Snhszn h nta opu vns(usses..[[6if;9]Uigmkfl-tl ocretbo nrnee .
[.. trightlgeet ipthr ytm-dv5c7[Gmo [94m8[2h[0.
..]Snhszn h nta opu vns(usses..[[6if;9]Uigmkfl-tl ocretbo nrnee .
[.. trightlgeet ipthr ytm-dv5c7[Gmo [94m8[2h[0.
..]Snhszn h nta opu vns(usses..[[6if;9]Uigmkfl-tl ocretbo nrnee .
[.. trightlgeet ipthr ytm-dv5c7[Gmo [94m8[2h[0.
..]Snhszn h nta opu vns(usses..[[6if;9]Uigmkfl-tl ocretbo nrnee .
[.. trightlgeet ipthr ytm-dv5c7[Gmo [94m8[2h[0.
..]Snhszn h nta opu vns(usses..[[6if;9]Uigmkfl-tl ocretbo nrnee .
[.. trightlgeet ipthr ytm-dv5c7[Gmo [94m8[2h[0.
..]Snhszn h nta opu vns(usses..[2l[1[[6if;9]Uigmkfl-tl ocretbo nrnee .
[.. trightlgeet ipthr ytm-dv5c7[Gmo [94m8[2h[0.
[.. yteiigteiiilhtlgeet dvcs..[[6if;9]Uigmkfl-tl ocretbo nrnee .
[.. trightlgeet ipthr ytm-dv5c7[Gmo [94m8[2h[0.
[.. yteiigteiiilhtlgeet dvcs..[[6if;9]Uigmkfl-tl ocretbo nrnee .
[.. trightlgeet ipthr ytm-dv5c7[Gmo [94m8[2h[0.
[.. yteiigteiiilhtlgeet dvcs..[[6if;9]Uigmkfl-tl ocretbo nrnee .
[.. trightlgeet ipthr ytm-dv5c7[Gmo [94m8[2h[0.
[.. yteiigteiiilhtlgeet dvcs..[[6if;9]Uigmkfl-tl ocretbo nrnee .
[.. trightlgeet ipthr ytm-dv5c7[Gmo [94m8[2h[0.
[.. yteiigteiiilhtlgeet dvcs..[  105.613420] debugfs: Directory 
'ff800280.cec' with parent 'regmap' already present!
[[6if;9]Uigmkfl-tl ocretbo nrnee .
[.. trightlgeet ipthr ytm-dv5c7[Gmo [94m8[2h[0.
..]Snhszn h nta opu vns(usses..[2l[11[[2 k;9?5[  105.638809] mc: Linux 
media interface: v0.10
[.. atn o dvt eflyppltd.[  105.707390] meson-vrtc ff8000a8.rtc: 
registered as rtc0


I found this patch by bisecting today's linux-next. I've checked the 
changes in the related UART drivers and I don't see any obvious issues 
though. Let me know if I can help debugging this issue somehow.

I've trimmed recipient list due to my smtp server limitation.


Best regards

-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

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

[Jiri Slaby]

On 15. 04. 24, 14:58, Marek Szyprowski wrote:
> Dear All,
> 
> On 05.04.2024 08:08, Jiri Slaby (SUSE) wrote:
>> 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.
>>
>> omap needed a bit more care as it needs to put a char into FIFO to start
>> the DMA transfer when OMAP_DMA_TX_KICK is set. In that case, we have to
>> do kfifo_dma_out_prepare twice: once to find out the tx_size (to find
>> out if it is worths to do DMA at all -- size >= 4), the second time for
>> the actual transfer.
>>
>> All traces of circ_buf are removed from serial_core.h (and its struct
>> uart_state).
>>
>> Signed-off-by: Jiri Slaby (SUSE) <jirislaby@kernel.org>
>> ...
> 
> This patch landed in linux-next as commit 1788cf6a91d9 ("tty: serial:
> switch from circ_buf to kfifo"). Unfortunately it breaks UART operation
> on thr Amlogic Meson based boards (drivers/tty/serial/meson_uart.c
> driver) and Qualcomm RB5 board (drivers/tty/serial/qcom_geni_serial.c).
> Once the init process is started, a complete garbage is printed to the
> serial console. Here is an example how it looks:

Oh my!

Both drivers move the tail using both kfifo and uart_xmit_advance() 
interfaces. Bah. Does it help to remove that uart_xmit_advance() for 
both of them? (TX stats will be broken.)

Users of uart_port_tx() are not affected.

This is my fault when merging uart_xmit_advance() with this series.

thanks,
-- 
js
suse labs

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

[Marek Szyprowski]

On 15.04.2024 15:28, Jiri Slaby wrote:
> On 15. 04. 24, 14:58, Marek Szyprowski wrote:
>> On 05.04.2024 08:08, Jiri Slaby (SUSE) wrote:
>>> 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.
>>>
>>> omap needed a bit more care as it needs to put a char into FIFO to 
>>> start
>>> the DMA transfer when OMAP_DMA_TX_KICK is set. In that case, we have to
>>> do kfifo_dma_out_prepare twice: once to find out the tx_size (to find
>>> out if it is worths to do DMA at all -- size >= 4), the second time for
>>> the actual transfer.
>>>
>>> All traces of circ_buf are removed from serial_core.h (and its struct
>>> uart_state).
>>>
>>> Signed-off-by: Jiri Slaby (SUSE) <jirislaby@kernel.org>
>>> ...
>>
>> This patch landed in linux-next as commit 1788cf6a91d9 ("tty: serial:
>> switch from circ_buf to kfifo"). Unfortunately it breaks UART operation
>> on thr Amlogic Meson based boards (drivers/tty/serial/meson_uart.c
>> driver) and Qualcomm RB5 board (drivers/tty/serial/qcom_geni_serial.c).
>> Once the init process is started, a complete garbage is printed to the
>> serial console. Here is an example how it looks:
>
> Oh my!
>
> Both drivers move the tail using both kfifo and uart_xmit_advance() 
> interfaces. Bah. Does it help to remove that uart_xmit_advance() for 
> both of them? (TX stats will be broken.)
>
> Users of uart_port_tx() are not affected.
>
> This is my fault when merging uart_xmit_advance() with this series.

Yes, removing uart_xmit_advance() from both drivers seems to be fixing 
the console output.


Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

Re: [PATCH 11/15] tty: msm_serial: use dmaengine_prep_slave_sg()

[Marek Szyprowski]

On 05.04.2024 08:08, Jiri Slaby (SUSE) wrote:
> This is a preparatory for the serial-to-kfifo switch. kfifo understands
> only scatter-gatter approach, so switch to that.
>
> No functional change intended, it's just dmaengine_prep_slave_single()
> inline expanded.
>
> And in this case, switch from dma_map_single() to dma_map_sg() too. This
> needs struct msm_dma changes. I split the rx and tx parts into an union.
> TX is now struct scatterlist, RX remains the old good phys-virt-count
> triple.
>
> Signed-off-by: Jiri Slaby (SUSE) <jirislaby@kernel.org>
> Cc: Bjorn Andersson <andersson@kernel.org>
> Cc: Konrad Dybcio <konrad.dybcio@linaro.org>
> Cc: linux-arm-msm@vger.kernel.org

I've just found that this patch broke UART operation on DragonBoard 
410c. I briefly checked and didn't notice anything obviously wrong here, 
but the board stops transmitting any data from its serial port after the 
first message. I will try to analyze this issue a bit more tomorrow.

> ---
>   drivers/tty/serial/msm_serial.c | 86 +++++++++++++++++++--------------
>   1 file changed, 49 insertions(+), 37 deletions(-)
>
> diff --git a/drivers/tty/serial/msm_serial.c b/drivers/tty/serial/msm_serial.c
> index d27c4c8c84e1..7bf30e632313 100644
> --- a/drivers/tty/serial/msm_serial.c
> +++ b/drivers/tty/serial/msm_serial.c
> @@ -161,11 +161,16 @@ enum {
>   struct msm_dma {
>   	struct dma_chan		*chan;
>   	enum dma_data_direction dir;
> -	dma_addr_t		phys;
> -	unsigned char		*virt;
> +	union {
> +		struct {
> +			dma_addr_t		phys;
> +			unsigned char		*virt;
> +			unsigned int		count;
> +		} rx;
> +		struct scatterlist tx_sg;
> +	};
>   	dma_cookie_t		cookie;
>   	u32			enable_bit;
> -	unsigned int		count;
>   	struct dma_async_tx_descriptor	*desc;
>   };
>   
> @@ -249,8 +254,12 @@ static void msm_stop_dma(struct uart_port *port, struct msm_dma *dma)
>   	unsigned int mapped;
>   	u32 val;
>   
> -	mapped = dma->count;
> -	dma->count = 0;
> +	if (dma->dir == DMA_TO_DEVICE) {
> +		mapped = sg_dma_len(&dma->tx_sg);
> +	} else {
> +		mapped = dma->rx.count;
> +		dma->rx.count = 0;
> +	}
>   
>   	dmaengine_terminate_all(dma->chan);
>   
> @@ -265,8 +274,13 @@ static void msm_stop_dma(struct uart_port *port, struct msm_dma *dma)
>   	val &= ~dma->enable_bit;
>   	msm_write(port, val, UARTDM_DMEN);
>   
> -	if (mapped)
> -		dma_unmap_single(dev, dma->phys, mapped, dma->dir);
> +	if (mapped) {
> +		if (dma->dir == DMA_TO_DEVICE) {
> +			dma_unmap_sg(dev, &dma->tx_sg, 1, dma->dir);
> +			sg_init_table(&dma->tx_sg, 1);
> +		} else
> +			dma_unmap_single(dev, dma->rx.phys, mapped, dma->dir);
> +	}
>   }
>   
>   static void msm_release_dma(struct msm_port *msm_port)
> @@ -285,7 +299,7 @@ static void msm_release_dma(struct msm_port *msm_port)
>   	if (dma->chan) {
>   		msm_stop_dma(&msm_port->uart, dma);
>   		dma_release_channel(dma->chan);
> -		kfree(dma->virt);
> +		kfree(dma->rx.virt);
>   	}
>   
>   	memset(dma, 0, sizeof(*dma));
> @@ -357,8 +371,8 @@ static void msm_request_rx_dma(struct msm_port *msm_port, resource_size_t base)
>   
>   	of_property_read_u32(dev->of_node, "qcom,rx-crci", &crci);
>   
> -	dma->virt = kzalloc(UARTDM_RX_SIZE, GFP_KERNEL);
> -	if (!dma->virt)
> +	dma->rx.virt = kzalloc(UARTDM_RX_SIZE, GFP_KERNEL);
> +	if (!dma->rx.virt)
>   		goto rel_rx;
>   
>   	memset(&conf, 0, sizeof(conf));
> @@ -385,7 +399,7 @@ static void msm_request_rx_dma(struct msm_port *msm_port, resource_size_t base)
>   
>   	return;
>   err:
> -	kfree(dma->virt);
> +	kfree(dma->rx.virt);
>   rel_rx:
>   	dma_release_channel(dma->chan);
>   no_rx:
> @@ -420,7 +434,7 @@ static void msm_start_tx(struct uart_port *port)
>   	struct msm_dma *dma = &msm_port->tx_dma;
>   
>   	/* Already started in DMA mode */
> -	if (dma->count)
> +	if (sg_dma_len(&dma->tx_sg))
>   		return;
>   
>   	msm_port->imr |= MSM_UART_IMR_TXLEV;
> @@ -448,12 +462,12 @@ static void msm_complete_tx_dma(void *args)
>   	uart_port_lock_irqsave(port, &flags);
>   
>   	/* Already stopped */
> -	if (!dma->count)
> +	if (!sg_dma_len(&dma->tx_sg))
>   		goto done;
>   
>   	dmaengine_tx_status(dma->chan, dma->cookie, &state);
>   
> -	dma_unmap_single(port->dev, dma->phys, dma->count, dma->dir);
> +	dma_unmap_sg(port->dev, &dma->tx_sg, 1, dma->dir);
>   
>   	val = msm_read(port, UARTDM_DMEN);
>   	val &= ~dma->enable_bit;
> @@ -464,9 +478,9 @@ static void msm_complete_tx_dma(void *args)
>   		msm_write(port, MSM_UART_CR_TX_ENABLE, MSM_UART_CR);
>   	}
>   
> -	count = dma->count - state.residue;
> +	count = sg_dma_len(&dma->tx_sg) - state.residue;
>   	uart_xmit_advance(port, count);
> -	dma->count = 0;
> +	sg_init_table(&dma->tx_sg, 1);
>   
>   	/* Restore "Tx FIFO below watermark" interrupt */
>   	msm_port->imr |= MSM_UART_IMR_TXLEV;
> @@ -485,19 +499,18 @@ static int msm_handle_tx_dma(struct msm_port *msm_port, unsigned int count)
>   	struct circ_buf *xmit = &msm_port->uart.state->xmit;
>   	struct uart_port *port = &msm_port->uart;
>   	struct msm_dma *dma = &msm_port->tx_dma;
> -	void *cpu_addr;
>   	int ret;
>   	u32 val;
>   
> -	cpu_addr = &xmit->buf[xmit->tail];
> +	sg_init_table(&dma->tx_sg, 1);
> +	sg_set_buf(&dma->tx_sg, &xmit->buf[xmit->tail], count);
>   
> -	dma->phys = dma_map_single(port->dev, cpu_addr, count, dma->dir);
> -	ret = dma_mapping_error(port->dev, dma->phys);
> +	ret = dma_map_sg(port->dev, &dma->tx_sg, 1, dma->dir);
>   	if (ret)
>   		return ret;
>   
> -	dma->desc = dmaengine_prep_slave_single(dma->chan, dma->phys,
> -						count, DMA_MEM_TO_DEV,
> +	dma->desc = dmaengine_prep_slave_sg(dma->chan, &dma->tx_sg, 1,
> +						DMA_MEM_TO_DEV,
>   						DMA_PREP_INTERRUPT |
>   						DMA_PREP_FENCE);
>   	if (!dma->desc) {
> @@ -520,8 +533,6 @@ static int msm_handle_tx_dma(struct msm_port *msm_port, unsigned int count)
>   	msm_port->imr &= ~MSM_UART_IMR_TXLEV;
>   	msm_write(port, msm_port->imr, MSM_UART_IMR);
>   
> -	dma->count = count;
> -
>   	val = msm_read(port, UARTDM_DMEN);
>   	val |= dma->enable_bit;
>   
> @@ -536,7 +547,8 @@ static int msm_handle_tx_dma(struct msm_port *msm_port, unsigned int count)
>   	dma_async_issue_pending(dma->chan);
>   	return 0;
>   unmap:
> -	dma_unmap_single(port->dev, dma->phys, count, dma->dir);
> +	dma_unmap_sg(port->dev, &dma->tx_sg, 1, dma->dir);
> +	sg_init_table(&dma->tx_sg, 1);
>   	return ret;
>   }
>   
> @@ -553,7 +565,7 @@ static void msm_complete_rx_dma(void *args)
>   	uart_port_lock_irqsave(port, &flags);
>   
>   	/* Already stopped */
> -	if (!dma->count)
> +	if (!dma->rx.count)
>   		goto done;
>   
>   	val = msm_read(port, UARTDM_DMEN);
> @@ -570,14 +582,14 @@ static void msm_complete_rx_dma(void *args)
>   
>   	port->icount.rx += count;
>   
> -	dma->count = 0;
> +	dma->rx.count = 0;
>   
> -	dma_unmap_single(port->dev, dma->phys, UARTDM_RX_SIZE, dma->dir);
> +	dma_unmap_single(port->dev, dma->rx.phys, UARTDM_RX_SIZE, dma->dir);
>   
>   	for (i = 0; i < count; i++) {
>   		char flag = TTY_NORMAL;
>   
> -		if (msm_port->break_detected && dma->virt[i] == 0) {
> +		if (msm_port->break_detected && dma->rx.virt[i] == 0) {
>   			port->icount.brk++;
>   			flag = TTY_BREAK;
>   			msm_port->break_detected = false;
> @@ -588,9 +600,9 @@ static void msm_complete_rx_dma(void *args)
>   		if (!(port->read_status_mask & MSM_UART_SR_RX_BREAK))
>   			flag = TTY_NORMAL;
>   
> -		sysrq = uart_prepare_sysrq_char(port, dma->virt[i]);
> +		sysrq = uart_prepare_sysrq_char(port, dma->rx.virt[i]);
>   		if (!sysrq)
> -			tty_insert_flip_char(tport, dma->virt[i], flag);
> +			tty_insert_flip_char(tport, dma->rx.virt[i], flag);
>   	}
>   
>   	msm_start_rx_dma(msm_port);
> @@ -614,13 +626,13 @@ static void msm_start_rx_dma(struct msm_port *msm_port)
>   	if (!dma->chan)
>   		return;
>   
> -	dma->phys = dma_map_single(uart->dev, dma->virt,
> +	dma->rx.phys = dma_map_single(uart->dev, dma->rx.virt,
>   				   UARTDM_RX_SIZE, dma->dir);
> -	ret = dma_mapping_error(uart->dev, dma->phys);
> +	ret = dma_mapping_error(uart->dev, dma->rx.phys);
>   	if (ret)
>   		goto sw_mode;
>   
> -	dma->desc = dmaengine_prep_slave_single(dma->chan, dma->phys,
> +	dma->desc = dmaengine_prep_slave_single(dma->chan, dma->rx.phys,
>   						UARTDM_RX_SIZE, DMA_DEV_TO_MEM,
>   						DMA_PREP_INTERRUPT);
>   	if (!dma->desc)
> @@ -648,7 +660,7 @@ static void msm_start_rx_dma(struct msm_port *msm_port)
>   
>   	msm_write(uart, msm_port->imr, MSM_UART_IMR);
>   
> -	dma->count = UARTDM_RX_SIZE;
> +	dma->rx.count = UARTDM_RX_SIZE;
>   
>   	dma_async_issue_pending(dma->chan);
>   
> @@ -668,7 +680,7 @@ static void msm_start_rx_dma(struct msm_port *msm_port)
>   
>   	return;
>   unmap:
> -	dma_unmap_single(uart->dev, dma->phys, UARTDM_RX_SIZE, dma->dir);
> +	dma_unmap_single(uart->dev, dma->rx.phys, UARTDM_RX_SIZE, dma->dir);
>   
>   sw_mode:
>   	/*
> @@ -955,7 +967,7 @@ static irqreturn_t msm_uart_irq(int irq, void *dev_id)
>   	}
>   
>   	if (misr & (MSM_UART_IMR_RXLEV | MSM_UART_IMR_RXSTALE)) {
> -		if (dma->count) {
> +		if (dma->rx.count) {
>   			val = MSM_UART_CR_CMD_STALE_EVENT_DISABLE;
>   			msm_write(port, val, MSM_UART_CR);
>   			val = MSM_UART_CR_CMD_RESET_STALE_INT;

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

[PATCH] serial: meson+qcom: don't advance the kfifo twice

[Jiri Slaby (SUSE)]

Marek reports, that the -next commit 1788cf6a91d9 (tty: serial: switch
from circ_buf to kfifo) broke meson_uart and qcom_geni_serial. The
commit mistakenly advanced the kfifo twice: once by
uart_fifo_get()/kfifo_out() and second time by uart_xmit_advance().

To advance the fifo only once, drop the superfluous uart_xmit_advance()
from both.

To count the TX statistics properly, use uart_fifo_out() in
qcom_geni_serial (meson_uart_start_tx() already uses that).

I checked all other uses of uart_xmit_advance() and they appear correct:
either they are finishing DMA transfers or are after peek/linear_ptr
(i.e. they do not advance fifo).

Signed-off-by: Jiri Slaby (SUSE) <jirislaby@kernel.org>
Reported-by: Marek Szyprowski <m.szyprowski@samsung.com>
Cc: Neil Armstrong <neil.armstrong@linaro.org>
Cc: Kevin Hilman <khilman@baylibre.com>
Cc: Jerome Brunet <jbrunet@baylibre.com>
Cc: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
---
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-amlogic@lists.infradead.org
Cc: linux-arm-msm@vger.kernel.org
---
 drivers/tty/serial/meson_uart.c       | 1 -
 drivers/tty/serial/qcom_geni_serial.c | 4 +---
 2 files changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/tty/serial/meson_uart.c b/drivers/tty/serial/meson_uart.c
index 4587ed4d4d5d..8eb586ac3b0d 100644
--- a/drivers/tty/serial/meson_uart.c
+++ b/drivers/tty/serial/meson_uart.c
@@ -162,7 +162,6 @@ static void meson_uart_start_tx(struct uart_port *port)
 			break;
 
 		writel(ch, port->membase + AML_UART_WFIFO);
-		uart_xmit_advance(port, 1);
 	}
 
 	if (!kfifo_is_empty(&tport->xmit_fifo)) {
diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
index 7814982f1921..2bd25afe0d92 100644
--- a/drivers/tty/serial/qcom_geni_serial.c
+++ b/drivers/tty/serial/qcom_geni_serial.c
@@ -855,7 +855,6 @@ static void qcom_geni_serial_send_chunk_fifo(struct uart_port *uport,
 					     unsigned int chunk)
 {
 	struct qcom_geni_serial_port *port = to_dev_port(uport);
-	struct tty_port *tport = &uport->state->port;
 	unsigned int tx_bytes, remaining = chunk;
 	u8 buf[BYTES_PER_FIFO_WORD];
 
@@ -863,8 +862,7 @@ static void qcom_geni_serial_send_chunk_fifo(struct uart_port *uport,
 		memset(buf, 0, sizeof(buf));
 		tx_bytes = min(remaining, BYTES_PER_FIFO_WORD);
 
-		tx_bytes = kfifo_out(&tport->xmit_fifo, buf, tx_bytes);
-		uart_xmit_advance(uport, tx_bytes);
+		tx_bytes = uart_fifo_out(uport, buf, tx_bytes);
 
 		iowrite32_rep(uport->membase + SE_GENI_TX_FIFOn, buf, 1);
 
-- 
2.44.0

Re: [PATCH 11/15] tty: msm_serial: use dmaengine_prep_slave_sg()

[Jiri Slaby]

On 15. 04. 24, 23:17, Marek Szyprowski wrote:
> On 05.04.2024 08:08, Jiri Slaby (SUSE) wrote:
>> This is a preparatory for the serial-to-kfifo switch. kfifo understands
>> only scatter-gatter approach, so switch to that.
>>
>> No functional change intended, it's just dmaengine_prep_slave_single()
>> inline expanded.
>>
>> And in this case, switch from dma_map_single() to dma_map_sg() too. This
>> needs struct msm_dma changes. I split the rx and tx parts into an union.
>> TX is now struct scatterlist, RX remains the old good phys-virt-count
>> triple.
>>
>> Signed-off-by: Jiri Slaby (SUSE) <jirislaby@kernel.org>
>> Cc: Bjorn Andersson <andersson@kernel.org>
>> Cc: Konrad Dybcio <konrad.dybcio@linaro.org>
>> Cc: linux-arm-msm@vger.kernel.org
> 
> I've just found that this patch broke UART operation on DragonBoard
> 410c. I briefly checked and didn't notice anything obviously wrong here,
> but the board stops transmitting any data from its serial port after the
> first message. I will try to analyze this issue a bit more tomorrow.

I double checked, but I see no immediate issues in the patch too. So 
please, if you can analyze this more…

thanks,
-- 
js
suse labs

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

[Anders Roxell]

On 2024-04-15 15:28, Jiri Slaby wrote:
> On 15. 04. 24, 14:58, Marek Szyprowski wrote:
> > Dear All,
> > 
> > On 05.04.2024 08:08, Jiri Slaby (SUSE) wrote:
> > > 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.
> > > 
> > > omap needed a bit more care as it needs to put a char into FIFO to start
> > > the DMA transfer when OMAP_DMA_TX_KICK is set. In that case, we have to
> > > do kfifo_dma_out_prepare twice: once to find out the tx_size (to find
> > > out if it is worths to do DMA at all -- size >= 4), the second time for
> > > the actual transfer.
> > > 
> > > All traces of circ_buf are removed from serial_core.h (and its struct
> > > uart_state).
> > > 
> > > Signed-off-by: Jiri Slaby (SUSE) <jirislaby@kernel.org>
> > > ...
> > 
> > This patch landed in linux-next as commit 1788cf6a91d9 ("tty: serial:
> > switch from circ_buf to kfifo"). Unfortunately it breaks UART operation
> > on thr Amlogic Meson based boards (drivers/tty/serial/meson_uart.c
> > driver) and Qualcomm RB5 board (drivers/tty/serial/qcom_geni_serial.c).
> > Once the init process is started, a complete garbage is printed to the
> > serial console. Here is an example how it looks:
> 
> Oh my!
> 
> Both drivers move the tail using both kfifo and uart_xmit_advance()
> interfaces. Bah. Does it help to remove that uart_xmit_advance() for both of
> them? (TX stats will be broken.)
> 
> Users of uart_port_tx() are not affected.
> 
> This is my fault when merging uart_xmit_advance() with this series.
> 

I'm trying to run on two dragonboard devices db410c and db845c and both
fails to boot see the boot failure from db845c [1], linux-next tag: next-20240415.
I tried to apply the patch [2] (that you proposed in this thread) ontop of next-20240415. However, that didn't
help bootlog on db845c [3].

Cheers,
Anders
[1] https://tuxapi.tuxsuite.com/v1/groups/linaro/projects/lkft/tests/2f7sLxYtIQXQzsnTzE1Dye2xweg/logs?format=html
[2] https://lore.kernel.org/lkml/20240416054825.6211-1-jirislaby@kernel.org/raw
[3] https://tuxapi.tuxsuite.com/v1/groups/linaro/projects/anders/tests/2fDgvWnyEmFm9mqMCxOaruBOfTe/logs?format=html

Re: [PATCH 11/15] tty: msm_serial: use dmaengine_prep_slave_sg()

[Marek Szyprowski]

Hi Jiri,

On 16.04.2024 12:23, Jiri Slaby wrote:
> On 15. 04. 24, 23:17, Marek Szyprowski wrote:
>> On 05.04.2024 08:08, Jiri Slaby (SUSE) wrote:
>>> This is a preparatory for the serial-to-kfifo switch. kfifo understands
>>> only scatter-gatter approach, so switch to that.
>>>
>>> No functional change intended, it's just dmaengine_prep_slave_single()
>>> inline expanded.
>>>
>>> And in this case, switch from dma_map_single() to dma_map_sg() too. 
>>> This
>>> needs struct msm_dma changes. I split the rx and tx parts into an 
>>> union.
>>> TX is now struct scatterlist, RX remains the old good phys-virt-count
>>> triple.
>>>
>>> Signed-off-by: Jiri Slaby (SUSE) <jirislaby@kernel.org>
>>> Cc: Bjorn Andersson <andersson@kernel.org>
>>> Cc: Konrad Dybcio <konrad.dybcio@linaro.org>
>>> Cc: linux-arm-msm@vger.kernel.org
>>
>> I've just found that this patch broke UART operation on DragonBoard
>> 410c. I briefly checked and didn't notice anything obviously wrong here,
>> but the board stops transmitting any data from its serial port after the
>> first message. I will try to analyze this issue a bit more tomorrow.
>
> I double checked, but I see no immediate issues in the patch too. So 
> please, if you can analyze this more…

I've spent some time digging into this issue and frankly speaking I 
still have no idea WHY it doesn't work (or I seriously mixed something 
in the scatterlist principles). However I found a workaround to make it 
working. Maybe it will help a bit guessing what happens there.

Here is a workaround:

diff --git a/drivers/tty/serial/msm_serial.c 
b/drivers/tty/serial/msm_serial.c
index ae7a8e3cf467..fd3f3bf03f33 100644
--- a/drivers/tty/serial/msm_serial.c
+++ b/drivers/tty/serial/msm_serial.c
@@ -169,6 +169,7 @@ struct msm_dma {
                 } rx;
                 struct scatterlist tx_sg;
         };
+       int                     mapped;
         dma_cookie_t            cookie;
         u32                     enable_bit;
         struct dma_async_tx_descriptor  *desc;
@@ -278,6 +279,7 @@ static void msm_stop_dma(struct uart_port *port, 
struct msm_dma *dma)
                 if (dma->dir == DMA_TO_DEVICE) {
                         dma_unmap_sg(dev, &dma->tx_sg, 1, dma->dir);
                         sg_init_table(&dma->tx_sg, 1);
+                       dma->mapped = 0;
                 } else
                         dma_unmap_single(dev, dma->rx.phys, mapped, 
dma->dir);
         }
@@ -434,7 +436,7 @@ static void msm_start_tx(struct uart_port *port)
         struct msm_dma *dma = &msm_port->tx_dma;

         /* Already started in DMA mode */
-       if (sg_dma_len(&dma->tx_sg))
+       if (dma->mapped)
                 return;

         msm_port->imr |= MSM_UART_IMR_TXLEV;
@@ -462,7 +464,7 @@ static void msm_complete_tx_dma(void *args)
         uart_port_lock_irqsave(port, &flags);

         /* Already stopped */
-       if (!sg_dma_len(&dma->tx_sg))
+       if (!dma->mapped)
                 goto done;

         dmaengine_tx_status(dma->chan, dma->cookie, &state);
@@ -481,6 +483,7 @@ static void msm_complete_tx_dma(void *args)
         count = sg_dma_len(&dma->tx_sg) - state.residue;
         uart_xmit_advance(port, count);
         sg_init_table(&dma->tx_sg, 1);
+       dma->mapped = 0;

         /* Restore "Tx FIFO below watermark" interrupt */
         msm_port->imr |= MSM_UART_IMR_TXLEV;
@@ -522,6 +525,7 @@ static int msm_handle_tx_dma(struct msm_port 
*msm_port, unsigned int count)
         dma->desc->callback_param = msm_port;

         dma->cookie = dmaengine_submit(dma->desc);
+       dma->mapped = 1;
         ret = dma_submit_error(dma->cookie);
         if (ret)
                 goto unmap;
@@ -549,6 +553,7 @@ static int msm_handle_tx_dma(struct msm_port 
*msm_port, unsigned int count)
  unmap:
         dma_unmap_sg(port->dev, &dma->tx_sg, 1, dma->dir);
         sg_init_table(&dma->tx_sg, 1);
+       dma->mapped = 0;
         return ret;
  }


Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

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

[Marek Szyprowski]

On 17.04.2024 12:08, Anders Roxell wrote:
> On 2024-04-15 15:28, Jiri Slaby wrote:
>> On 15. 04. 24, 14:58, Marek Szyprowski wrote:
>>> On 05.04.2024 08:08, Jiri Slaby (SUSE) wrote:
>>>> 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.
>>>>
>>>> omap needed a bit more care as it needs to put a char into FIFO to start
>>>> the DMA transfer when OMAP_DMA_TX_KICK is set. In that case, we have to
>>>> do kfifo_dma_out_prepare twice: once to find out the tx_size (to find
>>>> out if it is worths to do DMA at all -- size >= 4), the second time for
>>>> the actual transfer.
>>>>
>>>> All traces of circ_buf are removed from serial_core.h (and its struct
>>>> uart_state).
>>>>
>>>> Signed-off-by: Jiri Slaby (SUSE) <jirislaby@kernel.org>
>>>> ...
>>> This patch landed in linux-next as commit 1788cf6a91d9 ("tty: serial:
>>> switch from circ_buf to kfifo"). Unfortunately it breaks UART operation
>>> on thr Amlogic Meson based boards (drivers/tty/serial/meson_uart.c
>>> driver) and Qualcomm RB5 board (drivers/tty/serial/qcom_geni_serial.c).
>>> Once the init process is started, a complete garbage is printed to the
>>> serial console. Here is an example how it looks:
>> Oh my!
>>
>> Both drivers move the tail using both kfifo and uart_xmit_advance()
>> interfaces. Bah. Does it help to remove that uart_xmit_advance() for both of
>> them? (TX stats will be broken.)
>>
>> Users of uart_port_tx() are not affected.
>>
>> This is my fault when merging uart_xmit_advance() with this series.
>>
> I'm trying to run on two dragonboard devices db410c and db845c and both
> fails to boot see the boot failure from db845c [1], linux-next tag: next-20240415.
> I tried to apply the patch [2] (that you proposed in this thread) ontop of next-20240415. However, that didn't
> help bootlog on db845c [3].

This is a different issue, which I've reported 2 days ago. See the 
following thread:

https://lore.kernel.org/all/d3eb9f21-f3e1-43ec-bf41-984c6aa5cfc8@samsung.com/


Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

Re: [PATCH 11/15] tty: msm_serial: use dmaengine_prep_slave_sg()

[Jiri Slaby]

On 17. 04. 24, 12:15, Marek Szyprowski wrote:
> Hi Jiri,
> 
> On 16.04.2024 12:23, Jiri Slaby wrote:
>> On 15. 04. 24, 23:17, Marek Szyprowski wrote:
>>> On 05.04.2024 08:08, Jiri Slaby (SUSE) wrote:
>>>> This is a preparatory for the serial-to-kfifo switch. kfifo understands
>>>> only scatter-gatter approach, so switch to that.
>>>>
>>>> No functional change intended, it's just dmaengine_prep_slave_single()
>>>> inline expanded.
>>>>
>>>> And in this case, switch from dma_map_single() to dma_map_sg() too.
>>>> This
>>>> needs struct msm_dma changes. I split the rx and tx parts into an
>>>> union.
>>>> TX is now struct scatterlist, RX remains the old good phys-virt-count
>>>> triple.
>>>>
>>>> Signed-off-by: Jiri Slaby (SUSE) <jirislaby@kernel.org>
>>>> Cc: Bjorn Andersson <andersson@kernel.org>
>>>> Cc: Konrad Dybcio <konrad.dybcio@linaro.org>
>>>> Cc: linux-arm-msm@vger.kernel.org
>>>
>>> I've just found that this patch broke UART operation on DragonBoard
>>> 410c. I briefly checked and didn't notice anything obviously wrong here,
>>> but the board stops transmitting any data from its serial port after the
>>> first message. I will try to analyze this issue a bit more tomorrow.
>>
>> I double checked, but I see no immediate issues in the patch too. So
>> please, if you can analyze this more…
> 
> I've spent some time digging into this issue and frankly speaking I
> still have no idea WHY it doesn't work (or I seriously mixed something
> in the scatterlist principles). However I found a workaround to make it
> working. Maybe it will help a bit guessing what happens there.
...
> @@ -434,7 +436,7 @@ static void msm_start_tx(struct uart_port *port)
>           struct msm_dma *dma = &msm_port->tx_dma;
> 
>           /* Already started in DMA mode */
> -       if (sg_dma_len(&dma->tx_sg))
> +       if (dma->mapped)

Thanks for looking into this.

I was hesitant if I should use a flag. I should have, apparently.

Quick question:
What's value of CONFIG_NEED_SG_DMA_LENGTH in your .config?

thanks,
-- 
js
suse labs

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

[Anders Roxell]

On Wed, 17 Apr 2024 at 12:20, Marek Szyprowski <m.szyprowski@samsung.com> wrote:
>
> On 17.04.2024 12:08, Anders Roxell wrote:
> > On 2024-04-15 15:28, Jiri Slaby wrote:
> >> On 15. 04. 24, 14:58, Marek Szyprowski wrote:
> >>> On 05.04.2024 08:08, Jiri Slaby (SUSE) wrote:
> >>>> 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.
> >>>>
> >>>> omap needed a bit more care as it needs to put a char into FIFO to start
> >>>> the DMA transfer when OMAP_DMA_TX_KICK is set. In that case, we have to
> >>>> do kfifo_dma_out_prepare twice: once to find out the tx_size (to find
> >>>> out if it is worths to do DMA at all -- size >= 4), the second time for
> >>>> the actual transfer.
> >>>>
> >>>> All traces of circ_buf are removed from serial_core.h (and its struct
> >>>> uart_state).
> >>>>
> >>>> Signed-off-by: Jiri Slaby (SUSE) <jirislaby@kernel.org>
> >>>> ...
> >>> This patch landed in linux-next as commit 1788cf6a91d9 ("tty: serial:
> >>> switch from circ_buf to kfifo"). Unfortunately it breaks UART operation
> >>> on thr Amlogic Meson based boards (drivers/tty/serial/meson_uart.c
> >>> driver) and Qualcomm RB5 board (drivers/tty/serial/qcom_geni_serial.c).
> >>> Once the init process is started, a complete garbage is printed to the
> >>> serial console. Here is an example how it looks:
> >> Oh my!
> >>
> >> Both drivers move the tail using both kfifo and uart_xmit_advance()
> >> interfaces. Bah. Does it help to remove that uart_xmit_advance() for both of
> >> them? (TX stats will be broken.)
> >>
> >> Users of uart_port_tx() are not affected.
> >>
> >> This is my fault when merging uart_xmit_advance() with this series.
> >>
> > I'm trying to run on two dragonboard devices db410c and db845c and both
> > fails to boot see the boot failure from db845c [1], linux-next tag: next-20240415.
> > I tried to apply the patch [2] (that you proposed in this thread) ontop of next-20240415. However, that didn't
> > help bootlog on db845c [3].
>
> This is a different issue, which I've reported 2 days ago. See the
> following thread:
>
> https://lore.kernel.org/all/d3eb9f21-f3e1-43ec-bf41-984c6aa5cfc8@samsung.com/

Oh ok, I did the bisection on db845v, and that led me to this
patch 1788cf6a91d9 ("tty: serial: switch from circ_buf to kfifo")

Cheers,
Anders

Re: [PATCH 11/15] tty: msm_serial: use dmaengine_prep_slave_sg()

[Marek Szyprowski]

On 17.04.2024 12:50, Jiri Slaby wrote:
> On 17. 04. 24, 12:15, Marek Szyprowski wrote:
>> On 16.04.2024 12:23, Jiri Slaby wrote:
>>> On 15. 04. 24, 23:17, Marek Szyprowski wrote:
>>>> On 05.04.2024 08:08, Jiri Slaby (SUSE) wrote:
>>>>> This is a preparatory for the serial-to-kfifo switch. kfifo 
>>>>> understands
>>>>> only scatter-gatter approach, so switch to that.
>>>>>
>>>>> No functional change intended, it's just 
>>>>> dmaengine_prep_slave_single()
>>>>> inline expanded.
>>>>>
>>>>> And in this case, switch from dma_map_single() to dma_map_sg() too.
>>>>> This
>>>>> needs struct msm_dma changes. I split the rx and tx parts into an
>>>>> union.
>>>>> TX is now struct scatterlist, RX remains the old good phys-virt-count
>>>>> triple.
>>>>>
>>>>> Signed-off-by: Jiri Slaby (SUSE) <jirislaby@kernel.org>
>>>>> Cc: Bjorn Andersson <andersson@kernel.org>
>>>>> Cc: Konrad Dybcio <konrad.dybcio@linaro.org>
>>>>> Cc: linux-arm-msm@vger.kernel.org
>>>>
>>>> I've just found that this patch broke UART operation on DragonBoard
>>>> 410c. I briefly checked and didn't notice anything obviously wrong 
>>>> here,
>>>> but the board stops transmitting any data from its serial port 
>>>> after the
>>>> first message. I will try to analyze this issue a bit more tomorrow.
>>>
>>> I double checked, but I see no immediate issues in the patch too. So
>>> please, if you can analyze this more…
>>
>> I've spent some time digging into this issue and frankly speaking I
>> still have no idea WHY it doesn't work (or I seriously mixed something
>> in the scatterlist principles). However I found a workaround to make it
>> working. Maybe it will help a bit guessing what happens there.
> ...
>> @@ -434,7 +436,7 @@ static void msm_start_tx(struct uart_port *port)
>>           struct msm_dma *dma = &msm_port->tx_dma;
>>
>>           /* Already started in DMA mode */
>> -       if (sg_dma_len(&dma->tx_sg))
>> +       if (dma->mapped)
>
> Thanks for looking into this.
>
> I was hesitant if I should use a flag. I should have, apparently.
>
> Quick question:
> What's value of CONFIG_NEED_SG_DMA_LENGTH in your .config?


CONFIG_NEED_SG_DMA_LENGTH=y


I alse tried to change the "if (dma->mapped)" check in msm_start_tx() to:

1. if (dma->tx_sg.length)

2. if (dma->tx_sg.page_link & ~SG_PAGE_LINK_MASK)

but none of the above worked what is really strange and incomprehensible 
for me.


Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

Re: [PATCH 11/15] tty: msm_serial: use dmaengine_prep_slave_sg()

[Jiri Slaby]

On 17. 04. 24, 14:45, Marek Szyprowski wrote:
> On 17.04.2024 12:50, Jiri Slaby wrote:
>> On 17. 04. 24, 12:15, Marek Szyprowski wrote:
>>> On 16.04.2024 12:23, Jiri Slaby wrote:
>>>> On 15. 04. 24, 23:17, Marek Szyprowski wrote:
>>>>> On 05.04.2024 08:08, Jiri Slaby (SUSE) wrote:
>>>>>> This is a preparatory for the serial-to-kfifo switch. kfifo
>>>>>> understands
>>>>>> only scatter-gatter approach, so switch to that.
>>>>>>
>>>>>> No functional change intended, it's just
>>>>>> dmaengine_prep_slave_single()
>>>>>> inline expanded.
>>>>>>
>>>>>> And in this case, switch from dma_map_single() to dma_map_sg() too.
>>>>>> This
>>>>>> needs struct msm_dma changes. I split the rx and tx parts into an
>>>>>> union.
>>>>>> TX is now struct scatterlist, RX remains the old good phys-virt-count
>>>>>> triple.
>>>>>>
>>>>>> Signed-off-by: Jiri Slaby (SUSE) <jirislaby@kernel.org>
>>>>>> Cc: Bjorn Andersson <andersson@kernel.org>
>>>>>> Cc: Konrad Dybcio <konrad.dybcio@linaro.org>
>>>>>> Cc: linux-arm-msm@vger.kernel.org
>>>>>
>>>>> I've just found that this patch broke UART operation on DragonBoard
>>>>> 410c. I briefly checked and didn't notice anything obviously wrong
>>>>> here,
>>>>> but the board stops transmitting any data from its serial port
>>>>> after the
>>>>> first message. I will try to analyze this issue a bit more tomorrow.
>>>>
>>>> I double checked, but I see no immediate issues in the patch too. So
>>>> please, if you can analyze this more…
>>>
>>> I've spent some time digging into this issue and frankly speaking I
>>> still have no idea WHY it doesn't work (or I seriously mixed something
>>> in the scatterlist principles). However I found a workaround to make it
>>> working. Maybe it will help a bit guessing what happens there.
>> ...
>>> @@ -434,7 +436,7 @@ static void msm_start_tx(struct uart_port *port)
>>>            struct msm_dma *dma = &msm_port->tx_dma;
>>>
>>>            /* Already started in DMA mode */
>>> -       if (sg_dma_len(&dma->tx_sg))
>>> +       if (dma->mapped)
>>
>> Thanks for looking into this.
>>
>> I was hesitant if I should use a flag. I should have, apparently.
>>
>> Quick question:
>> What's value of CONFIG_NEED_SG_DMA_LENGTH in your .config?
> 
> 
> CONFIG_NEED_SG_DMA_LENGTH=y
> 
> 
> I alse tried to change the "if (dma->mapped)" check in msm_start_tx() to:
> 
> 1. if (dma->tx_sg.length)
> 
> 2. if (dma->tx_sg.page_link & ~SG_PAGE_LINK_MASK)
> 
> but none of the above worked what is really strange and incomprehensible
> for me.

Thanks. Neither for me. Could you add:
         {
                 static DEFINE_RATELIMIT_STATE(rs, 
DEFAULT_RATELIMIT_INTERVAL, DEFAULT_RATELIMIT_BURST);
                 if (dma->mapped != !!sg_dma_len(&dma->tx_sg) && 
__ratelimit(&rs))
                         printk_deferred(KERN_DEBUG "%s (%d): mapped=%u 
dma_len=%u\n",
                                         __func__, __LINE__, 
dma->mapped, sg_dma_len(&dma->tx_sg));
         }

before each of your 'if (dma->mapped)' to see where sg_dma_len() is 
wrong and what is its value in the bad case. I hope I did the logic right.

thanks,
-- 
js
suse labs

Re: [PATCH 11/15] tty: msm_serial: use dmaengine_prep_slave_sg()

[Jiri Slaby]

On 17. 04. 24, 14:45, Marek Szyprowski wrote:
> I alse tried to change the "if (dma->mapped)" check in msm_start_tx() to:
> 
> 1. if (dma->tx_sg.length)
> 
> 2. if (dma->tx_sg.page_link & ~SG_PAGE_LINK_MASK)
> 
> but none of the above worked what is really strange and incomprehensible
> for me.
> 

Aaaah, nevermind, what about this?

Two bugs:
1) dma_map_sg() returns the number of mapped entries. Not zero!
2) And I forgot to zero tx_sg in case of error.

--- a/drivers/tty/serial/msm_serial.c
+++ b/drivers/tty/serial/msm_serial.c
@@ -506,8 +506,8 @@ static int msm_handle_tx_dma(struct msm_port 
*msm_port, unsigned int count)
         kfifo_dma_out_prepare(&tport->xmit_fifo, &dma->tx_sg, 1, count);

         ret = dma_map_sg(port->dev, &dma->tx_sg, 1, dma->dir);
-       if (ret)
-               return ret;
+       if (!ret)
+               goto zero_out;

         dma->desc = dmaengine_prep_slave_sg(dma->chan, &dma->tx_sg, 1,
                                                 DMA_MEM_TO_DEV,
@@ -548,6 +548,7 @@ static int msm_handle_tx_dma(struct msm_port 
*msm_port, unsigned int count)
         return 0;
  unmap:
         dma_unmap_sg(port->dev, &dma->tx_sg, 1, dma->dir);
+zero_out:
         sg_init_table(&dma->tx_sg, 1);
         return ret;
  }


thanks,
-- 
js
suse labs

Re: [PATCH 11/15] tty: msm_serial: use dmaengine_prep_slave_sg()

[Jiri Slaby]

On 19. 04. 24, 9:43, Jiri Slaby wrote:
> On 17. 04. 24, 14:45, Marek Szyprowski wrote:
>> I alse tried to change the "if (dma->mapped)" check in msm_start_tx() to:
>>
>> 1. if (dma->tx_sg.length)
>>
>> 2. if (dma->tx_sg.page_link & ~SG_PAGE_LINK_MASK)
>>
>> but none of the above worked what is really strange and incomprehensible
>> for me.
>>
> 
> Aaaah, nevermind, what about this?
> 
> Two bugs:
> 1) dma_map_sg() returns the number of mapped entries. Not zero!
> 2) And I forgot to zero tx_sg in case of error.
> 
> --- a/drivers/tty/serial/msm_serial.c
> +++ b/drivers/tty/serial/msm_serial.c
> @@ -506,8 +506,8 @@ static int msm_handle_tx_dma(struct msm_port 
> *msm_port, unsigned int count)
>          kfifo_dma_out_prepare(&tport->xmit_fifo, &dma->tx_sg, 1, count);
> 
>          ret = dma_map_sg(port->dev, &dma->tx_sg, 1, dma->dir);
> -       if (ret)
> -               return ret;
> +       if (!ret)

Still wrong, ret = -EIO missing in here.

> +               goto zero_out;
> 
>          dma->desc = dmaengine_prep_slave_sg(dma->chan, &dma->tx_sg, 1,
>                                                  DMA_MEM_TO_DEV,
> @@ -548,6 +548,7 @@ static int msm_handle_tx_dma(struct msm_port 
> *msm_port, unsigned int count)
>          return 0;
>   unmap:
>          dma_unmap_sg(port->dev, &dma->tx_sg, 1, dma->dir);
> +zero_out:
>          sg_init_table(&dma->tx_sg, 1);
>          return ret;
>   }
> 
> 
> thanks,

-- 
js
suse labs

Re: [PATCH 11/15] tty: msm_serial: use dmaengine_prep_slave_sg()

[Marek Szyprowski]

On 19.04.2024 09:53, Jiri Slaby wrote:
> On 19. 04. 24, 9:43, Jiri Slaby wrote:
>> On 17. 04. 24, 14:45, Marek Szyprowski wrote:
>>> I alse tried to change the "if (dma->mapped)" check in 
>>> msm_start_tx() to:
>>>
>>> 1. if (dma->tx_sg.length)
>>>
>>> 2. if (dma->tx_sg.page_link & ~SG_PAGE_LINK_MASK)
>>>
>>> but none of the above worked what is really strange and 
>>> incomprehensible
>>> for me.
>>>
>>
>> Aaaah, nevermind, what about this?
>>
>> Two bugs:
>> 1) dma_map_sg() returns the number of mapped entries. Not zero!
>> 2) And I forgot to zero tx_sg in case of error.
>>
>> --- a/drivers/tty/serial/msm_serial.c
>> +++ b/drivers/tty/serial/msm_serial.c
>> @@ -506,8 +506,8 @@ static int msm_handle_tx_dma(struct msm_port 
>> *msm_port, unsigned int count)
>>          kfifo_dma_out_prepare(&tport->xmit_fifo, &dma->tx_sg, 1, 
>> count);
>>
>>          ret = dma_map_sg(port->dev, &dma->tx_sg, 1, dma->dir);
>> -       if (ret)
>> -               return ret;
>> +       if (!ret)
>
> Still wrong, ret = -EIO missing in here.

"if (ret <= 0)" seems to be better here.

Indeed this fixed the issue. I checked the code many times, but I missed 
this dma_map_sg() return value issue.

Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>

>
>> +               goto zero_out;
>>
>>          dma->desc = dmaengine_prep_slave_sg(dma->chan, &dma->tx_sg, 1,
>>                                                  DMA_MEM_TO_DEV,
>> @@ -548,6 +548,7 @@ static int msm_handle_tx_dma(struct msm_port 
>> *msm_port, unsigned int count)
>>          return 0;
>>   unmap:
>>          dma_unmap_sg(port->dev, &dma->tx_sg, 1, dma->dir);
>> +zero_out:
>>          sg_init_table(&dma->tx_sg, 1);
>>          return ret;
>>   }
>>
>>
>> thanks,
>
Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

Re: [PATCH 11/15] tty: msm_serial: use dmaengine_prep_slave_sg()

[Jiri Slaby]

On 19. 04. 24, 10:00, Marek Szyprowski wrote:
> On 19.04.2024 09:53, Jiri Slaby wrote:
>> On 19. 04. 24, 9:43, Jiri Slaby wrote:
>>> On 17. 04. 24, 14:45, Marek Szyprowski wrote:
>>>> I alse tried to change the "if (dma->mapped)" check in
>>>> msm_start_tx() to:
>>>>
>>>> 1. if (dma->tx_sg.length)
>>>>
>>>> 2. if (dma->tx_sg.page_link & ~SG_PAGE_LINK_MASK)
>>>>
>>>> but none of the above worked what is really strange and
>>>> incomprehensible
>>>> for me.
>>>>
>>>
>>> Aaaah, nevermind, what about this?
>>>
>>> Two bugs:
>>> 1) dma_map_sg() returns the number of mapped entries. Not zero!
>>> 2) And I forgot to zero tx_sg in case of error.
>>>
>>> --- a/drivers/tty/serial/msm_serial.c
>>> +++ b/drivers/tty/serial/msm_serial.c
>>> @@ -506,8 +506,8 @@ static int msm_handle_tx_dma(struct msm_port
>>> *msm_port, unsigned int count)
>>>           kfifo_dma_out_prepare(&tport->xmit_fifo, &dma->tx_sg, 1,
>>> count);
>>>
>>>           ret = dma_map_sg(port->dev, &dma->tx_sg, 1, dma->dir);
>>> -       if (ret)
>>> -               return ret;
>>> +       if (!ret)
>>
>> Still wrong, ret = -EIO missing in here.
> 
> "if (ret <= 0)" seems to be better here.

It returns unsigned, so I have a better patch. Will send in a second.

> Indeed this fixed the issue. I checked the code many times, but I missed
> this dma_map_sg() return value issue.

Perfect!

> Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>

Could you test that one too :)?

thanks,
-- 
js
suse labs

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

[Neil Armstrong]

Hi Jiri,

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.
> """
> 
> Cc: Al Cooper <alcooperx@gmail.com>
> Cc: Alexander Shiyan <shc_work@mail.ru>
> Cc: Alexandre Belloni <alexandre.belloni@bootlin.com>
> Cc: Alexandre Torgue <alexandre.torgue@foss.st.com>
> Cc: Alim Akhtar <alim.akhtar@samsung.com>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: "Aneesh Kumar K.V" <aneesh.kumar@kernel.org>
> Cc: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> Cc: Baolin Wang <baolin.wang@linux.alibaba.com>
> Cc: Baruch Siach <baruch@tkos.co.il>
> Cc: Bjorn Andersson <andersson@kernel.org>
> Cc: Claudiu Beznea <claudiu.beznea@tuxon.dev>
> Cc: "David S. Miller" <davem@davemloft.net>
> Cc: Fabio Estevam <festevam@gmail.com>
> Cc: Hammer Hsieh <hammerh0314@gmail.com>
> Cc: "Christian König" <christian.koenig@amd.com>
> Cc: Christophe Leroy <christophe.leroy@csgroup.eu>
> Cc: Chunyan Zhang <zhang.lyra@gmail.com>
> Cc: Jerome Brunet <jbrunet@baylibre.com>
> Cc: Jonathan Hunter <jonathanh@nvidia.com>
> Cc: Kevin Hilman <khilman@baylibre.com>
> Cc: Konrad Dybcio <konrad.dybcio@linaro.org>
> Cc: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> Cc: Kumaravel Thiagarajan <kumaravel.thiagarajan@microchip.com>
> Cc: Laxman Dewangan <ldewangan@nvidia.com>
> Cc: linux-arm-kernel@lists.infradead.org
> Cc: linux-arm-msm@vger.kernel.org
> Cc: "Maciej W. Rozycki" <macro@orcam.me.uk>
> Cc: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> Cc: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> Cc: Matthias Brugger <matthias.bgg@gmail.com>
> Cc: Maxime Coquelin <mcoquelin.stm32@gmail.com>
> Cc: Michael Ellerman <mpe@ellerman.id.au>
> Cc: Michal Simek <michal.simek@amd.com>
> Cc: "Naveen N. Rao" <naveen.n.rao@linux.ibm.com>
> Cc: Neil Armstrong <neil.armstrong@linaro.org>
> Cc: Nicolas Ferre <nicolas.ferre@microchip.com>
> Cc: Nicholas Piggin <npiggin@gmail.com>
> Cc: Orson Zhai <orsonzhai@gmail.com>
> Cc: "Pali Rohár" <pali@kernel.org>
> Cc: Patrice Chotard <patrice.chotard@foss.st.com>
> Cc: Peter Korsgaard <jacmet@sunsite.dk>
> Cc: Richard Genoud <richard.genoud@gmail.com>
> Cc: Russell King <linux@armlinux.org.uk>
> Cc: Sascha Hauer <s.hauer@pengutronix.de>
> Cc: Shawn Guo <shawnguo@kernel.org>
> Cc: Stefani Seibold <stefani@seibold.net>
> Cc: Sumit Semwal <sumit.semwal@linaro.org>
> Cc: Taichi Sugaya <sugaya.taichi@socionext.com>
> Cc: Takao Orito <orito.takao@socionext.com>
> Cc: Tharun Kumar P <tharunkumar.pasumarthi@microchip.com>
> Cc: Thierry Reding <thierry.reding@gmail.com>
> Cc: Timur Tabi <timur@kernel.org>
> Cc: Vineet Gupta <vgupta@kernel.org>
> 
> Jiri Slaby (SUSE) (15):
>    kfifo: drop __kfifo_dma_out_finish_r()
>    kfifo: introduce and use kfifo_skip_count()
>    kfifo: add kfifo_out_linear{,_ptr}()
>    kfifo: remove support for physically non-contiguous memory
>    kfifo: rename l to len_to_end in setup_sgl()
>    kfifo: pass offset to setup_sgl_buf() instead of a pointer
>    kfifo: add kfifo_dma_out_prepare_mapped()
>    kfifo: fix typos in kernel-doc
>    tty: 8250_dma: use dmaengine_prep_slave_sg()
>    tty: 8250_omap: use dmaengine_prep_slave_sg()
>    tty: msm_serial: use dmaengine_prep_slave_sg()
>    tty: serial: switch from circ_buf to kfifo
>    tty: atmel_serial: use single DMA mapping for TX
>    tty: atmel_serial: define macro for RX size
>    tty: atmel_serial: use single DMA mapping for RX
> 
>   drivers/tty/serial/8250/8250_bcm7271.c  |  14 +--
>   drivers/tty/serial/8250/8250_core.c     |   3 +-
>   drivers/tty/serial/8250/8250_dma.c      |  31 +++--
>   drivers/tty/serial/8250/8250_exar.c     |   5 +-
>   drivers/tty/serial/8250/8250_mtk.c      |   2 +-
>   drivers/tty/serial/8250/8250_omap.c     |  48 +++++---
>   drivers/tty/serial/8250/8250_pci1xxxx.c |  50 ++++----
>   drivers/tty/serial/8250/8250_port.c     |  22 ++--
>   drivers/tty/serial/amba-pl011.c         |  46 +++-----
>   drivers/tty/serial/ar933x_uart.c        |  15 ++-
>   drivers/tty/serial/arc_uart.c           |   8 +-
>   drivers/tty/serial/atmel_serial.c       | 150 +++++++++++-------------
>   drivers/tty/serial/clps711x.c           |  12 +-
>   drivers/tty/serial/cpm_uart.c           |  20 ++--
>   drivers/tty/serial/digicolor-usart.c    |  12 +-
>   drivers/tty/serial/dz.c                 |  13 +-
>   drivers/tty/serial/fsl_linflexuart.c    |  17 +--
>   drivers/tty/serial/fsl_lpuart.c         |  39 +++---
>   drivers/tty/serial/icom.c               |  25 +---
>   drivers/tty/serial/imx.c                |  54 ++++-----
>   drivers/tty/serial/ip22zilog.c          |  26 ++--
>   drivers/tty/serial/jsm/jsm_cls.c        |  29 ++---
>   drivers/tty/serial/jsm/jsm_neo.c        |  38 ++----
>   drivers/tty/serial/max3100.c            |  14 +--
>   drivers/tty/serial/max310x.c            |  35 +++---
>   drivers/tty/serial/men_z135_uart.c      |  26 ++--
>   drivers/tty/serial/meson_uart.c         |  11 +-
>   drivers/tty/serial/milbeaut_usio.c      |  15 +--
>   drivers/tty/serial/msm_serial.c         | 114 +++++++++---------
>   drivers/tty/serial/mvebu-uart.c         |   8 +-
>   drivers/tty/serial/mxs-auart.c          |  23 +---
>   drivers/tty/serial/pch_uart.c           |  21 ++--
>   drivers/tty/serial/pic32_uart.c         |  15 ++-
>   drivers/tty/serial/pmac_zilog.c         |  24 ++--
>   drivers/tty/serial/qcom_geni_serial.c   |  36 +++---
>   drivers/tty/serial/rda-uart.c           |  17 +--
>   drivers/tty/serial/samsung_tty.c        |  54 +++++----
>   drivers/tty/serial/sb1250-duart.c       |  13 +-
>   drivers/tty/serial/sc16is7xx.c          |  40 +++----
>   drivers/tty/serial/sccnxp.c             |  16 ++-
>   drivers/tty/serial/serial-tegra.c       |  43 ++++---
>   drivers/tty/serial/serial_core.c        |  56 ++++-----
>   drivers/tty/serial/serial_port.c        |   2 +-
>   drivers/tty/serial/sh-sci.c             |  51 ++++----
>   drivers/tty/serial/sprd_serial.c        |  20 ++--
>   drivers/tty/serial/st-asc.c             |   4 +-
>   drivers/tty/serial/stm32-usart.c        |  52 ++++----
>   drivers/tty/serial/sunhv.c              |  35 +++---
>   drivers/tty/serial/sunplus-uart.c       |  16 +--
>   drivers/tty/serial/sunsab.c             |  30 ++---
>   drivers/tty/serial/sunsu.c              |  15 +--
>   drivers/tty/serial/sunzilog.c           |  27 ++---
>   drivers/tty/serial/tegra-tcu.c          |  10 +-
>   drivers/tty/serial/timbuart.c           |  17 ++-
>   drivers/tty/serial/uartlite.c           |  13 +-
>   drivers/tty/serial/ucc_uart.c           |  20 ++--
>   drivers/tty/serial/xilinx_uartps.c      |  20 ++--
>   drivers/tty/serial/zs.c                 |  13 +-
>   include/linux/kfifo.h                   | 143 ++++++++++++++++------
>   include/linux/serial_core.h             |  49 +++++---
>   lib/kfifo.c                             | 107 +++++++++--------
>   61 files changed, 944 insertions(+), 960 deletions(-)
> 

This patchset has at least broken all Amlogic and Qualcomm boards so far, only part of them were fixed in next- but this serie has been merged in v1 with no serious testing and should've been dropped immediately when the first regressions were reported.

Thanks,
Neil

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

[Greg KH]

On Fri, Apr 19, 2024 at 05:12:28PM +0200, Neil Armstrong wrote:
> This patchset has at least broken all Amlogic and Qualcomm boards so
> far, only part of them were fixed in next- but this serie has been
> merged in v1 with no serious testing and should've been dropped
> immediately when the first regressions were reported.

What is not yet fixed with the recent patch that was just sent to the
list?

Doing core changes like this is hard, I have seen no lack of willingness
to fix reported problems or major breakages that would deserve a revert.

greg k-h

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

[Jiri Slaby]

Hi,

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.
>> """
...
> This patchset has at least broken all Amlogic and Qualcomm boards so 
> far, only part of them were fixed in next-

So are there still not fixed problems yet?

> but this serie has been 
> merged in v1

Ugh, are you saying that v1 patches are not worth taking? That doesn't 
fit with my experience.

> with no serious testing

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.

So no, dropping is not helping moving forward, actions taken by e.g. 
Marek Szyprowski <m.szyprowski@samsung.com> do, IMNSHO.

thanks,
-- 
js
suse labs

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

[Jiri Slaby]

On 17. 04. 24, 13:19, Anders Roxell wrote:
>> I'm trying to run on two dragonboard devices db410c and db845c and both
>>> fails to boot see the boot failure from db845c [1], linux-next tag: next-20240415.
>>> I tried to apply the patch [2] (that you proposed in this thread) ontop of next-20240415. However, that didn't
>>> help bootlog on db845c [3].
>>
>> This is a different issue, which I've reported 2 days ago. See the
>> following thread:
>>
>> https://lore.kernel.org/all/d3eb9f21-f3e1-43ec-bf41-984c6aa5cfc8@samsung.com/
> 
> Oh ok, I did the bisection on db845v, and that led me to this
> patch 1788cf6a91d9 ("tty: serial: switch from circ_buf to kfifo")

Could you re-test with the today's -next?

In particular, with this commit:
commit f70f95b485d78838ad28dbec804b986d11ad7bb0
Author: Jiri Slaby (SUSE) <jirislaby@kernel.org>
Date:   Fri Apr 19 10:09:31 2024 +0200

     serial: msm: check dma_map_sg() return value properly


thanks,
-- 
js
suse labs

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

[neil.armstrong]

Hi Jiri,

On 22/04/2024 07:51, Jiri Slaby wrote:
> Hi,
> 
> 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.
>>> """
> ...
>> This patchset has at least broken all Amlogic and Qualcomm boards so far, only part of them were fixed in next-
> 
> So are there still not fixed problems yet?

My last ci run on next-20240419 was still failing on db410c.

> 
>> but this serie has been merged in v1
> 
> Ugh, are you saying that v1 patches are not worth taking? That doesn't fit with my experience.

In my experience, most of my patches are taken in v2, it's not an uncommon thing to have more versions, especially when touching core subsystems.

> 
>> with no serious testing
> 
> 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.
> ===

This RFT was sent during the merge window, only a few people looks at the list between those 2 weeks.

> 
>> 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.

So since it was ignored, it's ok to apply it as-is ??????

> 
> So no, dropping is not helping moving forward, actions taken by e.g. Marek Szyprowski <m.szyprowski@samsung.com> do, IMNSHO.

well thanks to Marek, but most of Qualcomm maintainers and myself were in EOSS in Seattle for the week and came back home in Saturday, and we were busy. Hopefully Marek was available.

> 
> thanks,

Neil

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

[Neil Armstrong]

Hi Greg,

On 20/04/2024 07:42, Greg KH wrote:
> On Fri, Apr 19, 2024 at 05:12:28PM +0200, Neil Armstrong wrote:
>> This patchset has at least broken all Amlogic and Qualcomm boards so
>> far, only part of them were fixed in next- but this serie has been
>> merged in v1 with no serious testing and should've been dropped
>> immediately when the first regressions were reported.
> 
> What is not yet fixed with the recent patch that was just sent to the
> list?
> 
> Doing core changes like this is hard, I have seen no lack of willingness
> to fix reported problems or major breakages that would deserve a revert.

It broken all Amlogic and Qualcomm boards, are we sure it didn't break other systems that are not CI tested on -next ?

This serie clearly deserved a v2, patch 11 wasn't seriously reviewed, and it deserved a ping on the RFT before sending a v1.

I don't understand why speeding up this changeset and applying it without any reviews nor tests was so important.

Thanks,
Neil
> 
> greg k-h

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

[Anders Roxell]

On Mon, 22 Apr 2024 at 08:45, Jiri Slaby <jirislaby@kernel.org> wrote:
>
> On 17. 04. 24, 13:19, Anders Roxell wrote:
> >> I'm trying to run on two dragonboard devices db410c and db845c and both
> >>> fails to boot see the boot failure from db845c [1], linux-next tag: next-20240415.
> >>> I tried to apply the patch [2] (that you proposed in this thread) ontop of next-20240415. However, that didn't
> >>> help bootlog on db845c [3].
> >>
> >> This is a different issue, which I've reported 2 days ago. See the
> >> following thread:
> >>
> >> https://lore.kernel.org/all/d3eb9f21-f3e1-43ec-bf41-984c6aa5cfc8@samsung.com/
> >
> > Oh ok, I did the bisection on db845v, and that led me to this
> > patch 1788cf6a91d9 ("tty: serial: switch from circ_buf to kfifo")
>
> Could you re-test with the today's -next?

Tested todays next and it boots fine.

Tested-by: Anders Roxell <anders.roxell@linaro.org>

Cheers,
Anders

>
> In particular, with this commit:
> commit f70f95b485d78838ad28dbec804b986d11ad7bb0
> Author: Jiri Slaby (SUSE) <jirislaby@kernel.org>
> Date:   Fri Apr 19 10:09:31 2024 +0200
>
>      serial: msm: check dma_map_sg() return value properly
>
>
> thanks,
> --
> js
> suse labs
>

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

[Ferry Toth]

Hi,

Op 22-04-2024 om 07:51 schreef Jiri Slaby:
> Hi,
> 
> 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.
>>> """
> ...
>> This patchset has at least broken all Amlogic and Qualcomm boards so 
>> far, only part of them were fixed in next-
> 
> So are there still not fixed problems yet?
> 
>> but this serie has been merged in v1
> 
> Ugh, are you saying that v1 patches are not worth taking? That doesn't 
> fit with my experience.
> 
>> with no serious testing
> 
> 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?

So, what was the reason to "don't (quite yet)"?

> thanks,

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

[Ferry Toth]

Hi

Op 07-06-2024 om 22:32 schreef Ferry Toth:
> Hi,
> 
> Op 22-04-2024 om 07:51 schreef Jiri Slaby:
>> Hi,
>>
>> 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.
>>>> """
>> ...
>>> This patchset has at least broken all Amlogic and Qualcomm boards so 
>>> far, only part of them were fixed in next-
>>
>> So are there still not fixed problems yet?
>>
>>> but this serie has been merged in v1
>>
>> Ugh, are you saying that v1 patches are not worth taking? That doesn't 
>> fit with my experience.
>>
>>> with no serious testing
>>
>> 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?

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?

>> thanks,
> 

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

[Jiri Slaby]

On 10. 06. 24, 22:16, Ferry Toth wrote:
>> 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?
> 
> 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 any I am aware of. It just needed testers.

Please send a patch.

thanks,
-- 
js
suse labs

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

[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?
> 
> 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 
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 
is okay if it gets more than 1 sg entry (at worst, a HW-specific limit 
on nents might need to be imposed).

-- 
 i.

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

[Ferry Toth]

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>?

> 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.

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

[Ilpo Järvinen]

[-- 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.