lkml.org 
[lkml]   [2020]   [Mar]   [25]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    SubjectRE: [EXT] Re: [PATCH v4 1/4] tty: serial: fsl_lpuart: fix DMA operation when using IOMMU
    Date
    From: Michael Walle <michael@walle.cc> Sent: Wednesday, March 25, 2020 12:35 AM
    > Am 2020-03-24 17:28, schrieb Andy Duan:
    > > From: Michael Walle <michael@walle.cc> Sent: Wednesday, March 25,
    > 2020
    > > 12:18 AM
    > >> Am 2020-03-24 17:12, schrieb Andy Duan:
    > >> > From: Leonard Crestez <leonard.crestez@nxp.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.
    > >>
    > >> 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
    > >
    > > I think the user use wrong dtb file. The dtb file doesn't reflect the
    > > real enabled modules. For such case, there have many problems for
    > > syscon,... that other modules depends on them.
    >
    > But the user doesn't use the wrong dtb. I don't consider having the DMA
    > channels in the dtb makes it wrong, just because DMA is not enabled in the
    > kernel. If you'd follow that argument, then the dtb is also wrong if there is for
    > example a crypto device, although the kernel doesn't have support for it
    > enabled.
    >
    > -michael

    dma_request_chan() is not atomic context.
    Even if move it into .startup(), please move it out of spinlock context.

    >
    > >
    > > So we cannot support wrong usage cases, that is my thought.
    > >
    > > Thanks,
    > > Andy
    > >
    > >>
    > >> >
    > >> > 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@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.
    > >> >>
    > >> >> 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:
    > >> >> >

    \
     
     \ /
      Last update: 2020-03-25 05:09    [W:3.326 / U:0.536 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site