| Message ID | 20240405060826.2521-1-jirislaby@kernel.org (mailing list archive) |
|---|---|
| Headers | show |
| Series | tty: serial: switch from circ_buf to kfifo | expand |
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
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,
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
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
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
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
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
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
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,
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,
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
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
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 >
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,
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, >
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,
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).
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.
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.
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(-)