Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
From:   Andy Duan <fugang.duan@nxp.com>
To:     Michael Walle <michael@walle.cc>
CC:     Leonard Crestez <leonard.crestez@nxp.com>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Shawn Guo <shawnguo@kernel.org>,
        "linux-serial@vger.kernel.org" <linux-serial@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        Jiri Slaby <jslaby@suse.com>, Rob Herring <robh+dt@kernel.org>,
        Aisheng Dong <aisheng.dong@nxp.com>,
        Vabhav Sharma <vabhav.sharma@nxp.com>,
        Andrey Smirnov <andrew.smirnov@gmail.com>,
        Peng Fan <peng.fan@nxp.com>
Subject: RE: [EXT] Re: [PATCH v4 1/4] tty: serial: fsl_lpuart: fix DMA
 operation when using IOMMU
Thread-Topic: [EXT] Re: [PATCH v4 1/4] tty: serial: fsl_lpuart: fix DMA
 operation when using IOMMU
Thread-Index: AQHV9AB7XTSsAtbd1E+3Etbi7F0aCKhYBLIQgAACxwCAAAGMwIAAA1SAgAC/5pA=
Date:   Wed, 25 Mar 2020 04:08:23 +0000
Message-ID: <VI1PR0402MB360011B90343A8008BDB3A3AFFCE0@VI1PR0402MB3600.eurprd04.prod.outlook.com>
References: <20200306214433.23215-1-michael@walle.cc>
 <20200306214433.23215-2-michael@walle.cc>
 <VI1PR04MB69413E158203E33D42E3B3B3EEF10@VI1PR04MB6941.eurprd04.prod.outlook.com>
 <VI1PR0402MB36001AEE78E0F067D29F2816FFF10@VI1PR0402MB3600.eurprd04.prod.outlook.com>
 <725d2abdcdc0ab05cc1f03028f8a2919@walle.cc>
 <VI1PR0402MB360060ABA69BB138A33C9615FFF10@VI1PR0402MB3600.eurprd04.prod.outlook.com>
 <d9882e8769f234cff634cfdfa9a3dd33@walle.cc>
In-Reply-To: <d9882e8769f234cff634cfdfa9a3dd33@walle.cc>
Accept-Language: zh-CN, en-US
Content-Language: en-US
received-spf: None (protection.outlook.com: nxp.com does not designate
 permitted sender hosts)
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-Network-Message-Id: e106faaa-9c8e-472c-7d45-08d7d0722948
X-MS-Exchange-CrossTenant-originalarrivaltime: 25 Mar 2020 04:08:23.2450
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 686ea1d3-bc2b-4c6f-a92c-d99c5c301635
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: 9kiGPCMqo0EXGC9RCVCDfC5A2S5KGc5/zfGem1Q4s2DmoGgwdM1Z1GYS7scQc09j5QpidE7qccW5+Huj4ohAfA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: VI1PR0402MB3870
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

From: Michael Walle <michael@walle.cc> Sent: Wednesday, March 25, 2020 12:3=
5 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.
>=20
> 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 th=
e
> kernel. If you'd follow that argument, then the dtb is also wrong if ther=
e is for
> example a crypto device, although the kernel doesn't have support for it
> enabled.
>=20
> -michael

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

>=20
> >
> > 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 =3D true;
> >> >> > -          if (lpuart_is_32(sport)) {
> >> >> > -                  uartbaud =3D 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 =3D 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 print=
k.
> >> >>
> >> >> Maybe the port lock should only be held around register manipulatio=
n?
> >> >>
> >> >> > +          sport->dma_tx_chan =3D NULL;
> >> >> > +          goto err;
> >> >> > +  }
> >> >> > +
> >> >> > +  ret =3D 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 =3D true;  if (lpuart_is_32(sport)) {
> >> >> > +          uartbaud =3D lpuart32_read(&sport->port, UARTBAUD);
> >> >> > +          lpuart32_write(&sport->port,
> >> >> > +                         uartbaud | UARTBAUD_TDMAE,
> >> UARTBAUD);
> >> >> >    } else {
> >> >> > -          sport->lpuart_dma_tx_use =3D false;
> >> >> > +          writeb(readb(sport->port.membase + UARTCR5) |
> >> >> > +                 UARTCR5_TDMAS, sport->port.membase +
> >> UARTCR5);
> >> >> >    }
> >> >> > +
> >> >> > +  return;
> >> >> > +
> >> >> > +err:
> >> >> > +  sport->lpuart_dma_tx_use =3D 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 =3D
> >> msecs_to_jiffies(DMA_RX_TIMEOUT);
> >> >> > -          if (!sport->dma_rx_timeout)
> >> >> > -                  sport->dma_rx_timeout =3D 1;
> >> >> > +  int ret;
> >> >> >
> >> >> > -          sport->lpuart_dma_rx_use =3D true;
> >> >> > -          rx_dma_timer_init(sport);
> >> >> > -  } else {
> >> >> > -          sport->lpuart_dma_rx_use =3D false;
> >> >> > +  sport->dma_rx_chan =3D 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 =3D NULL;
> >> >> > +          goto err;
> >> >> >    }
> >> >> > +
> >> >> > +  ret =3D lpuart_start_rx_dma(sport);  if (ret)
> >> >> > +          goto err;
> >> >>
> >> >> This is not backwards.
> >> >>
> >> >> > +
> >> >> > +  /* set Rx DMA timeout */
> >> >> > +  sport->dma_rx_timeout =3D msecs_to_jiffies(DMA_RX_TIMEOUT);
> if
> >> >> > + (!sport->dma_rx_timeout)
> >> >> > +          sport->dma_rx_timeout =3D 1;
> >> >> > +
> >> >> > +  sport->lpuart_dma_rx_use =3D true;  rx_dma_timer_init(sport);
> >> >> > +
> >> >> > +  return;
> >> >> > +
> >> >> > +err:
> >> >> > +  sport->lpuart_dma_rx_use =3D 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 =3D
> >> >> > 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 =3D
> >> >> > 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:
> >> >> >