From patchwork Mon Jan 20 21:12:32 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: =?utf-8?q?Uwe_Kleine-K=C3=B6nig?= X-Patchwork-Id: 11342831 Return-Path: Received: from mail.kernel.org (pdx-korg-mail-1.web.codeaurora.org [172.30.200.123]) by pdx-korg-patchwork-2.web.codeaurora.org (Postfix) with ESMTP id 4DE8D1580 for ; Mon, 20 Jan 2020 21:12:51 +0000 (UTC) Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 2694622527 for ; Mon, 20 Jan 2020 21:12:51 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="EE9IAhjJ" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 2694622527 Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=pengutronix.de Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-arm-kernel-bounces+patchwork-linux-arm=patchwork.kernel.org@lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20170209; h=Sender: Content-Transfer-Encoding:Content-Type:Cc:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:MIME-Version:Message-Id:Date:Subject:To :From:Reply-To:Content-ID:Content-Description:Resent-Date:Resent-From: Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:In-Reply-To:References: List-Owner; bh=nqXzsa84iK2IGrS71WnNbGVnF6fAyJufHu1VRKDjFzs=; b=EE9IAhjJibMGx2 b/7QGq2xaq+uSDSDakf5eVcjTJ4M1cDKgJ5ihOUUcgNwFP13DgpJPoDYF/pgwdC/pHqwaC0PG6ptw pSOCcPebd2BDRNuQQ4QcXaVnrUJivJkgTj5KEm0qettXB96EaUg2Y2Mu3ijtAlrMKrJZnK5FAfuha vQmA0+m2L8Lhr7kRhCSAv5SViAkYKWaCFcnlGdeciaNSpEvjmsPx+tDUL8DUtf/jZA3ekWs4F2Lcq O2ijvN29t03NJIaRmDbTSUfu6lA11cCi+5IowDLImqoMSnbCyirMnx/OHxEcbMcB2mvSjeyPKE5gK qH5/rYlGz+EAEM1i7Qbw==; Received: from localhost ([127.0.0.1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1iteLo-0007CG-Hz; Mon, 20 Jan 2020 21:12:48 +0000 Received: from metis.ext.pengutronix.de ([2001:67c:670:201:290:27ff:fe1d:cc33]) by bombadil.infradead.org with esmtps (Exim 4.92.3 #3 (Red Hat Linux)) id 1iteLl-0007Bq-JX for linux-arm-kernel@lists.infradead.org; Mon, 20 Jan 2020 21:12:46 +0000 Received: from pty.hi.pengutronix.de ([2001:67c:670:100:1d::c5]) by metis.ext.pengutronix.de with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1iteLh-0006um-7f; Mon, 20 Jan 2020 22:12:41 +0100 Received: from ukl by pty.hi.pengutronix.de with local (Exim 4.89) (envelope-from ) id 1iteLc-0002MJ-EP; Mon, 20 Jan 2020 22:12:36 +0100 From: =?utf-8?q?Uwe_Kleine-K=C3=B6nig?= To: Greg Kroah-Hartman , Andre Renaud , Fabio Estevam , Andy Duan Subject: [PATCH] serial: imx: fix a race condition in receive path Date: Mon, 20 Jan 2020 22:12:32 +0100 Message-Id: <20200120211232.21329-1-u.kleine-koenig@pengutronix.de> X-Mailer: git-send-email 2.24.0 MIME-Version: 1.0 X-SA-Exim-Connect-IP: 2001:67c:670:100:1d::c5 X-SA-Exim-Mail-From: ukl@pengutronix.de X-SA-Exim-Scanned: No (on metis.ext.pengutronix.de); SAEximRunCond expanded to false X-PTX-Original-Recipient: linux-arm-kernel@lists.infradead.org X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20200120_131245_643592_93432C63 X-CRM114-Status: GOOD ( 15.74 ) X-Spam-Score: 0.0 (/) X-Spam-Report: SpamAssassin version 3.4.3 on bombadil.infradead.org summary: Content analysis details: (0.0 points) pts rule name description ---- ---------------------- -------------------------------------------------- -0.0 SPF_PASS SPF: sender matches SPF record 0.0 SPF_HELO_NONE SPF: HELO does not publish an SPF Record X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: linux-arm-kernel@lists.infradead.org, stable@vger.kernel.org, linux-imx@nxp.com, kernel@pengutronix.de, linux-serial@vger.kernel.org Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+patchwork-linux-arm=patchwork.kernel.org@lists.infradead.org The main irq handler function starts by first masking disabled interrupts in the status register values to ensure to only handle enabled interrupts. This is important as when the RX path in the hardware is disabled reading the RX fifo results in an external abort. This checking must be done under the port lock, otherwise the following can happen: CPU1 | CPU2 | irq triggers as there are chars | in the RX fifo | | grab port lock imx_uart_int finds RRDY enabled | and calls imx_uart_rxint which | has to wait for port lock | | disable RX (e.g. because we're | using RS485 with !RX_DURING_TX) | | release port lock read from RX fifo with RX | disabled => exception | So take the port lock only once in imx_uart_int() instead of in the functions called from there. Reported-by: Andre Renaud Cc: stable@vger.kernel.org Signed-off-by: Uwe Kleine-König --- Hello, this problem type was addressed already in commits 437768962f75 ("serial: imx: Only handle irqs that are actually enabled") 76821e222c18 ("serial: imx: ensure that RX irqs are off if RX is off") that entered 4.17-rc1. Backporting to older versions would require to backport these two, too. I didn't try that, but I think this gets messy, so I'd recommend to only backport to 4.19.x and 5.4.x (and 5.5.x assuming this patch won't make it into 5.5). Andre Renaud tested this patch and confirmed it to fix the problem, he didn't provide a Tested-by tag, so I didn't add that here. Best regards Uwe drivers/tty/serial/imx.c | 52 ++++++++++++++++++++++++++++++---------- 1 file changed, 39 insertions(+), 13 deletions(-) diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c index a9e20e6c63ad..679b2de27c4d 100644 --- a/drivers/tty/serial/imx.c +++ b/drivers/tty/serial/imx.c @@ -700,22 +700,33 @@ static void imx_uart_start_tx(struct uart_port *port) } } -static irqreturn_t imx_uart_rtsint(int irq, void *dev_id) +static irqreturn_t __imx_uart_rtsint(int irq, void *dev_id) { struct imx_port *sport = dev_id; u32 usr1; - spin_lock(&sport->port.lock); - imx_uart_writel(sport, USR1_RTSD, USR1); usr1 = imx_uart_readl(sport, USR1) & USR1_RTSS; uart_handle_cts_change(&sport->port, !!usr1); wake_up_interruptible(&sport->port.state->port.delta_msr_wait); - spin_unlock(&sport->port.lock); return IRQ_HANDLED; } +static irqreturn_t imx_uart_rtsint(int irq, void *dev_id) +{ + struct imx_port *sport = dev_id; + irqreturn_t ret; + + spin_lock(&sport->port.lock); + + ret = __imx_uart_rtsint(irq, dev_id); + + spin_unlock(&sport->port.lock); + + return ret; +} + static irqreturn_t imx_uart_txint(int irq, void *dev_id) { struct imx_port *sport = dev_id; @@ -726,14 +737,12 @@ static irqreturn_t imx_uart_txint(int irq, void *dev_id) return IRQ_HANDLED; } -static irqreturn_t imx_uart_rxint(int irq, void *dev_id) +static irqreturn_t __imx_uart_rxint(int irq, void *dev_id) { struct imx_port *sport = dev_id; unsigned int rx, flg, ignored = 0; struct tty_port *port = &sport->port.state->port; - spin_lock(&sport->port.lock); - while (imx_uart_readl(sport, USR2) & USR2_RDR) { u32 usr2; @@ -792,11 +801,26 @@ static irqreturn_t imx_uart_rxint(int irq, void *dev_id) } out: - spin_unlock(&sport->port.lock); tty_flip_buffer_push(port); + return IRQ_HANDLED; } +static irqreturn_t imx_uart_rxint(int irq, void *dev_id) +{ + struct imx_port *sport = dev_id; + struct tty_port *port = &sport->port.state->port; + irqreturn_t ret; + + spin_lock(&sport->port.lock); + + ret = __imx_uart_rxint(irq, dev_id); + + spin_unlock(&sport->port.lock); + + return ret; +} + static void imx_uart_clear_rx_errors(struct imx_port *sport); /* @@ -855,6 +879,8 @@ static irqreturn_t imx_uart_int(int irq, void *dev_id) unsigned int usr1, usr2, ucr1, ucr2, ucr3, ucr4; irqreturn_t ret = IRQ_NONE; + spin_lock(&sport->port.lock); + usr1 = imx_uart_readl(sport, USR1); usr2 = imx_uart_readl(sport, USR2); ucr1 = imx_uart_readl(sport, UCR1); @@ -888,27 +914,25 @@ static irqreturn_t imx_uart_int(int irq, void *dev_id) usr2 &= ~USR2_ORE; if (usr1 & (USR1_RRDY | USR1_AGTIM)) { - imx_uart_rxint(irq, dev_id); + __imx_uart_rxint(irq, dev_id); ret = IRQ_HANDLED; } if ((usr1 & USR1_TRDY) || (usr2 & USR2_TXDC)) { - imx_uart_txint(irq, dev_id); + imx_uart_transmit_buffer(sport); ret = IRQ_HANDLED; } if (usr1 & USR1_DTRD) { imx_uart_writel(sport, USR1_DTRD, USR1); - spin_lock(&sport->port.lock); imx_uart_mctrl_check(sport); - spin_unlock(&sport->port.lock); ret = IRQ_HANDLED; } if (usr1 & USR1_RTSD) { - imx_uart_rtsint(irq, dev_id); + __imx_uart_rtsint(irq, dev_id); ret = IRQ_HANDLED; } @@ -923,6 +947,8 @@ static irqreturn_t imx_uart_int(int irq, void *dev_id) ret = IRQ_HANDLED; } + spin_unlock(&sport->port.lock); + return ret; }