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

[Michael Walle <michael@walle.cc>]

Hi Andy,

Am 2020-03-25 05:08, schrieb Andy Duan:
> 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.

Agreed. I'll send a respin of my fixes patches later.

-michael

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