[PATCH 0/3] tty: serial_core: little cleanup

[PATCH 0/3] tty: serial_core: little cleanup

[Jiri Slaby (SUSE)]

Patches in this series are NOT supposed to change the behaviour. They
are suppose to make the reader more happy when reading the code.

Jiri Slaby (SUSE) (3):
  tty: serial_core: use more guard(mutex)
  tty: serial: get rid of exit label from uart_set_info()
  tty: serial: extract uart_change_port() from uart_set_info()

 drivers/tty/serial/serial_core.c | 261 ++++++++++++++-----------------
 1 file changed, 114 insertions(+), 147 deletions(-)

-- 
2.47.1

[PATCH 1/3] tty: serial_core: use more guard(mutex)

[Jiri Slaby (SUSE)]

Simplify 4 more functions using guard(mutex): uart_get_info(),
console_store(), serial_core_add_one_port(), and
serial_core_register_port(). Especially console_store() is now much less
convoluted. In the others, we save some goto-s and even local variables
are dropped in some.

Signed-off-by: Jiri Slaby (SUSE) <jirislaby@kernel.org>
---
 drivers/tty/serial/serial_core.c | 83 ++++++++++++--------------------
 1 file changed, 31 insertions(+), 52 deletions(-)

diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index 74fa02b23772..f595f2336fc0 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -790,7 +790,6 @@ static int uart_get_info(struct tty_port *port, struct serial_struct *retinfo)
 {
 	struct uart_state *state = container_of(port, struct uart_state, port);
 	struct uart_port *uport;
-	int ret = -ENODEV;
 
 	/* Initialize structure in case we error out later to prevent any stack info leakage. */
 	*retinfo = (struct serial_struct){};
@@ -799,10 +798,10 @@ static int uart_get_info(struct tty_port *port, struct serial_struct *retinfo)
 	 * Ensure the state we copy is consistent and no hardware changes
 	 * occur as we go
 	 */
-	mutex_lock(&port->mutex);
+	guard(mutex)(&port->mutex);
 	uport = uart_port_check(state);
 	if (!uport)
-		goto out;
+		return -ENODEV;
 
 	retinfo->type	    = uport->type;
 	retinfo->line	    = uport->line;
@@ -823,10 +822,7 @@ static int uart_get_info(struct tty_port *port, struct serial_struct *retinfo)
 	retinfo->iomem_reg_shift = uport->regshift;
 	retinfo->iomem_base      = (void *)(unsigned long)uport->mapbase;
 
-	ret = 0;
-out:
-	mutex_unlock(&port->mutex);
-	return ret;
+	return 0;
 }
 
 static int uart_get_info_user(struct tty_struct *tty,
@@ -3061,26 +3057,25 @@ static ssize_t console_store(struct device *dev,
 	if (ret)
 		return ret;
 
-	mutex_lock(&port->mutex);
+	guard(mutex)(&port->mutex);
 	uport = uart_port_check(state);
-	if (uport) {
-		oldconsole = uart_console_registered(uport);
-		if (oldconsole && !newconsole) {
-			ret = unregister_console(uport->cons);
-		} else if (!oldconsole && newconsole) {
-			if (uart_console(uport)) {
-				uport->console_reinit = 1;
-				register_console(uport->cons);
-			} else {
-				ret = -ENOENT;
-			}
-		}
-	} else {
-		ret = -ENXIO;
+	if (!uport)
+		return -ENXIO;
+
+	oldconsole = uart_console_registered(uport);
+	if (oldconsole && !newconsole) {
+		ret = unregister_console(uport->cons);
+		if (ret < 0)
+			return ret;
+	} else if (!oldconsole && newconsole) {
+		if (!uart_console(uport))
+			return -ENOENT;
+
+		uport->console_reinit = 1;
+		register_console(uport->cons);
 	}
-	mutex_unlock(&port->mutex);
 
-	return ret < 0 ? ret : count;
+	return count;
 }
 
 static DEVICE_ATTR_RO(uartclk);
@@ -3136,7 +3131,6 @@ static int serial_core_add_one_port(struct uart_driver *drv, struct uart_port *u
 {
 	struct uart_state *state;
 	struct tty_port *port;
-	int ret = 0;
 	struct device *tty_dev;
 	int num_groups;
 
@@ -3146,11 +3140,9 @@ static int serial_core_add_one_port(struct uart_driver *drv, struct uart_port *u
 	state = drv->state + uport->line;
 	port = &state->port;
 
-	mutex_lock(&port->mutex);
-	if (state->uart_port) {
-		ret = -EINVAL;
-		goto out;
-	}
+	guard(mutex)(&port->mutex);
+	if (state->uart_port)
+		return -EINVAL;
 
 	/* Link the port to the driver state table and vice versa */
 	atomic_set(&state->refcount, 1);
@@ -3170,10 +3162,8 @@ static int serial_core_add_one_port(struct uart_driver *drv, struct uart_port *u
 	uport->minor = drv->tty_driver->minor_start + uport->line;
 	uport->name = kasprintf(GFP_KERNEL, "%s%d", drv->dev_name,
 				drv->tty_driver->name_base + uport->line);
-	if (!uport->name) {
-		ret = -ENOMEM;
-		goto out;
-	}
+	if (!uport->name)
+		return -ENOMEM;
 
 	if (uport->cons && uport->dev)
 		of_console_check(uport->dev->of_node, uport->cons->name, uport->line);
@@ -3189,10 +3179,9 @@ static int serial_core_add_one_port(struct uart_driver *drv, struct uart_port *u
 
 	uport->tty_groups = kcalloc(num_groups, sizeof(*uport->tty_groups),
 				    GFP_KERNEL);
-	if (!uport->tty_groups) {
-		ret = -ENOMEM;
-		goto out;
-	}
+	if (!uport->tty_groups)
+		return -ENOMEM;
+
 	uport->tty_groups[0] = &tty_dev_attr_group;
 	if (uport->attr_group)
 		uport->tty_groups[1] = uport->attr_group;
@@ -3215,10 +3204,7 @@ static int serial_core_add_one_port(struct uart_driver *drv, struct uart_port *u
 		       uport->line);
 	}
 
- out:
-	mutex_unlock(&port->mutex);
-
-	return ret;
+	return 0;
 }
 
 /**
@@ -3384,7 +3370,7 @@ int serial_core_register_port(struct uart_driver *drv, struct uart_port *port)
 	struct serial_ctrl_device *ctrl_dev, *new_ctrl_dev = NULL;
 	int ret;
 
-	mutex_lock(&port_mutex);
+	guard(mutex)(&port_mutex);
 
 	/*
 	 * Prevent serial_port_runtime_resume() from trying to use the port
@@ -3396,10 +3382,8 @@ int serial_core_register_port(struct uart_driver *drv, struct uart_port *port)
 	ctrl_dev = serial_core_ctrl_find(drv, port->dev, port->ctrl_id);
 	if (!ctrl_dev) {
 		new_ctrl_dev = serial_core_ctrl_device_add(port);
-		if (IS_ERR(new_ctrl_dev)) {
-			ret = PTR_ERR(new_ctrl_dev);
-			goto err_unlock;
-		}
+		if (IS_ERR(new_ctrl_dev))
+			return PTR_ERR(new_ctrl_dev);
 		ctrl_dev = new_ctrl_dev;
 	}
 
@@ -3420,8 +3404,6 @@ int serial_core_register_port(struct uart_driver *drv, struct uart_port *port)
 	if (ret)
 		goto err_unregister_port_dev;
 
-	mutex_unlock(&port_mutex);
-
 	return 0;
 
 err_unregister_port_dev:
@@ -3430,9 +3412,6 @@ int serial_core_register_port(struct uart_driver *drv, struct uart_port *port)
 err_unregister_ctrl_dev:
 	serial_base_ctrl_device_remove(new_ctrl_dev);
 
-err_unlock:
-	mutex_unlock(&port_mutex);
-
 	return ret;
 }
 
-- 
2.47.1

[PATCH 2/3] tty: serial: get rid of exit label from uart_set_info()

[Jiri Slaby (SUSE)]

The label is unneeded since 7ba2e769825f (tty: Split the serial_core
helpers for setserial into two). Until then, there was a lock held in
uart_set_info().

Now it is not, so we can remove the label. This involves reordering the
code, so that it is clear what values are returned, where and why. Until
now, it was really hard to follow.

The "change_port" part of the function is extracted into a separate
function in the next patch. This patch makes the transition there easier
too.

Signed-off-by: Jiri Slaby (SUSE) <jirislaby@kernel.org>
---
 drivers/tty/serial/serial_core.c | 116 ++++++++++++++-----------------
 1 file changed, 51 insertions(+), 65 deletions(-)

diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index f595f2336fc0..ce3cf95fc910 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -843,7 +843,7 @@ static int uart_set_info(struct tty_struct *tty, struct tty_port *port,
 	unsigned int change_irq, change_port, closing_wait;
 	unsigned int old_custom_divisor, close_delay;
 	upf_t old_flags, new_flags;
-	int retval = 0;
+	int retval;
 
 	if (!uport)
 		return -EIO;
@@ -882,13 +882,10 @@ static int uart_set_info(struct tty_struct *tty, struct tty_port *port,
 	if (!(uport->flags & UPF_FIXED_PORT)) {
 		unsigned int uartclk = new_info->baud_base * 16;
 		/* check needs to be done here before other settings made */
-		if (uartclk == 0) {
-			retval = -EINVAL;
-			goto exit;
-		}
+		if (uartclk == 0)
+			return -EINVAL;
 	}
 	if (!capable(CAP_SYS_ADMIN)) {
-		retval = -EPERM;
 		if (change_irq || change_port ||
 		    (new_info->baud_base != uport->uartclk / 16) ||
 		    (close_delay != port->close_delay) ||
@@ -896,7 +893,7 @@ static int uart_set_info(struct tty_struct *tty, struct tty_port *port,
 		    (new_info->xmit_fifo_size &&
 		     new_info->xmit_fifo_size != uport->fifosize) ||
 		    (((new_flags ^ old_flags) & ~UPF_USR_MASK) != 0))
-			goto exit;
+			return -EPERM;
 		uport->flags = ((uport->flags & ~UPF_USR_MASK) |
 			       (new_flags & UPF_USR_MASK));
 		uport->custom_divisor = new_info->custom_divisor;
@@ -906,30 +903,24 @@ static int uart_set_info(struct tty_struct *tty, struct tty_port *port,
 	if (change_irq || change_port) {
 		retval = security_locked_down(LOCKDOWN_TIOCSSERIAL);
 		if (retval)
-			goto exit;
+			return retval;
 	}
 
-	/*
-	 * Ask the low level driver to verify the settings.
-	 */
-	if (uport->ops->verify_port)
+	 /* Ask the low level driver to verify the settings. */
+	if (uport->ops->verify_port) {
 		retval = uport->ops->verify_port(uport, new_info);
+		if (retval)
+			return retval;
+	}
 
 	if ((new_info->irq >= irq_get_nr_irqs()) || (new_info->irq < 0) ||
 	    (new_info->baud_base < 9600))
-		retval = -EINVAL;
-
-	if (retval)
-		goto exit;
+		return -EINVAL;
 
 	if (change_port || change_irq) {
-		retval = -EBUSY;
-
-		/*
-		 * Make sure that we are the sole user of this port.
-		 */
+		 /* Make sure that we are the sole user of this port. */
 		if (tty_port_users(port) > 1)
-			goto exit;
+			return -EBUSY;
 
 		/*
 		 * We need to shutdown the serial port at the old
@@ -967,40 +958,33 @@ static int uart_set_info(struct tty_struct *tty, struct tty_port *port,
 		 */
 		if (uport->type != PORT_UNKNOWN && uport->ops->request_port) {
 			retval = uport->ops->request_port(uport);
-		} else {
-			/* Always success - Jean II */
-			retval = 0;
-		}
-
-		/*
-		 * If we fail to request resources for the
-		 * new port, try to restore the old settings.
-		 */
-		if (retval) {
-			uport->iobase = old_iobase;
-			uport->type = old_type;
-			uport->hub6 = old_hub6;
-			uport->iotype = old_iotype;
-			uport->regshift = old_shift;
-			uport->mapbase = old_mapbase;
-
-			if (old_type != PORT_UNKNOWN) {
-				retval = uport->ops->request_port(uport);
-				/*
-				 * If we failed to restore the old settings,
-				 * we fail like this.
-				 */
-				if (retval)
-					uport->type = PORT_UNKNOWN;
-
-				/*
-				 * We failed anyway.
-				 */
-				retval = -EBUSY;
+			/*
+			 * If we fail to request resources for the
+			 * new port, try to restore the old settings.
+			 */
+			if (retval) {
+				uport->iobase = old_iobase;
+				uport->type = old_type;
+				uport->hub6 = old_hub6;
+				uport->iotype = old_iotype;
+				uport->regshift = old_shift;
+				uport->mapbase = old_mapbase;
+
+				if (old_type != PORT_UNKNOWN) {
+					retval = uport->ops->request_port(uport);
+					/*
+					 * If we failed to restore the old
+					 * settings, we fail like this.
+					 */
+					if (retval)
+						uport->type = PORT_UNKNOWN;
+
+					/* We failed anyway. */
+					return -EBUSY;
+				}
+
+				return retval;
 			}
-
-			/* Added to return the correct error -Ram Gupta */
-			goto exit;
 		}
 	}
 
@@ -1017,9 +1001,9 @@ static int uart_set_info(struct tty_struct *tty, struct tty_port *port,
 		uport->fifosize = new_info->xmit_fifo_size;
 
  check_and_exit:
-	retval = 0;
 	if (uport->type == PORT_UNKNOWN)
-		goto exit;
+		return 0;
+
 	if (tty_port_initialized(port)) {
 		if (((old_flags ^ uport->flags) & UPF_SPD_MASK) ||
 		    old_custom_divisor != uport->custom_divisor) {
@@ -1035,15 +1019,17 @@ static int uart_set_info(struct tty_struct *tty, struct tty_port *port,
 			}
 			uart_change_line_settings(tty, state, NULL);
 		}
-	} else {
-		retval = uart_startup(tty, state, true);
-		if (retval == 0)
-			tty_port_set_initialized(port, true);
-		if (retval > 0)
-			retval = 0;
+
+		return 0;
 	}
- exit:
-	return retval;
+
+	retval = uart_startup(tty, state, true);
+	if (retval < 0)
+		return retval;
+	if (retval == 0)
+		tty_port_set_initialized(port, true);
+
+	return 0;
 }
 
 static int uart_set_info_user(struct tty_struct *tty, struct serial_struct *ss)
-- 
2.47.1

[PATCH 3/3] tty: serial: extract uart_change_port() from uart_set_info()

[Jiri Slaby (SUSE)]

This "change_port" part of uart_set_info() is for no good reason
inlined there. It makes the function rather hard to read. Therefore,
extract it to a separate function.

This allows for flattening the ifs (with short path "return"s) and
avoiding two levels of indentation. Both making the code really flat and
comprehesible.

Signed-off-by: Jiri Slaby (SUSE) <jirislaby@kernel.org>
---
 drivers/tty/serial/serial_core.c | 114 ++++++++++++++++---------------
 1 file changed, 58 insertions(+), 56 deletions(-)

diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index ce3cf95fc910..c472594c3a9f 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -834,6 +834,61 @@ static int uart_get_info_user(struct tty_struct *tty,
 	return uart_get_info(port, ss) < 0 ? -EIO : 0;
 }
 
+static int uart_change_port(struct uart_port *uport,
+			    const struct serial_struct *new_info,
+			    unsigned long new_port)
+{
+	unsigned long old_iobase, old_mapbase;
+	unsigned int old_type, old_iotype, old_hub6, old_shift;
+	int retval;
+
+	old_iobase = uport->iobase;
+	old_mapbase = uport->mapbase;
+	old_type = uport->type;
+	old_hub6 = uport->hub6;
+	old_iotype = uport->iotype;
+	old_shift = uport->regshift;
+
+	if (old_type != PORT_UNKNOWN && uport->ops->release_port)
+		uport->ops->release_port(uport);
+
+	uport->iobase = new_port;
+	uport->type = new_info->type;
+	uport->hub6 = new_info->hub6;
+	uport->iotype = new_info->io_type;
+	uport->regshift = new_info->iomem_reg_shift;
+	uport->mapbase = (unsigned long)new_info->iomem_base;
+
+	if (uport->type == PORT_UNKNOWN || !uport->ops->request_port)
+		return 0;
+
+	retval = uport->ops->request_port(uport);
+	if (retval == 0)
+		return 0; /* succeeded => done */
+
+	/*
+	 * If we fail to request resources for the new port, try to restore the
+	 * old settings.
+	 */
+	uport->iobase = old_iobase;
+	uport->type = old_type;
+	uport->hub6 = old_hub6;
+	uport->iotype = old_iotype;
+	uport->regshift = old_shift;
+	uport->mapbase = old_mapbase;
+
+	if (old_type == PORT_UNKNOWN)
+		return retval;
+
+	retval = uport->ops->request_port(uport);
+	/* If we failed to restore the old settings, we fail like this. */
+	if (retval)
+		uport->type = PORT_UNKNOWN;
+
+	/* We failed anyway. */
+	return -EBUSY;
+}
+
 static int uart_set_info(struct tty_struct *tty, struct tty_port *port,
 			 struct uart_state *state,
 			 struct serial_struct *new_info)
@@ -930,62 +985,9 @@ static int uart_set_info(struct tty_struct *tty, struct tty_port *port,
 	}
 
 	if (change_port) {
-		unsigned long old_iobase, old_mapbase;
-		unsigned int old_type, old_iotype, old_hub6, old_shift;
-
-		old_iobase = uport->iobase;
-		old_mapbase = uport->mapbase;
-		old_type = uport->type;
-		old_hub6 = uport->hub6;
-		old_iotype = uport->iotype;
-		old_shift = uport->regshift;
-
-		/*
-		 * Free and release old regions
-		 */
-		if (old_type != PORT_UNKNOWN && uport->ops->release_port)
-			uport->ops->release_port(uport);
-
-		uport->iobase = new_port;
-		uport->type = new_info->type;
-		uport->hub6 = new_info->hub6;
-		uport->iotype = new_info->io_type;
-		uport->regshift = new_info->iomem_reg_shift;
-		uport->mapbase = (unsigned long)new_info->iomem_base;
-
-		/*
-		 * Claim and map the new regions
-		 */
-		if (uport->type != PORT_UNKNOWN && uport->ops->request_port) {
-			retval = uport->ops->request_port(uport);
-			/*
-			 * If we fail to request resources for the
-			 * new port, try to restore the old settings.
-			 */
-			if (retval) {
-				uport->iobase = old_iobase;
-				uport->type = old_type;
-				uport->hub6 = old_hub6;
-				uport->iotype = old_iotype;
-				uport->regshift = old_shift;
-				uport->mapbase = old_mapbase;
-
-				if (old_type != PORT_UNKNOWN) {
-					retval = uport->ops->request_port(uport);
-					/*
-					 * If we failed to restore the old
-					 * settings, we fail like this.
-					 */
-					if (retval)
-						uport->type = PORT_UNKNOWN;
-
-					/* We failed anyway. */
-					return -EBUSY;
-				}
-
-				return retval;
-			}
-		}
+		retval = uart_change_port(uport, new_info, new_port);
+		if (retval)
+			return retval;
 	}
 
 	if (change_irq)
-- 
2.47.1