Re: [PATCH v4 1/4] tty: serial: fsl_lpuart: fix DMA operation when using IOMMU

[Michael Walle <michael@walle.cc>]

Am 2020-03-24 16:27, schrieb Leonard Crestez:
> 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.
>> 
>> To workaround this, we request the DMA channel in _startup(). Other
>> serial drivers do it the same way.
>> 
>> Signed-off-by: Michael Walle <michael@walle.cc>
> 
> 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.

Shoot.
So basically, you cannot do any dev_dbg/info/warn/err/pr_* when you
the lock is taken (which could also be taken in serial_core.c), correct?

Because thats all over the place.. just happens now because there is a
dev_info() by default.

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

nice catch!

-michael

> 
>> +
>> +	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:
>>