| 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: <VI1PR0402MB36001AEE78E0F067D29F2816FFF10@VI1PR0402MB3600.eurprd04.prod.outlook.com>
Date: Tue, 24 Mar 2020 16:12:04 +0000
From: Andy Duan <fugang.duan@....com>
To: Leonard Crestez <leonard.crestez@....com>,
Michael Walle <michael@...le.cc>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
Shawn Guo <shawnguo@...nel.org>
CC: "linux-serial@...r.kernel.org" <linux-serial@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
Jiri Slaby <jslaby@...e.com>, Rob Herring <robh+dt@...nel.org>,
Aisheng Dong <aisheng.dong@....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
From: Leonard Crestez <leonard.crestez@....com> 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.
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@...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.
>
> 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:
> >
Powered by blists - more mailing lists