| Message ID | 20120910041335.GF29537@lizard (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
> + tport = &state->port; > + if (!(tport->flags & ASYNC_INITIALIZED) && port->ops->poll_init) { > + mutex_lock(&tport->mutex); > + ret = port->ops->poll_init(port); > + /* > + * We don't set ASYNCB_INITIALIZED as we only initialized the > + * hw, e.g. state->xmit is still uninitialized. > + */ > + mutex_unlock(&tport->mutex); > + if (ret) > + return ret; > + } What stops a parallel open or close changing ASYNC_INITIALIZED after you test and before you lock ? Alan
On Mon, Sep 10, 2012 at 12:13:20PM +0100, Alan Cox wrote: > > + tport = &state->port; > > + if (!(tport->flags & ASYNC_INITIALIZED) && port->ops->poll_init) { > > + mutex_lock(&tport->mutex); > > + ret = port->ops->poll_init(port); > > + /* > > + * We don't set ASYNCB_INITIALIZED as we only initialized the > > + * hw, e.g. state->xmit is still uninitialized. > > + */ > > + mutex_unlock(&tport->mutex); > > + if (ret) > > + return ret; > > + } > > What stops a parallel open or close changing ASYNC_INITIALIZED after you > test and before you lock ? Yeah, I should do the whole thing under the mutex. Not related to this particular issue, but the fact that close() can powerdown the hardware is quite bad. Today it is always possible to use open,close sequence on /dev/ttyXXXX, and polling would break if close() deinitializes the hardware (e.g. via uart_change_pm()). In console= case, serial core handles the issue via uart_console(), checking if the port is used for console, preventing it to power down the hardware. We can do the same, or make tty_find_polling_driver() refcount individual ports/lines. But the issue is orthogonal to this particular patch, although needs to be fixed some day. Thanks! Anton.
> > What stops a parallel open or close changing ASYNC_INITIALIZED after you > > test and before you lock ? > > Yeah, I should do the whole thing under the mutex. Can you use test_bit() as well. I'm trying to gradually push all the code that way so people habitually use set_bit() and we don't get any (more) races where some compile situations and architectures otherwise create load to register register ored with constant write back > Not related to this particular issue, but the fact that close() can powerdown > the hardware is quite bad. Today it is always possible to use open,close > sequence on /dev/ttyXXXX, and polling would break if close() deinitializes the > hardware (e.g. via uart_change_pm()). One of the long term goals of tty_port has always ben to have an object with the lifetime of the physical port so this kind of thing can be fixed. > In console= case, serial core handles the issue via uart_console(), checking if > the port is used for console, preventing it to power down the hardware. We can > do the same, or make tty_find_polling_driver() refcount individual ports/lines. > But the issue is orthogonal to this particular patch, although needs to be > fixed some day. Agreed - however you'll need a separate refcount than the main tty one for this, because we still need to do a hangup() on the final "tty" close even if the hardware is 'pinned' for a debugger. Alan
diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c index a21dc8e..cba8443 100644 --- a/drivers/tty/serial/serial_core.c +++ b/drivers/tty/serial/serial_core.c @@ -2108,11 +2108,13 @@ static int uart_poll_init(struct tty_driver *driver, int line, char *options) { struct uart_driver *drv = driver->driver_state; struct uart_state *state = drv->state + line; + struct tty_port *tport; struct uart_port *port; int baud = 9600; int bits = 8; int parity = 'n'; int flow = 'n'; + int ret; if (!state || !state->uart_port) return -1; @@ -2121,6 +2123,19 @@ static int uart_poll_init(struct tty_driver *driver, int line, char *options) if (!(port->ops->poll_get_char && port->ops->poll_put_char)) return -1; + tport = &state->port; + if (!(tport->flags & ASYNC_INITIALIZED) && port->ops->poll_init) { + mutex_lock(&tport->mutex); + ret = port->ops->poll_init(port); + /* + * We don't set ASYNCB_INITIALIZED as we only initialized the + * hw, e.g. state->xmit is still uninitialized. + */ + mutex_unlock(&tport->mutex); + if (ret) + return ret; + } + if (options) { uart_parse_options(options, &baud, &parity, &bits, &flow); return uart_set_options(port, NULL, baud, parity, bits, flow); diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h index 0253c20..3642710 100644 --- a/include/linux/serial_core.h +++ b/include/linux/serial_core.h @@ -274,6 +274,7 @@ struct uart_ops { int (*verify_port)(struct uart_port *, struct serial_struct *); int (*ioctl)(struct uart_port *, unsigned int, unsigned long); #ifdef CONFIG_CONSOLE_POLL + int (*poll_init)(struct uart_port *); void (*poll_put_char)(struct uart_port *, unsigned char); int (*poll_get_char)(struct uart_port *); #endif
It was noticed that polling drivers (like KGDB) are not able to use serial ports if the ports were not previously initialized via console. I.e. when booting with console=ttyAMA0 kgdboc=ttyAMA0, everything works fine, but with console=ttyFOO kgdboc=ttyAMA0, the kgdboc doesn't work. This is because we don't initialize the hardware. Calling ->startup() is not an option, because drivers request interrupts there, and drivers fail to handle situations when tty isn't opened with interrupts enabled. So, we have to implement a new callback (actually, tty_ops already have a similar callback), which does everything needed to initialize just the hardware. Signed-off-by: Anton Vorontsov <anton.vorontsov@linaro.org> --- drivers/tty/serial/serial_core.c | 15 +++++++++++++++ include/linux/serial_core.h | 1 + 2 files changed, 16 insertions(+)