[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <52a83cb5-ef4f-46f5-ad90-1f64b4d588e2@kernel.org>
Date: Wed, 10 Apr 2024 11:04:35 +0200
From: Jiri Slaby <jirislaby@...nel.org>
To: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
linux-kernel@...r.kernel.org, linux-serial@...r.kernel.org
Subject: Re: [PATCH v1 1/1] serial: core: Extract uart_alloc_xmit_buf() and
uart_free_xmit_buf()
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 <andriy.shevchenko@...ux.intel.com>
> ---
> 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
Powered by blists - more mailing lists