All of lore.kernel.org
 help / color / mirror / Atom feed
From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
To: Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	linux-serial@vger.kernel.org
Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Subject: [PATCH v1] serial: core: make static analyzer happy about locks
Date: Tue,  2 Jun 2020 17:47:39 +0300	[thread overview]
Message-ID: <20200602144739.85566-1-andriy.shevchenko@linux.intel.com> (raw)

Static analyzer can not see balanced locking if the lock is acquired and
released conditionally.

 context imbalance in 'uart_stop' - unexpected unlock
 context imbalance in 'uart_start' - different lock contexts for basic block
 context imbalance in 'uart_port_startup' - different lock contexts for basic block
 context imbalance in 'uart_shutdown' - different lock contexts for basic block
 context imbalance in 'uart_put_char' - different lock contexts for basic block
 context imbalance in 'uart_write' - different lock contexts for basic block
 context imbalance in 'uart_write_room' - different lock contexts for basic block
 context imbalance in 'uart_chars_in_buffer' - different lock contexts for basic block
 context imbalance in 'uart_flush_buffer' - different lock contexts for basic block

Get rid of macros and implement their functionality in place. This will also
help to enable runtime PM in cleaner way later on.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/tty/serial/serial_core.c | 106 +++++++++++++++++--------------
 1 file changed, 58 insertions(+), 48 deletions(-)

diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index 57840cf90388..b566127345c2 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -69,23 +69,6 @@ static inline void uart_port_deref(struct uart_port *uport)
 		wake_up(&uport->state->remove_wait);
 }
 
-#define uart_port_lock(state, flags)					\
-	({								\
-		struct uart_port *__uport = uart_port_ref(state);	\
-		if (__uport)						\
-			spin_lock_irqsave(&__uport->lock, flags);	\
-		__uport;						\
-	})
-
-#define uart_port_unlock(uport, flags)					\
-	({								\
-		struct uart_port *__uport = uport;			\
-		if (__uport) {						\
-			spin_unlock_irqrestore(&__uport->lock, flags);	\
-			uart_port_deref(__uport);			\
-		}							\
-	})
-
 static inline struct uart_port *uart_port_check(struct uart_state *state)
 {
 	lockdep_assert_held(&state->port.mutex);
@@ -113,10 +96,14 @@ static void uart_stop(struct tty_struct *tty)
 	struct uart_port *port;
 	unsigned long flags;
 
-	port = uart_port_lock(state, flags);
-	if (port)
-		port->ops->stop_tx(port);
-	uart_port_unlock(port, flags);
+	port = uart_port_ref(state);
+	if (!port)
+		return;
+
+	spin_lock_irqsave(&port->lock, flags);
+	port->ops->stop_tx(port);
+	spin_unlock_irqrestore(&port->lock, flags);
+	uart_port_deref(port);
 }
 
 static void __uart_start(struct tty_struct *tty)
@@ -134,9 +121,14 @@ static void uart_start(struct tty_struct *tty)
 	struct uart_port *port;
 	unsigned long flags;
 
-	port = uart_port_lock(state, flags);
+	port = uart_port_ref(state);
+	if (!port)
+		return;
+
+	spin_lock_irqsave(&port->lock, flags);
 	__uart_start(tty);
-	uart_port_unlock(port, flags);
+	spin_unlock_irqrestore(&port->lock, flags);
+	uart_port_deref(port);
 }
 
 static void
@@ -186,7 +178,7 @@ static int uart_port_startup(struct tty_struct *tty, struct uart_state *state,
 {
 	struct uart_port *uport = uart_port_check(state);
 	unsigned long page;
-	unsigned long flags = 0;
+	unsigned long flags;
 	int retval = 0;
 
 	if (uport->type == PORT_UNKNOWN)
@@ -205,13 +197,14 @@ static int uart_port_startup(struct tty_struct *tty, struct uart_state *state,
 	if (!page)
 		return -ENOMEM;
 
-	uart_port_lock(state, flags);
+	spin_lock_irqsave(&uport->lock, flags);
 	if (!state->xmit.buf) {
 		state->xmit.buf = (unsigned char *) page;
 		uart_circ_clear(&state->xmit);
-		uart_port_unlock(uport, flags);
+		spin_unlock_irqrestore(&uport->lock, flags);
 	} else {
-		uart_port_unlock(uport, flags);
+		spin_unlock_irqrestore(&uport->lock, flags);
+
 		/*
 		 * Do not free() the page under the port lock, see
 		 * uart_shutdown().
@@ -276,8 +269,8 @@ static void uart_shutdown(struct tty_struct *tty, struct uart_state *state)
 {
 	struct uart_port *uport = uart_port_check(state);
 	struct tty_port *port = &state->port;
-	unsigned long flags = 0;
 	char *xmit_buf = NULL;
+	unsigned long flags;
 
 	/*
 	 * Set the TTY IO error marker
@@ -311,12 +304,12 @@ static void uart_shutdown(struct tty_struct *tty, struct uart_state *state)
 	 * Do not free() the transmit buffer page under the port lock since
 	 * this can create various circular locking scenarios. For instance,
 	 * console driver may need to allocate/free a debug object, which
-	 * can endup in printk() recursion.
+	 * can end up in printk() recursion.
 	 */
-	uart_port_lock(state, flags);
+	spin_lock_irqsave(&uport->lock, flags);
 	xmit_buf = state->xmit.buf;
 	state->xmit.buf = NULL;
-	uart_port_unlock(uport, flags);
+	spin_unlock_irqrestore(&uport->lock, flags);
 
 	if (xmit_buf)
 		free_page((unsigned long)xmit_buf);
@@ -553,19 +546,19 @@ static int uart_put_char(struct tty_struct *tty, unsigned char c)
 	unsigned long flags;
 	int ret = 0;
 
-	circ = &state->xmit;
-	port = uart_port_lock(state, flags);
-	if (!circ->buf) {
-		uart_port_unlock(port, flags);
+	port = uart_port_ref(state);
+	if (!port)
 		return 0;
-	}
 
-	if (port && uart_circ_chars_free(circ) != 0) {
+	spin_lock_irqsave(&port->lock, flags);
+	circ = &state->xmit;
+	if (circ->buf && uart_circ_chars_free(circ) != 0) {
 		circ->buf[circ->head] = c;
 		circ->head = (circ->head + 1) & (UART_XMIT_SIZE - 1);
 		ret = 1;
 	}
-	uart_port_unlock(port, flags);
+	spin_unlock_irqrestore(&port->lock, flags);
+	uart_port_deref(port);
 	return ret;
 }
 
@@ -592,13 +585,17 @@ static int uart_write(struct tty_struct *tty,
 		return -EL3HLT;
 	}
 
-	port = uart_port_lock(state, flags);
+	port = uart_port_ref(state);
+	if (!port)
+		return 0;
+
+	spin_lock_irqsave(&port->lock, flags);
 	circ = &state->xmit;
 	if (!circ->buf) {
-		uart_port_unlock(port, flags);
+		spin_unlock_irqrestore(&port->lock, flags);
+		uart_port_deref(port);
 		return 0;
 	}
-
 	while (port) {
 		c = CIRC_SPACE_TO_END(circ->head, circ->tail, UART_XMIT_SIZE);
 		if (count < c)
@@ -613,7 +610,8 @@ static int uart_write(struct tty_struct *tty,
 	}
 
 	__uart_start(tty);
-	uart_port_unlock(port, flags);
+	spin_unlock_irqrestore(&port->lock, flags);
+	uart_port_deref(port);
 	return ret;
 }
 
@@ -624,9 +622,14 @@ static int uart_write_room(struct tty_struct *tty)
 	unsigned long flags;
 	int ret;
 
-	port = uart_port_lock(state, flags);
+	port = uart_port_ref(state);
+	if (!port)
+		return uart_circ_chars_free(&state->xmit);
+
+	spin_lock_irqsave(&port->lock, flags);
 	ret = uart_circ_chars_free(&state->xmit);
-	uart_port_unlock(port, flags);
+	spin_unlock_irqrestore(&port->lock, flags);
+	uart_port_deref(port);
 	return ret;
 }
 
@@ -637,9 +640,14 @@ static int uart_chars_in_buffer(struct tty_struct *tty)
 	unsigned long flags;
 	int ret;
 
-	port = uart_port_lock(state, flags);
+	port = uart_port_ref(state);
+	if (!port)
+		return uart_circ_chars_pending(&state->xmit);
+
+	spin_lock_irqsave(&port->lock, flags);
 	ret = uart_circ_chars_pending(&state->xmit);
-	uart_port_unlock(port, flags);
+	spin_unlock_irqrestore(&port->lock, flags);
+	uart_port_deref(port);
 	return ret;
 }
 
@@ -660,13 +668,15 @@ static void uart_flush_buffer(struct tty_struct *tty)
 
 	pr_debug("uart_flush_buffer(%d) called\n", tty->index);
 
-	port = uart_port_lock(state, flags);
+	port = uart_port_ref(state);
 	if (!port)
 		return;
+	spin_lock_irqsave(&port->lock, flags);
 	uart_circ_clear(&state->xmit);
 	if (port->ops->flush_buffer)
 		port->ops->flush_buffer(port);
-	uart_port_unlock(port, flags);
+	spin_unlock_irqrestore(&port->lock, flags);
+	uart_port_deref(port);
 	tty_port_tty_wakeup(&state->port);
 }
 
-- 
2.27.0.rc2


             reply	other threads:[~2020-06-02 14:47 UTC|newest]

Thread overview: 3+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-06-02 14:47 Andy Shevchenko [this message]
2020-06-12 17:08 ` [PATCH v1] serial: core: make static analyzer happy about locks Tony Lindgren
2020-06-27 11:45 ` Greg Kroah-Hartman

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200602144739.85566-1-andriy.shevchenko@linux.intel.com \
    --to=andriy.shevchenko@linux.intel.com \
    --cc=gregkh@linuxfoundation.org \
    --cc=linux-serial@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.