| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <59dab038c8a9adf82ee2185914c69810@walle.cc>
Date: Tue, 24 Mar 2020 17:22:34 +0100
From: Michael Walle <michael@...le.cc>
To: Leonard Crestez <leonard.crestez@....com>
Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
Shawn Guo <shawnguo@...nel.org>, linux-serial@...r.kernel.org,
linux-kernel@...r.kernel.org, Jiri Slaby <jslaby@...e.com>,
Rob Herring <robh+dt@...nel.org>,
Aisheng Dong <aisheng.dong@....com>,
Andy Duan <fugang.duan@....com>,
Vabhav Sharma <vabhav.sharma@....com>,
Andrey Smirnov <andrew.smirnov@...il.com>,
Peng Fan <peng.fan@....com>
Subject: Re: [PATCH v4 1/4] tty: serial: fsl_lpuart: fix DMA operation when
using IOMMU
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@...le.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:
>>
Powered by blists - more mailing lists