Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932202AbbHDHCw (ORCPT ); Tue, 4 Aug 2015 03:02:52 -0400 Received: from mx2.suse.de ([195.135.220.15]:43202 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932103AbbHDHCu convert rfc822-to-8bit (ORCPT ); Tue, 4 Aug 2015 03:02:50 -0400 From: Johannes Thumshirn To: Peter Hurley Cc: Greg Kroah-Hartman , Jiri Slaby , linux-serial@vger.kernel.org, linux-kernel@vger.kernel.org, Andreas Werner Subject: Re: [PATCH] tty: serial: men_z135_uart.c: Fix race between IRQ and set_termios() References: <1438610285-1441-1-git-send-email-jthumshirn@suse.de> <55BF893C.1000901@hurleysoftware.com> Date: Tue, 04 Aug 2015 09:02:42 +0200 In-Reply-To: <55BF893C.1000901@hurleysoftware.com> (Peter Hurley's message of "Mon, 03 Aug 2015 11:31:08 -0400") Message-ID: User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.5 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8BIT Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3318 Lines: 94 Hi Peter, Peter Hurley writes: > Hi Johannes, > > On 08/03/2015 09:58 AM, Johannes Thumshirn wrote: >> Fix panic caused by a race between men_z135_intr() and men_z135_set_termios(). >> >> men_z135_intr() and men_z135_set_termios() both hold the struct uart_port::lock >> spinlock, but men_z135_intr() does a spin_lock_irqsave() and >> men_z135_set_termios() does a normal spin_lock(), which can lead to a deadlock >> when an interrupt is called while the lock is being helt by >> men_z135_set_termios(). > > > The irq handler can and should use normal spin_lock()/unlock(). I always thought an irq handler _must_ use the irqsave versions. Good to know that. > > The set_termios() method should used spin_lock_irq(); there's no need to save the > interrupt state because that method will never be called from interrupt context. > > So the 'flags' local can be dropped from the patch. Given that the irqsave variant isn't needed that sounds reasonable. > > Also, the port lock is already initialized in uart_add_one_port() and should > not be initialized by the probe() function. OK, do you prefer (or better Greg and Jiri) prefer that change folded into this patch or an extra patch? > > Regards, > Peter Hurley > >> This was discovered using a insmod, hardware looppback send/receive, rmmod >> stress test. >> >> Signed-off-by: Johannes Thumshirn >> Cc: Andreas Werner >> --- >> drivers/tty/serial/men_z135_uart.c | 5 +++-- >> 1 file changed, 3 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/tty/serial/men_z135_uart.c b/drivers/tty/serial/men_z135_uart.c >> index 35c5550..d020435 100644 >> --- a/drivers/tty/serial/men_z135_uart.c >> +++ b/drivers/tty/serial/men_z135_uart.c >> @@ -656,6 +656,7 @@ static void men_z135_set_termios(struct uart_port *port, >> struct ktermios *old) >> { >> struct men_z135_port *uart = to_men_z135(port); >> + unsigned long flags; >> unsigned int baud; >> u32 conf_reg; >> u32 bd_reg; >> @@ -717,7 +718,7 @@ static void men_z135_set_termios(struct uart_port *port, >> >> baud = uart_get_baud_rate(port, termios, old, 0, uart_freq / 16); >> >> - spin_lock(&port->lock); >> + spin_lock_irqsave(&port->lock, flags); >> if (tty_termios_baud_rate(termios)) >> tty_termios_encode_baud_rate(termios, baud, baud); >> >> @@ -725,7 +726,7 @@ static void men_z135_set_termios(struct uart_port *port, >> iowrite32(bd_reg, port->membase + MEN_Z135_BAUD_REG); >> >> uart_update_timeout(port, termios->c_cflag, baud); >> - spin_unlock(&port->lock); >> + spin_unlock_irqrestore(&port->lock, flags); >> } >> >> static const char *men_z135_type(struct uart_port *port) >> > -- Johannes Thumshirn Storage jthumshirn@suse.de +49 911 74053 689 SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton HRB 21284 (AG Nürnberg) Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/