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>, Andy Duan <fugang.duan@nxp.com>
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>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        "shawnguo@kernel.org" <shawnguo@kernel.org>,
        dl-linux-imx <linux-imx@nxp.com>
Subject: Re: [RFC PATCH 3/3] tty: serial: fsl_lpuart: fix possible console
 deadlock
Thread-Topic: [RFC PATCH 3/3] tty: serial: fsl_lpuart: fix possible console
 deadlock
Thread-Index: AQHWAgzKEoI3mdcAQEisgh3SMoTgrg==
Date:   Wed, 25 Mar 2020 12:21:49 +0000
Message-ID: <VI1PR04MB6941AD02204DF50B8D24D23AEECE0@VI1PR04MB6941.eurprd04.prod.outlook.com>
References: <VI1PR04MB69413E158203E33D42E3B3B3EEF10@VI1PR04MB6941.eurprd04.prod.outlook.com>
 <20200324184758.8204-1-michael@walle.cc>
 <20200324184758.8204-3-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: 90c51c9d-a5e3-4d69-21c0-08d7d0b7183e
X-MS-Exchange-CrossTenant-originalarrivaltime: 25 Mar 2020 12:21:49.8789
 (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: agI5BRr8Q/q1NLgXX3LxOphIBMCa3NpoFzlWJafFEGpRpvdM2bVvaFfXPvE+6KBjcXPJvl4VrnwF7Sf8I7MoBw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: VI1PR04MB5344
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 2020-03-24 8:48 PM, Michael Walle wrote:=0A=
> If the kernel console output is on this console any=0A=
> dev_{err,warn,info}() may result in a deadlock if the sport->port.lock=0A=
> spinlock is already held. This is because the _console_write() try to=0A=
> aquire this lock, too. Remove any error messages where the spinlock is=0A=
> taken or print after the lock is released.=0A=
> =0A=
> Reported-by: Leonard Crestez <leonard.crestez@nxp.com>=0A=
> Signed-off-by: Michael Walle <michael@walle.cc>=0A=
=0A=
It seems that this was an issue even before commit 159381df1442 ("tty: =0A=
serial: fsl_lpuart: fix DMA operation when using IOMMU") but these error =
=0A=
prints never triggered.=0A=
=0A=
Would it be possible to move all the dma alloc/config/prep outside the =0A=
serial port lock? As it stands this still calls into dmaengine coode and =
=0A=
that might decide to print as well.=0A=
=0A=
Really I don't think the lock needs to protect more than bits like =0A=
TDMAE/RDMAE.=0A=
=0A=
BTW: You should add more people in CC for reviews, for example =0A=
linux-imx@nxp.com is checked by a lot of people.=0A=
=0A=
> ---=0A=
>   drivers/tty/serial/fsl_lpuart.c | 35 +++++++--------------------------=
=0A=
>   1 file changed, 7 insertions(+), 28 deletions(-)=0A=
> =0A=
> diff --git a/drivers/tty/serial/fsl_lpuart.c b/drivers/tty/serial/fsl_lpu=
art.c=0A=
> index bbba298b68a4..0910308b38b1 100644=0A=
> --- a/drivers/tty/serial/fsl_lpuart.c=0A=
> +++ b/drivers/tty/serial/fsl_lpuart.c=0A=
> @@ -420,7 +420,6 @@ static void lpuart_dma_tx(struct lpuart_port *sport)=
=0A=
>   {=0A=
>   	struct circ_buf *xmit =3D &sport->port.state->xmit;=0A=
>   	struct scatterlist *sgl =3D sport->tx_sgl;=0A=
> -	struct device *dev =3D sport->port.dev;=0A=
>   	struct dma_chan *chan =3D sport->dma_tx_chan;=0A=
>   	int ret;=0A=
>   =0A=
> @@ -442,10 +441,8 @@ static void lpuart_dma_tx(struct lpuart_port *sport)=
=0A=
>   =0A=
>   	ret =3D dma_map_sg(chan->device->dev, sgl, sport->dma_tx_nents,=0A=
>   			 DMA_TO_DEVICE);=0A=
> -	if (!ret) {=0A=
> -		dev_err(dev, "DMA mapping error for TX.\n");=0A=
> +	if (!ret)=0A=
>   		return;=0A=
> -	}=0A=
>   =0A=
>   	sport->dma_tx_desc =3D dmaengine_prep_slave_sg(chan, sgl,=0A=
>   					ret, DMA_MEM_TO_DEV,=0A=
> @@ -453,7 +450,6 @@ static void lpuart_dma_tx(struct lpuart_port *sport)=
=0A=
>   	if (!sport->dma_tx_desc) {=0A=
>   		dma_unmap_sg(chan->device->dev, sgl, sport->dma_tx_nents,=0A=
>   			      DMA_TO_DEVICE);=0A=
> -		dev_err(dev, "Cannot prepare TX slave DMA!\n");=0A=
>   		return;=0A=
>   	}=0A=
>   =0A=
> @@ -520,21 +516,12 @@ static int lpuart_dma_tx_request(struct uart_port *=
port)=0A=
>   	struct lpuart_port *sport =3D container_of(port,=0A=
>   					struct lpuart_port, port);=0A=
>   	struct dma_slave_config dma_tx_sconfig =3D {};=0A=
> -	int ret;=0A=
>   =0A=
>   	dma_tx_sconfig.dst_addr =3D lpuart_dma_datareg_addr(sport);=0A=
>   	dma_tx_sconfig.dst_addr_width =3D DMA_SLAVE_BUSWIDTH_1_BYTE;=0A=
>   	dma_tx_sconfig.dst_maxburst =3D 1;=0A=
>   	dma_tx_sconfig.direction =3D DMA_MEM_TO_DEV;=0A=
> -	ret =3D dmaengine_slave_config(sport->dma_tx_chan, &dma_tx_sconfig);=0A=
> -=0A=
> -	if (ret) {=0A=
> -		dev_err(sport->port.dev,=0A=
> -				"DMA slave config failed, err =3D %d\n", ret);=0A=
> -		return ret;=0A=
> -	}=0A=
> -=0A=
> -	return 0;=0A=
> +	return dmaengine_slave_config(sport->dma_tx_chan, &dma_tx_sconfig);=0A=
>   }=0A=
>   =0A=
>   static bool lpuart_is_32(struct lpuart_port *sport)=0A=
> @@ -1074,8 +1061,8 @@ static void lpuart_copy_rx_to_tty(struct lpuart_por=
t *sport)=0A=
>   =0A=
>   	dmastat =3D dmaengine_tx_status(chan, sport->dma_rx_cookie, &state);=
=0A=
>   	if (dmastat =3D=3D DMA_ERROR) {=0A=
> -		dev_err(sport->port.dev, "Rx DMA transfer failed!\n");=0A=
>   		spin_unlock_irqrestore(&sport->port.lock, flags);=0A=
> +		dev_err(sport->port.dev, "Rx DMA transfer failed!\n");=0A=
>   		return;=0A=
>   	}=0A=
>   =0A=
> @@ -1179,23 +1166,17 @@ static inline int lpuart_start_rx_dma(struct lpua=
rt_port *sport)=0A=
>   	sg_init_one(&sport->rx_sgl, ring->buf, sport->rx_dma_rng_buf_len);=0A=
>   	nent =3D dma_map_sg(chan->device->dev, &sport->rx_sgl, 1,=0A=
>   			  DMA_FROM_DEVICE);=0A=
> -=0A=
> -	if (!nent) {=0A=
> -		dev_err(sport->port.dev, "DMA Rx mapping error\n");=0A=
> +	if (!nent)=0A=
>   		return -EINVAL;=0A=
> -	}=0A=
>   =0A=
>   	dma_rx_sconfig.src_addr =3D lpuart_dma_datareg_addr(sport);=0A=
>   	dma_rx_sconfig.src_addr_width =3D DMA_SLAVE_BUSWIDTH_1_BYTE;=0A=
>   	dma_rx_sconfig.src_maxburst =3D 1;=0A=
>   	dma_rx_sconfig.direction =3D DMA_DEV_TO_MEM;=0A=
> -	ret =3D dmaengine_slave_config(chan, &dma_rx_sconfig);=0A=
>   =0A=
> -	if (ret < 0) {=0A=
> -		dev_err(sport->port.dev,=0A=
> -				"DMA Rx slave config failed, err =3D %d\n", ret);=0A=
> +	ret =3D dmaengine_slave_config(chan, &dma_rx_sconfig);=0A=
> +	if (ret < 0)=0A=
>   		return ret;=0A=
> -	}=0A=
>   =0A=
>   	sport->dma_rx_desc =3D dmaengine_prep_dma_cyclic(chan,=0A=
>   				 sg_dma_address(&sport->rx_sgl),=0A=
> @@ -1203,10 +1184,8 @@ static inline int lpuart_start_rx_dma(struct lpuar=
t_port *sport)=0A=
>   				 sport->rx_sgl.length / 2,=0A=
>   				 DMA_DEV_TO_MEM,=0A=
>   				 DMA_PREP_INTERRUPT);=0A=
> -	if (!sport->dma_rx_desc) {=0A=
> -		dev_err(sport->port.dev, "Cannot prepare cyclic DMA\n");=0A=
> +	if (!sport->dma_rx_desc)=0A=
>   		return -EFAULT;=0A=
> -	}=0A=
>   =0A=
>   	sport->dma_rx_desc->callback =3D lpuart_dma_rx_complete;=0A=
>   	sport->dma_rx_desc->callback_param =3D sport;=0A=
> =0A=
=0A=