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:   Leonard Crestez <leonard.crestez@nxp.com>
To:     Michael Walle <michael@walle.cc>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Shawn Guo <shawnguo@kernel.org>
CC:     "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>,
        Andy Duan <fugang.duan@nxp.com>,
        Vabhav Sharma <vabhav.sharma@nxp.com>,
        Andrey Smirnov <andrew.smirnov@gmail.com>,
        Peng Fan <peng.fan@nxp.com>
Subject: Re: [PATCH v4 1/4] tty: serial: fsl_lpuart: fix DMA operation when
 using IOMMU
Thread-Topic: [PATCH v4 1/4] tty: serial: fsl_lpuart: fix DMA operation when
 using IOMMU
Thread-Index: AQHV9AB7XTSsAtbd1E+3Etbi7F0aCA==
Date:   Tue, 24 Mar 2020 15:27:06 +0000
Message-ID: <VI1PR04MB69413E158203E33D42E3B3B3EEF10@VI1PR04MB6941.eurprd04.prod.outlook.com>
References: <20200306214433.23215-1-michael@walle.cc>
 <20200306214433.23215-2-michael@walle.cc>
Accept-Language: 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: 86f55109-45cb-4eb7-4f52-08d7d007cfe0
X-MS-Exchange-CrossTenant-originalarrivaltime: 24 Mar 2020 15:27:06.5892
 (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: ys88zf+e90w92HGJfF4+Yu1CeK2UVLCCINAV8TnmRj6woSN4mpmnPB8Eg1v86eDxOEfXpP5RaO1mWok5Mzvvjw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: VI1PR04MB5741
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On 06.03.2020 23:44, Michael Walle wrote:=0A=
> The DMA channel might not be available at probe time. This is esp. the=0A=
> case if the DMA controller has an IOMMU mapping.=0A=
> =0A=
> There is also another caveat. If there is no DMA controller at all,=0A=
> dma_request_chan() will also return -EPROBE_DEFER. Thus we cannot test=0A=
> for -EPROBE_DEFER in probe(). Otherwise the lpuart driver will fail to=0A=
> probe if, for example, the DMA driver is not enabled in the kernel=0A=
> configuration.=0A=
> =0A=
> To workaround this, we request the DMA channel in _startup(). Other=0A=
> serial drivers do it the same way.=0A=
> =0A=
> Signed-off-by: Michael Walle <michael@walle.cc>=0A=
=0A=
This appears to cause boot hangs on imx8qxp-mek (boards boots fine on =0A=
next-20200324 if this patch is reverted)=0A=
=0A=
> ---=0A=
>   drivers/tty/serial/fsl_lpuart.c | 88 +++++++++++++++++++++------------=
=0A=
>   1 file changed, 57 insertions(+), 31 deletions(-)=0A=
> =0A=
> diff --git a/drivers/tty/serial/fsl_lpuart.c b/drivers/tty/serial/fsl_lpu=
art.c=0A=
> index c31b8f3db6bf..33798df4d727 100644=0A=
> --- a/drivers/tty/serial/fsl_lpuart.c=0A=
> +++ b/drivers/tty/serial/fsl_lpuart.c=0A=
> @@ -1493,36 +1493,67 @@ static void rx_dma_timer_init(struct lpuart_port =
*sport)=0A=
>   static void lpuart_tx_dma_startup(struct lpuart_port *sport)=0A=
>   {=0A=
>   	u32 uartbaud;=0A=
> +	int ret;=0A=
>   =0A=
> -	if (sport->dma_tx_chan && !lpuart_dma_tx_request(&sport->port)) {=0A=
> -		init_waitqueue_head(&sport->dma_wait);=0A=
> -		sport->lpuart_dma_tx_use =3D true;=0A=
> -		if (lpuart_is_32(sport)) {=0A=
> -			uartbaud =3D lpuart32_read(&sport->port, UARTBAUD);=0A=
> -			lpuart32_write(&sport->port,=0A=
> -				       uartbaud | UARTBAUD_TDMAE, UARTBAUD);=0A=
> -		} else {=0A=
> -			writeb(readb(sport->port.membase + UARTCR5) |=0A=
> -				UARTCR5_TDMAS, sport->port.membase + UARTCR5);=0A=
> -		}=0A=
> +	sport->dma_tx_chan =3D dma_request_chan(sport->port.dev, "tx");=0A=
> +	if (IS_ERR(sport->dma_tx_chan)) {=0A=
> +		dev_info_once(sport->port.dev,=0A=
> +			      "DMA tx channel request failed, operating without tx DMA (%ld)\=
n",=0A=
> +			      PTR_ERR(sport->dma_tx_chan));=0A=
=0A=
It seems that this since this is called from lpuart32_startup with =0A=
&sport->port.lock held and lpuart32_console_write takes the same lock it =
=0A=
can and hang.=0A=
=0A=
As a workaround I can just remove this print but there are other =0A=
possible error conditions in dmaengine code which can cause a printk.=0A=
=0A=
Maybe the port lock should only be held around register manipulation?=0A=
=0A=
> +		sport->dma_tx_chan =3D NULL;=0A=
> +		goto err;=0A=
> +	}=0A=
> +=0A=
> +	ret =3D lpuart_dma_tx_request(&sport->port);=0A=
> +	if (!ret)=0A=
> +		goto err;=0A=
=0A=
This is backwards: lpuart_dma_tx_request returns negative errno on failure.=
=0A=
=0A=
> +=0A=
> +	init_waitqueue_head(&sport->dma_wait);=0A=
> +	sport->lpuart_dma_tx_use =3D true;=0A=
> +	if (lpuart_is_32(sport)) {=0A=
> +		uartbaud =3D lpuart32_read(&sport->port, UARTBAUD);=0A=
> +		lpuart32_write(&sport->port,=0A=
> +			       uartbaud | UARTBAUD_TDMAE, UARTBAUD);=0A=
>   	} else {=0A=
> -		sport->lpuart_dma_tx_use =3D false;=0A=
> +		writeb(readb(sport->port.membase + UARTCR5) |=0A=
> +		       UARTCR5_TDMAS, sport->port.membase + UARTCR5);=0A=
>   	}=0A=
> +=0A=
> +	return;=0A=
> +=0A=
> +err:=0A=
> +	sport->lpuart_dma_tx_use =3D false;=0A=
>   }=0A=
>   =0A=
>   static void lpuart_rx_dma_startup(struct lpuart_port *sport)=0A=
>   {=0A=
> -	if (sport->dma_rx_chan && !lpuart_start_rx_dma(sport)) {=0A=
> -		/* set Rx DMA timeout */=0A=
> -		sport->dma_rx_timeout =3D msecs_to_jiffies(DMA_RX_TIMEOUT);=0A=
> -		if (!sport->dma_rx_timeout)=0A=
> -			sport->dma_rx_timeout =3D 1;=0A=
> +	int ret;=0A=
>   =0A=
> -		sport->lpuart_dma_rx_use =3D true;=0A=
> -		rx_dma_timer_init(sport);=0A=
> -	} else {=0A=
> -		sport->lpuart_dma_rx_use =3D false;=0A=
> +	sport->dma_rx_chan =3D dma_request_chan(sport->port.dev, "rx");=0A=
> +	if (IS_ERR(sport->dma_rx_chan)) {=0A=
> +		dev_info_once(sport->port.dev,=0A=
> +			      "DMA rx channel request failed, operating without rx DMA (%ld)\=
n",=0A=
> +			      PTR_ERR(sport->dma_rx_chan));=0A=
> +		sport->dma_rx_chan =3D NULL;=0A=
> +		goto err;=0A=
>   	}=0A=
> +=0A=
> +	ret =3D lpuart_start_rx_dma(sport);=0A=
> +	if (ret)=0A=
> +		goto err;=0A=
=0A=
This is not backwards.=0A=
=0A=
> +=0A=
> +	/* set Rx DMA timeout */=0A=
> +	sport->dma_rx_timeout =3D msecs_to_jiffies(DMA_RX_TIMEOUT);=0A=
> +	if (!sport->dma_rx_timeout)=0A=
> +		sport->dma_rx_timeout =3D 1;=0A=
> +=0A=
> +	sport->lpuart_dma_rx_use =3D true;=0A=
> +	rx_dma_timer_init(sport);=0A=
> +=0A=
> +	return;=0A=
> +=0A=
> +err:=0A=
> +	sport->lpuart_dma_rx_use =3D false;=0A=
>   }=0A=
>   =0A=
>   static int lpuart_startup(struct uart_port *port)=0A=
> @@ -1615,6 +1646,11 @@ static void lpuart_dma_shutdown(struct lpuart_port=
 *sport)=0A=
>   			dmaengine_terminate_all(sport->dma_tx_chan);=0A=
>   		}=0A=
>   	}=0A=
> +=0A=
> +	if (sport->dma_tx_chan)=0A=
> +		dma_release_channel(sport->dma_tx_chan);=0A=
> +	if (sport->dma_rx_chan)=0A=
> +		dma_release_channel(sport->dma_rx_chan);=0A=
>   }=0A=
>   =0A=
>   static void lpuart_shutdown(struct uart_port *port)=0A=
> @@ -2520,16 +2556,6 @@ static int lpuart_probe(struct platform_device *pd=
ev)=0A=
>   =0A=
>   	sport->port.rs485_config(&sport->port, &sport->port.rs485);=0A=
>   =0A=
> -	sport->dma_tx_chan =3D dma_request_slave_channel(sport->port.dev, "tx")=
;=0A=
> -	if (!sport->dma_tx_chan)=0A=
> -		dev_info(sport->port.dev, "DMA tx channel request failed, "=0A=
> -				"operating without tx DMA\n");=0A=
> -=0A=
> -	sport->dma_rx_chan =3D dma_request_slave_channel(sport->port.dev, "rx")=
;=0A=
> -	if (!sport->dma_rx_chan)=0A=
> -		dev_info(sport->port.dev, "DMA rx channel request failed, "=0A=
> -				"operating without rx DMA\n");=0A=
> -=0A=
>   	return 0;=0A=
>   =0A=
>   failed_attach_port:=0A=
> =0A=
=0A=