After conversion to the kfifo, it becomes possible to extract two helper
functions for better maintenance and code deduplication. Do it here.
Signed-off-by: Andy Shevchenko <[email protected]>
---
drivers/tty/serial/serial_core.c | 98 ++++++++++++++++++--------------
1 file changed, 54 insertions(+), 44 deletions(-)
diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index dd6cf525d98d..ba2d6065fe02 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -243,25 +243,12 @@ static void uart_change_line_settings(struct tty_struct *tty, struct uart_state
uart_port_unlock_irq(uport);
}
-/*
- * Startup the port. This will be called once per open. All calls
- * will be serialised by the per-port mutex.
- */
-static int uart_port_startup(struct tty_struct *tty, struct uart_state *state,
- bool init_hw)
+static int uart_alloc_xmit_buf(struct tty_port *port)
{
- struct uart_port *uport = uart_port_check(state);
+ struct uart_state *state = container_of(port, struct uart_state, port);
+ struct uart_port *uport;
unsigned long flags;
unsigned long page;
- int retval = 0;
-
- if (uport->type == PORT_UNKNOWN)
- return 1;
-
- /*
- * Make sure the device is in D0 state.
- */
- uart_change_pm(state, UART_PM_STATE_ON);
/*
* Initialise and allocate the transmit and temporary
@@ -271,7 +258,7 @@ static int uart_port_startup(struct tty_struct *tty, struct uart_state *state,
if (!page)
return -ENOMEM;
- uart_port_lock(state, flags);
+ uport = uart_port_lock(state, flags);
if (!state->port.xmit_buf) {
state->port.xmit_buf = (unsigned char *)page;
kfifo_init(&state->port.xmit_fifo, state->port.xmit_buf,
@@ -281,11 +268,58 @@ static int uart_port_startup(struct tty_struct *tty, struct uart_state *state,
uart_port_unlock(uport, flags);
/*
* Do not free() the page under the port lock, see
- * uart_shutdown().
+ * uart_free_xmit_buf().
*/
free_page(page);
}
+ return 0;
+}
+
+static void uart_free_xmit_buf(struct tty_port *port)
+{
+ struct uart_state *state = container_of(port, struct uart_state, port);
+ struct uart_port *uport;
+ unsigned long flags;
+ char *xmit_buf;
+
+ /*
+ * 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 end up in printk() recursion.
+ */
+ uport = uart_port_lock(state, flags);
+ xmit_buf = port->xmit_buf;
+ port->xmit_buf = NULL;
+ INIT_KFIFO(port->xmit_fifo);
+ uart_port_unlock(uport, flags);
+
+ free_page((unsigned long)xmit_buf);
+}
+
+/*
+ * Startup the port. This will be called once per open. All calls
+ * will be serialised by the per-port mutex.
+ */
+static int uart_port_startup(struct tty_struct *tty, struct uart_state *state,
+ bool init_hw)
+{
+ struct uart_port *uport = uart_port_check(state);
+ int retval;
+
+ if (uport->type == PORT_UNKNOWN)
+ return 1;
+
+ /*
+ * Make sure the device is in D0 state.
+ */
+ uart_change_pm(state, UART_PM_STATE_ON);
+
+ retval = uart_alloc_xmit_buf(&state->port);
+ if (retval)
+ return retval;
+
retval = uport->ops->startup(uport);
if (retval == 0) {
if (uart_console(uport) && uport->cons->cflag) {
@@ -347,8 +381,6 @@ 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;
- char *xmit_buf = NULL;
/*
* Set the TTY IO error marker
@@ -381,19 +413,7 @@ static void uart_shutdown(struct tty_struct *tty, struct uart_state *state)
*/
tty_port_set_suspended(port, false);
- /*
- * 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.
- */
- uart_port_lock(state, flags);
- xmit_buf = port->xmit_buf;
- port->xmit_buf = NULL;
- INIT_KFIFO(port->xmit_fifo);
- uart_port_unlock(uport, flags);
-
- free_page((unsigned long)xmit_buf);
+ uart_free_xmit_buf(port);
}
/**
@@ -1747,7 +1767,6 @@ static void uart_tty_port_shutdown(struct tty_port *port)
{
struct uart_state *state = container_of(port, struct uart_state, port);
struct uart_port *uport = uart_port_check(state);
- char *buf;
/*
* At this point, we stop accepting input. To do this, we
@@ -1769,16 +1788,7 @@ static void uart_tty_port_shutdown(struct tty_port *port)
*/
tty_port_set_suspended(port, false);
- /*
- * Free the transmit buffer.
- */
- uart_port_lock_irq(uport);
- buf = port->xmit_buf;
- port->xmit_buf = NULL;
- INIT_KFIFO(port->xmit_fifo);
- uart_port_unlock_irq(uport);
-
- free_page((unsigned long)buf);
+ uart_free_xmit_buf(port);
uart_change_pm(state, UART_PM_STATE_OFF);
}
--
2.43.0.rc1.1.gbec44491f096
On 09. 04. 24, 19:40, Andy Shevchenko wrote:
> After conversion to the kfifo, it becomes possible to extract two helper
> functions for better maintenance and code deduplication. Do it here.
>
> Signed-off-by: Andy Shevchenko <[email protected]>
> ---
> drivers/tty/serial/serial_core.c | 98 ++++++++++++++++++--------------
> 1 file changed, 54 insertions(+), 44 deletions(-)
>
> diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
> index dd6cf525d98d..ba2d6065fe02 100644
> --- a/drivers/tty/serial/serial_core.c
> +++ b/drivers/tty/serial/serial_core.c
> @@ -243,25 +243,12 @@ static void uart_change_line_settings(struct tty_struct *tty, struct uart_state
> uart_port_unlock_irq(uport);
> }
>
> -/*
> - * Startup the port. This will be called once per open. All calls
> - * will be serialised by the per-port mutex.
> - */
> -static int uart_port_startup(struct tty_struct *tty, struct uart_state *state,
> - bool init_hw)
> +static int uart_alloc_xmit_buf(struct tty_port *port)
> {
> - struct uart_port *uport = uart_port_check(state);
> + struct uart_state *state = container_of(port, struct uart_state, port);
> + struct uart_port *uport;
> unsigned long flags;
> unsigned long page;
> - int retval = 0;
> -
> - if (uport->type == PORT_UNKNOWN)
> - return 1;
> -
> - /*
> - * Make sure the device is in D0 state.
> - */
> - uart_change_pm(state, UART_PM_STATE_ON);
>
> /*
> * Initialise and allocate the transmit and temporary
> @@ -271,7 +258,7 @@ static int uart_port_startup(struct tty_struct *tty, struct uart_state *state,
> if (!page)
> return -ENOMEM;
>
> - uart_port_lock(state, flags);
> + uport = uart_port_lock(state, flags);
> if (!state->port.xmit_buf) {
> state->port.xmit_buf = (unsigned char *)page;
> kfifo_init(&state->port.xmit_fifo, state->port.xmit_buf,
> @@ -281,11 +268,58 @@ static int uart_port_startup(struct tty_struct *tty, struct uart_state *state,
> uart_port_unlock(uport, flags);
> /*
> * Do not free() the page under the port lock, see
> - * uart_shutdown().
> + * uart_free_xmit_buf().
> */
> free_page(page);
> }
>
> + return 0;
> +}
> +
> +static void uart_free_xmit_buf(struct tty_port *port)
> +{
> + struct uart_state *state = container_of(port, struct uart_state, port);
> + struct uart_port *uport;
> + unsigned long flags;
> + char *xmit_buf;
> +
> + /*
> + * 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 end up in printk() recursion.
> + */
> + uport = uart_port_lock(state, flags);
> + xmit_buf = port->xmit_buf;
> + port->xmit_buf = NULL;
> + INIT_KFIFO(port->xmit_fifo);
> + uart_port_unlock(uport, flags);
> +
> + free_page((unsigned long)xmit_buf);
> +}
I see very much of tty_port_alloc_xmit_buf() and
tty_port_free_xmit_buf() in here :).
Currently, different locks are used, so the patch is, I think, good for
now. For future, we should switch to the tty port helpers.
Actually have you looked if the different locking is an issue at all?
IOW, isn't the tty_port's (and its xmit buf) lifetime enough w/o uart
port locks?
thanks,
--
js
suse labs
On Wed, Apr 10, 2024 at 11:04:35AM +0200, Jiri Slaby wrote:
> On 09. 04. 24, 19:40, Andy Shevchenko wrote:
..
> I see very much of tty_port_alloc_xmit_buf() and tty_port_free_xmit_buf() in
> here :).
A-ha!
> Currently, different locks are used, so the patch is, I think, good for now.
> For future, we should switch to the tty port helpers.
Sure, but I'm not familiar with TTY.
> Actually have you looked if the different locking is an issue at all? IOW,
> isn't the tty_port's (and its xmit buf) lifetime enough w/o uart port locks?
Nope. I only looked at spin lock differences (irq/irqsave) and decided that
irqsave variant is good for both cases.
..
So, do I need to do anything or is it good to go as-is?
Thanks for review.
--
With Best Regards,
Andy Shevchenko