Re: [PATCH v1 1/1] serial: core: Extract uart_alloc_xmit_buf() and uart_free_xmit_buf()

[Jiri Slaby]

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@xxxxxxxxxxxxxxx>
> ---
>   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