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