Messages in this thread | ![/](/images/icornerl.gif) | | Date | Wed, 10 Apr 2024 11:04:35 +0200 | Subject | Re: [PATCH v1 1/1] serial: core: Extract uart_alloc_xmit_buf() and uart_free_xmit_buf() | From | 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@linux.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
| ![\](/images/icornerr.gif) |