Message-ID: <b31d654f-07f5-7a4b-1dc2-97b30ec4dad5@linux.intel.com>
Date: Thu, 18 Jul 2024 15:07:07 +0300 (EEST)
From: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
To: Ferry Toth <ftoth@...londelft.nl>
cc: "Jiri Slaby (SUSE)" <jirislaby@...nel.org>, 
    Greg Kroah-Hartman <gregkh@...uxfoundation.org>, 
    Thomas Gleixner <tglx@...utronix.de>, 
    John Ogness <john.ogness@...utronix.de>, 
    LKML <linux-kernel@...r.kernel.org>, 
    linux-serial <linux-serial@...r.kernel.org>, neil.armstrong@...aro.org, 
    AlCooper <alcooperx@...il.com>, AlexanderShiyan <shc_work@...l.ru>, 
    AlexandreBelloni <alexandre.belloni@...tlin.com>, 
    AlexandreTorgue <alexandre.torgue@...s.st.com>, 
    AlimAkhtar <alim.akhtar@...sung.com>, 
    AndrewMorton <akpm@...ux-foundation.org>, 
    "AneeshKumarK . V" <aneesh.kumar@...nel.org>, 
    AngeloGioacchinoDelRegno <angelogioacchino.delregno@...labora.com>, 
    BaolinWang <baolin.wang@...ux.alibaba.com>, 
    BaruchSiach <baruch@...s.co.il>, BjornAndersson <andersson@...nel.org>, 
    ClaudiuBeznea <claudiu.beznea@...on.dev>, 
    "DavidS . Miller" <davem@...emloft.net>, FabioEstevam <festevam@...il.com>, 
    HammerHsieh <hammerh0314@...il.com>, 
    Christian König <christian.koenig@....com>, 
    ChristopheLeroy <christophe.leroy@...roup.eu>, 
    ChunyanZhang <zhang.lyra@...il.com>, JeromeBrunet <jbrunet@...libre.com>, 
    JonathanHunter <jonathanh@...dia.com>, KevinHilman <khilman@...libre.com>, 
    KonradDybcio <konrad.dybcio@...aro.org>, 
    KrzysztofKozlowski <krzysztof.kozlowski@...aro.org>, 
    KumaravelThiagarajan <kumaravel.thiagarajan@...rochip.com>, 
    LaxmanDewangan <ldewangan@...dia.com>, 
    linux-arm-kernel@...ts.infradead.org, linux-arm-msm@...r.kernel.org, 
    "MaciejW . Rozycki" <macro@...am.me.uk>, 
    ManivannanSadhasivam <manivannan.sadhasivam@...aro.org>, 
    MartinBlumenstingl <martin.blumenstingl@...glemail.com>, 
    MatthiasBrugger <matthias.bgg@...il.com>, 
    MaximeCoquelin <mcoquelin.stm32@...il.com>, 
    MichaelEllerman <mpe@...erman.id.au>, MichalSimek <michal.simek@....com>, 
    "NaveenN . Rao" <naveen.n.rao@...ux.ibm.com>, 
    NicolasFerre <nicolas.ferre@...rochip.com>, 
    NicholasPiggin <npiggin@...il.com>, OrsonZhai <orsonzhai@...il.com>, 
    Pali Rohár <pali@...nel.org>, 
    PatriceChotard <patrice.chotard@...s.st.com>, 
    PeterKorsgaard <jacmet@...site.dk>, 
    RichardGenoud <richard.genoud@...il.com>, 
    RussellKing <linux@...linux.org.uk>, SaschaHauer <s.hauer@...gutronix.de>, 
    ShawnGuo <shawnguo@...nel.org>, StefaniSeibold <stefani@...bold.net>, 
    SumitSemwal <sumit.semwal@...aro.org>, 
    TaichiSugaya <sugaya.taichi@...ionext.com>, 
    TakaoOrito <orito.takao@...ionext.com>, 
    TharunKumarP <tharunkumar.pasumarthi@...rochip.com>, 
    ThierryReding <thierry.reding@...il.com>, TimurTabi <timur@...nel.org>, 
    VineetGupta <vgupta@...nel.org>, 
    MarekSzyprowski <m.szyprowski@...sung.com>, 
    PhilEdworthy <phil.edworthy@...esas.com>
Subject: Re: [PATCH v1 1/1] tty: serial: 8250_dma: use sgl with 2 nents to
 take care of buffer wrap

On Tue, 16 Jul 2024, Ferry Toth wrote:

> Previously 8250_dma used a circular xmit->buf as DMA output buffer. This
> causes messages that wrap around in the circular buffer to be
> transmitted using 2 DMA transfers. Depending on baud rate and processor
> load this can cause an interchar gap in the middle of the message. On
> the receiving end the gap may cause a short receive timeout, possibly
> long enough to terminate a DMA transfer, but too short to restart a
> receive DMA transfer in time thus causing a receive buffer overrun.
> 
> This is especially a problem for devices with high speed UARTs (HSU)
> where even deep 64 byte FIFO's are not sufficient to handle interrupt
> latency.
> 
> The circular buffer has now been replaced by kfifo which requires a SG
> list with a single entry, which still causes 2 dma transfers when a wrap
> around occurs. Fix this by allowing up to 2 entries in the sgl.
> 
> Reviewed-by: Jiri Slaby <jirislaby@...nel.org>
> Signed-off-by: Ferry Toth <ftoth@...londelft.nl>
> ---
>  drivers/tty/serial/8250/8250_dma.c | 19 ++++++++++---------
>  1 file changed, 10 insertions(+), 9 deletions(-)
> 
> 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) {

Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>

I envision though that this would be part of serial core so that it's not 
limited to 8250 DMA but that can be changed on top of this at a later 
time.

-- 
 i.

Powered by blists - more mailing lists