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