[printk,v3,23/40] serial_core: replace uart_console_enabled() with uart_console_registered()
Commit Message
All users of uart_console_enabled() really want to know if a console
is registered. It is not reliable to check for CON_ENABLED in order
to identify if a console is registered. Use console_is_registered()
instead.
A _locked() variant is provided because uart_set_options() is always
called with the console_list_lock held and must check if a console
is registered in order to synchronize with kgdboc.
Signed-off-by: John Ogness <john.ogness@linutronix.de>
---
drivers/tty/serial/8250/8250_core.c | 2 +-
drivers/tty/serial/pic32_uart.c | 2 +-
drivers/tty/serial/serial_core.c | 14 +++++++-------
include/linux/serial_core.h | 15 +++++++++++++--
4 files changed, 22 insertions(+), 11 deletions(-)
Comments
Hi John,
On Mon, Nov 7, 2022 at 3:16 PM John Ogness <john.ogness@linutronix.de> wrote:
> All users of uart_console_enabled() really want to know if a console
> is registered. It is not reliable to check for CON_ENABLED in order
> to identify if a console is registered. Use console_is_registered()
> instead.
>
> A _locked() variant is provided because uart_set_options() is always
> called with the console_list_lock held and must check if a console
> is registered in order to synchronize with kgdboc.
>
> Signed-off-by: John Ogness <john.ogness@linutronix.de>
> --- a/include/linux/serial_core.h
> +++ b/include/linux/serial_core.h
> @@ -743,9 +743,20 @@ static const bool earlycon_acpi_spcr_enable EARLYCON_USED_OR_UNUSED;
> static inline int setup_earlycon(char *buf) { return 0; }
> #endif
>
> -static inline bool uart_console_enabled(struct uart_port *port)
> +/* Variant of uart_console_registered() when the console_list_lock is held. */
> +static inline bool uart_console_registered_locked(struct uart_port *port)
> {
> - return uart_console(port) && (port->cons->flags & CON_ENABLED);
> + return uart_console(port) && console_is_registered_locked(port->cons);
> +}
> +
> +static inline bool uart_console_registered(struct uart_port *port)
> +{
> + bool ret;
> +
> + console_list_lock();
> + ret = uart_console_registered_locked(port);
> + console_list_unlock();
> + return ret;
Perhaps
return uart_console(port) && console_is_registered();
to avoid locking the list when the first condition is not true?
> }
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
On Tue 2022-11-08 09:46:20, Geert Uytterhoeven wrote:
> Hi John,
>
> On Mon, Nov 7, 2022 at 3:16 PM John Ogness <john.ogness@linutronix.de> wrote:
> > All users of uart_console_enabled() really want to know if a console
> > is registered. It is not reliable to check for CON_ENABLED in order
> > to identify if a console is registered. Use console_is_registered()
> > instead.
> >
> > A _locked() variant is provided because uart_set_options() is always
> > called with the console_list_lock held and must check if a console
> > is registered in order to synchronize with kgdboc.
> >
> > Signed-off-by: John Ogness <john.ogness@linutronix.de>
>
> > --- a/include/linux/serial_core.h
> > +++ b/include/linux/serial_core.h
> > @@ -743,9 +743,20 @@ static const bool earlycon_acpi_spcr_enable EARLYCON_USED_OR_UNUSED;
> > static inline int setup_earlycon(char *buf) { return 0; }
> > #endif
> >
> > -static inline bool uart_console_enabled(struct uart_port *port)
> > +/* Variant of uart_console_registered() when the console_list_lock is held. */
> > +static inline bool uart_console_registered_locked(struct uart_port *port)
> > {
> > - return uart_console(port) && (port->cons->flags & CON_ENABLED);
> > + return uart_console(port) && console_is_registered_locked(port->cons);
> > +}
> > +
> > +static inline bool uart_console_registered(struct uart_port *port)
> > +{
> > + bool ret;
> > +
> > + console_list_lock();
> > + ret = uart_console_registered_locked(port);
> > + console_list_unlock();
> > + return ret;
>
> Perhaps
>
> return uart_console(port) && console_is_registered();
>
> to avoid locking the list when the first condition is not true?
I do not have strong opinion on this. It is true that the code
duplication is trivial but it is a code duplication. Either
way would work for me.
The reset of the code looks good. Feel free to use:
Reviewed-by: Petr Mladek <pmladek@suse.com>
Best Regards,
Petr
On 2022-11-10, Petr Mladek <pmladek@suse.com> wrote:
>>> -static inline bool uart_console_enabled(struct uart_port *port)
>>> +/* Variant of uart_console_registered() when the console_list_lock is held. */
>>> +static inline bool uart_console_registered_locked(struct uart_port *port)
>>> {
>>> - return uart_console(port) && (port->cons->flags & CON_ENABLED);
>>> + return uart_console(port) && console_is_registered_locked(port->cons);
>>> +}
>>> +
>>> +static inline bool uart_console_registered(struct uart_port *port)
>>> +{
>>> + bool ret;
>>> +
>>> + console_list_lock();
>>> + ret = uart_console_registered_locked(port);
>>> + console_list_unlock();
>>> + return ret;
>>
>> Perhaps
>>
>> return uart_console(port) && console_is_registered();
>>
>> to avoid locking the list when the first condition is not true?
>
> I do not have strong opinion on this. It is true that the code
> duplication is trivial but it is a code duplication. Either
> way would work for me.
I will go with Geert's suggestion for v4. It is important that we reduce
lock contention for non-console ports.
> The reset of the code looks good. Feel free to use:
>
> Reviewed-by: Petr Mladek <pmladek@suse.com>
Thanks.
John
@@ -565,7 +565,7 @@ serial8250_register_ports(struct uart_driver *drv, struct device *dev)
up->port.dev = dev;
- if (uart_console_enabled(&up->port))
+ if (uart_console_registered(&up->port))
pm_runtime_get_sync(up->port.dev);
serial8250_apply_quirks(up);
@@ -919,7 +919,7 @@ static int pic32_uart_probe(struct platform_device *pdev)
}
#ifdef CONFIG_SERIAL_PIC32_CONSOLE
- if (uart_console_enabled(port)) {
+ if (uart_console_registered(port)) {
/* The peripheral clock has been enabled by console_setup,
* so disable it till the port is used.
*/
@@ -2223,11 +2223,11 @@ uart_set_options(struct uart_port *port, struct console *co,
/*
* Ensure that the serial-console lock is initialised early.
*
- * Note that the console-enabled check is needed because of kgdboc,
- * which can end up calling uart_set_options() for an already enabled
+ * Note that the console-registered check is needed because
+ * kgdboc can call uart_set_options() for an already registered
* console via tty_find_polling_driver() and uart_poll_init().
*/
- if (!uart_console_enabled(port) && !port->console_reinit)
+ if (!uart_console_registered_locked(port) && !port->console_reinit)
uart_port_spin_lock_init(port);
memset(&termios, 0, sizeof(struct ktermios));
@@ -2573,7 +2573,7 @@ uart_configure_port(struct uart_driver *drv, struct uart_state *state,
* successfully registered yet, try to re-register it.
* It may be that the port was not available.
*/
- if (port->cons && !(port->cons->flags & CON_ENABLED))
+ if (port->cons && !console_is_registered(port->cons))
register_console(port->cons);
/*
@@ -2956,7 +2956,7 @@ static ssize_t console_show(struct device *dev,
mutex_lock(&port->mutex);
uport = uart_port_check(state);
if (uport)
- console = uart_console_enabled(uport);
+ console = uart_console_registered(uport);
mutex_unlock(&port->mutex);
return sprintf(buf, "%c\n", console ? 'Y' : 'N');
@@ -2978,7 +2978,7 @@ static ssize_t console_store(struct device *dev,
mutex_lock(&port->mutex);
uport = uart_port_check(state);
if (uport) {
- oldconsole = uart_console_enabled(uport);
+ oldconsole = uart_console_registered(uport);
if (oldconsole && !newconsole) {
ret = unregister_console(uport->cons);
} else if (!oldconsole && newconsole) {
@@ -3086,7 +3086,7 @@ int uart_add_one_port(struct uart_driver *drv, struct uart_port *uport)
* If this port is in use as a console then the spinlock is already
* initialised.
*/
- if (!uart_console_enabled(uport))
+ if (!uart_console_registered(uport))
uart_port_spin_lock_init(uport);
if (uport->cons && uport->dev)
@@ -743,9 +743,20 @@ static const bool earlycon_acpi_spcr_enable EARLYCON_USED_OR_UNUSED;
static inline int setup_earlycon(char *buf) { return 0; }
#endif
-static inline bool uart_console_enabled(struct uart_port *port)
+/* Variant of uart_console_registered() when the console_list_lock is held. */
+static inline bool uart_console_registered_locked(struct uart_port *port)
{
- return uart_console(port) && (port->cons->flags & CON_ENABLED);
+ return uart_console(port) && console_is_registered_locked(port->cons);
+}
+
+static inline bool uart_console_registered(struct uart_port *port)
+{
+ bool ret;
+
+ console_list_lock();
+ ret = uart_console_registered_locked(port);
+ console_list_unlock();
+ return ret;
}
struct uart_port *uart_get_console(struct uart_port *ports, int nr,