Re: [PATCH v4 1/4] tty: serial: fsl_lpuart: fix DMA operation when using IOMMU
From: Michael Walle
Date: Tue Mar 24 2020 - 12:17:46 EST
Am 2020-03-24 17:12, schrieb Andy Duan:
From: Leonard Crestez <leonard.crestez@xxxxxxx> Sent: Tuesday, March
24, 2020 11:27 PM
On 06.03.2020 23:44, Michael Walle wrote:
> The DMA channel might not be available at probe time. This is esp. the
> case if the DMA controller has an IOMMU mapping.
>
> There is also another caveat. If there is no DMA controller at all,
> dma_request_chan() will also return -EPROBE_DEFER. Thus we cannot test
> for -EPROBE_DEFER in probe(). Otherwise the lpuart driver will fail to
> probe if, for example, the DMA driver is not enabled in the kernel
> configuration.
If DMA driver is not enabled, we should disable DMA controller node in
dts file to match current sw environment, then driver doesn't do defer
probe.
So I still suggest to check -EPROBE_DEFER for
dma_request_slave_channel() in
.probe() function.
I don't know if I can follow you here. This would lead to non functional
setups,
eg. one build its own kernel with DMA disabled, but still have a device
tree
with the DMA nodes. And besides, the current workaround to request the
DMA
channel in startup() is basically working, isn't it? And once the
underlying
problem is fixed (the infinite EPROBE_DEFER), it could be moved back
into
_probe().
-michael
Andy
>
> To workaround this, we request the DMA channel in _startup(). Other
> serial drivers do it the same way.
>
> Signed-off-by: Michael Walle <michael@xxxxxxxx>
This appears to cause boot hangs on imx8qxp-mek (boards boots fine on
next-20200324 if this patch is reverted)
> ---
> drivers/tty/serial/fsl_lpuart.c | 88 +++++++++++++++++++++------------
> 1 file changed, 57 insertions(+), 31 deletions(-)
>
> diff --git a/drivers/tty/serial/fsl_lpuart.c
> b/drivers/tty/serial/fsl_lpuart.c index c31b8f3db6bf..33798df4d727
> 100644
> --- a/drivers/tty/serial/fsl_lpuart.c
> +++ b/drivers/tty/serial/fsl_lpuart.c
> @@ -1493,36 +1493,67 @@ static void rx_dma_timer_init(struct
lpuart_port *sport)
> static void lpuart_tx_dma_startup(struct lpuart_port *sport)
> {
> u32 uartbaud;
> + int ret;
>
> - if (sport->dma_tx_chan && !lpuart_dma_tx_request(&sport->port)) {
> - init_waitqueue_head(&sport->dma_wait);
> - sport->lpuart_dma_tx_use = true;
> - if (lpuart_is_32(sport)) {
> - uartbaud = lpuart32_read(&sport->port, UARTBAUD);
> - lpuart32_write(&sport->port,
> - uartbaud | UARTBAUD_TDMAE, UARTBAUD);
> - } else {
> - writeb(readb(sport->port.membase + UARTCR5) |
> - UARTCR5_TDMAS, sport->port.membase + UARTCR5);
> - }
> + sport->dma_tx_chan = dma_request_chan(sport->port.dev, "tx");
> + if (IS_ERR(sport->dma_tx_chan)) {
> + dev_info_once(sport->port.dev,
> + "DMA tx channel request failed, operating without tx
DMA (%ld)\n",
> + PTR_ERR(sport->dma_tx_chan));
It seems that this since this is called from lpuart32_startup with
&sport->port.lock held and lpuart32_console_write takes the same lock
it can
and hang.
As a workaround I can just remove this print but there are other
possible error
conditions in dmaengine code which can cause a printk.
Maybe the port lock should only be held around register manipulation?
> + sport->dma_tx_chan = NULL;
> + goto err;
> + }
> +
> + ret = lpuart_dma_tx_request(&sport->port);
> + if (!ret)
> + goto err;
This is backwards: lpuart_dma_tx_request returns negative errno on
failure.
> +
> + init_waitqueue_head(&sport->dma_wait);
> + sport->lpuart_dma_tx_use = true;
> + if (lpuart_is_32(sport)) {
> + uartbaud = lpuart32_read(&sport->port, UARTBAUD);
> + lpuart32_write(&sport->port,
> + uartbaud | UARTBAUD_TDMAE, UARTBAUD);
> } else {
> - sport->lpuart_dma_tx_use = false;
> + writeb(readb(sport->port.membase + UARTCR5) |
> + UARTCR5_TDMAS, sport->port.membase + UARTCR5);
> }
> +
> + return;
> +
> +err:
> + sport->lpuart_dma_tx_use = false;
> }
>
> static void lpuart_rx_dma_startup(struct lpuart_port *sport)
> {
> - if (sport->dma_rx_chan && !lpuart_start_rx_dma(sport)) {
> - /* set Rx DMA timeout */
> - sport->dma_rx_timeout = msecs_to_jiffies(DMA_RX_TIMEOUT);
> - if (!sport->dma_rx_timeout)
> - sport->dma_rx_timeout = 1;
> + int ret;
>
> - sport->lpuart_dma_rx_use = true;
> - rx_dma_timer_init(sport);
> - } else {
> - sport->lpuart_dma_rx_use = false;
> + sport->dma_rx_chan = dma_request_chan(sport->port.dev, "rx");
> + if (IS_ERR(sport->dma_rx_chan)) {
> + dev_info_once(sport->port.dev,
> + "DMA rx channel request failed, operating without rx
DMA (%ld)\n",
> + PTR_ERR(sport->dma_rx_chan));
> + sport->dma_rx_chan = NULL;
> + goto err;
> }
> +
> + ret = lpuart_start_rx_dma(sport);
> + if (ret)
> + goto err;
This is not backwards.
> +
> + /* set Rx DMA timeout */
> + sport->dma_rx_timeout = msecs_to_jiffies(DMA_RX_TIMEOUT);
> + if (!sport->dma_rx_timeout)
> + sport->dma_rx_timeout = 1;
> +
> + sport->lpuart_dma_rx_use = true;
> + rx_dma_timer_init(sport);
> +
> + return;
> +
> +err:
> + sport->lpuart_dma_rx_use = false;
> }
>
> static int lpuart_startup(struct uart_port *port) @@ -1615,6
> +1646,11 @@ static void lpuart_dma_shutdown(struct lpuart_port *sport)
> dmaengine_terminate_all(sport->dma_tx_chan);
> }
> }
> +
> + if (sport->dma_tx_chan)
> + dma_release_channel(sport->dma_tx_chan);
> + if (sport->dma_rx_chan)
> + dma_release_channel(sport->dma_rx_chan);
> }
>
> static void lpuart_shutdown(struct uart_port *port) @@ -2520,16
> +2556,6 @@ static int lpuart_probe(struct platform_device *pdev)
>
> sport->port.rs485_config(&sport->port, &sport->port.rs485);
>
> - sport->dma_tx_chan = dma_request_slave_channel(sport->port.dev,
"tx");
> - if (!sport->dma_tx_chan)
> - dev_info(sport->port.dev, "DMA tx channel request failed, "
> - "operating without tx DMA\n");
> -
> - sport->dma_rx_chan = dma_request_slave_channel(sport->port.dev,
"rx");
> - if (!sport->dma_rx_chan)
> - dev_info(sport->port.dev, "DMA rx channel request failed, "
> - "operating without rx DMA\n");
> -
> return 0;
>
> failed_attach_port:
>