[PATCH printk v3 00/40] reduce console_lock scope

[PATCH printk v3 00/40] reduce console_lock scope

[John Ogness]

This is v3 of a series to prepare for threaded/atomic
printing. v2 is here [0]. This series focuses on reducing the
scope of the BKL console_lock. It achieves this by switching to
SRCU and a dedicated mutex for console list iteration and
modification, respectively. The console_lock will no longer
offer this protection and is completely removed from
(un)register_console() and console_stop/start() code.

Also, during the review of v2 it came to our attention that
many console drivers are checking CON_ENABLED to see if they
are registered. Because this flag can change without
unregistering and because this flag does not represent an
atomic point when an (un)registration process is complete,
a new console_is_registered() function is introduced. This
function uses the console_list_lock to synchronize with the
(un)registration process to provide a reliable status.

All users of the console_lock for list iteration have been
modified. For the call sites where the console_lock is still
needed (because of other reasons), comments are added to
explain exactly why the console_lock was needed.

All users of CON_ENABLED for registration status have been
modified to use console_is_registered(). Note that there are
still users of CON_ENABLED, but this is for legitimate purposes
about a registered console being able to print.

The base commit for this series is from Paul McKenney's RCU tree
and provides an NMI-safe SRCU implementation [1]. Without the
NMI-safe SRCU implementation, this series is not less safe than
mainline. But we will need the NMI-safe SRCU implementation for
atomic consoles anyway, so we might as well get it in
now. Especially since it _does_ increase the reliability for
mainline in the panic path.

Changes since v3:

general:

- introduce a synchronized console_is_registered() to query if
  a console is registered, meant to replace CON_ENABLED
  (mis)use for this purpose

- directly read console->flags for registered consoles if it is
  race-free (and document that it is so)

- replace uart_console_enabled() with a new
  uart_console_registered() based on console_is_registered()

- change comments about why console_lock is used to synchronize
  console->device() by providing an example

registration check fixups:

- the following drivers were modified to use the new
  console_is_registered() instead of CON_ENABLED checks

   - arch/m68k/emu/nfcon.c
   - drivers/firmware/efi/earlycon.c
   - drivers/net/netconsole.c
   - drivers/tty/hvc/hvc_console.c
   - drivers/tty/serial/8250/8250_core.c
   - drivers/tty/serial/earlycon.c
   - drivers/tty/serial/pic32_uart.c
   - drivers/tty/serial/samsung_tty.c
   - drivers/tty/serial/serial_core.c
   - drivers/tty/serial/xilinx_uartps.c
   - drivers/usb/early/xhci-dbc.c

um: kmsg_dumper:

- change stdout dump criteria to match original intention

kgdb/kdb:

- in configure_kgdboc(), take console_list_lock to synchronize
  tty_find_polling_driver() against register_console()

- add comments explaining why calling console->write() without
  locking might work

tty: sh-sci:

- use a setup() callback to setup the early console

fbdev: xen:

- implement a cleaner approach for
  console_force_preferred_locked()

rcu:

- implement debug_lockdep_rcu_enabled() for
  !CONFIG_DEBUG_LOCK_ALLOC

printk:

- check CONFIG_DEBUG_LOCK_ALLOC for srcu_read_lock_held()
  availability

- for console_lock/_trylock/_unlock, replace "lock the console
  system" language with "block the console subsystem from
  printing"

- use WRITE_ONCE() for updating console->flags of registered
  consoles

- expand comments of synchronize_srcu() calls to explain why
  they are needed, and also expand comments to explain when it
  is not needed

- change CON_BOOT consoles to always begin at earliest message

- for non-BOOT/non-PRINTBUFFER consoles, initialize @seq to the
  minimal @seq of any of the enabled boot consoles

- add comments and lockdep assertion to
  unregister_console_locked() because it is not clear from the
  name which lock is implied

- dropped patches that caused unnecessary churn in the series

John Ogness

[0] https://lore.kernel.org/lkml/20221019145600.1282823-1-john.ogness@linutronix.de
[1] https://git.kernel.org/pub/scm/linux/kernel/git/paulmck/linux-rcu.git/log/?h=srcunmisafe.2022.10.21a

John Ogness (38):
  rcu: implement lockdep_rcu_enabled for !CONFIG_DEBUG_LOCK_ALLOC
  printk: Prepare for SRCU console list protection
  printk: fix setting first seq for consoles
  um: kmsg_dump: only dump when no output console available
  console: introduce console_is_enabled() wrapper
  printk: use console_is_enabled()
  um: kmsg_dump: use console_is_enabled()
  kdb: kdb_io: use console_is_enabled()
  um: kmsg_dumper: use srcu console list iterator
  tty: serial: kgdboc: document console_lock usage
  tty: tty_io: document console_lock usage
  proc: consoles: document console_lock usage
  kdb: use srcu console list iterator
  printk: console_flush_all: use srcu console list iterator
  printk: console_unblank: use srcu console list iterator
  printk: console_flush_on_panic: use srcu console list iterator
  printk: console_device: use srcu console list iterator
  printk: __pr_flush: use srcu console list iterator
  printk: introduce console_list_lock
  console: introduce console_is_registered()
  serial_core: replace uart_console_enabled() with
    uart_console_registered()
  tty: nfcon: use console_is_registered()
  efi: earlycon: use console_is_registered()
  tty: hvc: use console_is_registered()
  tty: serial: earlycon: use console_is_registered()
  tty: serial: pic32_uart: use console_is_registered()
  tty: serial: samsung_tty: use console_is_registered()
  tty: serial: xilinx_uartps: use console_is_registered()
  usb: early: xhci-dbc: use console_is_registered()
  netconsole: avoid CON_ENABLED misuse to track registration
  printk, xen: fbfront: create/use safe function for forcing preferred
  tty: tty_io: use console_list_lock for list synchronization
  proc: consoles: use console_list_lock for list iteration
  tty: serial: kgdboc: use console_list_lock for list traversal
  tty: serial: kgdboc: synchronize tty_find_polling_driver() and
    register_console()
  tty: serial: kgdboc: use console_list_lock to trap exit
  printk: relieve console_lock of list synchronization duties
  tty: serial: sh-sci: use setup() callback for early console

Thomas Gleixner (2):
  serial: kgdboc: Lock console list in probe function
  printk: Convert console_drivers list to hlist

 .clang-format                       |   1 +
 arch/m68k/emu/nfcon.c               |  10 +-
 arch/um/kernel/kmsg_dump.c          |  24 +-
 drivers/firmware/efi/earlycon.c     |   8 +-
 drivers/net/netconsole.c            |  21 +-
 drivers/tty/hvc/hvc_console.c       |   4 +-
 drivers/tty/serial/8250/8250_core.c |   2 +-
 drivers/tty/serial/earlycon.c       |   4 +-
 drivers/tty/serial/kgdboc.c         |  46 ++-
 drivers/tty/serial/pic32_uart.c     |   4 +-
 drivers/tty/serial/samsung_tty.c    |   2 +-
 drivers/tty/serial/serial_core.c    |  14 +-
 drivers/tty/serial/sh-sci.c         |  17 +-
 drivers/tty/serial/xilinx_uartps.c  |   2 +-
 drivers/tty/tty_io.c                |  18 +-
 drivers/usb/early/xhci-dbc.c        |   2 +-
 drivers/video/fbdev/xen-fbfront.c   |  12 +-
 fs/proc/consoles.c                  |  21 +-
 include/linux/console.h             | 111 +++++++-
 include/linux/rcupdate.h            |   5 +
 include/linux/serial_core.h         |  15 +-
 kernel/debug/kdb/kdb_io.c           |  14 +-
 kernel/printk/printk.c              | 424 +++++++++++++++++++++-------
 23 files changed, 605 insertions(+), 176 deletions(-)


base-commit: e29a4915db1480f96e0bc2e928699d086a71f43c
-- 
2.30.2

[PATCH printk v3 00/40] reduce console_lock scope

[John Ogness]

This is v3 of a series to prepare for threaded/atomic
printing. v2 is here [0]. This series focuses on reducing the
scope of the BKL console_lock. It achieves this by switching to
SRCU and a dedicated mutex for console list iteration and
modification, respectively. The console_lock will no longer
offer this protection and is completely removed from
(un)register_console() and console_stop/start() code.

Also, during the review of v2 it came to our attention that
many console drivers are checking CON_ENABLED to see if they
are registered. Because this flag can change without
unregistering and because this flag does not represent an
atomic point when an (un)registration process is complete,
a new console_is_registered() function is introduced. This
function uses the console_list_lock to synchronize with the
(un)registration process to provide a reliable status.

All users of the console_lock for list iteration have been
modified. For the call sites where the console_lock is still
needed (because of other reasons), comments are added to
explain exactly why the console_lock was needed.

All users of CON_ENABLED for registration status have been
modified to use console_is_registered(). Note that there are
still users of CON_ENABLED, but this is for legitimate purposes
about a registered console being able to print.

The base commit for this series is from Paul McKenney's RCU tree
and provides an NMI-safe SRCU implementation [1]. Without the
NMI-safe SRCU implementation, this series is not less safe than
mainline. But we will need the NMI-safe SRCU implementation for
atomic consoles anyway, so we might as well get it in
now. Especially since it _does_ increase the reliability for
mainline in the panic path.

Changes since v3:

general:

- introduce a synchronized console_is_registered() to query if
  a console is registered, meant to replace CON_ENABLED
  (mis)use for this purpose

- directly read console->flags for registered consoles if it is
  race-free (and document that it is so)

- replace uart_console_enabled() with a new
  uart_console_registered() based on console_is_registered()

- change comments about why console_lock is used to synchronize
  console->device() by providing an example

registration check fixups:

- the following drivers were modified to use the new
  console_is_registered() instead of CON_ENABLED checks

   - arch/m68k/emu/nfcon.c
   - drivers/firmware/efi/earlycon.c
   - drivers/net/netconsole.c
   - drivers/tty/hvc/hvc_console.c
   - drivers/tty/serial/8250/8250_core.c
   - drivers/tty/serial/earlycon.c
   - drivers/tty/serial/pic32_uart.c
   - drivers/tty/serial/samsung_tty.c
   - drivers/tty/serial/serial_core.c
   - drivers/tty/serial/xilinx_uartps.c
   - drivers/usb/early/xhci-dbc.c

um: kmsg_dumper:

- change stdout dump criteria to match original intention

kgdb/kdb:

- in configure_kgdboc(), take console_list_lock to synchronize
  tty_find_polling_driver() against register_console()

- add comments explaining why calling console->write() without
  locking might work

tty: sh-sci:

- use a setup() callback to setup the early console

fbdev: xen:

- implement a cleaner approach for
  console_force_preferred_locked()

rcu:

- implement debug_lockdep_rcu_enabled() for
  !CONFIG_DEBUG_LOCK_ALLOC

printk:

- check CONFIG_DEBUG_LOCK_ALLOC for srcu_read_lock_held()
  availability

- for console_lock/_trylock/_unlock, replace "lock the console
  system" language with "block the console subsystem from
  printing"

- use WRITE_ONCE() for updating console->flags of registered
  consoles

- expand comments of synchronize_srcu() calls to explain why
  they are needed, and also expand comments to explain when it
  is not needed

- change CON_BOOT consoles to always begin at earliest message

- for non-BOOT/non-PRINTBUFFER consoles, initialize @seq to the
  minimal @seq of any of the enabled boot consoles

- add comments and lockdep assertion to
  unregister_console_locked() because it is not clear from the
  name which lock is implied

- dropped patches that caused unnecessary churn in the series

John Ogness

[0] https://lore.kernel.org/lkml/20221019145600.1282823-1-john.ogness@linutronix.de
[1] https://git.kernel.org/pub/scm/linux/kernel/git/paulmck/linux-rcu.git/log/?h=srcunmisafe.2022.10.21a

John Ogness (38):
  rcu: implement lockdep_rcu_enabled for !CONFIG_DEBUG_LOCK_ALLOC
  printk: Prepare for SRCU console list protection
  printk: fix setting first seq for consoles
  um: kmsg_dump: only dump when no output console available
  console: introduce console_is_enabled() wrapper
  printk: use console_is_enabled()
  um: kmsg_dump: use console_is_enabled()
  kdb: kdb_io: use console_is_enabled()
  um: kmsg_dumper: use srcu console list iterator
  tty: serial: kgdboc: document console_lock usage
  tty: tty_io: document console_lock usage
  proc: consoles: document console_lock usage
  kdb: use srcu console list iterator
  printk: console_flush_all: use srcu console list iterator
  printk: console_unblank: use srcu console list iterator
  printk: console_flush_on_panic: use srcu console list iterator
  printk: console_device: use srcu console list iterator
  printk: __pr_flush: use srcu console list iterator
  printk: introduce console_list_lock
  console: introduce console_is_registered()
  serial_core: replace uart_console_enabled() with
    uart_console_registered()
  tty: nfcon: use console_is_registered()
  efi: earlycon: use console_is_registered()
  tty: hvc: use console_is_registered()
  tty: serial: earlycon: use console_is_registered()
  tty: serial: pic32_uart: use console_is_registered()
  tty: serial: samsung_tty: use console_is_registered()
  tty: serial: xilinx_uartps: use console_is_registered()
  usb: early: xhci-dbc: use console_is_registered()
  netconsole: avoid CON_ENABLED misuse to track registration
  printk, xen: fbfront: create/use safe function for forcing preferred
  tty: tty_io: use console_list_lock for list synchronization
  proc: consoles: use console_list_lock for list iteration
  tty: serial: kgdboc: use console_list_lock for list traversal
  tty: serial: kgdboc: synchronize tty_find_polling_driver() and
    register_console()
  tty: serial: kgdboc: use console_list_lock to trap exit
  printk: relieve console_lock of list synchronization duties
  tty: serial: sh-sci: use setup() callback for early console

Thomas Gleixner (2):
  serial: kgdboc: Lock console list in probe function
  printk: Convert console_drivers list to hlist

 .clang-format                       |   1 +
 arch/m68k/emu/nfcon.c               |  10 +-
 arch/um/kernel/kmsg_dump.c          |  24 +-
 drivers/firmware/efi/earlycon.c     |   8 +-
 drivers/net/netconsole.c            |  21 +-
 drivers/tty/hvc/hvc_console.c       |   4 +-
 drivers/tty/serial/8250/8250_core.c |   2 +-
 drivers/tty/serial/earlycon.c       |   4 +-
 drivers/tty/serial/kgdboc.c         |  46 ++-
 drivers/tty/serial/pic32_uart.c     |   4 +-
 drivers/tty/serial/samsung_tty.c    |   2 +-
 drivers/tty/serial/serial_core.c    |  14 +-
 drivers/tty/serial/sh-sci.c         |  17 +-
 drivers/tty/serial/xilinx_uartps.c  |   2 +-
 drivers/tty/tty_io.c                |  18 +-
 drivers/usb/early/xhci-dbc.c        |   2 +-
 drivers/video/fbdev/xen-fbfront.c   |  12 +-
 fs/proc/consoles.c                  |  21 +-
 include/linux/console.h             | 111 +++++++-
 include/linux/rcupdate.h            |   5 +
 include/linux/serial_core.h         |  15 +-
 kernel/debug/kdb/kdb_io.c           |  14 +-
 kernel/printk/printk.c              | 424 +++++++++++++++++++++-------
 23 files changed, 605 insertions(+), 176 deletions(-)


base-commit: e29a4915db1480f96e0bc2e928699d086a71f43c
-- 
2.30.2


_______________________________________________
linux-um mailing list
linux-um@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-um

[PATCH printk v3 00/40] reduce console_lock scope

[John Ogness]

This is v3 of a series to prepare for threaded/atomic
printing. v2 is here [0]. This series focuses on reducing the
scope of the BKL console_lock. It achieves this by switching to
SRCU and a dedicated mutex for console list iteration and
modification, respectively. The console_lock will no longer
offer this protection and is completely removed from
(un)register_console() and console_stop/start() code.

Also, during the review of v2 it came to our attention that
many console drivers are checking CON_ENABLED to see if they
are registered. Because this flag can change without
unregistering and because this flag does not represent an
atomic point when an (un)registration process is complete,
a new console_is_registered() function is introduced. This
function uses the console_list_lock to synchronize with the
(un)registration process to provide a reliable status.

All users of the console_lock for list iteration have been
modified. For the call sites where the console_lock is still
needed (because of other reasons), comments are added to
explain exactly why the console_lock was needed.

All users of CON_ENABLED for registration status have been
modified to use console_is_registered(). Note that there are
still users of CON_ENABLED, but this is for legitimate purposes
about a registered console being able to print.

The base commit for this series is from Paul McKenney's RCU tree
and provides an NMI-safe SRCU implementation [1]. Without the
NMI-safe SRCU implementation, this series is not less safe than
mainline. But we will need the NMI-safe SRCU implementation for
atomic consoles anyway, so we might as well get it in
now. Especially since it _does_ increase the reliability for
mainline in the panic path.

Changes since v3:

general:

- introduce a synchronized console_is_registered() to query if
  a console is registered, meant to replace CON_ENABLED
  (mis)use for this purpose

- directly read console->flags for registered consoles if it is
  race-free (and document that it is so)

- replace uart_console_enabled() with a new
  uart_console_registered() based on console_is_registered()

- change comments about why console_lock is used to synchronize
  console->device() by providing an example

registration check fixups:

- the following drivers were modified to use the new
  console_is_registered() instead of CON_ENABLED checks

   - arch/m68k/emu/nfcon.c
   - drivers/firmware/efi/earlycon.c
   - drivers/net/netconsole.c
   - drivers/tty/hvc/hvc_console.c
   - drivers/tty/serial/8250/8250_core.c
   - drivers/tty/serial/earlycon.c
   - drivers/tty/serial/pic32_uart.c
   - drivers/tty/serial/samsung_tty.c
   - drivers/tty/serial/serial_core.c
   - drivers/tty/serial/xilinx_uartps.c
   - drivers/usb/early/xhci-dbc.c

um: kmsg_dumper:

- change stdout dump criteria to match original intention

kgdb/kdb:

- in configure_kgdboc(), take console_list_lock to synchronize
  tty_find_polling_driver() against register_console()

- add comments explaining why calling console->write() without
  locking might work

tty: sh-sci:

- use a setup() callback to setup the early console

fbdev: xen:

- implement a cleaner approach for
  console_force_preferred_locked()

rcu:

- implement debug_lockdep_rcu_enabled() for
  !CONFIG_DEBUG_LOCK_ALLOC

printk:

- check CONFIG_DEBUG_LOCK_ALLOC for srcu_read_lock_held()
  availability

- for console_lock/_trylock/_unlock, replace "lock the console
  system" language with "block the console subsystem from
  printing"

- use WRITE_ONCE() for updating console->flags of registered
  consoles

- expand comments of synchronize_srcu() calls to explain why
  they are needed, and also expand comments to explain when it
  is not needed

- change CON_BOOT consoles to always begin at earliest message

- for non-BOOT/non-PRINTBUFFER consoles, initialize @seq to the
  minimal @seq of any of the enabled boot consoles

- add comments and lockdep assertion to
  unregister_console_locked() because it is not clear from the
  name which lock is implied

- dropped patches that caused unnecessary churn in the series

John Ogness

[0] https://lore.kernel.org/lkml/20221019145600.1282823-1-john.ogness@linutronix.de
[1] https://git.kernel.org/pub/scm/linux/kernel/git/paulmck/linux-rcu.git/log/?h=srcunmisafe.2022.10.21a

John Ogness (38):
  rcu: implement lockdep_rcu_enabled for !CONFIG_DEBUG_LOCK_ALLOC
  printk: Prepare for SRCU console list protection
  printk: fix setting first seq for consoles
  um: kmsg_dump: only dump when no output console available
  console: introduce console_is_enabled() wrapper
  printk: use console_is_enabled()
  um: kmsg_dump: use console_is_enabled()
  kdb: kdb_io: use console_is_enabled()
  um: kmsg_dumper: use srcu console list iterator
  tty: serial: kgdboc: document console_lock usage
  tty: tty_io: document console_lock usage
  proc: consoles: document console_lock usage
  kdb: use srcu console list iterator
  printk: console_flush_all: use srcu console list iterator
  printk: console_unblank: use srcu console list iterator
  printk: console_flush_on_panic: use srcu console list iterator
  printk: console_device: use srcu console list iterator
  printk: __pr_flush: use srcu console list iterator
  printk: introduce console_list_lock
  console: introduce console_is_registered()
  serial_core: replace uart_console_enabled() with
    uart_console_registered()
  tty: nfcon: use console_is_registered()
  efi: earlycon: use console_is_registered()
  tty: hvc: use console_is_registered()
  tty: serial: earlycon: use console_is_registered()
  tty: serial: pic32_uart: use console_is_registered()
  tty: serial: samsung_tty: use console_is_registered()
  tty: serial: xilinx_uartps: use console_is_registered()
  usb: early: xhci-dbc: use console_is_registered()
  netconsole: avoid CON_ENABLED misuse to track registration
  printk, xen: fbfront: create/use safe function for forcing preferred
  tty: tty_io: use console_list_lock for list synchronization
  proc: consoles: use console_list_lock for list iteration
  tty: serial: kgdboc: use console_list_lock for list traversal
  tty: serial: kgdboc: synchronize tty_find_polling_driver() and
    register_console()
  tty: serial: kgdboc: use console_list_lock to trap exit
  printk: relieve console_lock of list synchronization duties
  tty: serial: sh-sci: use setup() callback for early console

Thomas Gleixner (2):
  serial: kgdboc: Lock console list in probe function
  printk: Convert console_drivers list to hlist

 .clang-format                       |   1 +
 arch/m68k/emu/nfcon.c               |  10 +-
 arch/um/kernel/kmsg_dump.c          |  24 +-
 drivers/firmware/efi/earlycon.c     |   8 +-
 drivers/net/netconsole.c            |  21 +-
 drivers/tty/hvc/hvc_console.c       |   4 +-
 drivers/tty/serial/8250/8250_core.c |   2 +-
 drivers/tty/serial/earlycon.c       |   4 +-
 drivers/tty/serial/kgdboc.c         |  46 ++-
 drivers/tty/serial/pic32_uart.c     |   4 +-
 drivers/tty/serial/samsung_tty.c    |   2 +-
 drivers/tty/serial/serial_core.c    |  14 +-
 drivers/tty/serial/sh-sci.c         |  17 +-
 drivers/tty/serial/xilinx_uartps.c  |   2 +-
 drivers/tty/tty_io.c                |  18 +-
 drivers/usb/early/xhci-dbc.c        |   2 +-
 drivers/video/fbdev/xen-fbfront.c   |  12 +-
 fs/proc/consoles.c                  |  21 +-
 include/linux/console.h             | 111 +++++++-
 include/linux/rcupdate.h            |   5 +
 include/linux/serial_core.h         |  15 +-
 kernel/debug/kdb/kdb_io.c           |  14 +-
 kernel/printk/printk.c              | 424 +++++++++++++++++++++-------
 23 files changed, 605 insertions(+), 176 deletions(-)


base-commit: e29a4915db1480f96e0bc2e928699d086a71f43c
-- 
2.30.2

[PATCH printk v3 00/40] reduce console_lock scope

[John Ogness]

This is v3 of a series to prepare for threaded/atomic
printing. v2 is here [0]. This series focuses on reducing the
scope of the BKL console_lock. It achieves this by switching to
SRCU and a dedicated mutex for console list iteration and
modification, respectively. The console_lock will no longer
offer this protection and is completely removed from
(un)register_console() and console_stop/start() code.

Also, during the review of v2 it came to our attention that
many console drivers are checking CON_ENABLED to see if they
are registered. Because this flag can change without
unregistering and because this flag does not represent an
atomic point when an (un)registration process is complete,
a new console_is_registered() function is introduced. This
function uses the console_list_lock to synchronize with the
(un)registration process to provide a reliable status.

All users of the console_lock for list iteration have been
modified. For the call sites where the console_lock is still
needed (because of other reasons), comments are added to
explain exactly why the console_lock was needed.

All users of CON_ENABLED for registration status have been
modified to use console_is_registered(). Note that there are
still users of CON_ENABLED, but this is for legitimate purposes
about a registered console being able to print.

The base commit for this series is from Paul McKenney's RCU tree
and provides an NMI-safe SRCU implementation [1]. Without the
NMI-safe SRCU implementation, this series is not less safe than
mainline. But we will need the NMI-safe SRCU implementation for
atomic consoles anyway, so we might as well get it in
now. Especially since it _does_ increase the reliability for
mainline in the panic path.

Changes since v3:

general:

- introduce a synchronized console_is_registered() to query if
  a console is registered, meant to replace CON_ENABLED
  (mis)use for this purpose

- directly read console->flags for registered consoles if it is
  race-free (and document that it is so)

- replace uart_console_enabled() with a new
  uart_console_registered() based on console_is_registered()

- change comments about why console_lock is used to synchronize
  console->device() by providing an example

registration check fixups:

- the following drivers were modified to use the new
  console_is_registered() instead of CON_ENABLED checks

   - arch/m68k/emu/nfcon.c
   - drivers/firmware/efi/earlycon.c
   - drivers/net/netconsole.c
   - drivers/tty/hvc/hvc_console.c
   - drivers/tty/serial/8250/8250_core.c
   - drivers/tty/serial/earlycon.c
   - drivers/tty/serial/pic32_uart.c
   - drivers/tty/serial/samsung_tty.c
   - drivers/tty/serial/serial_core.c
   - drivers/tty/serial/xilinx_uartps.c
   - drivers/usb/early/xhci-dbc.c

um: kmsg_dumper:

- change stdout dump criteria to match original intention

kgdb/kdb:

- in configure_kgdboc(), take console_list_lock to synchronize
  tty_find_polling_driver() against register_console()

- add comments explaining why calling console->write() without
  locking might work

tty: sh-sci:

- use a setup() callback to setup the early console

fbdev: xen:

- implement a cleaner approach for
  console_force_preferred_locked()

rcu:

- implement debug_lockdep_rcu_enabled() for
  !CONFIG_DEBUG_LOCK_ALLOC

printk:

- check CONFIG_DEBUG_LOCK_ALLOC for srcu_read_lock_held()
  availability

- for console_lock/_trylock/_unlock, replace "lock the console
  system" language with "block the console subsystem from
  printing"

- use WRITE_ONCE() for updating console->flags of registered
  consoles

- expand comments of synchronize_srcu() calls to explain why
  they are needed, and also expand comments to explain when it
  is not needed

- change CON_BOOT consoles to always begin at earliest message

- for non-BOOT/non-PRINTBUFFER consoles, initialize @seq to the
  minimal @seq of any of the enabled boot consoles

- add comments and lockdep assertion to
  unregister_console_locked() because it is not clear from the
  name which lock is implied

- dropped patches that caused unnecessary churn in the series

John Ogness

[0] https://lore.kernel.org/lkml/20221019145600.1282823-1-john.ogness@linutronix.de
[1] https://git.kernel.org/pub/scm/linux/kernel/git/paulmck/linux-rcu.git/log/?h=srcunmisafe.2022.10.21a

John Ogness (38):
  rcu: implement lockdep_rcu_enabled for !CONFIG_DEBUG_LOCK_ALLOC
  printk: Prepare for SRCU console list protection
  printk: fix setting first seq for consoles
  um: kmsg_dump: only dump when no output console available
  console: introduce console_is_enabled() wrapper
  printk: use console_is_enabled()
  um: kmsg_dump: use console_is_enabled()
  kdb: kdb_io: use console_is_enabled()
  um: kmsg_dumper: use srcu console list iterator
  tty: serial: kgdboc: document console_lock usage
  tty: tty_io: document console_lock usage
  proc: consoles: document console_lock usage
  kdb: use srcu console list iterator
  printk: console_flush_all: use srcu console list iterator
  printk: console_unblank: use srcu console list iterator
  printk: console_flush_on_panic: use srcu console list iterator
  printk: console_device: use srcu console list iterator
  printk: __pr_flush: use srcu console list iterator
  printk: introduce console_list_lock
  console: introduce console_is_registered()
  serial_core: replace uart_console_enabled() with
    uart_console_registered()
  tty: nfcon: use console_is_registered()
  efi: earlycon: use console_is_registered()
  tty: hvc: use console_is_registered()
  tty: serial: earlycon: use console_is_registered()
  tty: serial: pic32_uart: use console_is_registered()
  tty: serial: samsung_tty: use console_is_registered()
  tty: serial: xilinx_uartps: use console_is_registered()
  usb: early: xhci-dbc: use console_is_registered()
  netconsole: avoid CON_ENABLED misuse to track registration
  printk, xen: fbfront: create/use safe function for forcing preferred
  tty: tty_io: use console_list_lock for list synchronization
  proc: consoles: use console_list_lock for list iteration
  tty: serial: kgdboc: use console_list_lock for list traversal
  tty: serial: kgdboc: synchronize tty_find_polling_driver() and
    register_console()
  tty: serial: kgdboc: use console_list_lock to trap exit
  printk: relieve console_lock of list synchronization duties
  tty: serial: sh-sci: use setup() callback for early console

Thomas Gleixner (2):
  serial: kgdboc: Lock console list in probe function
  printk: Convert console_drivers list to hlist

 .clang-format                       |   1 +
 arch/m68k/emu/nfcon.c               |  10 +-
 arch/um/kernel/kmsg_dump.c          |  24 +-
 drivers/firmware/efi/earlycon.c     |   8 +-
 drivers/net/netconsole.c            |  21 +-
 drivers/tty/hvc/hvc_console.c       |   4 +-
 drivers/tty/serial/8250/8250_core.c |   2 +-
 drivers/tty/serial/earlycon.c       |   4 +-
 drivers/tty/serial/kgdboc.c         |  46 ++-
 drivers/tty/serial/pic32_uart.c     |   4 +-
 drivers/tty/serial/samsung_tty.c    |   2 +-
 drivers/tty/serial/serial_core.c    |  14 +-
 drivers/tty/serial/sh-sci.c         |  17 +-
 drivers/tty/serial/xilinx_uartps.c  |   2 +-
 drivers/tty/tty_io.c                |  18 +-
 drivers/usb/early/xhci-dbc.c        |   2 +-
 drivers/video/fbdev/xen-fbfront.c   |  12 +-
 fs/proc/consoles.c                  |  21 +-
 include/linux/console.h             | 111 +++++++-
 include/linux/rcupdate.h            |   5 +
 include/linux/serial_core.h         |  15 +-
 kernel/debug/kdb/kdb_io.c           |  14 +-
 kernel/printk/printk.c              | 424 +++++++++++++++++++++-------
 23 files changed, 605 insertions(+), 176 deletions(-)


base-commit: e29a4915db1480f96e0bc2e928699d086a71f43c
-- 
2.30.2

[PATCH printk v3 01/40] rcu: implement lockdep_rcu_enabled for !CONFIG_DEBUG_LOCK_ALLOC

[John Ogness]

Provide an implementation for debug_lockdep_rcu_enabled() when
CONFIG_DEBUG_LOCK_ALLOC is not enabled. This allows code to check
if rcu lockdep debugging is available without needing an extra
check if CONFIG_DEBUG_LOCK_ALLOC is enabled.

Signed-off-by: John Ogness <john.ogness@linutronix.de>
---
 I also sent this patch to Paul as a suggestion. If it is not
 acceptable, I just need to add an ifdef CONFIG_DEBUG_LOCK_ALLOC
 into console_list_lock() of patch 21.

 include/linux/rcupdate.h | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
index 08605ce7379d..65178c40ab6f 100644
--- a/include/linux/rcupdate.h
+++ b/include/linux/rcupdate.h
@@ -340,6 +340,11 @@ static inline int rcu_read_lock_any_held(void)
 	return !preemptible();
 }
 
+static inline int debug_lockdep_rcu_enabled(void)
+{
+	return 0;
+}
+
 #endif /* #else #ifdef CONFIG_DEBUG_LOCK_ALLOC */
 
 #ifdef CONFIG_PROVE_RCU
-- 
2.30.2

Re: [PATCH printk v3 01/40] rcu: implement lockdep_rcu_enabled for !CONFIG_DEBUG_LOCK_ALLOC

[Paul E. McKenney]

On Mon, Nov 07, 2022 at 03:21:59PM +0106, John Ogness wrote:
> Provide an implementation for debug_lockdep_rcu_enabled() when
> CONFIG_DEBUG_LOCK_ALLOC is not enabled. This allows code to check
> if rcu lockdep debugging is available without needing an extra
> check if CONFIG_DEBUG_LOCK_ALLOC is enabled.
> 
> Signed-off-by: John Ogness <john.ogness@linutronix.de>

If you would like me to take this one, please let me know.

Otherwise:

Acked-by: Paul E. McKenney <paulmck@kernel.org>

> ---
>  I also sent this patch to Paul as a suggestion. If it is not
>  acceptable, I just need to add an ifdef CONFIG_DEBUG_LOCK_ALLOC
>  into console_list_lock() of patch 21.
> 
>  include/linux/rcupdate.h | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
> index 08605ce7379d..65178c40ab6f 100644
> --- a/include/linux/rcupdate.h
> +++ b/include/linux/rcupdate.h
> @@ -340,6 +340,11 @@ static inline int rcu_read_lock_any_held(void)
>  	return !preemptible();
>  }
>  
> +static inline int debug_lockdep_rcu_enabled(void)
> +{
> +	return 0;
> +}
> +
>  #endif /* #else #ifdef CONFIG_DEBUG_LOCK_ALLOC */
>  
>  #ifdef CONFIG_PROVE_RCU
> -- 
> 2.30.2
> 

Re: [PATCH printk v3 01/40] rcu: implement lockdep_rcu_enabled for !CONFIG_DEBUG_LOCK_ALLOC

[John Ogness]

On 2022-11-07, "Paul E. McKenney" <paulmck@kernel.org> wrote:
>> Provide an implementation for debug_lockdep_rcu_enabled() when
>> CONFIG_DEBUG_LOCK_ALLOC is not enabled. This allows code to check
>> if rcu lockdep debugging is available without needing an extra
>> check if CONFIG_DEBUG_LOCK_ALLOC is enabled.
>> 
>> Signed-off-by: John Ogness <john.ogness@linutronix.de>
>
> If you would like me to take this one, please let me know.

Yes, it would be great if you would carry this in the rcu tree. This
printk series is already relying on the rcu tree for the NMI-safe
work. Thanks!

John

Re: [PATCH printk v3 01/40] rcu: implement lockdep_rcu_enabled for !CONFIG_DEBUG_LOCK_ALLOC

[Paul E. McKenney]

On Mon, Nov 07, 2022 at 08:29:01PM +0106, John Ogness wrote:
> On 2022-11-07, "Paul E. McKenney" <paulmck@kernel.org> wrote:
> >> Provide an implementation for debug_lockdep_rcu_enabled() when
> >> CONFIG_DEBUG_LOCK_ALLOC is not enabled. This allows code to check
> >> if rcu lockdep debugging is available without needing an extra
> >> check if CONFIG_DEBUG_LOCK_ALLOC is enabled.
> >> 
> >> Signed-off-by: John Ogness <john.ogness@linutronix.de>
> >
> > If you would like me to take this one, please let me know.
> 
> Yes, it would be great if you would carry this in the rcu tree. This
> printk series is already relying on the rcu tree for the NMI-safe
> work. Thanks!

Very good, I have queued it.  It is currently on -rcu branch "dev",
but will find its way to srcunmisafe.2022.10.21a in the next day or two.

							Thanx, Paul

Re: [PATCH printk v3 01/40] rcu: implement lockdep_rcu_enabled for !CONFIG_DEBUG_LOCK_ALLOC

[Paul E. McKenney]

On Tue, Nov 08, 2022 at 11:27:24AM -0800, Paul E. McKenney wrote:
> On Mon, Nov 07, 2022 at 08:29:01PM +0106, John Ogness wrote:
> > On 2022-11-07, "Paul E. McKenney" <paulmck@kernel.org> wrote:
> > >> Provide an implementation for debug_lockdep_rcu_enabled() when
> > >> CONFIG_DEBUG_LOCK_ALLOC is not enabled. This allows code to check
> > >> if rcu lockdep debugging is available without needing an extra
> > >> check if CONFIG_DEBUG_LOCK_ALLOC is enabled.
> > >> 
> > >> Signed-off-by: John Ogness <john.ogness@linutronix.de>
> > >
> > > If you would like me to take this one, please let me know.
> > 
> > Yes, it would be great if you would carry this in the rcu tree. This
> > printk series is already relying on the rcu tree for the NMI-safe
> > work. Thanks!
> 
> Very good, I have queued it.  It is currently on -rcu branch "dev",
> but will find its way to srcunmisafe.2022.10.21a in the next day or two.

But make that srcunmisafe.2022.11.09a, where it now resides.

The old srcunmisafe.2022.10.21a remains in its previous location, just
in case someone else is using it.  Branches are cheap...

							Thanx, Paul

Re: [PATCH printk v3 01/40] rcu: implement lockdep_rcu_enabled for !CONFIG_DEBUG_LOCK_ALLOC

[Petr Mladek]

On Mon 2022-11-07 15:21:59, John Ogness wrote:
> Provide an implementation for debug_lockdep_rcu_enabled() when
> CONFIG_DEBUG_LOCK_ALLOC is not enabled. This allows code to check
> if rcu lockdep debugging is available without needing an extra
> check if CONFIG_DEBUG_LOCK_ALLOC is enabled.
> 
> Signed-off-by: John Ogness <john.ogness@linutronix.de>

Reviewed-by: Petr Mladek <pmladek@suse.com>

Just a small nit below.

> ---
>  I also sent this patch to Paul as a suggestion. If it is not
>  acceptable, I just need to add an ifdef CONFIG_DEBUG_LOCK_ALLOC
>  into console_list_lock() of patch 21.
> 
>  include/linux/rcupdate.h | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
> index 08605ce7379d..65178c40ab6f 100644
> --- a/include/linux/rcupdate.h
> +++ b/include/linux/rcupdate.h
> @@ -340,6 +340,11 @@ static inline int rcu_read_lock_any_held(void)
>  	return !preemptible();
>  }
>  
> +static inline int debug_lockdep_rcu_enabled(void)
> +{
> +	return 0;
> +}
> +

It would make sense to move this up before rcu_read_lock_held()
definition so that the declarations and definitions are in
the same order in both #ifdef CONFIG_DEBUG_LOCK_ALLOC branches.

>  #endif /* #else #ifdef CONFIG_DEBUG_LOCK_ALLOC */
>  
>  #ifdef CONFIG_PROVE_RCU

Best Regards,
PEtr

[PATCH printk v3 02/40] serial: kgdboc: Lock console list in probe function

[John Ogness]

From: Thomas Gleixner <tglx@linutronix.de>

Unprotected list walks are not necessarily safe.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Signed-off-by: John Ogness <john.ogness@linutronix.de>
Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Reviewed-by: Douglas Anderson <dianders@chromium.org>
Reviewed-by: Sergey Senozhatsky <senozhatsky@chromium.org>
Reviewed-by: Petr Mladek <pmladek@suse.com>
---
 drivers/tty/serial/kgdboc.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/tty/serial/kgdboc.c b/drivers/tty/serial/kgdboc.c
index 7aa37be3216a..e76f0186c335 100644
--- a/drivers/tty/serial/kgdboc.c
+++ b/drivers/tty/serial/kgdboc.c
@@ -193,6 +193,7 @@ static int configure_kgdboc(void)
 	if (!p)
 		goto noconfig;
 
+	console_lock();
 	for_each_console(cons) {
 		int idx;
 		if (cons->device && cons->device(cons, &idx) == p &&
@@ -201,6 +202,7 @@ static int configure_kgdboc(void)
 			break;
 		}
 	}
+	console_unlock();
 
 	kgdb_tty_driver = p;
 	kgdb_tty_line = tty_line;
-- 
2.30.2

Re: [PATCH printk v3 02/40] serial: kgdboc: Lock console list in probe function

[Daniel Thompson]

On Mon, Nov 07, 2022 at 03:22:00PM +0106, John Ogness wrote:
> From: Thomas Gleixner <tglx@linutronix.de>
>
> Unprotected list walks are not necessarily safe.
>
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> Signed-off-by: John Ogness <john.ogness@linutronix.de>
> Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Reviewed-by: Douglas Anderson <dianders@chromium.org>
> Reviewed-by: Sergey Senozhatsky <senozhatsky@chromium.org>
> Reviewed-by: Petr Mladek <pmladek@suse.com>

Reviewed-by: Daniel Thompson <daniel.thompson@linaro.org>


Daniel.

[PATCH printk v3 03/40] printk: Convert console_drivers list to hlist

[John Ogness]

From: Thomas Gleixner <tglx@linutronix.de>

Replace the open coded single linked list with a hlist so a conversion
to SRCU protected list walks can reuse the existing primitives.

Co-developed-by: John Ogness <jogness@linutronix.de>
Signed-off-by: John Ogness <john.ogness@linutronix.de>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Reviewed-by: Sergey Senozhatsky <senozhatsky@chromium.org>
Reviewed-by: Petr Mladek <pmladek@suse.com>
---
 fs/proc/consoles.c      |   3 +-
 include/linux/console.h |   8 ++--
 kernel/printk/printk.c  | 101 ++++++++++++++++++++++------------------
 3 files changed, 62 insertions(+), 50 deletions(-)

diff --git a/fs/proc/consoles.c b/fs/proc/consoles.c
index dfe6ce3505ce..cf2e0788f9c7 100644
--- a/fs/proc/consoles.c
+++ b/fs/proc/consoles.c
@@ -74,8 +74,9 @@ static void *c_start(struct seq_file *m, loff_t *pos)
 static void *c_next(struct seq_file *m, void *v, loff_t *pos)
 {
 	struct console *con = v;
+
 	++*pos;
-	return con->next;
+	return hlist_entry_safe(con->node.next, struct console, node);
 }
 
 static void c_stop(struct seq_file *m, void *v)
diff --git a/include/linux/console.h b/include/linux/console.h
index 8c1686e2c233..7b5f21f9e469 100644
--- a/include/linux/console.h
+++ b/include/linux/console.h
@@ -15,6 +15,7 @@
 #define _LINUX_CONSOLE_H_ 1
 
 #include <linux/atomic.h>
+#include <linux/list.h>
 #include <linux/types.h>
 
 struct vc_data;
@@ -154,14 +155,16 @@ struct console {
 	u64	seq;
 	unsigned long dropped;
 	void	*data;
-	struct	 console *next;
+	struct hlist_node node;
 };
 
+extern struct hlist_head console_list;
+
 /*
  * for_each_console() allows you to iterate on each console
  */
 #define for_each_console(con) \
-	for (con = console_drivers; con != NULL; con = con->next)
+	hlist_for_each_entry(con, &console_list, node)
 
 extern int console_set_on_cmdline;
 extern struct console *early_console;
@@ -174,7 +177,6 @@ enum con_flush_mode {
 extern int add_preferred_console(char *name, int idx, char *options);
 extern void register_console(struct console *);
 extern int unregister_console(struct console *);
-extern struct console *console_drivers;
 extern void console_lock(void);
 extern int console_trylock(void);
 extern void console_unlock(void);
diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index e4f1e7478b52..e6f0832e71f0 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -79,13 +79,12 @@ int oops_in_progress;
 EXPORT_SYMBOL(oops_in_progress);
 
 /*
- * console_sem protects the console_drivers list, and also
- * provides serialisation for access to the entire console
- * driver system.
+ * console_sem protects console_list and console->flags updates, and also
+ * provides serialization for access to the entire console driver system.
  */
 static DEFINE_SEMAPHORE(console_sem);
-struct console *console_drivers;
-EXPORT_SYMBOL_GPL(console_drivers);
+HLIST_HEAD(console_list);
+EXPORT_SYMBOL_GPL(console_list);
 
 /*
  * System may need to suppress printk message under certain
@@ -2556,7 +2555,7 @@ static int console_cpu_notify(unsigned int cpu)
  * console_lock - lock the console system for exclusive use.
  *
  * Acquires a lock which guarantees that the caller has
- * exclusive access to the console system and the console_drivers list.
+ * exclusive access to the console system and console_list.
  *
  * Can sleep, returns nothing.
  */
@@ -2576,7 +2575,7 @@ EXPORT_SYMBOL(console_lock);
  * console_trylock - try to lock the console system for exclusive use.
  *
  * Try to acquire a lock which guarantees that the caller has exclusive
- * access to the console system and the console_drivers list.
+ * access to the console system and console_list.
  *
  * returns 1 on success, and 0 on failure to acquire the lock.
  */
@@ -2940,11 +2939,20 @@ void console_flush_on_panic(enum con_flush_mode mode)
 	console_may_schedule = 0;
 
 	if (mode == CONSOLE_REPLAY_ALL) {
+		struct hlist_node *tmp;
 		struct console *c;
 		u64 seq;
 
 		seq = prb_first_valid_seq(prb);
-		for_each_console(c)
+		/*
+		 * This cannot use for_each_console() because it's not established
+		 * that the current context has console locked and neither there is
+		 * a guarantee that there is no concurrency in that case.
+		 *
+		 * Open code it for documentation purposes and pretend that
+		 * it works.
+		 */
+		hlist_for_each_entry_safe(c, tmp, &console_list, node)
 			c->seq = seq;
 	}
 	console_unlock();
@@ -3081,6 +3089,9 @@ static void try_enable_default_console(struct console *newcon)
 	       (con->flags & CON_BOOT) ? "boot" : "",	\
 	       con->name, con->index, ##__VA_ARGS__)
 
+#define console_first()				\
+	hlist_entry(console_list.first, struct console, node)
+
 /*
  * The console driver calls this routine during kernel initialization
  * to register the console printing procedure with printk() and to
@@ -3140,8 +3151,8 @@ void register_console(struct console *newcon)
 	 * flag set and will be first in the list.
 	 */
 	if (preferred_console < 0) {
-		if (!console_drivers || !console_drivers->device ||
-		    console_drivers->flags & CON_BOOT) {
+		if (hlist_empty(&console_list) || !console_first()->device ||
+		    console_first()->flags & CON_BOOT) {
 			try_enable_default_console(newcon);
 		}
 	}
@@ -3169,20 +3180,22 @@ void register_console(struct console *newcon)
 	}
 
 	/*
-	 *	Put this console in the list - keep the
-	 *	preferred driver at the head of the list.
+	 * Put this console in the list - keep the
+	 * preferred driver at the head of the list.
 	 */
 	console_lock();
-	if ((newcon->flags & CON_CONSDEV) || console_drivers == NULL) {
-		newcon->next = console_drivers;
-		console_drivers = newcon;
-		if (newcon->next)
-			newcon->next->flags &= ~CON_CONSDEV;
-		/* Ensure this flag is always set for the head of the list */
+	if (hlist_empty(&console_list)) {
+		/* Ensure CON_CONSDEV is always set for the head. */
 		newcon->flags |= CON_CONSDEV;
+		hlist_add_head(&newcon->node, &console_list);
+
+	} else if (newcon->flags & CON_CONSDEV) {
+		/* Only the new head can have CON_CONSDEV set. */
+		console_first()->flags &= ~CON_CONSDEV;
+		hlist_add_head(&newcon->node, &console_list);
+
 	} else {
-		newcon->next = console_drivers->next;
-		console_drivers->next = newcon;
+		hlist_add_behind(&newcon->node, console_list.first);
 	}
 
 	newcon->dropped = 0;
@@ -3209,16 +3222,18 @@ void register_console(struct console *newcon)
 	if (bootcon_enabled &&
 	    ((newcon->flags & (CON_CONSDEV | CON_BOOT)) == CON_CONSDEV) &&
 	    !keep_bootcon) {
-		for_each_console(con)
+		struct hlist_node *tmp;
+
+		hlist_for_each_entry_safe(con, tmp, &console_list, node) {
 			if (con->flags & CON_BOOT)
 				unregister_console(con);
+		}
 	}
 }
 EXPORT_SYMBOL(register_console);
 
 int unregister_console(struct console *console)
 {
-	struct console *con;
 	int res;
 
 	con_printk(KERN_INFO, console, "disabled\n");
@@ -3229,32 +3244,30 @@ int unregister_console(struct console *console)
 	if (res > 0)
 		return 0;
 
-	res = -ENODEV;
 	console_lock();
-	if (console_drivers == console) {
-		console_drivers=console->next;
-		res = 0;
-	} else {
-		for_each_console(con) {
-			if (con->next == console) {
-				con->next = console->next;
-				res = 0;
-				break;
-			}
-		}
+
+	/* Disable it unconditionally */
+	console->flags &= ~CON_ENABLED;
+
+	if (hlist_unhashed(&console->node)) {
+		console_unlock();
+		return -ENODEV;
 	}
 
-	if (res)
-		goto out_disable_unlock;
+	hlist_del_init(&console->node);
 
 	/*
+	 * <HISTORICAL>
 	 * If this isn't the last console and it has CON_CONSDEV set, we
 	 * need to set it on the next preferred console.
+	 * </HISTORICAL>
+	 *
+	 * The above makes no sense as there is no guarantee that the next
+	 * console has any device attached. Oh well....
 	 */
-	if (console_drivers != NULL && console->flags & CON_CONSDEV)
-		console_drivers->flags |= CON_CONSDEV;
+	if (!hlist_empty(&console_list) && console->flags & CON_CONSDEV)
+		console_first()->flags |= CON_CONSDEV;
 
-	console->flags &= ~CON_ENABLED;
 	console_unlock();
 	console_sysfs_notify();
 
@@ -3262,12 +3275,6 @@ int unregister_console(struct console *console)
 		res = console->exit(console);
 
 	return res;
-
-out_disable_unlock:
-	console->flags &= ~CON_ENABLED;
-	console_unlock();
-
-	return res;
 }
 EXPORT_SYMBOL(unregister_console);
 
@@ -3317,10 +3324,11 @@ void __init console_init(void)
  */
 static int __init printk_late_init(void)
 {
+	struct hlist_node *tmp;
 	struct console *con;
 	int ret;
 
-	for_each_console(con) {
+	hlist_for_each_entry_safe(con, tmp, &console_list, node) {
 		if (!(con->flags & CON_BOOT))
 			continue;
 
@@ -3340,6 +3348,7 @@ static int __init printk_late_init(void)
 			unregister_console(con);
 		}
 	}
+
 	ret = cpuhp_setup_state_nocalls(CPUHP_PRINTK_DEAD, "printk:dead", NULL,
 					console_cpu_notify);
 	WARN_ON(ret < 0);
-- 
2.30.2

[PATCH printk v3 04/40] printk: Prepare for SRCU console list protection

[John Ogness]

Provide an NMI-safe SRCU protected variant to walk the console list.

Note that all console fields are now set before adding the console
to the list to avoid the console becoming visible by SCRU readers
before being fully initialized.

This is a preparatory change for a new console infrastructure which
operates independent of the console BKL.

Suggested-by: Thomas Gleixner <tglx@linutronix.de>
Signed-off-by: John Ogness <john.ogness@linutronix.de>
Acked-by: Miguel Ojeda <ojeda@kernel.org>
Reviewed-by: Paul E. McKenney <paulmck@kernel.org>
---
 .clang-format           |  1 +
 include/linux/console.h | 28 ++++++++++++-
 kernel/printk/printk.c  | 87 ++++++++++++++++++++++++++++++++++-------
 3 files changed, 100 insertions(+), 16 deletions(-)

diff --git a/.clang-format b/.clang-format
index 1247d54f9e49..04a675b56b57 100644
--- a/.clang-format
+++ b/.clang-format
@@ -222,6 +222,7 @@ ForEachMacros:
   - 'for_each_component_dais'
   - 'for_each_component_dais_safe'
   - 'for_each_console'
+  - 'for_each_console_srcu'
   - 'for_each_cpu'
   - 'for_each_cpu_and'
   - 'for_each_cpu_not'
diff --git a/include/linux/console.h b/include/linux/console.h
index 7b5f21f9e469..f4f0c9523835 100644
--- a/include/linux/console.h
+++ b/include/linux/console.h
@@ -15,7 +15,7 @@
 #define _LINUX_CONSOLE_H_ 1
 
 #include <linux/atomic.h>
-#include <linux/list.h>
+#include <linux/rculist.h>
 #include <linux/types.h>
 
 struct vc_data;
@@ -158,8 +158,34 @@ struct console {
 	struct hlist_node node;
 };
 
+#ifdef CONFIG_DEBUG_LOCK_ALLOC
+extern bool console_srcu_read_lock_is_held(void);
+#else
+static inline bool console_srcu_read_lock_is_held(void)
+{
+	return 1;
+}
+#endif
+
+extern int console_srcu_read_lock(void);
+extern void console_srcu_read_unlock(int cookie);
+
 extern struct hlist_head console_list;
 
+/**
+ * for_each_console_srcu() - Iterator over registered consoles
+ * @con:	struct console pointer used as loop cursor
+ *
+ * Although SRCU guarantees the console list will be consistent, the
+ * struct console fields may be updated by other CPUs while iterating.
+ *
+ * Requires console_srcu_read_lock to be held. Can be invoked from
+ * any context.
+ */
+#define for_each_console_srcu(con)					\
+	hlist_for_each_entry_srcu(con, &console_list, node,		\
+				  console_srcu_read_lock_is_held())
+
 /*
  * for_each_console() allows you to iterate on each console
  */
diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index e6f0832e71f0..173f46a29252 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -85,6 +85,7 @@ EXPORT_SYMBOL(oops_in_progress);
 static DEFINE_SEMAPHORE(console_sem);
 HLIST_HEAD(console_list);
 EXPORT_SYMBOL_GPL(console_list);
+DEFINE_STATIC_SRCU(console_srcu);
 
 /*
  * System may need to suppress printk message under certain
@@ -104,6 +105,13 @@ static struct lockdep_map console_lock_dep_map = {
 };
 #endif
 
+#ifdef CONFIG_DEBUG_LOCK_ALLOC
+bool console_srcu_read_lock_is_held(void)
+{
+	return srcu_read_lock_held(&console_srcu);
+}
+#endif
+
 enum devkmsg_log_bits {
 	__DEVKMSG_LOG_BIT_ON = 0,
 	__DEVKMSG_LOG_BIT_OFF,
@@ -219,6 +227,32 @@ int devkmsg_sysctl_set_loglvl(struct ctl_table *table, int write,
 }
 #endif /* CONFIG_PRINTK && CONFIG_SYSCTL */
 
+/**
+ * console_srcu_read_lock - Register a new reader for the
+ *	SRCU-protected console list
+ *
+ * Use for_each_console_srcu() to iterate the console list
+ *
+ * Context: Any context.
+ */
+int console_srcu_read_lock(void)
+{
+	return srcu_read_lock_nmisafe(&console_srcu);
+}
+EXPORT_SYMBOL(console_srcu_read_lock);
+
+/**
+ * console_srcu_read_unlock - Unregister an old reader from
+ *	the SRCU-protected console list
+ *
+ * Counterpart to console_srcu_read_lock()
+ */
+void console_srcu_read_unlock(int cookie)
+{
+	srcu_read_unlock_nmisafe(&console_srcu, cookie);
+}
+EXPORT_SYMBOL(console_srcu_read_unlock);
+
 /*
  * Helper macros to handle lockdep when locking/unlocking console_sem. We use
  * macros instead of functions so that _RET_IP_ contains useful information.
@@ -2989,6 +3023,14 @@ void console_stop(struct console *console)
 	console_lock();
 	console->flags &= ~CON_ENABLED;
 	console_unlock();
+
+	/*
+	 * Ensure that all SRCU list walks have completed. All contexts must
+	 * be able to see that this console is disabled so that (for example)
+	 * the caller can suspend the port without risk of another context
+	 * using the port.
+	 */
+	synchronize_srcu(&console_srcu);
 }
 EXPORT_SYMBOL(console_stop);
 
@@ -3179,6 +3221,17 @@ void register_console(struct console *newcon)
 		newcon->flags &= ~CON_PRINTBUFFER;
 	}
 
+	newcon->dropped = 0;
+	if (newcon->flags & CON_PRINTBUFFER) {
+		/* Get a consistent copy of @syslog_seq. */
+		mutex_lock(&syslog_lock);
+		newcon->seq = syslog_seq;
+		mutex_unlock(&syslog_lock);
+	} else {
+		/* Begin with next message. */
+		newcon->seq = prb_next_seq(prb);
+	}
+
 	/*
 	 * Put this console in the list - keep the
 	 * preferred driver at the head of the list.
@@ -3187,28 +3240,24 @@ void register_console(struct console *newcon)
 	if (hlist_empty(&console_list)) {
 		/* Ensure CON_CONSDEV is always set for the head. */
 		newcon->flags |= CON_CONSDEV;
-		hlist_add_head(&newcon->node, &console_list);
+		hlist_add_head_rcu(&newcon->node, &console_list);
 
 	} else if (newcon->flags & CON_CONSDEV) {
 		/* Only the new head can have CON_CONSDEV set. */
 		console_first()->flags &= ~CON_CONSDEV;
-		hlist_add_head(&newcon->node, &console_list);
+		hlist_add_head_rcu(&newcon->node, &console_list);
 
 	} else {
-		hlist_add_behind(&newcon->node, console_list.first);
-	}
-
-	newcon->dropped = 0;
-	if (newcon->flags & CON_PRINTBUFFER) {
-		/* Get a consistent copy of @syslog_seq. */
-		mutex_lock(&syslog_lock);
-		newcon->seq = syslog_seq;
-		mutex_unlock(&syslog_lock);
-	} else {
-		/* Begin with next message. */
-		newcon->seq = prb_next_seq(prb);
+		hlist_add_behind_rcu(&newcon->node, console_list.first);
 	}
 	console_unlock();
+
+	/*
+	 * No need to synchronize SRCU here! The caller does not rely
+	 * on all contexts being able to see the new console before
+	 * register_console() completes.
+	 */
+
 	console_sysfs_notify();
 
 	/*
@@ -3254,7 +3303,7 @@ int unregister_console(struct console *console)
 		return -ENODEV;
 	}
 
-	hlist_del_init(&console->node);
+	hlist_del_init_rcu(&console->node);
 
 	/*
 	 * <HISTORICAL>
@@ -3269,6 +3318,14 @@ int unregister_console(struct console *console)
 		console_first()->flags |= CON_CONSDEV;
 
 	console_unlock();
+
+	/*
+	 * Ensure that all SRCU list walks have completed. All contexts
+	 * must not be able to see this console in the list so that any
+	 * exit/cleanup routines can be performed safely.
+	 */
+	synchronize_srcu(&console_srcu);
+
 	console_sysfs_notify();
 
 	if (console->exit)
-- 
2.30.2

Re: [PATCH printk v3 04/40] printk: Prepare for SRCU console list protection

[Petr Mladek]

On Mon 2022-11-07 15:22:02, John Ogness wrote:
> Provide an NMI-safe SRCU protected variant to walk the console list.
> 
> Note that all console fields are now set before adding the console
> to the list to avoid the console becoming visible by SCRU readers
> before being fully initialized.
> 
> This is a preparatory change for a new console infrastructure which
> operates independent of the console BKL.
> 
> Suggested-by: Thomas Gleixner <tglx@linutronix.de>
> Signed-off-by: John Ogness <john.ogness@linutronix.de>
> Acked-by: Miguel Ojeda <ojeda@kernel.org>
> Reviewed-by: Paul E. McKenney <paulmck@kernel.org>

Reviewed-by: Petr Mladek <pmladek@suse.com>

Best Regards,
Petr

[PATCH printk v3 05/40] printk: fix setting first seq for consoles

[John Ogness]

It used to be that all consoles were synchronized with respect to
which message they were printing. After commit a699449bb13b ("printk:
refactor and rework printing logic"), all consoles have their own
@seq for tracking which message they are on. That commit also changed
how the initial sequence number was chosen. Instead of choosing the
next non-printed message, it chose the sequence number of the next
message that will be added to the ringbuffer.

That change created a possibility that a non-boot console taking over
for a boot console might skip messages if the boot console was behind
and did not have a chance to catch up before being unregistered.

Since it is not possible to know which boot console a console is
taking over, use the lowest @seq of all the enabled boot consoles. If
no boot consoles are available/enabled, begin with the next message
that will be added to the ringbuffer.

Also, since boot consoles are meant to be used at boot time, handle
them the same as CON_PRINTBUFFER to ensure that no initial messages
are skipped.

Signed-off-by: John Ogness <john.ogness@linutronix.de>
---
 kernel/printk/printk.c | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index 173f46a29252..8974523f3107 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -3222,14 +3222,26 @@ void register_console(struct console *newcon)
 	}
 
 	newcon->dropped = 0;
-	if (newcon->flags & CON_PRINTBUFFER) {
+	if (newcon->flags & (CON_PRINTBUFFER | CON_BOOT)) {
 		/* Get a consistent copy of @syslog_seq. */
 		mutex_lock(&syslog_lock);
 		newcon->seq = syslog_seq;
 		mutex_unlock(&syslog_lock);
 	} else {
-		/* Begin with next message. */
+		/* Begin with next message added to ringbuffer. */
 		newcon->seq = prb_next_seq(prb);
+
+		/*
+		 * If an enabled boot console is not caught up, start with
+		 * that message instead. That boot console will be
+		 * unregistered shortly and may be the same device.
+		 */
+		for_each_console(con) {
+			if ((con->flags & (CON_BOOT | CON_ENABLED)) == (CON_BOOT | CON_ENABLED) &&
+			    con->seq < newcon->seq) {
+				newcon->seq = con->seq;
+			}
+		}
 	}
 
 	/*
-- 
2.30.2

Re: [PATCH printk v3 05/40] printk: fix setting first seq for consoles

[Petr Mladek]

On Mon 2022-11-07 15:22:03, John Ogness wrote:
> It used to be that all consoles were synchronized with respect to
> which message they were printing. After commit a699449bb13b ("printk:
> refactor and rework printing logic"), all consoles have their own
> @seq for tracking which message they are on. That commit also changed
> how the initial sequence number was chosen. Instead of choosing the
> next non-printed message, it chose the sequence number of the next
> message that will be added to the ringbuffer.
> 
> That change created a possibility that a non-boot console taking over
> for a boot console might skip messages if the boot console was behind
> and did not have a chance to catch up before being unregistered.
> 
> Since it is not possible to know which boot console a console is
> taking over, use the lowest @seq of all the enabled boot consoles. If
> no boot consoles are available/enabled, begin with the next message
> that will be added to the ringbuffer.
> 
> Also, since boot consoles are meant to be used at boot time, handle
> them the same as CON_PRINTBUFFER to ensure that no initial messages
> are skipped.
> 
> Signed-off-by: John Ogness <john.ogness@linutronix.de>

Reviewed-by: Petr Mladek <pmladek@suse.com>

Best Regards,
Petr

[PATCH printk v3 06/40] um: kmsg_dump: only dump when no output console available

[John Ogness]

The initial intention of the UML kmsg_dumper is to dump the kernel
buffers to stdout if there is no console available to perform the
regular crash output.

However, if ttynull was registered as a console, no crash output was
seen. Commit e23fe90dec28 ("um: kmsg_dumper: always dump when not tty
console") tried to fix this by performing the kmsg_dump unless the
stdio console was behind /dev/console or enabled. But this allowed
kmsg dumping to occur even if other non-stdio consoles will output
the crash output. Also, a console being the driver behind
/dev/console has nothing to do with a crash scenario.

Restore the initial intention by dumping the kernel buffers to stdout
only if a non-ttynull console is registered and enabled. Also add
detailed comments so that it is clear why these rules are applied.

Signed-off-by: John Ogness <john.ogness@linutronix.de>
---
 arch/um/kernel/kmsg_dump.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/arch/um/kernel/kmsg_dump.c b/arch/um/kernel/kmsg_dump.c
index 0224fcb36e22..40abf1e9ccb1 100644
--- a/arch/um/kernel/kmsg_dump.c
+++ b/arch/um/kernel/kmsg_dump.c
@@ -17,13 +17,22 @@ static void kmsg_dumper_stdout(struct kmsg_dumper *dumper,
 	unsigned long flags;
 	size_t len = 0;
 
-	/* only dump kmsg when no console is available */
+	/*
+	 * If no consoles are available to output crash information, dump
+	 * the kmsg buffer to stdout.
+	 */
+
 	if (!console_trylock())
 		return;
 
 	for_each_console(con) {
-		if(strcmp(con->name, "tty") == 0 &&
-		   (con->flags & (CON_ENABLED | CON_CONSDEV)) != 0) {
+		/*
+		 * The ttynull console and disabled consoles are ignored
+		 * since they cannot output. All other consoles are
+		 * expected to output the crash information.
+		 */
+		if (strcmp(con->name, "ttynull") != 0 &&
+		    (con->flags & CON_ENABLED)) {
 			break;
 		}
 	}
-- 
2.30.2

[PATCH printk v3 06/40] um: kmsg_dump: only dump when no output console available

[John Ogness]

The initial intention of the UML kmsg_dumper is to dump the kernel
buffers to stdout if there is no console available to perform the
regular crash output.

However, if ttynull was registered as a console, no crash output was
seen. Commit e23fe90dec28 ("um: kmsg_dumper: always dump when not tty
console") tried to fix this by performing the kmsg_dump unless the
stdio console was behind /dev/console or enabled. But this allowed
kmsg dumping to occur even if other non-stdio consoles will output
the crash output. Also, a console being the driver behind
/dev/console has nothing to do with a crash scenario.

Restore the initial intention by dumping the kernel buffers to stdout
only if a non-ttynull console is registered and enabled. Also add
detailed comments so that it is clear why these rules are applied.

Signed-off-by: John Ogness <john.ogness@linutronix.de>
---
 arch/um/kernel/kmsg_dump.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/arch/um/kernel/kmsg_dump.c b/arch/um/kernel/kmsg_dump.c
index 0224fcb36e22..40abf1e9ccb1 100644
--- a/arch/um/kernel/kmsg_dump.c
+++ b/arch/um/kernel/kmsg_dump.c
@@ -17,13 +17,22 @@ static void kmsg_dumper_stdout(struct kmsg_dumper *dumper,
 	unsigned long flags;
 	size_t len = 0;
 
-	/* only dump kmsg when no console is available */
+	/*
+	 * If no consoles are available to output crash information, dump
+	 * the kmsg buffer to stdout.
+	 */
+
 	if (!console_trylock())
 		return;
 
 	for_each_console(con) {
-		if(strcmp(con->name, "tty") == 0 &&
-		   (con->flags & (CON_ENABLED | CON_CONSDEV)) != 0) {
+		/*
+		 * The ttynull console and disabled consoles are ignored
+		 * since they cannot output. All other consoles are
+		 * expected to output the crash information.
+		 */
+		if (strcmp(con->name, "ttynull") != 0 &&
+		    (con->flags & CON_ENABLED)) {
 			break;
 		}
 	}
-- 
2.30.2


_______________________________________________
linux-um mailing list
linux-um@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-um

Re: [PATCH printk v3 06/40] um: kmsg_dump: only dump when no output console available

[Petr Mladek]

On Mon 2022-11-07 15:22:04, John Ogness wrote:
> The initial intention of the UML kmsg_dumper is to dump the kernel
> buffers to stdout if there is no console available to perform the
> regular crash output.
> 
> However, if ttynull was registered as a console, no crash output was
> seen. Commit e23fe90dec28 ("um: kmsg_dumper: always dump when not tty
> console") tried to fix this by performing the kmsg_dump unless the
> stdio console was behind /dev/console or enabled. But this allowed
> kmsg dumping to occur even if other non-stdio consoles will output
> the crash output. Also, a console being the driver behind
> /dev/console has nothing to do with a crash scenario.
> 
> Restore the initial intention by dumping the kernel buffers to stdout
> only if a non-ttynull console is registered and enabled. Also add
> detailed comments so that it is clear why these rules are applied.
> 
> Signed-off-by: John Ogness <john.ogness@linutronix.de>

The change makes sense to me:

Reviewed-by: Petr Mladek <pmladek@suse.com>

Best Regards,
Petr

Re: [PATCH printk v3 06/40] um: kmsg_dump: only dump when no output console available

[Petr Mladek]

On Mon 2022-11-07 15:22:04, John Ogness wrote:
> The initial intention of the UML kmsg_dumper is to dump the kernel
> buffers to stdout if there is no console available to perform the
> regular crash output.
> 
> However, if ttynull was registered as a console, no crash output was
> seen. Commit e23fe90dec28 ("um: kmsg_dumper: always dump when not tty
> console") tried to fix this by performing the kmsg_dump unless the
> stdio console was behind /dev/console or enabled. But this allowed
> kmsg dumping to occur even if other non-stdio consoles will output
> the crash output. Also, a console being the driver behind
> /dev/console has nothing to do with a crash scenario.
> 
> Restore the initial intention by dumping the kernel buffers to stdout
> only if a non-ttynull console is registered and enabled. Also add
> detailed comments so that it is clear why these rules are applied.
> 
> Signed-off-by: John Ogness <john.ogness@linutronix.de>

The change makes sense to me:

Reviewed-by: Petr Mladek <pmladek@suse.com>

Best Regards,
Petr

_______________________________________________
linux-um mailing list
linux-um@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-um

[PATCH printk v3 07/40] console: introduce console_is_enabled() wrapper

[John Ogness]

After switching to SRCU for console list iteration, some readers
will begin readings console->flags as a data race. Locklessly
reading console->flags provides a consistent value because there
is at most one CPU modifying console->flags and that CPU is
using only read-modify-write operations.

The primary reason for readers to access console->flags is to
check if the console is enabled. Introduce console_is_enabled()
to mark such access as a data race.

Signed-off-by: John Ogness <john.ogness@linutronix.de>
---
 include/linux/console.h | 27 +++++++++++++++++++++++++++
 kernel/printk/printk.c  | 10 +++++-----
 2 files changed, 32 insertions(+), 5 deletions(-)

diff --git a/include/linux/console.h b/include/linux/console.h
index f4f0c9523835..d9c636011364 100644
--- a/include/linux/console.h
+++ b/include/linux/console.h
@@ -172,6 +172,33 @@ extern void console_srcu_read_unlock(int cookie);
 
 extern struct hlist_head console_list;
 
+/**
+ * console_is_enabled - Locklessly check if the console is enabled
+ * @con:	struct console pointer of console to check
+ *
+ * Unless the caller is explicitly synchronizing against the console
+ * register/unregister/stop/start functions, this function should be
+ * used instead of manually readings console->flags and testing for
+ * the CON_ENABLED bit.
+ *
+ * This function provides the necessary READ_ONCE() and data_race()
+ * notation for locklessly reading the console flags. The READ_ONCE()
+ * in this function matches the WRITE_ONCE() when @flags are modified
+ * for registered consoles.
+ *
+ * Context: Any context.
+ */
+static inline bool console_is_enabled(const struct console *con)
+{
+	/*
+	 * Locklessly reading console->flags provides a consistent
+	 * read value because there is at most one CPU modifying
+	 * console->flags and that CPU is using only read-modify-write
+	 * operations to do so.
+	 */
+	return (data_race(READ_ONCE(con->flags)) & CON_ENABLED);
+}
+
 /**
  * for_each_console_srcu() - Iterator over registered consoles
  * @con:	struct console pointer used as loop cursor
diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index 8974523f3107..79811984da34 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -3021,7 +3021,7 @@ void console_stop(struct console *console)
 {
 	__pr_flush(console, 1000, true);
 	console_lock();
-	console->flags &= ~CON_ENABLED;
+	WRITE_ONCE(console->flags, console->flags & ~CON_ENABLED);
 	console_unlock();
 
 	/*
@@ -3037,7 +3037,7 @@ EXPORT_SYMBOL(console_stop);
 void console_start(struct console *console)
 {
 	console_lock();
-	console->flags |= CON_ENABLED;
+	WRITE_ONCE(console->flags, console->flags | CON_ENABLED);
 	console_unlock();
 	__pr_flush(console, 1000, true);
 }
@@ -3256,7 +3256,7 @@ void register_console(struct console *newcon)
 
 	} else if (newcon->flags & CON_CONSDEV) {
 		/* Only the new head can have CON_CONSDEV set. */
-		console_first()->flags &= ~CON_CONSDEV;
+		WRITE_ONCE(console_first()->flags, console_first()->flags & ~CON_CONSDEV);
 		hlist_add_head_rcu(&newcon->node, &console_list);
 
 	} else {
@@ -3308,7 +3308,7 @@ int unregister_console(struct console *console)
 	console_lock();
 
 	/* Disable it unconditionally */
-	console->flags &= ~CON_ENABLED;
+	WRITE_ONCE(console->flags, console->flags & ~CON_ENABLED);
 
 	if (hlist_unhashed(&console->node)) {
 		console_unlock();
@@ -3327,7 +3327,7 @@ int unregister_console(struct console *console)
 	 * console has any device attached. Oh well....
 	 */
 	if (!hlist_empty(&console_list) && console->flags & CON_CONSDEV)
-		console_first()->flags |= CON_CONSDEV;
+		WRITE_ONCE(console_first()->flags, console_first()->flags | CON_CONSDEV);
 
 	console_unlock();
 
-- 
2.30.2

Re: [PATCH printk v3 07/40] console: introduce console_is_enabled() wrapper

[Petr Mladek]

On Mon 2022-11-07 15:22:05, John Ogness wrote:
> After switching to SRCU for console list iteration, some readers
> will begin readings console->flags as a data race. Locklessly
> reading console->flags provides a consistent value because there
> is at most one CPU modifying console->flags and that CPU is
> using only read-modify-write operations.
> 
> The primary reason for readers to access console->flags is to
> check if the console is enabled. Introduce console_is_enabled()
> to mark such access as a data race.
>
> Signed-off-by: John Ogness <john.ogness@linutronix.de>
> ---
>  include/linux/console.h | 27 +++++++++++++++++++++++++++
>  kernel/printk/printk.c  | 10 +++++-----
>  2 files changed, 32 insertions(+), 5 deletions(-)
> 
> diff --git a/include/linux/console.h b/include/linux/console.h
> index f4f0c9523835..d9c636011364 100644
> --- a/include/linux/console.h
> +++ b/include/linux/console.h
> @@ -172,6 +172,33 @@ extern void console_srcu_read_unlock(int cookie);
>  
>  extern struct hlist_head console_list;
>  
> +/**
> + * console_is_enabled - Locklessly check if the console is enabled
> + * @con:	struct console pointer of console to check
> + *
> + * Unless the caller is explicitly synchronizing against the console
> + * register/unregister/stop/start functions, this function should be
> + * used instead of manually readings console->flags and testing for
> + * the CON_ENABLED bit.
> + *
> + * This function provides the necessary READ_ONCE() and data_race()
> + * notation for locklessly reading the console flags. The READ_ONCE()
> + * in this function matches the WRITE_ONCE() when @flags are modified
> + * for registered consoles.
> + *
> + * Context: Any context.
> + */
> +static inline bool console_is_enabled(const struct console *con)
> +{
> +	/*
> +	 * Locklessly reading console->flags provides a consistent
> +	 * read value because there is at most one CPU modifying
> +	 * console->flags and that CPU is using only read-modify-write
> +	 * operations to do so.
> +	 */
> +	return (data_race(READ_ONCE(con->flags)) & CON_ENABLED);

This API is step into the right direction, definitely. I am just not
sure about all the related READ_WRITE() calls, see below. It is not
easy to check and maintain.

> +}
> +
>  /**
>   * for_each_console_srcu() - Iterator over registered consoles
>   * @con:	struct console pointer used as loop cursor
> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> index 8974523f3107..79811984da34 100644
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -3021,7 +3021,7 @@ void console_stop(struct console *console)
>  {
>  	__pr_flush(console, 1000, true);
>  	console_lock();
> -	console->flags &= ~CON_ENABLED;
> +	WRITE_ONCE(console->flags, console->flags & ~CON_ENABLED);

My first reaction is that using the atomic operation only for the
store side is suspicious. It is correct because the read is serialized
by console_lock(). But it is far from obvious why we need and can do it
this way.

It would deserve a comment. But there are several other writes.
Also it is not obvious why many other con->flags modifications
do not need this.

I think about hiding this into an API. We could also add some
checks that it is used the right way. Also it might make sense
to avoid using the READ_ONCE()/WRITE_ONCE by using
set_bit()/test_bit().

I mean the following:

/**
  * console_set_flag - set the given console flag
  *
  * @con:	 A pointer to struct console
  * @flag_bit:	 Number of the bit that will be set
  *
  * Must be called under console_lock() when the console
  * is registered. Serialization for non-registered
  * console is up to the related console driver code.
  *
  * Never access console->flags directly.
  */
void console_set_flag(struct console *con, int flag_nr)
{
	WARN_ON_ONCE(console_is_registered(con) && !held_console_lock());

	if (WARN_ON_ONCE(flag_nr) >= sizeof(con->flags);
		return;

	set_bit(flag_nr, &con->flags);
}

bool console_check_flag(const struct console *con, int flag_nr)
{
	WARN_ON_ONCE(console_is_registered(con) && !held_console_lock());

	if (WARN_ON_ONCE(flag_nr) >= sizeof(con->flags);
		return;

	return test_bit(flag_nr, &con->flags);
}

We could then use it directly:

	if (console_check_flag(con, CON_ENABLED_BIT))
		...

or we could hide it into a wrapper. Safe access:

static inline bool console_is_enabled(const struct console *con)
   {
	return console_check_bit(con, CON_ENABLED_BIT);
   }


The above is for the safe access. Another wrapper would be needed
for the unsafe access.

/**
  * console_check_flag_unsafe - check console flag without
  *	synchronization
  *
  * @con:	 A pointer to struct console
  * @flag_bit:	 Number of the bit that will be set
  *
  * Must be called under console_srcu_read_lock() when the console
  * is registered. Use console_check_flag() in all other situations.
  *
  * Never access console->flags directly.
  */
bool console_check_flag_unsafe(const struct console *con, int flag_nr)
{
	WARN_ON_ONCE(console_is_registered(con) && !console_srcu_read_lock_is_held());

	if (WARN_ON_ONCE(flag_nr) >= sizeof(con->flags);
		return;

	return data_race(test_bit(flag_nr, &con->flags));
}

Most callers should use the safe variant. It will even check that it
is really used the safe way. Only the few users of _unsafe() variant
would need an extra comment why it is acceptable.

I know that it is more work. There seem to be 49 locations that would
need update at this point. Some of them are touched by the patchset
anyway. There are "only" 39 accesses at the end of the patchset.

I used the following query:

$> git grep -e "\->flags.*CON_" | grep -v "\.flags" | grep -E "(c|con|cons|console)->flags" | wc -l
39

>  	console_unlock();
>  
>  	/*

I would prefer to use the proposed API because it should make all accesses more
clear and safe. And most importantly, it would help use to catch bugs.

But I do not resist on it. The patch looks correct and we could do
this later. I could live with it if we add some comments
above the WRITE_ONCE() calls.

Best Regards,
Petr

Re: [PATCH printk v3 07/40] console: introduce console_is_enabled() wrapper

[John Ogness]

On 2022-11-08, Petr Mladek <pmladek@suse.com> wrote:
>> --- a/kernel/printk/printk.c
>> +++ b/kernel/printk/printk.c
>> @@ -3021,7 +3021,7 @@ void console_stop(struct console *console)
>>  {
>>  	__pr_flush(console, 1000, true);
>>  	console_lock();
>> -	console->flags &= ~CON_ENABLED;
>> +	WRITE_ONCE(console->flags, console->flags & ~CON_ENABLED);
>
> My first reaction is that using the atomic operation only for the
> store side is suspicious. It is correct because the read is serialized
> by console_lock(). But it is far from obvious why we need and can do
> it this way.

The READ_ONCE()/WRITE_ONCE() usage is really about documenting data-race
reads and the writes that they are racing with.

For WRITE_ONCE() the rule is:

- If console->flags is modified for a registered console, it is done
  under the console_list_lock and using WRITE_ONCE().

If we use a wrapper for this rule, then we can also add the lockdep
assertion that console_list_lock is held.


For READ_ONCE() the rule is:

- If console->flags is read for a registered console, then either
  console_list_lock must be held _or_ it must be read via READ_ONCE().

If we use wrappers here, then we can use lockdep assertion on
console_list_lock for the non-READ_ONCE wrapper, and scru_read_lock
assertion for the READ_ONCE wrapper.

> It would deserve a comment. But there are several other writes.
> Also it is not obvious why many other con->flags modifications
> do not need this.
>
> I think about hiding this into an API. We could also add some
> checks that it is used the right way. Also it might make sense
> to avoid using the READ_ONCE()/WRITE_ONCE by using
> set_bit()/test_bit().

I do not see any advantage of set_bit()/test_bit(). They have the
disadvantage that they only work with 1 bit at a time. And there are
multiple sites where more than 1 bit is set/tested. It is important that
the multi-bit tests are simultaneous.

READ_ONCE()/WRITE_ONCE() are perfectly fine for what we are doing. The
writes (for registered consoles) are synchronized by the
console_list_lock. There is no need to use atomic operations.

> I would prefer to use the proposed API because it should make all
> accesses more clear and safe. And most importantly, it would help use
> to catch bugs.
>
> But I do not resist on it. The patch looks correct and we could do
> this later. I could live with it if we add some comments above the
> WRITE_ONCE() calls.

I do not want to do a full API replacement for all console->flags access
in this series or at this time. I am concerned that it is taking us too
far away from our current goal. Also, with the upcoming atomic/threaded
model, all consoles need to be modified that want to use it anyway. So
that would be a more appropriate time to require the use of new API's.

For console_is_enabled() I will add the srcu_read_lock check. I suppose
I should also name the function console_srcu_is_enabled().

For the WRITE_ONCE() calls, I will add a static inline wrapper in
printk.c that includes the lockdep console_list_lock assertion. Perhaps
called console_srcu_write_flags(struct console *con, short flags).

In console_srcu_write_flags() and console_srcu_is_enabled() I can
document their relationship and when they are to be used. Both these
functions are used rarely and should be considered the exception, not
the rule.

For code that is reading registered console->flags under the
console_list_lock, I will leave the "normal access" as is. Just as I am
leaving the "normal access" for non-registered console-flags as is. We
can convert those to a new generic API later if we think it is really
necessary.

John

Re: [PATCH printk v3 07/40] console: introduce console_is_enabled() wrapper

[Petr Mladek]

On Thu 2022-11-10 16:11:43, John Ogness wrote:
> On 2022-11-08, Petr Mladek <pmladek@suse.com> wrote:
> >> --- a/kernel/printk/printk.c
> >> +++ b/kernel/printk/printk.c
> >> @@ -3021,7 +3021,7 @@ void console_stop(struct console *console)
> >>  {
> >>  	__pr_flush(console, 1000, true);
> >>  	console_lock();
> >> -	console->flags &= ~CON_ENABLED;
> >> +	WRITE_ONCE(console->flags, console->flags & ~CON_ENABLED);
> >
> > My first reaction is that using the atomic operation only for the
> > store side is suspicious. It is correct because the read is serialized
> > by console_lock(). But it is far from obvious why we need and can do
> > it this way.
> 
> The READ_ONCE()/WRITE_ONCE() usage is really about documenting data-race
> reads and the writes that they are racing with.
> 
> For WRITE_ONCE() the rule is:
> 
> - If console->flags is modified for a registered console, it is done
>   under the console_list_lock and using WRITE_ONCE().
> 
> If we use a wrapper for this rule, then we can also add the lockdep
> assertion that console_list_lock is held.
>
> For READ_ONCE() the rule is:
> 
> - If console->flags is read for a registered console, then either
>   console_list_lock must be held _or_ it must be read via READ_ONCE().
>
> If we use wrappers here, then we can use lockdep assertion on
> console_list_lock for the non-READ_ONCE wrapper, and scru_read_lock
> assertion for the READ_ONCE wrapper.

Exactly, all the assertions are one big advantage for hiding this into
an API.

> > It would deserve a comment. But there are several other writes.
> > Also it is not obvious why many other con->flags modifications
> > do not need this.
> >
> > I think about hiding this into an API. We could also add some
> > checks that it is used the right way. Also it might make sense
> > to avoid using the READ_ONCE()/WRITE_ONCE by using
> > set_bit()/test_bit().
> 
> I do not see any advantage of set_bit()/test_bit(). They have the
> disadvantage that they only work with 1 bit at a time. And there are
> multiple sites where more than 1 bit is set/tested. It is important that
> the multi-bit tests are simultaneous.

Fair enough.

> > I would prefer to use the proposed API because it should make all
> > accesses more clear and safe. And most importantly, it would help use
> > to catch bugs.
> >
> > But I do not resist on it. The patch looks correct and we could do
> > this later. I could live with it if we add some comments above the
> > WRITE_ONCE() calls.
> 
> I do not want to do a full API replacement for all console->flags access
> in this series or at this time. I am concerned that it is taking us too
> far away from our current goal. Also, with the upcoming atomic/threaded
> model, all consoles need to be modified that want to use it anyway. So
> that would be a more appropriate time to require the use of new API's.

Fair enough.

> For console_is_enabled() I will add the srcu_read_lock check. I suppose
> I should also name the function console_srcu_is_enabled().
> 
> For the WRITE_ONCE() calls, I will add a static inline wrapper in
> printk.c that includes the lockdep console_list_lock assertion. Perhaps
> called console_srcu_write_flags(struct console *con, short flags).
> 
> In console_srcu_write_flags() and console_srcu_is_enabled() I can
> document their relationship and when they are to be used. Both these
> functions are used rarely and should be considered the exception, not
> the rule.
> 
> For code that is reading registered console->flags under the
> console_list_lock, I will leave the "normal access" as is. Just as I am
> leaving the "normal access" for non-registered console-flags as is. We
> can convert those to a new generic API later if we think it is really
> necessary.

Sounds reasonable.

The main motivation for introducing the wrappers is that there are
currently 40+ locations where console->flags are touched. It is easy
to miss something especially when we are reworking the locking around
this code. Some of the callers are outside kernel/printk/* which
even increases the risk of a misuse. The lockdep checks
would help us to catch potential mistakes.

Anyway, I am fine with adding the wrappers for the synchronized reads
later. This patchset is already big enough.

Best Regards,
Petr

[PATCH printk v3 08/40] printk: use console_is_enabled()

[John Ogness]

Replace (console->flags & CON_ENABLED) usage with console_is_enabled()
if it involves a data race. Otherwise add comments mentioning why the
wrapper is not used.

Note that this is a preparatory change for when console_lock no longer
provides synchronization for console->flags.

Signed-off-by: John Ogness <john.ogness@linutronix.de>
---
 kernel/printk/printk.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index 79811984da34..f243bb56a3ba 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -2660,7 +2660,7 @@ static bool abandon_console_lock_in_panic(void)
  */
 static inline bool console_is_usable(struct console *con)
 {
-	if (!(con->flags & CON_ENABLED))
+	if (!console_is_enabled(con))
 		return false;
 
 	if (!con->write)
@@ -2946,7 +2946,7 @@ void console_unblank(void)
 	console_locked = 1;
 	console_may_schedule = 0;
 	for_each_console(c)
-		if ((c->flags & CON_ENABLED) && c->unblank)
+		if (console_is_enabled(c) && c->unblank)
 			c->unblank();
 	console_unlock();
 
@@ -3104,8 +3104,11 @@ static int try_enable_preferred_console(struct console *newcon,
 	 * Some consoles, such as pstore and netconsole, can be enabled even
 	 * without matching. Accept the pre-enabled consoles only when match()
 	 * and setup() had a chance to be called.
+	 *
+	 * Note that reading @flags is race-free because the console is not
+	 * yet added to the console list.
 	 */
-	if (newcon->flags & CON_ENABLED && c->user_specified ==	user_specified)
+	if ((newcon->flags & CON_ENABLED) && (c->user_specified == user_specified))
 		return 0;
 
 	return -ENOENT;
-- 
2.30.2

Re: [PATCH printk v3 08/40] printk: use console_is_enabled()

[Petr Mladek]

On Mon 2022-11-07 15:22:06, John Ogness wrote:
> Replace (console->flags & CON_ENABLED) usage with console_is_enabled()
> if it involves a data race. Otherwise add comments mentioning why the
> wrapper is not used.

The wrapper will be used/needed only on few locations. There are many
more locations where the console flags are modified without any
locking.

Note that it is not only about CON_ENABLE flag. If we started playing
the game with WRITE_ONCE()/READ_ONCE() then we would need to consider
all locations where the flags are modified.

In the end, it might be easier to use the proposed console_set_flag(),
console_clear_flag(), console_check_flag(), and
console_check_flag_unsafe(), instead of documenting all the locations.

Also it is more important to document why it is acceptable to use
the racy variant. The wrappers would make sure that all the other
accesses are safe.


> Note that this is a preparatory change for when console_lock no longer
> provides synchronization for console->flags.
>
> Signed-off-by: John Ogness <john.ogness@linutronix.de>
> ---
>  kernel/printk/printk.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> index 79811984da34..f243bb56a3ba 100644
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -2660,7 +2660,7 @@ static bool abandon_console_lock_in_panic(void)
>   */
>  static inline bool console_is_usable(struct console *con)
>  {
> -	if (!(con->flags & CON_ENABLED))
> +	if (!console_is_enabled(con))

I agree that it makes sense to do this now. console_is_usable() is
used in two cycles. They will get switched to the srcu walk one by one.

Just please document that this allows to use console_is_usable() under
console_srcu_read_lock. And that in this case, the value of the flag might
get cleared at any time but the console still might be used
as long as the console_srcu_read_lock is held.

We should actually add a check into console_is_enabled() that either
console_lock or console_srcu_read_lock is held. The console_lock
should later be changed to console_list_lock.


>  		return false;
>  
>  	if (!con->write)
> @@ -2946,7 +2946,7 @@ void console_unblank(void)
>  	console_locked = 1;
>  	console_may_schedule = 0;
>  	for_each_console(c)
> -		if ((c->flags & CON_ENABLED) && c->unblank)
> +		if (console_is_enabled(c) && c->unblank)

I would prefer to do this change together with switching to
for_each_console_srcu(). It would be more clear why this change
is needed.

>  			c->unblank();
>  	console_unlock();
>  
> @@ -3104,8 +3104,11 @@ static int try_enable_preferred_console(struct console *newcon,
>  	 * Some consoles, such as pstore and netconsole, can be enabled even
>  	 * without matching. Accept the pre-enabled consoles only when match()
>  	 * and setup() had a chance to be called.
> +	 *
> +	 * Note that reading @flags is race-free because the console is not
> +	 * yet added to the console list.

Nit: This is not completely true. We just know that it will not get
     modified by the printk/console framework because the console is not
     registered yet.

Well, I could live with it. The comment is good enough. I am still
more concerned about how to distinguish when READ_ONCE()/WRITE_ONCE()
is needed. And it would affect all accesses to the flags.

>  	 */
> -	if (newcon->flags & CON_ENABLED && c->user_specified ==	user_specified)
> +	if ((newcon->flags & CON_ENABLED) && (c->user_specified == user_specified))
>  		return 0;
>  
>  	return -ENOENT;

Best Regards,
Petr

[PATCH printk v3 09/40] um: kmsg_dump: use console_is_enabled()

[John Ogness]

Replace (console->flags & CON_ENABLED) usage with console_is_enabled().

Signed-off-by: John Ogness <john.ogness@linutronix.de>
---
 arch/um/kernel/kmsg_dump.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/um/kernel/kmsg_dump.c b/arch/um/kernel/kmsg_dump.c
index 40abf1e9ccb1..f0233e2f8de0 100644
--- a/arch/um/kernel/kmsg_dump.c
+++ b/arch/um/kernel/kmsg_dump.c
@@ -32,7 +32,7 @@ static void kmsg_dumper_stdout(struct kmsg_dumper *dumper,
 		 * expected to output the crash information.
 		 */
 		if (strcmp(con->name, "ttynull") != 0 &&
-		    (con->flags & CON_ENABLED)) {
+		    console_is_enabled(con)) {
 			break;
 		}
 	}
-- 
2.30.2

[PATCH printk v3 09/40] um: kmsg_dump: use console_is_enabled()

[John Ogness]

Replace (console->flags & CON_ENABLED) usage with console_is_enabled().

Signed-off-by: John Ogness <john.ogness@linutronix.de>
---
 arch/um/kernel/kmsg_dump.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/um/kernel/kmsg_dump.c b/arch/um/kernel/kmsg_dump.c
index 40abf1e9ccb1..f0233e2f8de0 100644
--- a/arch/um/kernel/kmsg_dump.c
+++ b/arch/um/kernel/kmsg_dump.c
@@ -32,7 +32,7 @@ static void kmsg_dumper_stdout(struct kmsg_dumper *dumper,
 		 * expected to output the crash information.
 		 */
 		if (strcmp(con->name, "ttynull") != 0 &&
-		    (con->flags & CON_ENABLED)) {
+		    console_is_enabled(con)) {
 			break;
 		}
 	}
-- 
2.30.2


_______________________________________________
linux-um mailing list
linux-um@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-um

Re: [PATCH printk v3 09/40] um: kmsg_dump: use console_is_enabled()

[Petr Mladek]

On Mon 2022-11-07 15:22:07, John Ogness wrote:
> Replace (console->flags & CON_ENABLED) usage with console_is_enabled().
> 
> Signed-off-by: John Ogness <john.ogness@linutronix.de>
> ---
>  arch/um/kernel/kmsg_dump.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/um/kernel/kmsg_dump.c b/arch/um/kernel/kmsg_dump.c
> index 40abf1e9ccb1..f0233e2f8de0 100644
> --- a/arch/um/kernel/kmsg_dump.c
> +++ b/arch/um/kernel/kmsg_dump.c
> @@ -32,7 +32,7 @@ static void kmsg_dumper_stdout(struct kmsg_dumper *dumper,
>  		 * expected to output the crash information.
>  		 */
>  		if (strcmp(con->name, "ttynull") != 0 &&
> -		    (con->flags & CON_ENABLED)) {
> +		    console_is_enabled(con)) {

The patch does not explain why the racy check is needed here.

I would prefer to merge this patch with the patch switching to the srcu
console list iterator. It will become more or less self-explaining.

Anyway, I agree that the racy check is acceptable here.

Best Regards,
Petr

Re: [PATCH printk v3 09/40] um: kmsg_dump: use console_is_enabled()

[Petr Mladek]

On Mon 2022-11-07 15:22:07, John Ogness wrote:
> Replace (console->flags & CON_ENABLED) usage with console_is_enabled().
> 
> Signed-off-by: John Ogness <john.ogness@linutronix.de>
> ---
>  arch/um/kernel/kmsg_dump.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/um/kernel/kmsg_dump.c b/arch/um/kernel/kmsg_dump.c
> index 40abf1e9ccb1..f0233e2f8de0 100644
> --- a/arch/um/kernel/kmsg_dump.c
> +++ b/arch/um/kernel/kmsg_dump.c
> @@ -32,7 +32,7 @@ static void kmsg_dumper_stdout(struct kmsg_dumper *dumper,
>  		 * expected to output the crash information.
>  		 */
>  		if (strcmp(con->name, "ttynull") != 0 &&
> -		    (con->flags & CON_ENABLED)) {
> +		    console_is_enabled(con)) {

The patch does not explain why the racy check is needed here.

I would prefer to merge this patch with the patch switching to the srcu
console list iterator. It will become more or less self-explaining.

Anyway, I agree that the racy check is acceptable here.

Best Regards,
Petr

_______________________________________________
linux-um mailing list
linux-um@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-um

Re: [PATCH printk v3 09/40] um: kmsg_dump: use console_is_enabled()

[Petr Mladek]

On Mon 2022-11-07 15:22:07, John Ogness wrote:
> Replace (console->flags & CON_ENABLED) usage with console_is_enabled().
> 
> Signed-off-by: John Ogness <john.ogness@linutronix.de>
> ---
>  arch/um/kernel/kmsg_dump.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/um/kernel/kmsg_dump.c b/arch/um/kernel/kmsg_dump.c
> index 40abf1e9ccb1..f0233e2f8de0 100644
> --- a/arch/um/kernel/kmsg_dump.c
> +++ b/arch/um/kernel/kmsg_dump.c
> @@ -32,7 +32,7 @@ static void kmsg_dumper_stdout(struct kmsg_dumper *dumper,
>  		 * expected to output the crash information.
>  		 */
>  		if (strcmp(con->name, "ttynull") != 0 &&
> -		    (con->flags & CON_ENABLED)) {
> +		    console_is_enabled(con)) {

Same as with the 9th patch. I would merge this with the patch
switching to the srcu console list iterator. It will explain
why the racy check is needed here. This change does not make
sense without the other.

Best Regards,
Petr

Re: [PATCH printk v3 09/40] um: kmsg_dump: use console_is_enabled()

[Petr Mladek]

On Mon 2022-11-07 15:22:07, John Ogness wrote:
> Replace (console->flags & CON_ENABLED) usage with console_is_enabled().
> 
> Signed-off-by: John Ogness <john.ogness@linutronix.de>
> ---
>  arch/um/kernel/kmsg_dump.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/um/kernel/kmsg_dump.c b/arch/um/kernel/kmsg_dump.c
> index 40abf1e9ccb1..f0233e2f8de0 100644
> --- a/arch/um/kernel/kmsg_dump.c
> +++ b/arch/um/kernel/kmsg_dump.c
> @@ -32,7 +32,7 @@ static void kmsg_dumper_stdout(struct kmsg_dumper *dumper,
>  		 * expected to output the crash information.
>  		 */
>  		if (strcmp(con->name, "ttynull") != 0 &&
> -		    (con->flags & CON_ENABLED)) {
> +		    console_is_enabled(con)) {

Same as with the 9th patch. I would merge this with the patch
switching to the srcu console list iterator. It will explain
why the racy check is needed here. This change does not make
sense without the other.

Best Regards,
Petr

_______________________________________________
linux-um mailing list
linux-um@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-um

Re: [PATCH printk v3 09/40] um: kmsg_dump: use console_is_enabled()

[Petr Mladek]

On Wed 2022-11-09 15:56:11, Petr Mladek wrote:
> On Mon 2022-11-07 15:22:07, John Ogness wrote:
> > Replace (console->flags & CON_ENABLED) usage with console_is_enabled().
> > 
> > Signed-off-by: John Ogness <john.ogness@linutronix.de>
> > ---
> >  arch/um/kernel/kmsg_dump.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/arch/um/kernel/kmsg_dump.c b/arch/um/kernel/kmsg_dump.c
> > index 40abf1e9ccb1..f0233e2f8de0 100644
> > --- a/arch/um/kernel/kmsg_dump.c
> > +++ b/arch/um/kernel/kmsg_dump.c
> > @@ -32,7 +32,7 @@ static void kmsg_dumper_stdout(struct kmsg_dumper *dumper,
> >  		 * expected to output the crash information.
> >  		 */
> >  		if (strcmp(con->name, "ttynull") != 0 &&
> > -		    (con->flags & CON_ENABLED)) {
> > +		    console_is_enabled(con)) {
> 
> Same as with the 9th patch. I would merge this with the patch
> switching to the srcu console list iterator. It will explain
> why the racy check is needed here. This change does not make
> sense without the other.

Ah, this was supposed to be for the 10th patch.

I am sorry for confusion.

Best Regards,
Petr

Re: [PATCH printk v3 09/40] um: kmsg_dump: use console_is_enabled()

[Petr Mladek]

On Wed 2022-11-09 15:56:11, Petr Mladek wrote:
> On Mon 2022-11-07 15:22:07, John Ogness wrote:
> > Replace (console->flags & CON_ENABLED) usage with console_is_enabled().
> > 
> > Signed-off-by: John Ogness <john.ogness@linutronix.de>
> > ---
> >  arch/um/kernel/kmsg_dump.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/arch/um/kernel/kmsg_dump.c b/arch/um/kernel/kmsg_dump.c
> > index 40abf1e9ccb1..f0233e2f8de0 100644
> > --- a/arch/um/kernel/kmsg_dump.c
> > +++ b/arch/um/kernel/kmsg_dump.c
> > @@ -32,7 +32,7 @@ static void kmsg_dumper_stdout(struct kmsg_dumper *dumper,
> >  		 * expected to output the crash information.
> >  		 */
> >  		if (strcmp(con->name, "ttynull") != 0 &&
> > -		    (con->flags & CON_ENABLED)) {
> > +		    console_is_enabled(con)) {
> 
> Same as with the 9th patch. I would merge this with the patch
> switching to the srcu console list iterator. It will explain
> why the racy check is needed here. This change does not make
> sense without the other.

Ah, this was supposed to be for the 10th patch.

I am sorry for confusion.

Best Regards,
Petr

_______________________________________________
linux-um mailing list
linux-um@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-um

[PATCH printk v3 10/40] kdb: kdb_io: use console_is_enabled()

[John Ogness]

Replace (console->flags & CON_ENABLED) usage with console_is_enabled().

Signed-off-by: John Ogness <john.ogness@linutronix.de>
Reviewed-by: Petr Mladek <pmladek@suse.com>
Reviewed-by: Douglas Anderson <dianders@chromium.org>
---
 kernel/debug/kdb/kdb_io.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/debug/kdb/kdb_io.c b/kernel/debug/kdb/kdb_io.c
index 67d3c48a1522..550fe8b456ec 100644
--- a/kernel/debug/kdb/kdb_io.c
+++ b/kernel/debug/kdb/kdb_io.c
@@ -559,7 +559,7 @@ static void kdb_msg_write(const char *msg, int msg_len)
 	}
 
 	for_each_console(c) {
-		if (!(c->flags & CON_ENABLED))
+		if (!console_is_enabled(c))
 			continue;
 		if (c == dbg_io_ops->cons)
 			continue;
-- 
2.30.2

Re: [PATCH printk v3 10/40] kdb: kdb_io: use console_is_enabled()

[Daniel Thompson]

On Mon, Nov 07, 2022 at 03:22:08PM +0106, John Ogness wrote:
> Replace (console->flags & CON_ENABLED) usage with console_is_enabled().
>
> Signed-off-by: John Ogness <john.ogness@linutronix.de>
> Reviewed-by: Petr Mladek <pmladek@suse.com>
> Reviewed-by: Douglas Anderson <dianders@chromium.org>

Reviewed-by: Daniel Thompson <daniel.thompson@linaro.org>


Daniel.

Re: [PATCH printk v3 10/40] kdb: kdb_io: use console_is_enabled()

[Petr Mladek]

On Mon 2022-11-07 15:22:08, John Ogness wrote:
> Replace (console->flags & CON_ENABLED) usage with console_is_enabled().
> 
> Signed-off-by: John Ogness <john.ogness@linutronix.de>
> Reviewed-by: Petr Mladek <pmladek@suse.com>
> Reviewed-by: Douglas Anderson <dianders@chromium.org>
> ---
>  kernel/debug/kdb/kdb_io.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/kernel/debug/kdb/kdb_io.c b/kernel/debug/kdb/kdb_io.c
> index 67d3c48a1522..550fe8b456ec 100644
> --- a/kernel/debug/kdb/kdb_io.c
> +++ b/kernel/debug/kdb/kdb_io.c
> @@ -559,7 +559,7 @@ static void kdb_msg_write(const char *msg, int msg_len)
>  	}
>  
>  	for_each_console(c) {
> -		if (!(c->flags & CON_ENABLED))
> +		if (!console_is_enabled(c))

Same as with the 9th patch. I would prefer to merge this with
the patch switching to the srcu console list iterator.

It will explain why the racy check is needed here. This change
does not make sense without the other.

Best Regards,
Petr

[PATCH printk v3 11/40] um: kmsg_dumper: use srcu console list iterator

[John Ogness]

Rather than using the console_lock to guarantee safe console list
traversal, use srcu console list iteration.

Signed-off-by: John Ogness <john.ogness@linutronix.de>
---
 arch/um/kernel/kmsg_dump.c | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/arch/um/kernel/kmsg_dump.c b/arch/um/kernel/kmsg_dump.c
index f0233e2f8de0..b6acb3837f1d 100644
--- a/arch/um/kernel/kmsg_dump.c
+++ b/arch/um/kernel/kmsg_dump.c
@@ -16,16 +16,15 @@ static void kmsg_dumper_stdout(struct kmsg_dumper *dumper,
 	struct console *con;
 	unsigned long flags;
 	size_t len = 0;
+	int cookie;
 
 	/*
 	 * If no consoles are available to output crash information, dump
 	 * the kmsg buffer to stdout.
 	 */
 
-	if (!console_trylock())
-		return;
-
-	for_each_console(con) {
+	cookie = console_srcu_read_lock();
+	for_each_console_srcu(con) {
 		/*
 		 * The ttynull console and disabled consoles are ignored
 		 * since they cannot output. All other consoles are
@@ -36,9 +35,7 @@ static void kmsg_dumper_stdout(struct kmsg_dumper *dumper,
 			break;
 		}
 	}
-
-	console_unlock();
-
+	console_srcu_read_unlock(cookie);
 	if (con)
 		return;
 
-- 
2.30.2

[PATCH printk v3 11/40] um: kmsg_dumper: use srcu console list iterator

[John Ogness]

Rather than using the console_lock to guarantee safe console list
traversal, use srcu console list iteration.

Signed-off-by: John Ogness <john.ogness@linutronix.de>
---
 arch/um/kernel/kmsg_dump.c | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/arch/um/kernel/kmsg_dump.c b/arch/um/kernel/kmsg_dump.c
index f0233e2f8de0..b6acb3837f1d 100644
--- a/arch/um/kernel/kmsg_dump.c
+++ b/arch/um/kernel/kmsg_dump.c
@@ -16,16 +16,15 @@ static void kmsg_dumper_stdout(struct kmsg_dumper *dumper,
 	struct console *con;
 	unsigned long flags;
 	size_t len = 0;
+	int cookie;
 
 	/*
 	 * If no consoles are available to output crash information, dump
 	 * the kmsg buffer to stdout.
 	 */
 
-	if (!console_trylock())
-		return;
-
-	for_each_console(con) {
+	cookie = console_srcu_read_lock();
+	for_each_console_srcu(con) {
 		/*
 		 * The ttynull console and disabled consoles are ignored
 		 * since they cannot output. All other consoles are
@@ -36,9 +35,7 @@ static void kmsg_dumper_stdout(struct kmsg_dumper *dumper,
 			break;
 		}
 	}
-
-	console_unlock();
-
+	console_srcu_read_unlock(cookie);
 	if (con)
 		return;
 
-- 
2.30.2


_______________________________________________
linux-um mailing list
linux-um@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-um

Re: [PATCH printk v3 11/40] um: kmsg_dumper: use srcu console list iterator

[Petr Mladek]

On Mon 2022-11-07 15:22:09, John Ogness wrote:
> Rather than using the console_lock to guarantee safe console list
> traversal, use srcu console list iteration.

It might be obvious. But I would add:

"It allows to dump the messages even when console_lock() is blocked
during panic()."

It will help to understand this is a clear win.

> Signed-off-by: John Ogness <john.ogness@linutronix.de>

After merging with 9th patch for using the racy console_is_enabled(con):

Reviewed-by: Petr Mladek <pmladek@suse.com>

Best Regards,
Petr

Re: [PATCH printk v3 11/40] um: kmsg_dumper: use srcu console list iterator

[Petr Mladek]

On Mon 2022-11-07 15:22:09, John Ogness wrote:
> Rather than using the console_lock to guarantee safe console list
> traversal, use srcu console list iteration.

It might be obvious. But I would add:

"It allows to dump the messages even when console_lock() is blocked
during panic()."

It will help to understand this is a clear win.

> Signed-off-by: John Ogness <john.ogness@linutronix.de>

After merging with 9th patch for using the racy console_is_enabled(con):

Reviewed-by: Petr Mladek <pmladek@suse.com>

Best Regards,
Petr

_______________________________________________
linux-um mailing list
linux-um@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-um

[PATCH printk v3 12/40] tty: serial: kgdboc: document console_lock usage

[John Ogness]

kgdboc_earlycon_init() uses the console_lock to ensure that no consoles
are unregistered until the kgdboc_earlycon is setup. This is necessary
because the trapping of the exit() callback assumes that the exit()
callback is not called before the trap is setup.

Explicitly document this non-typical console_lock usage.

Signed-off-by: John Ogness <john.ogness@linutronix.de>
Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Reviewed-by: Douglas Anderson <dianders@chromium.org>
---
 drivers/tty/serial/kgdboc.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/tty/serial/kgdboc.c b/drivers/tty/serial/kgdboc.c
index e76f0186c335..5be381003e58 100644
--- a/drivers/tty/serial/kgdboc.c
+++ b/drivers/tty/serial/kgdboc.c
@@ -530,6 +530,14 @@ static int __init kgdboc_earlycon_init(char *opt)
 	 * Look for a matching console, or if the name was left blank just
 	 * pick the first one we find.
 	 */
+
+	/*
+	 * Hold the console_lock to guarantee that no consoles are
+	 * unregistered until the kgdboc_earlycon setup is complete.
+	 * Trapping the exit() callback relies on exit() not being
+	 * called until the trap is setup. This also allows safe
+	 * traversal of the console list and race-free reading of @flags.
+	 */
 	console_lock();
 	for_each_console(con) {
 		if (con->write && con->read &&
-- 
2.30.2

Re: [PATCH printk v3 12/40] tty: serial: kgdboc: document console_lock usage

[Daniel Thompson]

On Mon, Nov 07, 2022 at 03:22:10PM +0106, John Ogness wrote:
> kgdboc_earlycon_init() uses the console_lock to ensure that no consoles
> are unregistered until the kgdboc_earlycon is setup. This is necessary
> because the trapping of the exit() callback assumes that the exit()
> callback is not called before the trap is setup.
>
> Explicitly document this non-typical console_lock usage.
>
> Signed-off-by: John Ogness <john.ogness@linutronix.de>
> Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Reviewed-by: Douglas Anderson <dianders@chromium.org>

Reviewed-by: Daniel Thompson <daniel.thompson@linaro.org>


Daniel.

Re: [PATCH printk v3 12/40] tty: serial: kgdboc: document console_lock usage

[Petr Mladek]

On Mon 2022-11-07 15:22:10, John Ogness wrote:
> kgdboc_earlycon_init() uses the console_lock to ensure that no consoles
> are unregistered until the kgdboc_earlycon is setup. This is necessary
> because the trapping of the exit() callback assumes that the exit()
> callback is not called before the trap is setup.
> 
> Explicitly document this non-typical console_lock usage.

It is great to have the description.

> Signed-off-by: John Ogness <john.ogness@linutronix.de>
> Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Reviewed-by: Douglas Anderson <dianders@chromium.org>

Reviewed-by: Petr Mladek <pmladek@suse.com>

Best Regards,
Petr

[PATCH printk v3 13/40] tty: tty_io: document console_lock usage

[John Ogness]

show_cons_active() uses the console_lock to gather information
on registered consoles. Since the console_lock is being used for
multiple reasons, explicitly document these reasons. This will
be useful when the console_lock is split into fine-grained
locking.

Signed-off-by: John Ogness <john.ogness@linutronix.de>
---
 drivers/tty/tty_io.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index de06c3c2ff70..ee4da2fec328 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -3526,6 +3526,16 @@ static ssize_t show_cons_active(struct device *dev,
 	struct console *c;
 	ssize_t count = 0;
 
+	/*
+	 * Hold the console_lock to guarantee that no consoles are
+	 * unregistered until all console processing is complete.
+	 * This also allows safe traversal of the console list and
+	 * race-free reading of @flags.
+	 *
+	 * Take console_lock to serialize device() callback with
+	 * other console operations. For example, fg_console is
+	 * modified under console_lock when switching vt.
+	 */
 	console_lock();
 	for_each_console(c) {
 		if (!c->device)
-- 
2.30.2

Re: [PATCH printk v3 13/40] tty: tty_io: document console_lock usage

[Petr Mladek]

On Mon 2022-11-07 15:22:11, John Ogness wrote:
> show_cons_active() uses the console_lock to gather information
> on registered consoles. Since the console_lock is being used for
> multiple reasons, explicitly document these reasons. This will
> be useful when the console_lock is split into fine-grained
> locking.

It is great to have this documented.

> Signed-off-by: John Ogness <john.ogness@linutronix.de>

Reviewed-by: Petr Mladek <pmladek@suse.com>

Best Regards,
Petr

[PATCH printk v3 14/40] proc: consoles: document console_lock usage

[John Ogness]

The console_lock is held throughout the start/show/stop procedure
to print out device/driver information about all registered
consoles. Since the console_lock is being used for multiple reasons,
explicitly document these reasons. This will be useful when the
console_lock is split into fine-grained locking.

Signed-off-by: John Ogness <john.ogness@linutronix.de>
---
 fs/proc/consoles.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/fs/proc/consoles.c b/fs/proc/consoles.c
index cf2e0788f9c7..46b305fa04ed 100644
--- a/fs/proc/consoles.c
+++ b/fs/proc/consoles.c
@@ -63,6 +63,15 @@ static void *c_start(struct seq_file *m, loff_t *pos)
 	struct console *con;
 	loff_t off = 0;
 
+	/*
+	 * Take console_lock to serialize device() callback with
+	 * other console operations. For example, fg_console is
+	 * modified under console_lock when switching vt.
+	 *
+	 * Hold the console_lock to guarantee safe traversal of the
+	 * console list. SRCU cannot be used because there is no
+	 * place to store the SRCU cookie.
+	 */
 	console_lock();
 	for_each_console(con)
 		if (off++ == *pos)
-- 
2.30.2

Re: [PATCH printk v3 14/40] proc: consoles: document console_lock usage

[Petr Mladek]

On Mon 2022-11-07 15:22:12, John Ogness wrote:
> The console_lock is held throughout the start/show/stop procedure
> to print out device/driver information about all registered
> consoles. Since the console_lock is being used for multiple reasons,
> explicitly document these reasons. This will be useful when the
> console_lock is split into fine-grained locking.
>
> Signed-off-by: John Ogness <john.ogness@linutronix.de>

Reviewed-by: Petr Mladek <pmladek@suse.com>

Best Regards,
Petr

[PATCH printk v3 15/40] kdb: use srcu console list iterator

[John Ogness]

Guarantee safe iteration of the console list by using SRCU.

Signed-off-by: John Ogness <john.ogness@linutronix.de>
---
 kernel/debug/kdb/kdb_io.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/kernel/debug/kdb/kdb_io.c b/kernel/debug/kdb/kdb_io.c
index 550fe8b456ec..ed8289ce4fcb 100644
--- a/kernel/debug/kdb/kdb_io.c
+++ b/kernel/debug/kdb/kdb_io.c
@@ -545,6 +545,7 @@ static void kdb_msg_write(const char *msg, int msg_len)
 {
 	struct console *c;
 	const char *cp;
+	int cookie;
 	int len;
 
 	if (msg_len == 0)
@@ -558,7 +559,15 @@ static void kdb_msg_write(const char *msg, int msg_len)
 		cp++;
 	}
 
-	for_each_console(c) {
+	/*
+	 * The console_srcu_read_lock() only provides safe console list
+	 * traversal. The use of the ->write() callback relies on all other
+	 * CPUs being stopped at the moment and console drivers being able to
+	 * handle reentrance when @oops_in_progress is set. (Note that there
+	 * is no guarantee for either criteria.)
+	 */
+	cookie = console_srcu_read_lock();
+	for_each_console_srcu(c) {
 		if (!console_is_enabled(c))
 			continue;
 		if (c == dbg_io_ops->cons)
@@ -577,6 +586,7 @@ static void kdb_msg_write(const char *msg, int msg_len)
 		--oops_in_progress;
 		touch_nmi_watchdog();
 	}
+	console_srcu_read_unlock(cookie);
 }
 
 int vkdb_printf(enum kdb_msgsrc src, const char *fmt, va_list ap)
-- 
2.30.2

Re: [PATCH printk v3 15/40] kdb: use srcu console list iterator

[Daniel Thompson]

On Mon, Nov 07, 2022 at 03:22:13PM +0106, John Ogness wrote:
> Guarantee safe iteration of the console list by using SRCU.
>
> Signed-off-by: John Ogness <john.ogness@linutronix.de>
> ---
>  kernel/debug/kdb/kdb_io.c | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
>
> diff --git a/kernel/debug/kdb/kdb_io.c b/kernel/debug/kdb/kdb_io.c
> index 550fe8b456ec..ed8289ce4fcb 100644
> --- a/kernel/debug/kdb/kdb_io.c
> +++ b/kernel/debug/kdb/kdb_io.c
> @@ -545,6 +545,7 @@ static void kdb_msg_write(const char *msg, int msg_len)
>  {
>  	struct console *c;
>  	const char *cp;
> +	int cookie;
>  	int len;
>
>  	if (msg_len == 0)
> @@ -558,7 +559,15 @@ static void kdb_msg_write(const char *msg, int msg_len)
>  		cp++;
>  	}
>
> -	for_each_console(c) {
> +	/*
> +	 * The console_srcu_read_lock() only provides safe console list
> +	 * traversal. The use of the ->write() callback relies on all other
> +	 * CPUs being stopped at the moment and console drivers being able to
> +	 * handle reentrance when @oops_in_progress is set. (Note that there
> +	 * is no guarantee for either criteria.)
> +	 */

The debugger entry protocol does ensure that other CPUs are either
stopped or unresponsive. In the case where the other CPU is unresponsive
(e.g. timed out after being asked to stop) then there is a "real" printk()
issued prior to any of the above interference with the console system to
the developer driving the debugger gets as much clue as we can offer them
about what is going on (typically this is emitted from regular interrupt
context).

Given this comment is part of the debugger code then for the
oops_in_progress hack it might be more helpful to describe what
the developer in front the debugger needs to do to have the most
reliable debug session possible.

  There is no guarantee that every console drivers can handle reentrance
  in this way; the developer deploying the debugger is responsible for
  ensuring that the console drivers they have selected handle reentrance
  appropriately.


Daniel.


> +	cookie = console_srcu_read_lock();
> +	for_each_console_srcu(c) {
>  		if (!console_is_enabled(c))
>  			continue;
>  		if (c == dbg_io_ops->cons)
> @@ -577,6 +586,7 @@ static void kdb_msg_write(const char *msg, int msg_len)
>  		--oops_in_progress;
>  		touch_nmi_watchdog();
>  	}
> +	console_srcu_read_unlock(cookie);
>  }
>
>  int vkdb_printf(enum kdb_msgsrc src, const char *fmt, va_list ap)
> --
> 2.30.2
>

Re: [PATCH printk v3 15/40] kdb: use srcu console list iterator

[John Ogness]

Hi Daniel,

On 2022-11-09, Daniel Thompson <daniel.thompson@linaro.org> wrote:
>> +	/*
>> +	 * The console_srcu_read_lock() only provides safe console list
>> +	 * traversal. The use of the ->write() callback relies on all other
>> +	 * CPUs being stopped at the moment and console drivers being able to
>> +	 * handle reentrance when @oops_in_progress is set. (Note that there
>> +	 * is no guarantee for either criteria.)
>> +	 */
>
> The debugger entry protocol does ensure that other CPUs are either
> stopped or unresponsive. In the case where the other CPU is unresponsive
> (e.g. timed out after being asked to stop) then there is a "real" printk()
> issued prior to any of the above interference with the console system to
> the developer driving the debugger gets as much clue as we can offer them
> about what is going on (typically this is emitted from regular interrupt
> context).
>
> Given this comment is part of the debugger code then for the
> oops_in_progress hack it might be more helpful to describe what
> the developer in front the debugger needs to do to have the most
> reliable debug session possible.
>
>   There is no guarantee that every console drivers can handle reentrance
>   in this way; the developer deploying the debugger is responsible for
>   ensuring that the console drivers they have selected handle reentrance
>   appropriately.

Thanks for the explanation. I will change the comment to:

	/*
	 * The console_srcu_read_lock() only provides safe console list
	 * traversal. The use of the ->write() callback relies on all other
	 * CPUs being stopped at the moment and console drivers being able to
	 * handle reentrance when @oops_in_progress is set.
	 *
	 * There is no guarantee that every console driver can handle
	 * reentrance in this way; the developer deploying the debugger
	 * is responsible for ensuring that the console drivers they
	 * have selected handle reentrance appropriately.
	 */

John

Re: [PATCH printk v3 15/40] kdb: use srcu console list iterator

[Petr Mladek]

On Wed 2022-11-09 10:33:55, John Ogness wrote:
> Hi Daniel,
> 
> On 2022-11-09, Daniel Thompson <daniel.thompson@linaro.org> wrote:
> >> +	/*
> >> +	 * The console_srcu_read_lock() only provides safe console list
> >> +	 * traversal. The use of the ->write() callback relies on all other
> >> +	 * CPUs being stopped at the moment and console drivers being able to
> >> +	 * handle reentrance when @oops_in_progress is set. (Note that there
> >> +	 * is no guarantee for either criteria.)
> >> +	 */
> >
> > The debugger entry protocol does ensure that other CPUs are either
> > stopped or unresponsive. In the case where the other CPU is unresponsive
> > (e.g. timed out after being asked to stop) then there is a "real" printk()
> > issued prior to any of the above interference with the console system to
> > the developer driving the debugger gets as much clue as we can offer them
> > about what is going on (typically this is emitted from regular interrupt
> > context).
> >
> > Given this comment is part of the debugger code then for the
> > oops_in_progress hack it might be more helpful to describe what
> > the developer in front the debugger needs to do to have the most
> > reliable debug session possible.
> >
> >   There is no guarantee that every console drivers can handle reentrance
> >   in this way; the developer deploying the debugger is responsible for
> >   ensuring that the console drivers they have selected handle reentrance
> >   appropriately.
> 
> Thanks for the explanation. I will change the comment to:
> 
> 	/*
> 	 * The console_srcu_read_lock() only provides safe console list
> 	 * traversal. The use of the ->write() callback relies on all other
> 	 * CPUs being stopped at the moment and console drivers being able to
> 	 * handle reentrance when @oops_in_progress is set.
> 	 *
> 	 * There is no guarantee that every console driver can handle
> 	 * reentrance in this way; the developer deploying the debugger
> 	 * is responsible for ensuring that the console drivers they
> 	 * have selected handle reentrance appropriately.
> 	 */

Looks good to me.

After merging this with the 10th patch that adds the data_race() annotated
check of CON_ENABLED flag:

Reviewed-by: Petr Mladek <pmladek@suse.com>

Best Regards,
Petr

[PATCH printk v3 16/40] printk: console_flush_all: use srcu console list iterator

[John Ogness]

Guarantee safe iteration of the console list by using SRCU.

Note that in the case of a handover, the SRCU read lock is also
released. This is documented in the function description and as
comments in the code. It is a bit tricky, but this preserves the
lockdep lock ordering for the context handing over the
console_lock:

  console_lock()
  | mutex_acquire(&console_lock_dep_map)       <-- console lock
  |
  console_unlock()
  | console_flush_all()
  | | srcu_read_lock(&console_srcu)            <-- srcu lock
  | | console_emit_next_record()
  | | | console_lock_spinning_disable_and_check()
  | | | | srcu_read_unlock(&console_srcu)      <-- srcu unlock
  | | | | mutex_release(&console_lock_dep_map) <-- console unlock

Signed-off-by: John Ogness <john.ogness@linutronix.de>
Reviewed-by: Petr Mladek <pmladek@suse.com>
---
 kernel/printk/printk.c | 48 ++++++++++++++++++++++++++++++------------
 1 file changed, 34 insertions(+), 14 deletions(-)

diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index f243bb56a3ba..49b9ba00eae8 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -1847,13 +1847,13 @@ static void console_lock_spinning_enable(void)
  * safe to start busy waiting for the lock. Second, it checks if
  * there is a busy waiter and passes the lock rights to her.
  *
- * Important: Callers lose the lock if there was a busy waiter.
- *	They must not touch items synchronized by console_lock
- *	in this case.
+ * Important: Callers lose both the console_lock and the SRCU read lock if
+ *	there was a busy waiter. They must not touch items synchronized by
+ *	console_lock or SRCU read lock in this case.
  *
  * Return: 1 if the lock rights were passed, 0 otherwise.
  */
-static int console_lock_spinning_disable_and_check(void)
+static int console_lock_spinning_disable_and_check(int cookie)
 {
 	int waiter;
 
@@ -1872,6 +1872,12 @@ static int console_lock_spinning_disable_and_check(void)
 
 	spin_release(&console_owner_dep_map, _THIS_IP_);
 
+	/*
+	 * Preserve lockdep lock ordering. Release the SRCU read lock before
+	 * releasing the console_lock.
+	 */
+	console_srcu_read_unlock(cookie);
+
 	/*
 	 * Hand off console_lock to waiter. The waiter will perform
 	 * the up(). After this, the waiter is the console_lock owner.
@@ -2355,7 +2361,7 @@ static ssize_t msg_print_ext_body(char *buf, size_t size,
 				  char *text, size_t text_len,
 				  struct dev_printk_info *dev_info) { return 0; }
 static void console_lock_spinning_enable(void) { }
-static int console_lock_spinning_disable_and_check(void) { return 0; }
+static int console_lock_spinning_disable_and_check(int cookie) { return 0; }
 static void call_console_driver(struct console *con, const char *text, size_t len,
 				char *dropped_text)
 {
@@ -2697,16 +2703,18 @@ static void __console_unlock(void)
  * DROPPED_TEXT_MAX. Otherwise @dropped_text must be NULL.
  *
  * @handover will be set to true if a printk waiter has taken over the
- * console_lock, in which case the caller is no longer holding the
- * console_lock. Otherwise it is set to false.
+ * console_lock, in which case the caller is no longer holding both the
+ * console_lock and the SRCU read lock. Otherwise it is set to false.
+ *
+ * @cookie is the cookie from the SRCU read lock.
  *
  * Returns false if the given console has no next record to print, otherwise
  * true.
  *
- * Requires the console_lock.
+ * Requires the console_lock and the SRCU read lock.
  */
 static bool console_emit_next_record(struct console *con, char *text, char *ext_text,
-				     char *dropped_text, bool *handover)
+				     char *dropped_text, bool *handover, int cookie)
 {
 	static int panic_console_dropped;
 	struct printk_info info;
@@ -2766,7 +2774,7 @@ static bool console_emit_next_record(struct console *con, char *text, char *ext_
 
 	con->seq++;
 
-	*handover = console_lock_spinning_disable_and_check();
+	*handover = console_lock_spinning_disable_and_check(cookie);
 	printk_safe_exit_irqrestore(flags);
 skip:
 	return true;
@@ -2803,6 +2811,7 @@ static bool console_flush_all(bool do_cond_resched, u64 *next_seq, bool *handove
 	bool any_usable = false;
 	struct console *con;
 	bool any_progress;
+	int cookie;
 
 	*next_seq = 0;
 	*handover = false;
@@ -2810,7 +2819,8 @@ static bool console_flush_all(bool do_cond_resched, u64 *next_seq, bool *handove
 	do {
 		any_progress = false;
 
-		for_each_console(con) {
+		cookie = console_srcu_read_lock();
+		for_each_console_srcu(con) {
 			bool progress;
 
 			if (!console_is_usable(con))
@@ -2821,12 +2831,17 @@ static bool console_flush_all(bool do_cond_resched, u64 *next_seq, bool *handove
 				/* Extended consoles do not print "dropped messages". */
 				progress = console_emit_next_record(con, &text[0],
 								    &ext_text[0], NULL,
-								    handover);
+								    handover, cookie);
 			} else {
 				progress = console_emit_next_record(con, &text[0],
 								    NULL, &dropped_text[0],
-								    handover);
+								    handover, cookie);
 			}
+
+			/*
+			 * If a handover has occurred, the SRCU read lock
+			 * is already released.
+			 */
 			if (*handover)
 				return false;
 
@@ -2840,14 +2855,19 @@ static bool console_flush_all(bool do_cond_resched, u64 *next_seq, bool *handove
 
 			/* Allow panic_cpu to take over the consoles safely. */
 			if (abandon_console_lock_in_panic())
-				return false;
+				goto abandon;
 
 			if (do_cond_resched)
 				cond_resched();
 		}
+		console_srcu_read_unlock(cookie);
 	} while (any_progress);
 
 	return any_usable;
+
+abandon:
+	console_srcu_read_unlock(cookie);
+	return false;
 }
 
 /**
-- 
2.30.2

[PATCH printk v3 17/40] printk: console_unblank: use srcu console list iterator

[John Ogness]

Use srcu console list iteration for console list traversal.

Document why the console_lock is still necessary. Note that this
is a preparatory change for when console_lock no longer provides
synchronization for the console list.

Signed-off-by: John Ogness <john.ogness@linutronix.de>
Reviewed-by: Petr Mladek <pmladek@suse.com>
---
 kernel/printk/printk.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index 49b9ba00eae8..c3191dc24742 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -2952,10 +2952,14 @@ EXPORT_SYMBOL(console_conditional_schedule);
 void console_unblank(void)
 {
 	struct console *c;
+	int cookie;
 
 	/*
-	 * console_unblank can no longer be called in interrupt context unless
-	 * oops_in_progress is set to 1..
+	 * Stop console printing because the unblank() callback may
+	 * assume the console is not within its write() callback.
+	 *
+	 * If @oops_in_progress is set, this may be an atomic context.
+	 * In that case, attempt a trylock as best-effort.
 	 */
 	if (oops_in_progress) {
 		if (down_trylock_console_sem() != 0)
@@ -2965,9 +2969,14 @@ void console_unblank(void)
 
 	console_locked = 1;
 	console_may_schedule = 0;
-	for_each_console(c)
+
+	cookie = console_srcu_read_lock();
+	for_each_console_srcu(c) {
 		if (console_is_enabled(c) && c->unblank)
 			c->unblank();
+	}
+	console_srcu_read_unlock(cookie);
+
 	console_unlock();
 
 	if (!oops_in_progress)
-- 
2.30.2

[PATCH printk v3 18/40] printk: console_flush_on_panic: use srcu console list iterator

[John Ogness]

With SRCU it is now safe to traverse the console list, even if
the console_trylock() failed. However, overwriting console->seq
when console_trylock() failed is still an issue.

Switch to SRCU iteration and document remaining issue with
console->seq.

Signed-off-by: John Ogness <john.ogness@linutronix.de>
Reviewed-by: Petr Mladek <pmladek@suse.com>
---
 kernel/printk/printk.c | 21 +++++++++++----------
 1 file changed, 11 insertions(+), 10 deletions(-)

diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index c3191dc24742..de86a502b50b 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -3002,21 +3002,22 @@ void console_flush_on_panic(enum con_flush_mode mode)
 	console_may_schedule = 0;
 
 	if (mode == CONSOLE_REPLAY_ALL) {
-		struct hlist_node *tmp;
 		struct console *c;
+		int cookie;
 		u64 seq;
 
 		seq = prb_first_valid_seq(prb);
-		/*
-		 * This cannot use for_each_console() because it's not established
-		 * that the current context has console locked and neither there is
-		 * a guarantee that there is no concurrency in that case.
-		 *
-		 * Open code it for documentation purposes and pretend that
-		 * it works.
-		 */
-		hlist_for_each_entry_safe(c, tmp, &console_list, node)
+
+		cookie = console_srcu_read_lock();
+		for_each_console_srcu(c) {
+			/*
+			 * If the above console_trylock() failed, this is an
+			 * unsynchronized assignment. But in that case, the
+			 * kernel is in "hope and pray" mode anyway.
+			 */
 			c->seq = seq;
+		}
+		console_srcu_read_unlock(cookie);
 	}
 	console_unlock();
 }
-- 
2.30.2

[PATCH printk v3 19/40] printk: console_device: use srcu console list iterator

[John Ogness]

Use srcu console list iteration for console list traversal.

Document why the console_lock is still necessary. Note that this
is a preparatory change for when console_lock no longer provides
synchronization for the console list.

Signed-off-by: John Ogness <john.ogness@linutronix.de>
---
 kernel/printk/printk.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index de86a502b50b..ec50777d0301 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -3029,15 +3029,25 @@ struct tty_driver *console_device(int *index)
 {
 	struct console *c;
 	struct tty_driver *driver = NULL;
+	int cookie;
 
+	/*
+	 * Take console_lock to serialize device() callback with
+	 * other console operations. For example, fg_console is
+	 * modified under console_lock when switching vt.
+	 */
 	console_lock();
-	for_each_console(c) {
+
+	cookie = console_srcu_read_lock();
+	for_each_console_srcu(c) {
 		if (!c->device)
 			continue;
 		driver = c->device(c, index);
 		if (driver)
 			break;
 	}
+	console_srcu_read_unlock(cookie);
+
 	console_unlock();
 	return driver;
 }
-- 
2.30.2

Re: [PATCH printk v3 19/40] printk: console_device: use srcu console list iterator

[Petr Mladek]

On Mon 2022-11-07 15:22:17, John Ogness wrote:
> Use srcu console list iteration for console list traversal.

I wonder if it would make sense to describe why this is acceptable.
But I am not sure how to explain this a reasonable way. I am afraid
that it might cause more confusion.

I would write something like:

<proposal>
It is acceptable because the consoles might come and go at any time.
Strict synchronizing with console registration code would not bring
any advantage over RCU.
</proposal>

> Document why the console_lock is still necessary. Note that this
> is a preparatory change for when console_lock no longer provides
> synchronization for the console list.
> 
> Signed-off-by: John Ogness <john.ogness@linutronix.de>

As I said, I do not have strong opinion if the extra explanation
would help. So, with or without it:

Reviewed-by: Petr Mladek <pmladek@suse.com>

Best Regards,
Petr

[PATCH printk v3 20/40] printk: __pr_flush: use srcu console list iterator

[John Ogness]

Use srcu console list iteration for console list traversal.

Document why the console_lock is still necessary. Note that this
is a preparatory change for when console_lock no longer provides
synchronization for the console list.

Signed-off-by: John Ogness <john.ogness@linutronix.de>
Reviewed-by: Petr Mladek <pmladek@suse.com>
---
 kernel/printk/printk.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index ec50777d0301..d8ccb7b523e2 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -3480,6 +3480,7 @@ static bool __pr_flush(struct console *con, int timeout_ms, bool reset_on_progre
 	struct console *c;
 	u64 last_diff = 0;
 	u64 printk_seq;
+	int cookie;
 	u64 diff;
 	u64 seq;
 
@@ -3490,9 +3491,15 @@ static bool __pr_flush(struct console *con, int timeout_ms, bool reset_on_progre
 	for (;;) {
 		diff = 0;
 
+		/*
+		 * Hold the console_lock to guarantee safe access to
+		 * console->seq and to prevent changes to @console_suspended
+		 * until all consoles have been processed.
+		 */
 		console_lock();
 
-		for_each_console(c) {
+		cookie = console_srcu_read_lock();
+		for_each_console_srcu(c) {
 			if (con && con != c)
 				continue;
 			if (!console_is_usable(c))
@@ -3501,6 +3508,7 @@ static bool __pr_flush(struct console *con, int timeout_ms, bool reset_on_progre
 			if (printk_seq < seq)
 				diff += seq - printk_seq;
 		}
+		console_srcu_read_unlock(cookie);
 
 		/*
 		 * If consoles are suspended, it cannot be expected that they
-- 
2.30.2

[PATCH printk v3 21/40] printk: introduce console_list_lock

[John Ogness]

Currently there exist races in register_console(), where the types
of registered consoles are checked (without holding the console_lock)
and then after acquiring the console_lock, it is assumed that the list
has not changed. Also, some code that performs console_unregister()
make similar assumptions.

It might be possible to fix these races using the console_lock. But
it would require a complex analysis of all console drivers to make
sure that the console_lock is not taken in match() and setup()
callbacks. And we really prefer to split up and reduce the
responsibilities of console_lock rather than expand its complexity.
Therefore, introduce a new console_list_lock to provide full
synchronization for any console list changes.

In addition, also use console_list_lock for synchronization of
console->flags updates. All flags are either static or modified only
during the console registration. There are only two exceptions.

The first exception is CON_ENABLED, which is also modified by
console_start()/console_stop(). Therefore, these functions must
also take the console_list_lock.

The second exception is when the flags are modified by the console
driver init code before the console is registered. These will be
ignored because they are not visible to the rest of the system
via the console_drivers list.

Note that one of the various responsibilities of the console_lock is
also intended to provide console list and console->flags
synchronization. Later changes will update call sites relying on the
console_lock for these purposes. Once all call sites have been
updated, the console_lock will be relieved of synchronizing
console_list and console->flags updates.

Suggested-by: Thomas Gleixner <tglx@linutronix.de>
Signed-off-by: John Ogness <john.ogness@linutronix.de>
---
 include/linux/console.h | 23 +++++++++--
 kernel/printk/printk.c  | 87 ++++++++++++++++++++++++++++++++++++-----
 2 files changed, 98 insertions(+), 12 deletions(-)

diff --git a/include/linux/console.h b/include/linux/console.h
index d9c636011364..47be23be8a88 100644
--- a/include/linux/console.h
+++ b/include/linux/console.h
@@ -158,6 +158,14 @@ struct console {
 	struct hlist_node node;
 };
 
+#ifdef CONFIG_LOCKDEP
+extern void lockdep_assert_console_list_lock_held(void);
+#else
+static inline void lockdep_assert_console_list_lock_held(void)
+{
+}
+#endif
+
 #ifdef CONFIG_DEBUG_LOCK_ALLOC
 extern bool console_srcu_read_lock_is_held(void);
 #else
@@ -170,6 +178,9 @@ static inline bool console_srcu_read_lock_is_held(void)
 extern int console_srcu_read_lock(void);
 extern void console_srcu_read_unlock(int cookie);
 
+extern void console_list_lock(void) __acquires(console_mutex);
+extern void console_list_unlock(void) __releases(console_mutex);
+
 extern struct hlist_head console_list;
 
 /**
@@ -213,10 +224,16 @@ static inline bool console_is_enabled(const struct console *con)
 	hlist_for_each_entry_srcu(con, &console_list, node,		\
 				  console_srcu_read_lock_is_held())
 
-/*
- * for_each_console() allows you to iterate on each console
+/**
+ * for_each_console() - Iterator over registered consoles
+ * @con:	struct console pointer used as loop cursor
+ *
+ * The console list and the console->flags are immutable while iterating.
+ *
+ * Requires console_list_lock to be held.
  */
-#define for_each_console(con) \
+#define for_each_console(con)						\
+	lockdep_assert_console_list_lock_held();			\
 	hlist_for_each_entry(con, &console_list, node)
 
 extern int console_set_on_cmdline;
diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index d8ccb7b523e2..31387ba3fa1a 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -78,6 +78,13 @@ EXPORT_SYMBOL(ignore_console_lock_warning);
 int oops_in_progress;
 EXPORT_SYMBOL(oops_in_progress);
 
+/*
+ * console_mutex protects console_list updates and console->flags updates.
+ * The flags are synchronized only for consoles that are registered, i.e.
+ * accessible via the console list.
+ */
+static DEFINE_MUTEX(console_mutex);
+
 /*
  * console_sem protects console_list and console->flags updates, and also
  * provides serialization for access to the entire console driver system.
@@ -103,6 +110,11 @@ static int __read_mostly suppress_panic_printk;
 static struct lockdep_map console_lock_dep_map = {
 	.name = "console_lock"
 };
+
+void lockdep_assert_console_list_lock_held(void)
+{
+	lockdep_assert_held(&console_mutex);
+}
 #endif
 
 #ifdef CONFIG_DEBUG_LOCK_ALLOC
@@ -227,6 +239,39 @@ int devkmsg_sysctl_set_loglvl(struct ctl_table *table, int write,
 }
 #endif /* CONFIG_PRINTK && CONFIG_SYSCTL */
 
+/**
+ * console_list_lock - Lock the console list
+ *
+ * For console list or console->flags updates
+ */
+void console_list_lock(void)
+{
+	/*
+	 * In unregister_console(), synchronize_srcu() is called with the
+	 * console_list_lock held. Therefore it is not allowed that the
+	 * console_list_lock is taken with the srcu_lock held.
+	 *
+	 * Whether or not this context is in the read-side critical section
+	 * can only be detected if the appropriate debug options are enabled.
+	 */
+	WARN_ON_ONCE(debug_lockdep_rcu_enabled() &&
+		     srcu_read_lock_held(&console_srcu));
+
+	mutex_lock(&console_mutex);
+}
+EXPORT_SYMBOL(console_list_lock);
+
+/**
+ * console_list_unlock - Unlock the console list
+ *
+ * Counterpart to console_list_lock()
+ */
+void console_list_unlock(void)
+{
+	mutex_unlock(&console_mutex);
+}
+EXPORT_SYMBOL(console_list_unlock);
+
 /**
  * console_srcu_read_lock - Register a new reader for the
  *	SRCU-protected console list
@@ -3060,9 +3105,11 @@ struct tty_driver *console_device(int *index)
 void console_stop(struct console *console)
 {
 	__pr_flush(console, 1000, true);
+	console_list_lock();
 	console_lock();
 	WRITE_ONCE(console->flags, console->flags & ~CON_ENABLED);
 	console_unlock();
+	console_list_unlock();
 
 	/*
 	 * Ensure that all SRCU list walks have completed. All contexts must
@@ -3076,9 +3123,11 @@ EXPORT_SYMBOL(console_stop);
 
 void console_start(struct console *console)
 {
+	console_list_lock();
 	console_lock();
 	WRITE_ONCE(console->flags, console->flags | CON_ENABLED);
 	console_unlock();
+	console_list_unlock();
 	__pr_flush(console, 1000, true);
 }
 EXPORT_SYMBOL(console_start);
@@ -3177,6 +3226,8 @@ static void try_enable_default_console(struct console *newcon)
 #define console_first()				\
 	hlist_entry(console_list.first, struct console, node)
 
+static int unregister_console_locked(struct console *console);
+
 /*
  * The console driver calls this routine during kernel initialization
  * to register the console printing procedure with printk() and to
@@ -3203,13 +3254,14 @@ void register_console(struct console *newcon)
 	bool realcon_enabled = false;
 	int err;
 
+	console_list_lock();
+
 	for_each_console(con) {
 		if (WARN(con == newcon, "console '%s%d' already registered\n",
-					 con->name, con->index))
-			return;
-	}
+					 con->name, con->index)) {
+			goto unlock;
+		}
 
-	for_each_console(con) {
 		if (con->flags & CON_BOOT)
 			bootcon_enabled = true;
 		else
@@ -3220,7 +3272,7 @@ void register_console(struct console *newcon)
 	if (newcon->flags & CON_BOOT && realcon_enabled) {
 		pr_info("Too late to register bootconsole %s%d\n",
 			newcon->name, newcon->index);
-		return;
+		goto unlock;
 	}
 
 	/*
@@ -3251,7 +3303,7 @@ void register_console(struct console *newcon)
 
 	/* printk() messages are not printed to the Braille console. */
 	if (err || newcon->flags & CON_BRL)
-		return;
+		goto unlock;
 
 	/*
 	 * If we have a bootconsole, and are switching to a real console,
@@ -3330,16 +3382,21 @@ void register_console(struct console *newcon)
 
 		hlist_for_each_entry_safe(con, tmp, &console_list, node) {
 			if (con->flags & CON_BOOT)
-				unregister_console(con);
+				unregister_console_locked(con);
 		}
 	}
+unlock:
+	console_list_unlock();
 }
 EXPORT_SYMBOL(register_console);
 
-int unregister_console(struct console *console)
+/* Must be called under console_list_lock(). */
+static int unregister_console_locked(struct console *console)
 {
 	int res;
 
+	lockdep_assert_console_list_lock_held();
+
 	con_printk(KERN_INFO, console, "disabled\n");
 
 	res = _braille_unregister_console(console);
@@ -3388,6 +3445,16 @@ int unregister_console(struct console *console)
 
 	return res;
 }
+
+int unregister_console(struct console *console)
+{
+	int res;
+
+	console_list_lock();
+	res = unregister_console_locked(console);
+	console_list_unlock();
+	return res;
+}
 EXPORT_SYMBOL(unregister_console);
 
 /*
@@ -3440,6 +3507,7 @@ static int __init printk_late_init(void)
 	struct console *con;
 	int ret;
 
+	console_list_lock();
 	hlist_for_each_entry_safe(con, tmp, &console_list, node) {
 		if (!(con->flags & CON_BOOT))
 			continue;
@@ -3457,9 +3525,10 @@ static int __init printk_late_init(void)
 			 */
 			pr_warn("bootconsole [%s%d] uses init memory and must be disabled even before the real one is ready\n",
 				con->name, con->index);
-			unregister_console(con);
+			unregister_console_locked(con);
 		}
 	}
+	console_list_unlock();
 
 	ret = cpuhp_setup_state_nocalls(CPUHP_PRINTK_DEAD, "printk:dead", NULL,
 					console_cpu_notify);
-- 
2.30.2

Re: [PATCH printk v3 21/40] printk: introduce console_list_lock

[Petr Mladek]

On Mon 2022-11-07 15:22:19, John Ogness wrote:
> Currently there exist races in register_console(), where the types
> of registered consoles are checked (without holding the console_lock)
> and then after acquiring the console_lock, it is assumed that the list
> has not changed. Also, some code that performs console_unregister()
> make similar assumptions.
> 
> It might be possible to fix these races using the console_lock. But
> it would require a complex analysis of all console drivers to make
> sure that the console_lock is not taken in match() and setup()
> callbacks. And we really prefer to split up and reduce the
> responsibilities of console_lock rather than expand its complexity.
> Therefore, introduce a new console_list_lock to provide full
> synchronization for any console list changes.
> 
> In addition, also use console_list_lock for synchronization of
> console->flags updates. All flags are either static or modified only
> during the console registration. There are only two exceptions.
> 
> The first exception is CON_ENABLED, which is also modified by
> console_start()/console_stop(). Therefore, these functions must
> also take the console_list_lock.
> 
> The second exception is when the flags are modified by the console
> driver init code before the console is registered. These will be
> ignored because they are not visible to the rest of the system
> via the console_drivers list.
> 
> Note that one of the various responsibilities of the console_lock is
> also intended to provide console list and console->flags
> synchronization. Later changes will update call sites relying on the
> console_lock for these purposes. Once all call sites have been
> updated, the console_lock will be relieved of synchronizing
> console_list and console->flags updates.
> 
> Suggested-by: Thomas Gleixner <tglx@linutronix.de>
> Signed-off-by: John Ogness <john.ogness@linutronix.de>

Reviewed-by: Petr Mladek <pmladek@suse.com>

Best Regards,
Petr

[PATCH printk v3 22/40] console: introduce console_is_registered()

[John Ogness]

Currently it is not possible for drivers to detect if they have
already successfully registered their console. Several drivers
have multiple paths that lead to console registration. To avoid
attempting a 2nd registration (which leads to a WARN), drivers
are implementing their own solution.

Introduce console_is_registered() so drivers can easily identify
if their console is currently registered. A _locked() variant
is also provided if the caller is already holding the
console_list_lock.

Signed-off-by: John Ogness <john.ogness@linutronix.de>
---
 include/linux/console.h | 28 ++++++++++++++++++++++++++++
 kernel/printk/printk.c  |  2 +-
 2 files changed, 29 insertions(+), 1 deletion(-)

diff --git a/include/linux/console.h b/include/linux/console.h
index 47be23be8a88..cdae70e27377 100644
--- a/include/linux/console.h
+++ b/include/linux/console.h
@@ -210,6 +210,34 @@ static inline bool console_is_enabled(const struct console *con)
 	return (data_race(READ_ONCE(con->flags)) & CON_ENABLED);
 }
 
+/* Variant of console_is_registered() when the console_list_lock is held. */
+static inline bool console_is_registered_locked(const struct console *con)
+{
+	lockdep_assert_console_list_lock_held();
+	return !hlist_unhashed(&con->node);
+}
+
+/*
+ * console_is_registered - Check if the console is registered
+ * @con:	struct console pointer of console to check
+ *
+ * Context: Process context. May sleep while acquiring console list lock.
+ * Return: true if the console is in the console list, otherwise false.
+ *
+ * If false is returned for a console that was previously registered, it
+ * can be assumed that the console's unregistration is fully completed,
+ * including the exit() callback after console list removal.
+ */
+static inline bool console_is_registered(const struct console *con)
+{
+	bool ret;
+
+	console_list_lock();
+	ret = console_is_registered_locked(con);
+	console_list_unlock();
+	return ret;
+}
+
 /**
  * for_each_console_srcu() - Iterator over registered consoles
  * @con:	struct console pointer used as loop cursor
diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index 31387ba3fa1a..be40a9688403 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -3410,7 +3410,7 @@ static int unregister_console_locked(struct console *console)
 	/* Disable it unconditionally */
 	WRITE_ONCE(console->flags, console->flags & ~CON_ENABLED);
 
-	if (hlist_unhashed(&console->node)) {
+	if (!console_is_registered_locked(console)) {
 		console_unlock();
 		return -ENODEV;
 	}
-- 
2.30.2

Re: [PATCH printk v3 22/40] console: introduce console_is_registered()

[Petr Mladek]

On Mon 2022-11-07 15:22:20, John Ogness wrote:
> Currently it is not possible for drivers to detect if they have
> already successfully registered their console. Several drivers
> have multiple paths that lead to console registration. To avoid
> attempting a 2nd registration (which leads to a WARN), drivers
> are implementing their own solution.
> 
> Introduce console_is_registered() so drivers can easily identify
> if their console is currently registered. A _locked() variant
> is also provided if the caller is already holding the
> console_list_lock.
> 
> Signed-off-by: John Ogness <john.ogness@linutronix.de>

Reviewed-by: Petr Mladek <pmladek@suse.com>

Best Regards,
Petr

[PATCH printk v3 23/40] serial_core: replace uart_console_enabled() with uart_console_registered()

[John Ogness]

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(-)

diff --git a/drivers/tty/serial/8250/8250_core.c b/drivers/tty/serial/8250/8250_core.c
index 94fbf0add2ce..74568292186f 100644
--- a/drivers/tty/serial/8250/8250_core.c
+++ b/drivers/tty/serial/8250/8250_core.c
@@ -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);
diff --git a/drivers/tty/serial/pic32_uart.c b/drivers/tty/serial/pic32_uart.c
index 2beada66c824..1183b2a26539 100644
--- a/drivers/tty/serial/pic32_uart.c
+++ b/drivers/tty/serial/pic32_uart.c
@@ -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.
 		 */
diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index 179ee199df34..b9fbbee598b8 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -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)
diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
index d657f2a42a7b..2f910f2bbe53 100644
--- 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;
 }
 
 struct uart_port *uart_get_console(struct uart_port *ports, int nr,
-- 
2.30.2

Re: [PATCH printk v3 23/40] serial_core: replace uart_console_enabled() with uart_console_registered()

[Geert Uytterhoeven]

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

Re: [PATCH printk v3 23/40] serial_core: replace uart_console_enabled() with uart_console_registered()

[Petr Mladek]

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

Re: [PATCH printk v3 23/40] serial_core: replace uart_console_enabled() with uart_console_registered()

[John Ogness]

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

[PATCH printk v3 24/40] tty: nfcon: use console_is_registered()

[John Ogness]

Currently CON_ENABLED is being (mis)used to identify if the console
has been registered. This is not reliable because it can be set even
though registration failed or it can be unset, even though the console
is registered.

Instead, use console_is_registered().

Signed-off-by: John Ogness <john.ogness@linutronix.de>
---
 arch/m68k/emu/nfcon.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/arch/m68k/emu/nfcon.c b/arch/m68k/emu/nfcon.c
index 557d60867f98..292669fa480f 100644
--- a/arch/m68k/emu/nfcon.c
+++ b/arch/m68k/emu/nfcon.c
@@ -49,14 +49,14 @@ static void nfcon_write(struct console *con, const char *str,
 static struct tty_driver *nfcon_device(struct console *con, int *index)
 {
 	*index = 0;
-	return (con->flags & CON_ENABLED) ? nfcon_tty_driver : NULL;
+	return console_is_registered(con) ? nfcon_tty_driver : NULL;
 }
 
 static struct console nf_console = {
 	.name	= "nfcon",
 	.write	= nfcon_write,
 	.device	= nfcon_device,
-	.flags	= CON_PRINTBUFFER,
+	.flags	= CON_PRINTBUFFER | CON_ENABLED,
 	.index	= -1,
 };
 
@@ -106,10 +106,8 @@ static int __init nf_debug_setup(char *arg)
 		return 0;
 
 	stderr_id = nf_get_id("NF_STDERR");
-	if (stderr_id) {
-		nf_console.flags |= CON_ENABLED;
+	if (stderr_id)
 		register_console(&nf_console);
-	}
 
 	return 0;
 }
@@ -151,7 +149,7 @@ static int __init nfcon_init(void)
 
 	nfcon_tty_driver = driver;
 
-	if (!(nf_console.flags & CON_ENABLED))
+	if (!console_is_registered(&nf_console))
 		register_console(&nf_console);
 
 	return 0;
-- 
2.30.2

Re: [PATCH printk v3 24/40] tty: nfcon: use console_is_registered()

[Geert Uytterhoeven]

Hi John,

On Mon, Nov 7, 2022 at 3:16 PM John Ogness <john.ogness@linutronix.de> wrote:
> Currently CON_ENABLED is being (mis)used to identify if the console
> has been registered. This is not reliable because it can be set even
> though registration failed or it can be unset, even though the console
> is registered.
>
> Instead, use console_is_registered().
>
> Signed-off-by: John Ogness <john.ogness@linutronix.de>

Thanks for your patch!

LGTM, so
Reviewed-by: Geert Uytterhoeven <geert@linux-m68k.org>
Acked-by: Geert Uytterhoeven <geert@linux-m68k.org>

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

Re: [PATCH printk v3 24/40] tty: nfcon: use console_is_registered()

[Petr Mladek]

On Mon 2022-11-07 15:22:22, John Ogness wrote:
> Currently CON_ENABLED is being (mis)used to identify if the console
> has been registered. This is not reliable because it can be set even
> though registration failed or it can be unset, even though the console
> is registered.
>
> Signed-off-by: John Ogness <john.ogness@linutronix.de>
> ---
>  arch/m68k/emu/nfcon.c | 10 ++++------
>  1 file changed, 4 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/m68k/emu/nfcon.c b/arch/m68k/emu/nfcon.c
> index 557d60867f98..292669fa480f 100644
> --- a/arch/m68k/emu/nfcon.c
> +++ b/arch/m68k/emu/nfcon.c
> @@ -49,14 +49,14 @@ static void nfcon_write(struct console *con, const char *str,
>  static struct tty_driver *nfcon_device(struct console *con, int *index)
>  {
>  	*index = 0;
> -	return (con->flags & CON_ENABLED) ? nfcon_tty_driver : NULL;
> +	return console_is_registered(con) ? nfcon_tty_driver : NULL;
>  }
>  
>  static struct console nf_console = {
>  	.name	= "nfcon",
>  	.write	= nfcon_write,
>  	.device	= nfcon_device,
> -	.flags	= CON_PRINTBUFFER,
> +	.flags	= CON_PRINTBUFFER | CON_ENABLED,

This causes that register_console() will always put the console into
console_list. It causes regression, see below.


>  	.index	= -1,
>  };
>  
> @@ -106,10 +106,8 @@ static int __init nf_debug_setup(char *arg)
>  		return 0;
>  
>  	stderr_id = nf_get_id("NF_STDERR");
> -	if (stderr_id) {
> -		nf_console.flags |= CON_ENABLED;
> +	if (stderr_id)
>  		register_console(&nf_console);

My understanding is that this should enable the console
when debug=nfcon kernel parameter is used.

It is a non-standard way. This is why CON_ENABLED flag
has to be explicitly set.

> -	}
>  
>  	return 0;
>  }
> @@ -151,7 +149,7 @@ static int __init nfcon_init(void)
>  
>  	nfcon_tty_driver = driver;
>  
> -	if (!(nf_console.flags & CON_ENABLED))
> +	if (!console_is_registered(&nf_console))
>  		register_console(&nf_console);

This calls register_console() when it was not already
registered by the debug=nfcon early parameter.

It is the standard way. It should enable the console only
when console=nfcon is defined on the commandline. Or when
there is no preferred console and no console enabled by default yet.

We should not pre-set CON_ENABLED in this case.

Best Regards,
Petr

Re: [PATCH printk v3 24/40] tty: nfcon: use console_is_registered()

[John Ogness]

On 2022-11-10, Petr Mladek <pmladek@suse.com> wrote:
>> diff --git a/arch/m68k/emu/nfcon.c b/arch/m68k/emu/nfcon.c
>> index 557d60867f98..292669fa480f 100644
>> --- a/arch/m68k/emu/nfcon.c
>> +++ b/arch/m68k/emu/nfcon.c
>> @@ -49,14 +49,14 @@ static void nfcon_write(struct console *con, const char *str,
>>  static struct tty_driver *nfcon_device(struct console *con, int *index)
>>  {
>>  	*index = 0;
>> -	return (con->flags & CON_ENABLED) ? nfcon_tty_driver : NULL;
>> +	return console_is_registered(con) ? nfcon_tty_driver : NULL;
>>  }
>>  
>>  static struct console nf_console = {
>>  	.name	= "nfcon",
>>  	.write	= nfcon_write,
>>  	.device	= nfcon_device,
>> -	.flags	= CON_PRINTBUFFER,
>> +	.flags	= CON_PRINTBUFFER | CON_ENABLED,
>
> This causes that register_console() will always put the console into
> console_list. It causes regression, see below.

Agreed. Nice catch. I will remove initializing with CON_ENABLED.

>>  	.index	= -1,
>>  };
>>  
>> @@ -106,10 +106,8 @@ static int __init nf_debug_setup(char *arg)
>>  		return 0;
>>  
>>  	stderr_id = nf_get_id("NF_STDERR");
>> -	if (stderr_id) {
>> -		nf_console.flags |= CON_ENABLED;
>> +	if (stderr_id)
>>  		register_console(&nf_console);
>
> My understanding is that this should enable the console
> when debug=nfcon kernel parameter is used.
>
> It is a non-standard way. This is why CON_ENABLED flag
> has to be explicitly set.

Understood. I will add a comment explaining why CON_ENABLED is set here.

>> -	}
>>  
>>  	return 0;
>>  }
>> @@ -151,7 +149,7 @@ static int __init nfcon_init(void)
>>  
>>  	nfcon_tty_driver = driver;
>>  
>> -	if (!(nf_console.flags & CON_ENABLED))
>> +	if (!console_is_registered(&nf_console))
>>  		register_console(&nf_console);
>
> This calls register_console() when it was not already
> registered by the debug=nfcon early parameter.
>
> It is the standard way. It should enable the console only
> when console=nfcon is defined on the commandline. Or when
> there is no preferred console and no console enabled by default yet.
>
> We should not pre-set CON_ENABLED in this case.

Agreed. The hunk itself is OK.

John

Re: [PATCH printk v3 24/40] tty: nfcon: use console_is_registered()

[Eero Tamminen]

Hi,

On 10.11.2022 16.19, John Ogness wrote:
> On 2022-11-10, Petr Mladek <pmladek@suse.com> wrote:
...
>>> @@ -106,10 +106,8 @@ static int __init nf_debug_setup(char *arg)
>>>   		return 0;
>>>   
>>>   	stderr_id = nf_get_id("NF_STDERR");
>>> -	if (stderr_id) {
>>> -		nf_console.flags |= CON_ENABLED;
>>> +	if (stderr_id)
>>>   		register_console(&nf_console);
>>
>> My understanding is that this should enable the console
>> when debug=nfcon kernel parameter is used.
>>
>> It is a non-standard way. This is why CON_ENABLED flag
>> has to be explicitly set.
> 
> Understood. I will add a comment explaining why CON_ENABLED is set here.

NatFeats is emulator feature.  If you want to test the resulting kernel, 
you can use either Aranym or Hatari emulator.

Aranym NF docs are here:
https://github.com/aranym/aranym/wiki/natfeats-proposal

Hatari m68k linux docs are here:
https://hatari.tuxfamily.org/doc/m68k-linux.txt


	- Eero

[PATCH printk v3 25/40] efi: earlycon: use console_is_registered()

[John Ogness]

The CON_ENABLED status of a console is a runtime setting that does not
involve the console driver. Drivers must not assume that if the console
is disabled then proper hardware management is not needed. For the EFI
earlycon case, it is about remapping/unmapping memory for the
framebuffer.

Use console_is_registered() instead of checking CON_ENABLED.

Signed-off-by: John Ogness <john.ogness@linutronix.de>
---
 drivers/firmware/efi/earlycon.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/firmware/efi/earlycon.c b/drivers/firmware/efi/earlycon.c
index a52236e11e5f..4d6c5327471a 100644
--- a/drivers/firmware/efi/earlycon.c
+++ b/drivers/firmware/efi/earlycon.c
@@ -29,8 +29,8 @@ static void *efi_fb;
  */
 static int __init efi_earlycon_remap_fb(void)
 {
-	/* bail if there is no bootconsole or it has been disabled already */
-	if (!earlycon_console || !(earlycon_console->flags & CON_ENABLED))
+	/* bail if there is no bootconsole or it was unregistered already */
+	if (!earlycon_console || !console_is_registered(earlycon_console))
 		return 0;
 
 	efi_fb = memremap(fb_base, screen_info.lfb_size,
@@ -42,8 +42,8 @@ early_initcall(efi_earlycon_remap_fb);
 
 static int __init efi_earlycon_unmap_fb(void)
 {
-	/* unmap the bootconsole fb unless keep_bootcon has left it enabled */
-	if (efi_fb && !(earlycon_console->flags & CON_ENABLED))
+	/* unmap the bootconsole fb unless keep_bootcon left it registered */
+	if (efi_fb && !console_is_registered(earlycon_console))
 		memunmap(efi_fb);
 	return 0;
 }
-- 
2.30.2

Re: [PATCH printk v3 25/40] efi: earlycon: use console_is_registered()

[Petr Mladek]

On Mon 2022-11-07 15:22:23, John Ogness wrote:
> The CON_ENABLED status of a console is a runtime setting that does not
> involve the console driver. Drivers must not assume that if the console
> is disabled then proper hardware management is not needed. For the EFI
> earlycon case, it is about remapping/unmapping memory for the
> framebuffer.
> 
> Use console_is_registered() instead of checking CON_ENABLED.
> 
> Signed-off-by: John Ogness <john.ogness@linutronix.de>

Reviewed-by: Petr Mladek <pmladek@suse.com>

Best Regards,
Petr

[PATCH printk v3 26/40] tty: hvc: use console_is_registered()

[John Ogness]

It is not reliable to check for CON_ENABLED in order to identify if a
console is registered. Use console_is_registered() instead.

Signed-off-by: John Ogness <john.ogness@linutronix.de>
---
 drivers/tty/hvc/hvc_console.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/hvc/hvc_console.c b/drivers/tty/hvc/hvc_console.c
index 4802cfaa107f..a683e21df19c 100644
--- a/drivers/tty/hvc/hvc_console.c
+++ b/drivers/tty/hvc/hvc_console.c
@@ -264,8 +264,8 @@ static void hvc_port_destruct(struct tty_port *port)
 
 static void hvc_check_console(int index)
 {
-	/* Already enabled, bail out */
-	if (hvc_console.flags & CON_ENABLED)
+	/* Already registered, bail out */
+	if (console_is_registered(&hvc_console))
 		return;
 
  	/* If this index is what the user requested, then register
-- 
2.30.2

[PATCH printk v3 26/40] tty: hvc: use console_is_registered()

[John Ogness]

It is not reliable to check for CON_ENABLED in order to identify if a
console is registered. Use console_is_registered() instead.

Signed-off-by: John Ogness <john.ogness@linutronix.de>
---
 drivers/tty/hvc/hvc_console.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/hvc/hvc_console.c b/drivers/tty/hvc/hvc_console.c
index 4802cfaa107f..a683e21df19c 100644
--- a/drivers/tty/hvc/hvc_console.c
+++ b/drivers/tty/hvc/hvc_console.c
@@ -264,8 +264,8 @@ static void hvc_port_destruct(struct tty_port *port)
 
 static void hvc_check_console(int index)
 {
-	/* Already enabled, bail out */
-	if (hvc_console.flags & CON_ENABLED)
+	/* Already registered, bail out */
+	if (console_is_registered(&hvc_console))
 		return;
 
  	/* If this index is what the user requested, then register
-- 
2.30.2

Re: [PATCH printk v3 26/40] tty: hvc: use console_is_registered()

[Petr Mladek]

On Mon 2022-11-07 15:22:24, John Ogness wrote:
> It is not reliable to check for CON_ENABLED in order to identify if a
> console is registered. Use console_is_registered() instead.
> 
> Signed-off-by: John Ogness <john.ogness@linutronix.de>

Reviewed-by: Petr Mladek <pmladek@suse.com>

Best Regards,
Petr

Re: [PATCH printk v3 26/40] tty: hvc: use console_is_registered()

[Petr Mladek]

On Mon 2022-11-07 15:22:24, John Ogness wrote:
> It is not reliable to check for CON_ENABLED in order to identify if a
> console is registered. Use console_is_registered() instead.
> 
> Signed-off-by: John Ogness <john.ogness@linutronix.de>

Reviewed-by: Petr Mladek <pmladek@suse.com>

Best Regards,
Petr

[PATCH printk v3 27/40] tty: serial: earlycon: use console_is_registered()

[John Ogness]

It is not reliable to check for CON_ENABLED in order to identify if a
console is registered. Use console_is_registered() instead.

Signed-off-by: John Ogness <john.ogness@linutronix.de>
---
 drivers/tty/serial/earlycon.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/serial/earlycon.c b/drivers/tty/serial/earlycon.c
index a5f380584cda..4f6e9bf57169 100644
--- a/drivers/tty/serial/earlycon.c
+++ b/drivers/tty/serial/earlycon.c
@@ -181,7 +181,7 @@ int __init setup_earlycon(char *buf)
 	if (!buf || !buf[0])
 		return -EINVAL;
 
-	if (early_con.flags & CON_ENABLED)
+	if (console_is_registered(&early_con))
 		return -EALREADY;
 
 again:
@@ -253,7 +253,7 @@ int __init of_setup_earlycon(const struct earlycon_id *match,
 	bool big_endian;
 	u64 addr;
 
-	if (early_con.flags & CON_ENABLED)
+	if (console_is_registered(&early_con))
 		return -EALREADY;
 
 	spin_lock_init(&port->lock);
-- 
2.30.2

Re: [PATCH printk v3 27/40] tty: serial: earlycon: use console_is_registered()

[Petr Mladek]

On Mon 2022-11-07 15:22:25, John Ogness wrote:
> It is not reliable to check for CON_ENABLED in order to identify if a
> console is registered. Use console_is_registered() instead.
> 
> Signed-off-by: John Ogness <john.ogness@linutronix.de>

Reviewed-by: Petr Mladek <pmladek@suse.com>

Best Regards,
Petr

[PATCH printk v3 28/40] tty: serial: pic32_uart: use console_is_registered()

[John Ogness]

It is not reliable to check for CON_ENABLED in order to identify if a
console is registered. Use console_is_registered() instead.

Signed-off-by: John Ogness <john.ogness@linutronix.de>
---
 drivers/tty/serial/pic32_uart.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/tty/serial/pic32_uart.c b/drivers/tty/serial/pic32_uart.c
index 1183b2a26539..c38754d593ca 100644
--- a/drivers/tty/serial/pic32_uart.c
+++ b/drivers/tty/serial/pic32_uart.c
@@ -843,7 +843,7 @@ console_initcall(pic32_console_init);
  */
 static int __init pic32_late_console_init(void)
 {
-	if (!(pic32_console.flags & CON_ENABLED))
+	if (!console_is_registered(&pic32_console))
 		register_console(&pic32_console);
 
 	return 0;
-- 
2.30.2

Re: [PATCH printk v3 28/40] tty: serial: pic32_uart: use console_is_registered()

[Petr Mladek]

On Mon 2022-11-07 15:22:26, John Ogness wrote:
> It is not reliable to check for CON_ENABLED in order to identify if a
> console is registered. Use console_is_registered() instead.
> 
> Signed-off-by: John Ogness <john.ogness@linutronix.de>

Reviewed-by: Petr Mladek <pmladek@suse.com>

Best Regards,
Petr

[PATCH printk v3 29/40] tty: serial: samsung_tty: use console_is_registered()

[John Ogness]

It is not reliable to check for CON_ENABLED in order to identify if a
console is registered. Use console_is_registered() instead.

Signed-off-by: John Ogness <john.ogness@linutronix.de>
---
 drivers/tty/serial/samsung_tty.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/tty/serial/samsung_tty.c b/drivers/tty/serial/samsung_tty.c
index 77d1363029f5..9c252c9ca95a 100644
--- a/drivers/tty/serial/samsung_tty.c
+++ b/drivers/tty/serial/samsung_tty.c
@@ -1732,7 +1732,7 @@ static void __init s3c24xx_serial_register_console(void)
 
 static void s3c24xx_serial_unregister_console(void)
 {
-	if (s3c24xx_serial_console.flags & CON_ENABLED)
+	if (console_is_registered(&s3c24xx_serial_console))
 		unregister_console(&s3c24xx_serial_console);
 }
 
-- 
2.30.2

[PATCH printk v3 29/40] tty: serial: samsung_tty: use console_is_registered()

[John Ogness]

It is not reliable to check for CON_ENABLED in order to identify if a
console is registered. Use console_is_registered() instead.

Signed-off-by: John Ogness <john.ogness@linutronix.de>
---
 drivers/tty/serial/samsung_tty.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/tty/serial/samsung_tty.c b/drivers/tty/serial/samsung_tty.c
index 77d1363029f5..9c252c9ca95a 100644
--- a/drivers/tty/serial/samsung_tty.c
+++ b/drivers/tty/serial/samsung_tty.c
@@ -1732,7 +1732,7 @@ static void __init s3c24xx_serial_register_console(void)
 
 static void s3c24xx_serial_unregister_console(void)
 {
-	if (s3c24xx_serial_console.flags & CON_ENABLED)
+	if (console_is_registered(&s3c24xx_serial_console))
 		unregister_console(&s3c24xx_serial_console);
 }
 
-- 
2.30.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH printk v3 29/40] tty: serial: samsung_tty: use console_is_registered()

[Petr Mladek]

On Mon 2022-11-07 15:22:27, John Ogness wrote:
> It is not reliable to check for CON_ENABLED in order to identify if a
> console is registered. Use console_is_registered() instead.
> 
> Signed-off-by: John Ogness <john.ogness@linutronix.de>

Reviewed-by: Petr Mladek <pmladek@suse.com>

Best Regards,
Petr

Re: [PATCH printk v3 29/40] tty: serial: samsung_tty: use console_is_registered()

[Petr Mladek]

On Mon 2022-11-07 15:22:27, John Ogness wrote:
> It is not reliable to check for CON_ENABLED in order to identify if a
> console is registered. Use console_is_registered() instead.
> 
> Signed-off-by: John Ogness <john.ogness@linutronix.de>

Reviewed-by: Petr Mladek <pmladek@suse.com>

Best Regards,
Petr

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH printk v3 30/40] tty: serial: xilinx_uartps: use console_is_registered()

[John Ogness]

It is not reliable to check for CON_ENABLED in order to identify if a
console is registered. Use console_is_registered() instead.

Signed-off-by: John Ogness <john.ogness@linutronix.de>
---
 drivers/tty/serial/xilinx_uartps.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/tty/serial/xilinx_uartps.c b/drivers/tty/serial/xilinx_uartps.c
index 2eff7cff57c4..0cbd1892c53b 100644
--- a/drivers/tty/serial/xilinx_uartps.c
+++ b/drivers/tty/serial/xilinx_uartps.c
@@ -1631,7 +1631,7 @@ static int cdns_uart_probe(struct platform_device *pdev)
 #ifdef CONFIG_SERIAL_XILINX_PS_UART_CONSOLE
 	/* This is not port which is used for console that's why clean it up */
 	if (console_port == port &&
-	    !(cdns_uart_uart_driver.cons->flags & CON_ENABLED)) {
+	    !console_is_registered(cdns_uart_uart_driver.cons)) {
 		console_port = NULL;
 		cdns_uart_console.index = -1;
 	}
-- 
2.30.2

[PATCH printk v3 30/40] tty: serial: xilinx_uartps: use console_is_registered()

[John Ogness]

It is not reliable to check for CON_ENABLED in order to identify if a
console is registered. Use console_is_registered() instead.

Signed-off-by: John Ogness <john.ogness@linutronix.de>
---
 drivers/tty/serial/xilinx_uartps.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/tty/serial/xilinx_uartps.c b/drivers/tty/serial/xilinx_uartps.c
index 2eff7cff57c4..0cbd1892c53b 100644
--- a/drivers/tty/serial/xilinx_uartps.c
+++ b/drivers/tty/serial/xilinx_uartps.c
@@ -1631,7 +1631,7 @@ static int cdns_uart_probe(struct platform_device *pdev)
 #ifdef CONFIG_SERIAL_XILINX_PS_UART_CONSOLE
 	/* This is not port which is used for console that's why clean it up */
 	if (console_port == port &&
-	    !(cdns_uart_uart_driver.cons->flags & CON_ENABLED)) {
+	    !console_is_registered(cdns_uart_uart_driver.cons)) {
 		console_port = NULL;
 		cdns_uart_console.index = -1;
 	}
-- 
2.30.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH printk v3 30/40] tty: serial: xilinx_uartps: use console_is_registered()

[Petr Mladek]

On Mon 2022-11-07 15:22:28, John Ogness wrote:
> It is not reliable to check for CON_ENABLED in order to identify if a
> console is registered. Use console_is_registered() instead.
> 
> Signed-off-by: John Ogness <john.ogness@linutronix.de>

Reviewed-by: Petr Mladek <pmladek@suse.com>

Best Regards,
Petr

Re: [PATCH printk v3 30/40] tty: serial: xilinx_uartps: use console_is_registered()

[Petr Mladek]

On Mon 2022-11-07 15:22:28, John Ogness wrote:
> It is not reliable to check for CON_ENABLED in order to identify if a
> console is registered. Use console_is_registered() instead.
> 
> Signed-off-by: John Ogness <john.ogness@linutronix.de>

Reviewed-by: Petr Mladek <pmladek@suse.com>

Best Regards,
Petr

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH printk v3 31/40] usb: early: xhci-dbc: use console_is_registered()

[John Ogness]

It is not reliable to check for CON_ENABLED in order to identify if a
console is registered. Use console_is_registered() instead.

Signed-off-by: John Ogness <john.ogness@linutronix.de>
---
 drivers/usb/early/xhci-dbc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/early/xhci-dbc.c b/drivers/usb/early/xhci-dbc.c
index bfb7e2b85299..797047154820 100644
--- a/drivers/usb/early/xhci-dbc.c
+++ b/drivers/usb/early/xhci-dbc.c
@@ -927,7 +927,7 @@ void __init early_xdbc_register_console(void)
 
 static void xdbc_unregister_console(void)
 {
-	if (early_xdbc_console.flags & CON_ENABLED)
+	if (console_is_registered(&early_xdbc_console))
 		unregister_console(&early_xdbc_console);
 }
 
-- 
2.30.2

Re: [PATCH printk v3 31/40] usb: early: xhci-dbc: use console_is_registered()

[Petr Mladek]

On Mon 2022-11-07 15:22:29, John Ogness wrote:
> It is not reliable to check for CON_ENABLED in order to identify if a
> console is registered. Use console_is_registered() instead.
> 
> Signed-off-by: John Ogness <john.ogness@linutronix.de>

Reviewed-by: Petr Mladek <pmladek@suse.com>

Best Regards,
Petr

[PATCH printk v3 32/40] netconsole: avoid CON_ENABLED misuse to track registration

[John Ogness]

The CON_ENABLED flag is being misused to track whether or not the
extended console should be or has been registered. Instead use
a local variable to decide if the extended console should be
registered and console_is_registered() to determine if it has
been registered.

Also add a check in cleanup_netconsole() to only unregister the
extended console if it has been registered.

Signed-off-by: John Ogness <john.ogness@linutronix.de>
---
 drivers/net/netconsole.c | 21 +++++++++++----------
 1 file changed, 11 insertions(+), 10 deletions(-)

diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c
index bdff9ac5056d..4f4f79532c6c 100644
--- a/drivers/net/netconsole.c
+++ b/drivers/net/netconsole.c
@@ -332,10 +332,8 @@ static ssize_t enabled_store(struct config_item *item,
 	}
 
 	if (enabled) {	/* true */
-		if (nt->extended && !(netconsole_ext.flags & CON_ENABLED)) {
-			netconsole_ext.flags |= CON_ENABLED;
+		if (nt->extended && !console_is_registered(&netconsole_ext))
 			register_console(&netconsole_ext);
-		}
 
 		/*
 		 * Skip netpoll_parse_options() -- all the attributes are
@@ -869,7 +867,7 @@ static void write_msg(struct console *con, const char *msg, unsigned int len)
 
 static struct console netconsole_ext = {
 	.name	= "netcon_ext",
-	.flags	= CON_EXTENDED,	/* starts disabled, registered on first use */
+	.flags	= CON_ENABLED | CON_EXTENDED,
 	.write	= write_ext_msg,
 };
 
@@ -883,6 +881,7 @@ static int __init init_netconsole(void)
 {
 	int err;
 	struct netconsole_target *nt, *tmp;
+	bool extended = false;
 	unsigned long flags;
 	char *target_config;
 	char *input = config;
@@ -895,11 +894,12 @@ static int __init init_netconsole(void)
 				goto fail;
 			}
 			/* Dump existing printks when we register */
-			if (nt->extended)
-				netconsole_ext.flags |= CON_PRINTBUFFER |
-							CON_ENABLED;
-			else
+			if (nt->extended) {
+				extended = true;
+				netconsole_ext.flags |= CON_PRINTBUFFER;
+			} else {
 				netconsole.flags |= CON_PRINTBUFFER;
+			}
 
 			spin_lock_irqsave(&target_list_lock, flags);
 			list_add(&nt->list, &target_list);
@@ -915,7 +915,7 @@ static int __init init_netconsole(void)
 	if (err)
 		goto undonotifier;
 
-	if (netconsole_ext.flags & CON_ENABLED)
+	if (extended)
 		register_console(&netconsole_ext);
 	register_console(&netconsole);
 	pr_info("network logging started\n");
@@ -945,7 +945,8 @@ static void __exit cleanup_netconsole(void)
 {
 	struct netconsole_target *nt, *tmp;
 
-	unregister_console(&netconsole_ext);
+	if (console_is_registered(&netconsole_ext))
+		unregister_console(&netconsole_ext);
 	unregister_console(&netconsole);
 	dynamic_netconsole_exit();
 	unregister_netdevice_notifier(&netconsole_netdev_notifier);
-- 
2.30.2

Re: [PATCH printk v3 32/40] netconsole: avoid CON_ENABLED misuse to track registration

[Petr Mladek]

On Mon 2022-11-07 15:22:30, John Ogness wrote:
> The CON_ENABLED flag is being misused to track whether or not the
> extended console should be or has been registered. Instead use
> a local variable to decide if the extended console should be
> registered and console_is_registered() to determine if it has
> been registered.
> 
> Also add a check in cleanup_netconsole() to only unregister the
> extended console if it has been registered.
> 
> Signed-off-by: John Ogness <john.ogness@linutronix.de>

Reviewed-by: Petr Mladek <pmladek@suse.com>

Best Regards,
Petr

[PATCH printk v3 33/40] printk, xen: fbfront: create/use safe function for forcing preferred

[John Ogness]

With commit 9e124fe16ff2("xen: Enable console tty by default in domU
if it's not a dummy") a hack was implemented to make sure that the
tty console remains the console behind the /dev/console device. The
main problem with the hack is that, after getting the console pointer
to the tty console, it is assumed the pointer is still valid after
releasing the console_sem. This assumption is incorrect and unsafe.

Make the hack safe by introducing a new function
console_force_preferred_locked() and perform the full operation
under the console_list_lock.

Signed-off-by: John Ogness <john.ogness@linutronix.de>
---
 drivers/video/fbdev/xen-fbfront.c | 12 +++------
 include/linux/console.h           |  1 +
 kernel/printk/printk.c            | 44 ++++++++++++++++++++++++++++---
 3 files changed, 46 insertions(+), 11 deletions(-)

diff --git a/drivers/video/fbdev/xen-fbfront.c b/drivers/video/fbdev/xen-fbfront.c
index 4d2694d904aa..8752d389e382 100644
--- a/drivers/video/fbdev/xen-fbfront.c
+++ b/drivers/video/fbdev/xen-fbfront.c
@@ -504,18 +504,14 @@ static void xenfb_make_preferred_console(void)
 	if (console_set_on_cmdline)
 		return;
 
-	console_lock();
+	console_list_lock();
 	for_each_console(c) {
 		if (!strcmp(c->name, "tty") && c->index == 0)
 			break;
 	}
-	console_unlock();
-	if (c) {
-		unregister_console(c);
-		c->flags |= CON_CONSDEV;
-		c->flags &= ~CON_PRINTBUFFER; /* don't print again */
-		register_console(c);
-	}
+	if (c)
+		console_force_preferred_locked(c);
+	console_list_unlock();
 }
 
 static int xenfb_resume(struct xenbus_device *dev)
diff --git a/include/linux/console.h b/include/linux/console.h
index cdae70e27377..b6b5d796d15c 100644
--- a/include/linux/console.h
+++ b/include/linux/console.h
@@ -273,6 +273,7 @@ enum con_flush_mode {
 };
 
 extern int add_preferred_console(char *name, int idx, char *options);
+extern void console_force_preferred_locked(struct console *con);
 extern void register_console(struct console *);
 extern int unregister_console(struct console *);
 extern void console_lock(void);
diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index be40a9688403..d74e6e609f7d 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -247,9 +247,10 @@ int devkmsg_sysctl_set_loglvl(struct ctl_table *table, int write,
 void console_list_lock(void)
 {
 	/*
-	 * In unregister_console(), synchronize_srcu() is called with the
-	 * console_list_lock held. Therefore it is not allowed that the
-	 * console_list_lock is taken with the srcu_lock held.
+	 * In unregister_console() and console_force_preferred_locked(),
+	 * synchronize_srcu() is called with the console_list_lock held.
+	 * Therefore it is not allowed that the console_list_lock is taken
+	 * with the srcu_lock held.
 	 *
 	 * Whether or not this context is in the read-side critical section
 	 * can only be detected if the appropriate debug options are enabled.
@@ -3457,6 +3458,43 @@ int unregister_console(struct console *console)
 }
 EXPORT_SYMBOL(unregister_console);
 
+/**
+ * console_force_preferred_locked - force a registered console preferred
+ * @con: The registered console to force preferred.
+ *
+ * Must be called under console_list_lock().
+ */
+void console_force_preferred_locked(struct console *con)
+{
+	struct console *cur_pref_con;
+
+	if (!console_is_registered_locked(con))
+		return;
+
+	cur_pref_con = console_first();
+
+	/* Already preferred? */
+	if (cur_pref_con == con)
+		return;
+
+	hlist_del_init_rcu(&con->node);
+
+	/*
+	 * Ensure that all SRCU list walks have completed so that the console
+	 * can be added to the beginning of the console list and its forward
+	 * list pointer can be re-initialized.
+	 */
+	synchronize_srcu(&console_srcu);
+
+	con->flags |= CON_CONSDEV;
+	WARN_ON(!con->device);
+
+	/* Only the new head can have CON_CONSDEV set. */
+	WRITE_ONCE(cur_pref_con->flags, cur_pref_con->flags & ~CON_CONSDEV);
+	hlist_add_behind_rcu(&con->node, console_list.first);
+}
+EXPORT_SYMBOL(console_force_preferred_locked);
+
 /*
  * Initialize the console device. This is called *early*, so
  * we can't necessarily depend on lots of kernel help here.
-- 
2.30.2

[PATCH printk v3 33/40] printk, xen: fbfront: create/use safe function for forcing preferred

[John Ogness]

With commit 9e124fe16ff2("xen: Enable console tty by default in domU
if it's not a dummy") a hack was implemented to make sure that the
tty console remains the console behind the /dev/console device. The
main problem with the hack is that, after getting the console pointer
to the tty console, it is assumed the pointer is still valid after
releasing the console_sem. This assumption is incorrect and unsafe.

Make the hack safe by introducing a new function
console_force_preferred_locked() and perform the full operation
under the console_list_lock.

Signed-off-by: John Ogness <john.ogness@linutronix.de>
---
 drivers/video/fbdev/xen-fbfront.c | 12 +++------
 include/linux/console.h           |  1 +
 kernel/printk/printk.c            | 44 ++++++++++++++++++++++++++++---
 3 files changed, 46 insertions(+), 11 deletions(-)

diff --git a/drivers/video/fbdev/xen-fbfront.c b/drivers/video/fbdev/xen-fbfront.c
index 4d2694d904aa..8752d389e382 100644
--- a/drivers/video/fbdev/xen-fbfront.c
+++ b/drivers/video/fbdev/xen-fbfront.c
@@ -504,18 +504,14 @@ static void xenfb_make_preferred_console(void)
 	if (console_set_on_cmdline)
 		return;
 
-	console_lock();
+	console_list_lock();
 	for_each_console(c) {
 		if (!strcmp(c->name, "tty") && c->index == 0)
 			break;
 	}
-	console_unlock();
-	if (c) {
-		unregister_console(c);
-		c->flags |= CON_CONSDEV;
-		c->flags &= ~CON_PRINTBUFFER; /* don't print again */
-		register_console(c);
-	}
+	if (c)
+		console_force_preferred_locked(c);
+	console_list_unlock();
 }
 
 static int xenfb_resume(struct xenbus_device *dev)
diff --git a/include/linux/console.h b/include/linux/console.h
index cdae70e27377..b6b5d796d15c 100644
--- a/include/linux/console.h
+++ b/include/linux/console.h
@@ -273,6 +273,7 @@ enum con_flush_mode {
 };
 
 extern int add_preferred_console(char *name, int idx, char *options);
+extern void console_force_preferred_locked(struct console *con);
 extern void register_console(struct console *);
 extern int unregister_console(struct console *);
 extern void console_lock(void);
diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index be40a9688403..d74e6e609f7d 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -247,9 +247,10 @@ int devkmsg_sysctl_set_loglvl(struct ctl_table *table, int write,
 void console_list_lock(void)
 {
 	/*
-	 * In unregister_console(), synchronize_srcu() is called with the
-	 * console_list_lock held. Therefore it is not allowed that the
-	 * console_list_lock is taken with the srcu_lock held.
+	 * In unregister_console() and console_force_preferred_locked(),
+	 * synchronize_srcu() is called with the console_list_lock held.
+	 * Therefore it is not allowed that the console_list_lock is taken
+	 * with the srcu_lock held.
 	 *
 	 * Whether or not this context is in the read-side critical section
 	 * can only be detected if the appropriate debug options are enabled.
@@ -3457,6 +3458,43 @@ int unregister_console(struct console *console)
 }
 EXPORT_SYMBOL(unregister_console);
 
+/**
+ * console_force_preferred_locked - force a registered console preferred
+ * @con: The registered console to force preferred.
+ *
+ * Must be called under console_list_lock().
+ */
+void console_force_preferred_locked(struct console *con)
+{
+	struct console *cur_pref_con;
+
+	if (!console_is_registered_locked(con))
+		return;
+
+	cur_pref_con = console_first();
+
+	/* Already preferred? */
+	if (cur_pref_con == con)
+		return;
+
+	hlist_del_init_rcu(&con->node);
+
+	/*
+	 * Ensure that all SRCU list walks have completed so that the console
+	 * can be added to the beginning of the console list and its forward
+	 * list pointer can be re-initialized.
+	 */
+	synchronize_srcu(&console_srcu);
+
+	con->flags |= CON_CONSDEV;
+	WARN_ON(!con->device);
+
+	/* Only the new head can have CON_CONSDEV set. */
+	WRITE_ONCE(cur_pref_con->flags, cur_pref_con->flags & ~CON_CONSDEV);
+	hlist_add_behind_rcu(&con->node, console_list.first);
+}
+EXPORT_SYMBOL(console_force_preferred_locked);
+
 /*
  * Initialize the console device. This is called *early*, so
  * we can't necessarily depend on lots of kernel help here.
-- 
2.30.2

Re: [PATCH printk v3 33/40] printk, xen: fbfront: create/use safe function for forcing preferred

[Petr Mladek]

On Mon 2022-11-07 15:22:31, John Ogness wrote:
> With commit 9e124fe16ff2("xen: Enable console tty by default in domU
> if it's not a dummy") a hack was implemented to make sure that the
> tty console remains the console behind the /dev/console device. The
> main problem with the hack is that, after getting the console pointer
> to the tty console, it is assumed the pointer is still valid after
> releasing the console_sem. This assumption is incorrect and unsafe.
> 
> Make the hack safe by introducing a new function
> console_force_preferred_locked() and perform the full operation
> under the console_list_lock.
> 
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -3457,6 +3458,43 @@ int unregister_console(struct console *console)
>  }
>  EXPORT_SYMBOL(unregister_console);
>  
> +/**
> + * console_force_preferred_locked - force a registered console preferred
> + * @con: The registered console to force preferred.
> + *
> + * Must be called under console_list_lock().
> + */
> +void console_force_preferred_locked(struct console *con)
> +{
> +	struct console *cur_pref_con;
> +
> +	if (!console_is_registered_locked(con))
> +		return;
> +
> +	cur_pref_con = console_first();
> +
> +	/* Already preferred? */
> +	if (cur_pref_con == con)
> +		return;
> +
> +	hlist_del_init_rcu(&con->node);

We actually should re-initialize the node only after all existing
console list walks are finished. Se we should use here:

	hlist_del_rcu(&con->node);

> +
> +	/*
> +	 * Ensure that all SRCU list walks have completed so that the console
> +	 * can be added to the beginning of the console list and its forward
> +	 * list pointer can be re-initialized.

The comment is right ;-)

> +	 */
> +	synchronize_srcu(&console_srcu);
> +
> +	con->flags |= CON_CONSDEV;
> +	WARN_ON(!con->device);
> +
> +	/* Only the new head can have CON_CONSDEV set. */
> +	WRITE_ONCE(cur_pref_con->flags, cur_pref_con->flags & ~CON_CONSDEV);

As mentioned in the reply for 7th patch, I would prefer to hide this
WRITE_ONCE into a wrapper, e.g. console_set_flag(). It might also
check that the console_list_lock is taken...


> +	hlist_add_behind_rcu(&con->node, console_list.first);
> +}
> +EXPORT_SYMBOL(console_force_preferred_locked);
> +
>  /*
>   * Initialize the console device. This is called *early*, so
>   * we can't necessarily depend on lots of kernel help here.

Best Regards,
Petr

Re: [PATCH printk v3 33/40] printk, xen: fbfront: create/use safe function for forcing preferred

[Petr Mladek]

On Mon 2022-11-07 15:22:31, John Ogness wrote:
> With commit 9e124fe16ff2("xen: Enable console tty by default in domU
> if it's not a dummy") a hack was implemented to make sure that the
> tty console remains the console behind the /dev/console device. The
> main problem with the hack is that, after getting the console pointer
> to the tty console, it is assumed the pointer is still valid after
> releasing the console_sem. This assumption is incorrect and unsafe.
> 
> Make the hack safe by introducing a new function
> console_force_preferred_locked() and perform the full operation
> under the console_list_lock.
> 
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -3457,6 +3458,43 @@ int unregister_console(struct console *console)
>  }
>  EXPORT_SYMBOL(unregister_console);
>  
> +/**
> + * console_force_preferred_locked - force a registered console preferred
> + * @con: The registered console to force preferred.
> + *
> + * Must be called under console_list_lock().
> + */
> +void console_force_preferred_locked(struct console *con)
> +{
> +	struct console *cur_pref_con;
> +
> +	if (!console_is_registered_locked(con))
> +		return;
> +
> +	cur_pref_con = console_first();
> +
> +	/* Already preferred? */
> +	if (cur_pref_con == con)
> +		return;
> +
> +	hlist_del_init_rcu(&con->node);

We actually should re-initialize the node only after all existing
console list walks are finished. Se we should use here:

	hlist_del_rcu(&con->node);

> +
> +	/*
> +	 * Ensure that all SRCU list walks have completed so that the console
> +	 * can be added to the beginning of the console list and its forward
> +	 * list pointer can be re-initialized.

The comment is right ;-)

> +	 */
> +	synchronize_srcu(&console_srcu);
> +
> +	con->flags |= CON_CONSDEV;
> +	WARN_ON(!con->device);
> +
> +	/* Only the new head can have CON_CONSDEV set. */
> +	WRITE_ONCE(cur_pref_con->flags, cur_pref_con->flags & ~CON_CONSDEV);

As mentioned in the reply for 7th patch, I would prefer to hide this
WRITE_ONCE into a wrapper, e.g. console_set_flag(). It might also
check that the console_list_lock is taken...


> +	hlist_add_behind_rcu(&con->node, console_list.first);
> +}
> +EXPORT_SYMBOL(console_force_preferred_locked);
> +
>  /*
>   * Initialize the console device. This is called *early*, so
>   * we can't necessarily depend on lots of kernel help here.

Best Regards,
Petr

Re: [PATCH printk v3 33/40] printk, xen: fbfront: create/use safe function for forcing preferred

[John Ogness]

On 2022-11-10, Petr Mladek <pmladek@suse.com> wrote:
>> +void console_force_preferred_locked(struct console *con)
>> +{
>> +	struct console *cur_pref_con;
>> +
>> +	if (!console_is_registered_locked(con))
>> +		return;
>> +
>> +	cur_pref_con = console_first();
>> +
>> +	/* Already preferred? */
>> +	if (cur_pref_con == con)
>> +		return;
>> +
>> +	hlist_del_init_rcu(&con->node);
>
> We actually should re-initialize the node only after all existing
> console list walks are finished. Se we should use here:
>
> 	hlist_del_rcu(&con->node);

hlist_del_init_rcu() only re-initializes @pprev pointer. But maybe you
are concerned that there is a window where list_unhashed() becomes true?
I agree that it should be changed to hlist_del_rcu() because there
should not be a window where this console appears unregistered.

>> +	/* Only the new head can have CON_CONSDEV set. */
>> +	WRITE_ONCE(cur_pref_con->flags, cur_pref_con->flags & ~CON_CONSDEV);
>
> As mentioned in the reply for 7th patch, I would prefer to hide this
> WRITE_ONCE into a wrapper, e.g. console_set_flag(). It might also
> check that the console_list_lock is taken...

Agreed. For v4 it will become:

console_srcu_write_flags(cur_pref_con->flags & ~CON_CONSDEV);

John

Re: [PATCH printk v3 33/40] printk, xen: fbfront: create/use safe function for forcing preferred

[John Ogness]

On 2022-11-10, Petr Mladek <pmladek@suse.com> wrote:
>> +void console_force_preferred_locked(struct console *con)
>> +{
>> +	struct console *cur_pref_con;
>> +
>> +	if (!console_is_registered_locked(con))
>> +		return;
>> +
>> +	cur_pref_con = console_first();
>> +
>> +	/* Already preferred? */
>> +	if (cur_pref_con == con)
>> +		return;
>> +
>> +	hlist_del_init_rcu(&con->node);
>
> We actually should re-initialize the node only after all existing
> console list walks are finished. Se we should use here:
>
> 	hlist_del_rcu(&con->node);

hlist_del_init_rcu() only re-initializes @pprev pointer. But maybe you
are concerned that there is a window where list_unhashed() becomes true?
I agree that it should be changed to hlist_del_rcu() because there
should not be a window where this console appears unregistered.

>> +	/* Only the new head can have CON_CONSDEV set. */
>> +	WRITE_ONCE(cur_pref_con->flags, cur_pref_con->flags & ~CON_CONSDEV);
>
> As mentioned in the reply for 7th patch, I would prefer to hide this
> WRITE_ONCE into a wrapper, e.g. console_set_flag(). It might also
> check that the console_list_lock is taken...

Agreed. For v4 it will become:

console_srcu_write_flags(cur_pref_con->flags & ~CON_CONSDEV);

John

Re: [PATCH printk v3 33/40] printk, xen: fbfront: create/use safe function for forcing preferred

[Petr Mladek]

On Thu 2022-11-10 17:09:12, John Ogness wrote:
> On 2022-11-10, Petr Mladek <pmladek@suse.com> wrote:
> >> +void console_force_preferred_locked(struct console *con)
> >> +{
> >> +	struct console *cur_pref_con;
> >> +
> >> +	if (!console_is_registered_locked(con))
> >> +		return;
> >> +
> >> +	cur_pref_con = console_first();
> >> +
> >> +	/* Already preferred? */
> >> +	if (cur_pref_con == con)
> >> +		return;
> >> +
> >> +	hlist_del_init_rcu(&con->node);
> >
> > We actually should re-initialize the node only after all existing
> > console list walks are finished. Se we should use here:
> >
> > 	hlist_del_rcu(&con->node);
> 
> hlist_del_init_rcu() only re-initializes @pprev pointer.

Ah, I was not aware of it.

> But maybe you
> are concerned that there is a window where list_unhashed() becomes true?
> I agree that it should be changed to hlist_del_rcu() because there
> should not be a window where this console appears unregistered.

Makes sense.

> >> +	/* Only the new head can have CON_CONSDEV set. */
> >> +	WRITE_ONCE(cur_pref_con->flags, cur_pref_con->flags & ~CON_CONSDEV);
> >
> > As mentioned in the reply for 7th patch, I would prefer to hide this
> > WRITE_ONCE into a wrapper, e.g. console_set_flag(). It might also
> > check that the console_list_lock is taken...
> 
> Agreed. For v4 it will become:
> 
> console_srcu_write_flags(cur_pref_con->flags & ~CON_CONSDEV);

I am happy that your are going to introduce an API for this.

Just to be sure. The _srcu_ in the name means that the write
will use WRITE_ONCE() so that it can be read safely in SRCU
context using READ_ONCE(). Do I get it correctly, please?

I expect that the counter part will be console_srcu_read_flags().
I like the name. It is better than _unsafe_ that I proposed earlier.

Best Regards,
Petr

Re: [PATCH printk v3 33/40] printk, xen: fbfront: create/use safe function for forcing preferred

[Petr Mladek]

On Thu 2022-11-10 17:09:12, John Ogness wrote:
> On 2022-11-10, Petr Mladek <pmladek@suse.com> wrote:
> >> +void console_force_preferred_locked(struct console *con)
> >> +{
> >> +	struct console *cur_pref_con;
> >> +
> >> +	if (!console_is_registered_locked(con))
> >> +		return;
> >> +
> >> +	cur_pref_con = console_first();
> >> +
> >> +	/* Already preferred? */
> >> +	if (cur_pref_con == con)
> >> +		return;
> >> +
> >> +	hlist_del_init_rcu(&con->node);
> >
> > We actually should re-initialize the node only after all existing
> > console list walks are finished. Se we should use here:
> >
> > 	hlist_del_rcu(&con->node);
> 
> hlist_del_init_rcu() only re-initializes @pprev pointer.

Ah, I was not aware of it.

> But maybe you
> are concerned that there is a window where list_unhashed() becomes true?
> I agree that it should be changed to hlist_del_rcu() because there
> should not be a window where this console appears unregistered.

Makes sense.

> >> +	/* Only the new head can have CON_CONSDEV set. */
> >> +	WRITE_ONCE(cur_pref_con->flags, cur_pref_con->flags & ~CON_CONSDEV);
> >
> > As mentioned in the reply for 7th patch, I would prefer to hide this
> > WRITE_ONCE into a wrapper, e.g. console_set_flag(). It might also
> > check that the console_list_lock is taken...
> 
> Agreed. For v4 it will become:
> 
> console_srcu_write_flags(cur_pref_con->flags & ~CON_CONSDEV);

I am happy that your are going to introduce an API for this.

Just to be sure. The _srcu_ in the name means that the write
will use WRITE_ONCE() so that it can be read safely in SRCU
context using READ_ONCE(). Do I get it correctly, please?

I expect that the counter part will be console_srcu_read_flags().
I like the name. It is better than _unsafe_ that I proposed earlier.

Best Regards,
Petr

Re: [PATCH printk v3 33/40] printk, xen: fbfront: create/use safe function for forcing preferred

[John Ogness]

On 2022-11-10, Petr Mladek <pmladek@suse.com> wrote:
>>>> +	/* Only the new head can have CON_CONSDEV set. */
>>>> +	WRITE_ONCE(cur_pref_con->flags, cur_pref_con->flags & ~CON_CONSDEV);
>>>
>>> As mentioned in the reply for 7th patch, I would prefer to hide this
>>> WRITE_ONCE into a wrapper, e.g. console_set_flag(). It might also
>>> check that the console_list_lock is taken...
>> 
>> Agreed. For v4 it will become:
>> 
>> console_srcu_write_flags(cur_pref_con, cur_pref_con->flags & ~CON_CONSDEV);
>
> I am happy that your are going to introduce an API for this.
>
> Just to be sure. The _srcu_ in the name means that the write
> will use WRITE_ONCE() so that it can be read safely in SRCU
> context using READ_ONCE(). Do I get it correctly, please?

Yes.

> I expect that the counter part will be console_srcu_read_flags().
> I like the name. It is better than _unsafe_ that I proposed earlier.

Since the only flag that is ever checked in this way is CON_ENABLED, I
was planning on calling it console_srcu_is_enabled(). But I suppose I
could have it be: (console_srcu_read_flags() & CON_ENABLED). That would
keep it more abstract in case anyone should want to read other flag bits
under SRCU.

There are only 4 call sites that need this, so I suppose we don't need a
special _is_enabled() variant. Thanks for the suggestion!

John

Re: [PATCH printk v3 33/40] printk, xen: fbfront: create/use safe function for forcing preferred

[John Ogness]

On 2022-11-10, Petr Mladek <pmladek@suse.com> wrote:
>>>> +	/* Only the new head can have CON_CONSDEV set. */
>>>> +	WRITE_ONCE(cur_pref_con->flags, cur_pref_con->flags & ~CON_CONSDEV);
>>>
>>> As mentioned in the reply for 7th patch, I would prefer to hide this
>>> WRITE_ONCE into a wrapper, e.g. console_set_flag(). It might also
>>> check that the console_list_lock is taken...
>> 
>> Agreed. For v4 it will become:
>> 
>> console_srcu_write_flags(cur_pref_con, cur_pref_con->flags & ~CON_CONSDEV);
>
> I am happy that your are going to introduce an API for this.
>
> Just to be sure. The _srcu_ in the name means that the write
> will use WRITE_ONCE() so that it can be read safely in SRCU
> context using READ_ONCE(). Do I get it correctly, please?

Yes.

> I expect that the counter part will be console_srcu_read_flags().
> I like the name. It is better than _unsafe_ that I proposed earlier.

Since the only flag that is ever checked in this way is CON_ENABLED, I
was planning on calling it console_srcu_is_enabled(). But I suppose I
could have it be: (console_srcu_read_flags() & CON_ENABLED). That would
keep it more abstract in case anyone should want to read other flag bits
under SRCU.

There are only 4 call sites that need this, so I suppose we don't need a
special _is_enabled() variant. Thanks for the suggestion!

John

Re: [PATCH printk v3 33/40] printk, xen: fbfront: create/use safe function for forcing preferred

[Petr Mladek]

On Mon 2022-11-07 15:22:31, John Ogness wrote:
> With commit 9e124fe16ff2("xen: Enable console tty by default in domU
> if it's not a dummy") a hack was implemented to make sure that the
> tty console remains the console behind the /dev/console device. The
> main problem with the hack is that, after getting the console pointer
> to the tty console, it is assumed the pointer is still valid after
> releasing the console_sem. This assumption is incorrect and unsafe.
> 
> Make the hack safe by introducing a new function
> console_force_preferred_locked() and perform the full operation
> under the console_list_lock.
> 
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -3457,6 +3458,43 @@ int unregister_console(struct console *console)
>  }
>  EXPORT_SYMBOL(unregister_console);
>  
> +/**
> + * console_force_preferred_locked - force a registered console preferred
> + * @con: The registered console to force preferred.
> + *
> + * Must be called under console_list_lock().
> + */
> +void console_force_preferred_locked(struct console *con)
> +{
> +	struct console *cur_pref_con;

One more thing. It would make sense to check that it has
really been called under console_list_lock():

	lockdep_assert_console_list_lock_held();

> +
> +	if (!console_is_registered_locked(con))
> +		return;
> +
> +	cur_pref_con = console_first();
> +
> +	/* Already preferred? */
> +	if (cur_pref_con == con)
> +		return;
> +

Best Regards,
Petr

Re: [PATCH printk v3 33/40] printk, xen: fbfront: create/use safe function for forcing preferred

[Petr Mladek]

On Mon 2022-11-07 15:22:31, John Ogness wrote:
> With commit 9e124fe16ff2("xen: Enable console tty by default in domU
> if it's not a dummy") a hack was implemented to make sure that the
> tty console remains the console behind the /dev/console device. The
> main problem with the hack is that, after getting the console pointer
> to the tty console, it is assumed the pointer is still valid after
> releasing the console_sem. This assumption is incorrect and unsafe.
> 
> Make the hack safe by introducing a new function
> console_force_preferred_locked() and perform the full operation
> under the console_list_lock.
> 
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -3457,6 +3458,43 @@ int unregister_console(struct console *console)
>  }
>  EXPORT_SYMBOL(unregister_console);
>  
> +/**
> + * console_force_preferred_locked - force a registered console preferred
> + * @con: The registered console to force preferred.
> + *
> + * Must be called under console_list_lock().
> + */
> +void console_force_preferred_locked(struct console *con)
> +{
> +	struct console *cur_pref_con;

One more thing. It would make sense to check that it has
really been called under console_list_lock():

	lockdep_assert_console_list_lock_held();

> +
> +	if (!console_is_registered_locked(con))
> +		return;
> +
> +	cur_pref_con = console_first();
> +
> +	/* Already preferred? */
> +	if (cur_pref_con == con)
> +		return;
> +

Best Regards,
Petr

[PATCH printk v3 34/40] tty: tty_io: use console_list_lock for list synchronization

[John Ogness]

show_cons_active() uses the console_lock to gather information
on registered consoles. It requires that no consoles are unregistered
until it is finished. The console_list_lock should be used because
list synchronization responsibility will be removed from the
console_lock in a later change.

Note, the console_lock is still needed to serialize the device()
callback with other console operations.

Signed-off-by: John Ogness <john.ogness@linutronix.de>
Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Reviewed-by: Petr Mladek <pmladek@suse.com>
---
 drivers/tty/tty_io.c | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index ee4da2fec328..cafdff575716 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -3527,16 +3527,13 @@ static ssize_t show_cons_active(struct device *dev,
 	ssize_t count = 0;
 
 	/*
-	 * Hold the console_lock to guarantee that no consoles are
+	 * Hold the console_list_lock to guarantee that no consoles are
 	 * unregistered until all console processing is complete.
 	 * This also allows safe traversal of the console list and
 	 * race-free reading of @flags.
-	 *
-	 * Take console_lock to serialize device() callback with
-	 * other console operations. For example, fg_console is
-	 * modified under console_lock when switching vt.
 	 */
-	console_lock();
+	console_list_lock();
+
 	for_each_console(c) {
 		if (!c->device)
 			continue;
@@ -3548,6 +3545,13 @@ static ssize_t show_cons_active(struct device *dev,
 		if (i >= ARRAY_SIZE(cs))
 			break;
 	}
+
+	/*
+	 * Take console_lock to serialize device() callback with
+	 * other console operations. For example, fg_console is
+	 * modified under console_lock when switching vt.
+	 */
+	console_lock();
 	while (i--) {
 		int index = cs[i]->index;
 		struct tty_driver *drv = cs[i]->device(cs[i], &index);
@@ -3563,6 +3567,8 @@ static ssize_t show_cons_active(struct device *dev,
 	}
 	console_unlock();
 
+	console_list_unlock();
+
 	return count;
 }
 static DEVICE_ATTR(active, S_IRUGO, show_cons_active, NULL);
-- 
2.30.2

[PATCH printk v3 35/40] proc: consoles: use console_list_lock for list iteration

[John Ogness]

The console_lock is used in part to guarantee safe list iteration.
The console_list_lock should be used because list synchronization
responsibility will be removed from the console_lock in a later
change.

Note, the console_lock is still needed to serialize the device()
callback with other console operations.

Signed-off-by: John Ogness <john.ogness@linutronix.de>
Reviewed-by: Petr Mladek <pmladek@suse.com>
---
 fs/proc/consoles.c | 19 ++++++++++++-------
 1 file changed, 12 insertions(+), 7 deletions(-)

diff --git a/fs/proc/consoles.c b/fs/proc/consoles.c
index 46b305fa04ed..e0758fe7936d 100644
--- a/fs/proc/consoles.c
+++ b/fs/proc/consoles.c
@@ -33,7 +33,16 @@ static int show_console_dev(struct seq_file *m, void *v)
 	if (con->device) {
 		const struct tty_driver *driver;
 		int index;
+
+		/*
+		 * Take console_lock to serialize device() callback with
+		 * other console operations. For example, fg_console is
+		 * modified under console_lock when switching vt.
+		 */
+		console_lock();
 		driver = con->device(con, &index);
+		console_unlock();
+
 		if (driver) {
 			dev = MKDEV(driver->major, driver->minor_start);
 			dev += index;
@@ -64,15 +73,11 @@ static void *c_start(struct seq_file *m, loff_t *pos)
 	loff_t off = 0;
 
 	/*
-	 * Take console_lock to serialize device() callback with
-	 * other console operations. For example, fg_console is
-	 * modified under console_lock when switching vt.
-	 *
-	 * Hold the console_lock to guarantee safe traversal of the
+	 * Hold the console_list_lock to guarantee safe traversal of the
 	 * console list. SRCU cannot be used because there is no
 	 * place to store the SRCU cookie.
 	 */
-	console_lock();
+	console_list_lock();
 	for_each_console(con)
 		if (off++ == *pos)
 			break;
@@ -90,7 +95,7 @@ static void *c_next(struct seq_file *m, void *v, loff_t *pos)
 
 static void c_stop(struct seq_file *m, void *v)
 {
-	console_unlock();
+	console_list_unlock();
 }
 
 static const struct seq_operations consoles_op = {
-- 
2.30.2

[PATCH printk v3 36/40] tty: serial: kgdboc: use console_list_lock for list traversal

[John Ogness]

configure_kgdboc() uses the console_lock for console list iteration. Use
the console_list_lock instead because list synchronization responsibility
will be removed from the console_lock in a later change.

The SRCU iterator could have been used here, but a later change will
relocate the locking of the console_list_lock to also provide
synchronization against register_console().

Note, the console_lock is still needed to serialize the device()
callback with other console operations.

Signed-off-by: John Ogness <john.ogness@linutronix.de>
---
 drivers/tty/serial/kgdboc.c | 22 ++++++++++++++++++++--
 1 file changed, 20 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/serial/kgdboc.c b/drivers/tty/serial/kgdboc.c
index 5be381003e58..82b4b4d67823 100644
--- a/drivers/tty/serial/kgdboc.c
+++ b/drivers/tty/serial/kgdboc.c
@@ -193,7 +193,16 @@ static int configure_kgdboc(void)
 	if (!p)
 		goto noconfig;
 
+	/* For safe traversal of the console list. */
+	console_list_lock();
+
+	/*
+	 * Take console_lock to serialize device() callback with
+	 * other console operations. For example, fg_console is
+	 * modified under console_lock when switching vt.
+	 */
 	console_lock();
+
 	for_each_console(cons) {
 		int idx;
 		if (cons->device && cons->device(cons, &idx) == p &&
@@ -202,8 +211,11 @@ static int configure_kgdboc(void)
 			break;
 		}
 	}
+
 	console_unlock();
 
+	console_list_unlock();
+
 	kgdb_tty_driver = p;
 	kgdb_tty_line = tty_line;
 
@@ -451,6 +463,7 @@ static void kgdboc_earlycon_pre_exp_handler(void)
 {
 	struct console *con;
 	static bool already_warned;
+	int cookie;
 
 	if (already_warned)
 		return;
@@ -463,9 +476,14 @@ static void kgdboc_earlycon_pre_exp_handler(void)
 	 * serial drivers might be OK with this, print a warning once per
 	 * boot if we detect this case.
 	 */
-	for_each_console(con)
+	cookie = console_srcu_read_lock();
+	for_each_console_srcu(con) {
 		if (con == kgdboc_earlycon_io_ops.cons)
-			return;
+			break;
+	}
+	console_srcu_read_unlock(cookie);
+	if (con)
+		return;
 
 	already_warned = true;
 	pr_warn("kgdboc_earlycon is still using bootconsole\n");
-- 
2.30.2

Re: [PATCH printk v3 36/40] tty: serial: kgdboc: use console_list_lock for list traversal

[Daniel Thompson]

On Mon, Nov 07, 2022 at 03:22:34PM +0106, John Ogness wrote:
> configure_kgdboc() uses the console_lock for console list iteration. Use
> the console_list_lock instead because list synchronization responsibility
> will be removed from the console_lock in a later change.
>
> The SRCU iterator could have been used here, but a later change will
> relocate the locking of the console_list_lock to also provide
> synchronization against register_console().
>
> Note, the console_lock is still needed to serialize the device()
> callback with other console operations.
>
> Signed-off-by: John Ogness <john.ogness@linutronix.de>
> ---
>  drivers/tty/serial/kgdboc.c | 22 ++++++++++++++++++++--
>  1 file changed, 20 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/tty/serial/kgdboc.c b/drivers/tty/serial/kgdboc.c
> index 5be381003e58..82b4b4d67823 100644
> --- a/drivers/tty/serial/kgdboc.c
> +++ b/drivers/tty/serial/kgdboc.c
> @@ -451,6 +463,7 @@ static void kgdboc_earlycon_pre_exp_handler(void)
>  {
>  	struct console *con;
>  	static bool already_warned;
> +	int cookie;
>
>  	if (already_warned)
>  		return;
> @@ -463,9 +476,14 @@ static void kgdboc_earlycon_pre_exp_handler(void)
>  	 * serial drivers might be OK with this, print a warning once per
>  	 * boot if we detect this case.
>  	 */
> -	for_each_console(con)
> +	cookie = console_srcu_read_lock();
> +	for_each_console_srcu(con) {
>  		if (con == kgdboc_earlycon_io_ops.cons)
> -			return;
> +			break;
> +	}
> +	console_srcu_read_unlock(cookie);
> +	if (con)
> +		return;

This change isn't mentioned in the patch description.


Daniel.

Re: [PATCH printk v3 36/40] tty: serial: kgdboc: use console_list_lock for list traversal

[John Ogness]

On 2022-11-09, Daniel Thompson <daniel.thompson@linaro.org> wrote:
>> @@ -463,9 +476,14 @@ static void kgdboc_earlycon_pre_exp_handler(void)
>>  	 * serial drivers might be OK with this, print a warning once per
>>  	 * boot if we detect this case.
>>  	 */
>> -	for_each_console(con)
>> +	cookie = console_srcu_read_lock();
>> +	for_each_console_srcu(con) {
>>  		if (con == kgdboc_earlycon_io_ops.cons)
>> -			return;
>> +			break;
>> +	}
>> +	console_srcu_read_unlock(cookie);
>> +	if (con)
>> +		return;
>
> This change isn't mentioned in the patch description.

I will move this change into its own separate patch.

    tty: serial: kgdboc: use srcu console list iterator

    Use srcu console list iteration for safe console list traversal.

Thanks.

John

Re: [PATCH printk v3 36/40] tty: serial: kgdboc: use console_list_lock for list traversal

[Petr Mladek]

On Wed 2022-11-09 10:50:43, John Ogness wrote:
> On 2022-11-09, Daniel Thompson <daniel.thompson@linaro.org> wrote:
> >> @@ -463,9 +476,14 @@ static void kgdboc_earlycon_pre_exp_handler(void)
> >>  	 * serial drivers might be OK with this, print a warning once per
> >>  	 * boot if we detect this case.
> >>  	 */
> >> -	for_each_console(con)
> >> +	cookie = console_srcu_read_lock();
> >> +	for_each_console_srcu(con) {
> >>  		if (con == kgdboc_earlycon_io_ops.cons)
> >> -			return;
> >> +			break;
> >> +	}
> >> +	console_srcu_read_unlock(cookie);
> >> +	if (con)
> >> +		return;
> >
> > This change isn't mentioned in the patch description.
> 
> I will move this change into its own separate patch.
> 
>     tty: serial: kgdboc: use srcu console list iterator
> 
>     Use srcu console list iteration for safe console list traversal.

Yes, split it please :-) Anyway, both changes look good to me.

Best Regards,
Petr

[PATCH printk v3 37/40] tty: serial: kgdboc: synchronize tty_find_polling_driver() and register_console()

[John Ogness]

Calling tty_find_polling_driver() can lead to uart_set_options() being
called (via the poll_init() callback of tty_operations) to configure the
uart. But uart_set_options() can also be called by register_console()
(via the setup() callback of console).

Take the console_list_lock to synchronize against register_console() and
also use it for console list traversal. This also ensures the console list
cannot change until the polling console has been chosen.

Signed-off-by: John Ogness <john.ogness@linutronix.de>
---
 drivers/tty/serial/kgdboc.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/drivers/tty/serial/kgdboc.c b/drivers/tty/serial/kgdboc.c
index 82b4b4d67823..8c2b7ccdfebf 100644
--- a/drivers/tty/serial/kgdboc.c
+++ b/drivers/tty/serial/kgdboc.c
@@ -189,12 +189,20 @@ static int configure_kgdboc(void)
 	if (kgdboc_register_kbd(&cptr))
 		goto do_register;
 
+	/*
+	 * tty_find_polling_driver() can call uart_set_options()
+	 * (via poll_init) to configure the uart. Take the console_list_lock
+	 * in order to synchronize against register_console(), which can also
+	 * configure the uart via uart_set_options(). This also allows safe
+	 * traversal of the console list.
+	 */
+	console_list_lock();
+
 	p = tty_find_polling_driver(cptr, &tty_line);
-	if (!p)
+	if (!p) {
+		console_list_unlock();
 		goto noconfig;
-
-	/* For safe traversal of the console list. */
-	console_list_lock();
+	}
 
 	/*
 	 * Take console_lock to serialize device() callback with
-- 
2.30.2

Re: [PATCH printk v3 37/40] tty: serial: kgdboc: synchronize tty_find_polling_driver() and register_console()

[Daniel Thompson]

On Mon, Nov 07, 2022 at 03:22:35PM +0106, John Ogness wrote:
> Calling tty_find_polling_driver() can lead to uart_set_options() being
> called (via the poll_init() callback of tty_operations) to configure the
> uart. But uart_set_options() can also be called by register_console()
> (via the setup() callback of console).
>
> Take the console_list_lock to synchronize against register_console() and
> also use it for console list traversal. This also ensures the console list
> cannot change until the polling console has been chosen.
>
> Signed-off-by: John Ogness <john.ogness@linutronix.de>

Reviewed-by: Daniel Thompson <daniel.thompson@linaro.org>


Daniel.

Re: [PATCH printk v3 37/40] tty: serial: kgdboc: synchronize tty_find_polling_driver() and register_console()

[Petr Mladek]

On Mon 2022-11-07 15:22:35, John Ogness wrote:
> Calling tty_find_polling_driver() can lead to uart_set_options() being
> called (via the poll_init() callback of tty_operations) to configure the
> uart. But uart_set_options() can also be called by register_console()
> (via the setup() callback of console).
> 
> Take the console_list_lock to synchronize against register_console() and
> also use it for console list traversal. This also ensures the console list
> cannot change until the polling console has been chosen.
> 
> Signed-off-by: John Ogness <john.ogness@linutronix.de>

Huh, this is a maze of related calls. At least for me. But the change
seems to be correct.

Reviewed-by: Petr Mladek <pmladek@suse.com>

Best Regards,
Petr

[PATCH printk v3 38/40] tty: serial: kgdboc: use console_list_lock to trap exit

[John Ogness]

kgdboc_earlycon_init() uses the console_lock to ensure that no consoles
are unregistered until the kgdboc_earlycon is setup. The console_list_lock
should be used instead because list synchronization responsibility will
be removed from the console_lock in a later change.

Signed-off-by: John Ogness <john.ogness@linutronix.de>
---
 drivers/tty/serial/kgdboc.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/tty/serial/kgdboc.c b/drivers/tty/serial/kgdboc.c
index 8c2b7ccdfebf..a3ed9b34e2ab 100644
--- a/drivers/tty/serial/kgdboc.c
+++ b/drivers/tty/serial/kgdboc.c
@@ -558,13 +558,13 @@ static int __init kgdboc_earlycon_init(char *opt)
 	 */
 
 	/*
-	 * Hold the console_lock to guarantee that no consoles are
+	 * Hold the console_list_lock to guarantee that no consoles are
 	 * unregistered until the kgdboc_earlycon setup is complete.
 	 * Trapping the exit() callback relies on exit() not being
 	 * called until the trap is setup. This also allows safe
 	 * traversal of the console list and race-free reading of @flags.
 	 */
-	console_lock();
+	console_list_lock();
 	for_each_console(con) {
 		if (con->write && con->read &&
 		    (con->flags & (CON_BOOT | CON_ENABLED)) &&
@@ -606,7 +606,7 @@ static int __init kgdboc_earlycon_init(char *opt)
 	}
 
 unlock:
-	console_unlock();
+	console_list_unlock();
 
 	/* Non-zero means malformed option so we always return zero */
 	return 0;
-- 
2.30.2

Re: [PATCH printk v3 38/40] tty: serial: kgdboc: use console_list_lock to trap exit

[Daniel Thompson]

On Mon, Nov 07, 2022 at 03:22:36PM +0106, John Ogness wrote:
> kgdboc_earlycon_init() uses the console_lock to ensure that no consoles
> are unregistered until the kgdboc_earlycon is setup. The console_list_lock
> should be used instead because list synchronization responsibility will
> be removed from the console_lock in a later change.
> 
> Signed-off-by: John Ogness <john.ogness@linutronix.de>

I've not looked at the other patches in the series to understand the
future tense here (e.g. why we need intermediate patches like this one).

However I've no objections to the change so:
Reviewed-by: Daniel Thompson <daniel.thompson@linaro.org>


Daniel.


> ---
>  drivers/tty/serial/kgdboc.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/tty/serial/kgdboc.c b/drivers/tty/serial/kgdboc.c
> index 8c2b7ccdfebf..a3ed9b34e2ab 100644
> --- a/drivers/tty/serial/kgdboc.c
> +++ b/drivers/tty/serial/kgdboc.c
> @@ -558,13 +558,13 @@ static int __init kgdboc_earlycon_init(char *opt)
>  	 */
>  
>  	/*
> -	 * Hold the console_lock to guarantee that no consoles are
> +	 * Hold the console_list_lock to guarantee that no consoles are
>  	 * unregistered until the kgdboc_earlycon setup is complete.
>  	 * Trapping the exit() callback relies on exit() not being
>  	 * called until the trap is setup. This also allows safe
>  	 * traversal of the console list and race-free reading of @flags.
>  	 */
> -	console_lock();
> +	console_list_lock();
>  	for_each_console(con) {
>  		if (con->write && con->read &&
>  		    (con->flags & (CON_BOOT | CON_ENABLED)) &&
> @@ -606,7 +606,7 @@ static int __init kgdboc_earlycon_init(char *opt)
>  	}
>  
>  unlock:
> -	console_unlock();
> +	console_list_unlock();
>  
>  	/* Non-zero means malformed option so we always return zero */
>  	return 0;
> -- 
> 2.30.2
> 

Re: [PATCH printk v3 38/40] tty: serial: kgdboc: use console_list_lock to trap exit

[Petr Mladek]

On Mon 2022-11-07 15:22:36, John Ogness wrote:
> kgdboc_earlycon_init() uses the console_lock to ensure that no consoles
> are unregistered until the kgdboc_earlycon is setup. The console_list_lock
> should be used instead because list synchronization responsibility will
> be removed from the console_lock in a later change.
> 
> Signed-off-by: John Ogness <john.ogness@linutronix.de>

Reviewed-by: Petr Mladek <pmladek@suse.com>

Best Regards,
Petr

[PATCH printk v3 39/40] printk: relieve console_lock of list synchronization duties

[John Ogness]

The console_list_lock provides synchronization for console list and
console->flags updates. All call sites that were using the console_lock
for this synchronization have either switched to use the
console_list_lock or the SRCU list iterator.

Remove console_lock usage for console list updates and console->flags
updates.

Signed-off-by: John Ogness <john.ogness@linutronix.de>
---
 kernel/printk/printk.c | 36 ++++++++++++------------------------
 1 file changed, 12 insertions(+), 24 deletions(-)

diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index d74e6e609f7d..17765166ac42 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -86,8 +86,8 @@ EXPORT_SYMBOL(oops_in_progress);
 static DEFINE_MUTEX(console_mutex);
 
 /*
- * console_sem protects console_list and console->flags updates, and also
- * provides serialization for access to the entire console driver system.
+ * console_sem protects updates to console->seq and console_suspended,
+ * and also provides serialization for console printing.
  */
 static DEFINE_SEMAPHORE(console_sem);
 HLIST_HEAD(console_list);
@@ -2638,10 +2638,10 @@ static int console_cpu_notify(unsigned int cpu)
 }
 
 /**
- * console_lock - lock the console system for exclusive use.
+ * console_lock - block the console subsystem from printing
  *
- * Acquires a lock which guarantees that the caller has
- * exclusive access to the console system and console_list.
+ * Acquires a lock which guarantees that no consoles will
+ * be in or enter their write() callback.
  *
  * Can sleep, returns nothing.
  */
@@ -2658,10 +2658,10 @@ void console_lock(void)
 EXPORT_SYMBOL(console_lock);
 
 /**
- * console_trylock - try to lock the console system for exclusive use.
+ * console_trylock - try to block the console subsystem from printing
  *
- * Try to acquire a lock which guarantees that the caller has exclusive
- * access to the console system and console_list.
+ * Try to acquire a lock which guarantees that no consoles will
+ * be in or enter their write() callback.
  *
  * returns 1 on success, and 0 on failure to acquire the lock.
  */
@@ -2917,10 +2917,10 @@ static bool console_flush_all(bool do_cond_resched, u64 *next_seq, bool *handove
 }
 
 /**
- * console_unlock - unlock the console system
+ * console_unlock - unblock the console subsystem from printing
  *
- * Releases the console_lock which the caller holds on the console system
- * and the console driver list.
+ * Releases the console_lock which the caller holds to block printing of
+ * the console subsystem.
  *
  * While the console_lock was held, console output may have been buffered
  * by printk().  If this is the case, console_unlock(); emits
@@ -3107,9 +3107,7 @@ void console_stop(struct console *console)
 {
 	__pr_flush(console, 1000, true);
 	console_list_lock();
-	console_lock();
 	WRITE_ONCE(console->flags, console->flags & ~CON_ENABLED);
-	console_unlock();
 	console_list_unlock();
 
 	/*
@@ -3125,9 +3123,7 @@ EXPORT_SYMBOL(console_stop);
 void console_start(struct console *console)
 {
 	console_list_lock();
-	console_lock();
 	WRITE_ONCE(console->flags, console->flags | CON_ENABLED);
-	console_unlock();
 	console_list_unlock();
 	__pr_flush(console, 1000, true);
 }
@@ -3344,7 +3340,6 @@ void register_console(struct console *newcon)
 	 * Put this console in the list - keep the
 	 * preferred driver at the head of the list.
 	 */
-	console_lock();
 	if (hlist_empty(&console_list)) {
 		/* Ensure CON_CONSDEV is always set for the head. */
 		newcon->flags |= CON_CONSDEV;
@@ -3358,7 +3353,6 @@ void register_console(struct console *newcon)
 	} else {
 		hlist_add_behind_rcu(&newcon->node, console_list.first);
 	}
-	console_unlock();
 
 	/*
 	 * No need to synchronize SRCU here! The caller does not rely
@@ -3406,15 +3400,11 @@ static int unregister_console_locked(struct console *console)
 	if (res > 0)
 		return 0;
 
-	console_lock();
-
 	/* Disable it unconditionally */
 	WRITE_ONCE(console->flags, console->flags & ~CON_ENABLED);
 
-	if (!console_is_registered_locked(console)) {
-		console_unlock();
+	if (!console_is_registered_locked(console))
 		return -ENODEV;
-	}
 
 	hlist_del_init_rcu(&console->node);
 
@@ -3430,8 +3420,6 @@ static int unregister_console_locked(struct console *console)
 	if (!hlist_empty(&console_list) && console->flags & CON_CONSDEV)
 		WRITE_ONCE(console_first()->flags, console_first()->flags | CON_CONSDEV);
 
-	console_unlock();
-
 	/*
 	 * Ensure that all SRCU list walks have completed. All contexts
 	 * must not be able to see this console in the list so that any
-- 
2.30.2

Re: [PATCH printk v3 39/40] printk: relieve console_lock of list synchronization duties

[John Ogness]

On 2022-11-07, John Ogness <john.ogness@linutronix.de> wrote:
> @@ -3344,7 +3340,6 @@ void register_console(struct console *newcon)
>  	 * Put this console in the list - keep the
>  	 * preferred driver at the head of the list.
>  	 */
> -	console_lock();
>  	if (hlist_empty(&console_list)) {
>  		/* Ensure CON_CONSDEV is always set for the head. */
>  		newcon->flags |= CON_CONSDEV;
> @@ -3358,7 +3353,6 @@ void register_console(struct console *newcon)
>  	} else {
>  		hlist_add_behind_rcu(&newcon->node, console_list.first);
>  	}
> -	console_unlock();
>  
>  	/*
>  	 * No need to synchronize SRCU here! The caller does not rely

I just realized that because of the new @seq initialization (patch 5/40)
that we cannot completely remove the console_lock from
register_console(). It will still be needed for @seq synchronization
when registering non-boot/non-printbuffer consoles. So something like
the patch below will need to be folded into this one.

I am not happy with this. If an enabled boot console is behind, the
console_unlock() will probably catch it up and we will end up with some
repeat messages. But maybe this is "good enough" until we implement some
real coordination between boot console and normal console takeovers.

John Ogness

diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index 17765166ac42..bb119001df56 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -3328,12 +3328,21 @@ void register_console(struct console *newcon)
 		 * that message instead. That boot console will be
 		 * unregistered shortly and may be the same device.
 		 */
+
+		/*
+		 * Hold the console_lock to guarantee safe access to
+		 * console->seq.
+		 */
+		console_lock();
+
 		for_each_console(con) {
 			if ((con->flags & (CON_BOOT | CON_ENABLED)) == (CON_BOOT | CON_ENABLED) &&
 			    con->seq < newcon->seq) {
 				newcon->seq = con->seq;
 			}
 		}
+
+		console_unlock();
 	}
 
 	/*

Re: [PATCH printk v3 39/40] printk: relieve console_lock of list synchronization duties

[Petr Mladek]

On Mon 2022-11-07 17:36:48, John Ogness wrote:
> On 2022-11-07, John Ogness <john.ogness@linutronix.de> wrote:
> > @@ -3344,7 +3340,6 @@ void register_console(struct console *newcon)
> >  	 * Put this console in the list - keep the
> >  	 * preferred driver at the head of the list.
> >  	 */
> > -	console_lock();
> >  	if (hlist_empty(&console_list)) {
> >  		/* Ensure CON_CONSDEV is always set for the head. */
> >  		newcon->flags |= CON_CONSDEV;
> > @@ -3358,7 +3353,6 @@ void register_console(struct console *newcon)
> >  	} else {
> >  		hlist_add_behind_rcu(&newcon->node, console_list.first);
> >  	}
> > -	console_unlock();
> >  
> >  	/*
> >  	 * No need to synchronize SRCU here! The caller does not rely
> 
> I just realized that because of the new @seq initialization (patch 5/40)
> that we cannot completely remove the console_lock from
> register_console(). It will still be needed for @seq synchronization
> when registering non-boot/non-printbuffer consoles. So something like
> the patch below will need to be folded into this one.

Great catch!

> I am not happy with this. If an enabled boot console is behind, the
> console_unlock() will probably catch it up and we will end up with some
> repeat messages. But maybe this is "good enough" until we implement some
> real coordination between boot console and normal console takeovers.

The same problem actually has been there even before. The new console
was added in console_list under console_lock(). console_unlock() was
called before the early consoles were unregistered.

A solution would be to call pr_flush() before. But it should be
done in a separate patch.

> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> index 17765166ac42..bb119001df56 100644
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -3328,12 +3328,21 @@ void register_console(struct console *newcon)
>  		 * that message instead. That boot console will be
>  		 * unregistered shortly and may be the same device.
>  		 */
> +
> +		/*
> +		 * Hold the console_lock to guarantee safe access to
> +		 * console->seq.
> +		 */
> +		console_lock();
> +
>  		for_each_console(con) {
>  			if ((con->flags & (CON_BOOT | CON_ENABLED)) == (CON_BOOT | CON_ENABLED) &&
>  			    con->seq < newcon->seq) {
>  				newcon->seq = con->seq;
>  			}
>  		}
> +
> +		console_unlock();
>  	}

This should be added already into the 5th patch that added this cycle.
We just must keep it in this patch.

Best Regards,
Petr

Re: [PATCH printk v3 39/40] printk: relieve console_lock of list synchronization duties

[Petr Mladek]

On Mon 2022-11-07 15:22:37, John Ogness wrote:
> The console_list_lock provides synchronization for console list and
> console->flags updates. All call sites that were using the console_lock
> for this synchronization have either switched to use the
> console_list_lock or the SRCU list iterator.
> 
> Remove console_lock usage for console list updates and console->flags
> updates.
> 
> Signed-off-by: John Ogness <john.ogness@linutronix.de>

All the accesses to console->flags and all the console_list walks
looks safe, so:

Reviewed-by: Petr Mladek <pmladek@suse.com>

Best Regards,
Petr

[PATCH printk v3 40/40] tty: serial: sh-sci: use setup() callback for early console

[John Ogness]

When setting up the early console, the setup() callback of the
regular console is used. It is called manually before registering
the early console instead of providing a setup() callback for the
early console. This is probably because the early setup needs a
different @options during the early stage.

The issue here is that the setup() callback is called without the
console_list_lock held and functions such as uart_set_options()
expect that.

Rather than manually calling the setup() function before registering,
provide an early console setup() callback that will use the different
early options. This ensures that the error checking, ordering, and
locking context when setting up the early console are correct.

Note that technically the current implementation works because it is
only used in early boot. And since the early console setup is
performed before registering, it cannot race with anything and thus
does not need any locking. However, longterm maintenance is easier
when drivers rely on the subsystem API rather than manually
implementing steps that could cause breakage in the future.

Signed-off-by: John Ogness <john.ogness@linutronix.de>
---
 drivers/tty/serial/sh-sci.c | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c
index 62f773286d44..f3a1cfec757a 100644
--- a/drivers/tty/serial/sh-sci.c
+++ b/drivers/tty/serial/sh-sci.c
@@ -3054,15 +3054,26 @@ static struct console serial_console = {
 };
 
 #ifdef CONFIG_SUPERH
+static char early_serial_buf[32];
+
+static int early_serial_console_setup(struct console *co, char *options)
+{
+	WARN_ON(options);
+	/*
+	 * Use @early_serial_buf because @options will always be
+	 * NULL at this early stage.
+	 */
+	return serial_console_setup(co, early_serial_buf);
+}
+
 static struct console early_serial_console = {
 	.name           = "early_ttySC",
 	.write          = serial_console_write,
+	.setup		= early_serial_console_setup,
 	.flags          = CON_PRINTBUFFER,
 	.index		= -1,
 };
 
-static char early_serial_buf[32];
-
 static int sci_probe_earlyprintk(struct platform_device *pdev)
 {
 	const struct plat_sci_port *cfg = dev_get_platdata(&pdev->dev);
@@ -3074,8 +3085,6 @@ static int sci_probe_earlyprintk(struct platform_device *pdev)
 
 	sci_init_single(pdev, &sci_ports[pdev->id], pdev->id, cfg, true);
 
-	serial_console_setup(&early_serial_console, early_serial_buf);
-
 	if (!strstr(early_serial_buf, "keep"))
 		early_serial_console.flags |= CON_BOOT;
 
-- 
2.30.2

Re: [PATCH printk v3 40/40] tty: serial: sh-sci: use setup() callback for early console

[Geert Uytterhoeven]

Hi John,

CC linux-sh (SH-specific code)
CC linux-renesas-soc (JFYI)

On Mon, Nov 7, 2022 at 3:20 PM John Ogness <john.ogness@linutronix.de> wrote:
> When setting up the early console, the setup() callback of the
> regular console is used. It is called manually before registering
> the early console instead of providing a setup() callback for the
> early console. This is probably because the early setup needs a
> different @options during the early stage.
>
> The issue here is that the setup() callback is called without the
> console_list_lock held and functions such as uart_set_options()
> expect that.
>
> Rather than manually calling the setup() function before registering,
> provide an early console setup() callback that will use the different
> early options. This ensures that the error checking, ordering, and
> locking context when setting up the early console are correct.
>
> Note that technically the current implementation works because it is
> only used in early boot. And since the early console setup is
> performed before registering, it cannot race with anything and thus
> does not need any locking. However, longterm maintenance is easier
> when drivers rely on the subsystem API rather than manually
> implementing steps that could cause breakage in the future.
>
> Signed-off-by: John Ogness <john.ogness@linutronix.de>

Thanks for your patch!

> --- a/drivers/tty/serial/sh-sci.c
> +++ b/drivers/tty/serial/sh-sci.c
> @@ -3054,15 +3054,26 @@ static struct console serial_console = {
>  };
>
>  #ifdef CONFIG_SUPERH
> +static char early_serial_buf[32];
> +
> +static int early_serial_console_setup(struct console *co, char *options)
> +{
> +       WARN_ON(options);
> +       /*
> +        * Use @early_serial_buf because @options will always be
> +        * NULL at this early stage.
> +        */
> +       return serial_console_setup(co, early_serial_buf);
> +}
> +
>  static struct console early_serial_console = {
>         .name           = "early_ttySC",
>         .write          = serial_console_write,
> +       .setup          = early_serial_console_setup,
>         .flags          = CON_PRINTBUFFER,
>         .index          = -1,
>  };
>
> -static char early_serial_buf[32];
> -
>  static int sci_probe_earlyprintk(struct platform_device *pdev)
>  {
>         const struct plat_sci_port *cfg = dev_get_platdata(&pdev->dev);
> @@ -3074,8 +3085,6 @@ static int sci_probe_earlyprintk(struct platform_device *pdev)
>
>         sci_init_single(pdev, &sci_ports[pdev->id], pdev->id, cfg, true);
>
> -       serial_console_setup(&early_serial_console, early_serial_buf);
> -
>         if (!strstr(early_serial_buf, "keep"))
>                 early_serial_console.flags |= CON_BOOT;
>
> --
> 2.30.2

LGTM, so
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

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

Re: [PATCH printk v3 40/40] tty: serial: sh-sci: use setup() callback for early console

[Petr Mladek]

Ccing Bartosz who should be familiar with the early platform code.

On Mon 2022-11-07 15:22:38, John Ogness wrote:
> When setting up the early console, the setup() callback of the
> regular console is used. It is called manually before registering
> the early console instead of providing a setup() callback for the
> early console. This is probably because the early setup needs a
> different @options during the early stage.

This last sentece makes a bit nervous ;-)

I think that I understood it in the end, see below.

> The issue here is that the setup() callback is called without the
> console_list_lock held and functions such as uart_set_options()
> expect that.
> 
> Rather than manually calling the setup() function before registering,
> provide an early console setup() callback that will use the different
> early options. This ensures that the error checking, ordering, and
> locking context when setting up the early console are correct.
> 
> Note that technically the current implementation works because it is
> only used in early boot. And since the early console setup is
> performed before registering, it cannot race with anything and thus
> does not need any locking. However, longterm maintenance is easier
> when drivers rely on the subsystem API rather than manually
> implementing steps that could cause breakage in the future.
> 
> Signed-off-by: John Ogness <john.ogness@linutronix.de>
> ---
>  drivers/tty/serial/sh-sci.c | 17 +++++++++++++----
>  1 file changed, 13 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c
> index 62f773286d44..f3a1cfec757a 100644
> --- a/drivers/tty/serial/sh-sci.c
> +++ b/drivers/tty/serial/sh-sci.c
> @@ -3054,15 +3054,26 @@ static struct console serial_console = {
>  };
>  
>  #ifdef CONFIG_SUPERH
> +static char early_serial_buf[32];
> +
> +static int early_serial_console_setup(struct console *co, char *options)
> +{
> +	WARN_ON(options);
> +	/*
> +	 * Use @early_serial_buf because @options will always be
> +	 * NULL at this early stage.
> +	 */

The commit message says that we use @early_serial_buf because
the early console probably needs another parameters.

It suggests that @options might be for the later stage and
we need to replace them there. Are we sure that this will always
be NULL?

Background:

The console->setup() is called in two situations:

   1. when the console is registered as the default console, see
     try_enable_default_console(). In this case, @options
     is really NULL.

   2. when the console is preferred either via the commnadline,
      or device tree, or SPCR, see try_enable_preferred_console().
      In this case, some real @options would be passed.

     From the code POV, the preferred consoles are added by calling
     add_preferred_console().


Now, it means that the WARN_ON() is correct only when this console
is always registered before the preferred consoles are defined.

I think that this is really the case. This console
is actually registered via the "earlyprintk" parameter that
is proceed by the arch-specific code before the preferred
consoles are added the standard way via the kernel commandline.

Note that "earlyprintk" and "earlycon" are two different parameters.

"earlyprintk" normally initializes "early_console" that is
called directly by early_printk(). It is used for super early
debugging. These messages even do not end in the ring buffer.

"earlycon" defines a "normal" console that is used by the standard
printk(). They are later replaced by properly initialized console
drivers that are in sysfs, ...

Note that "earlycon" calls add_preferred_console() so that
the @options are stored and passed from try_enable_preferred_console().

But "earlyprintk" does not call add_preferred_console() so
we need this hack to store and pass the console options
another way.

> +	return serial_console_setup(co, early_serial_buf);
> +}
> +

So I would do something like:

static int early_serial_console_setup(struct console *co, char *options)
{
	/*
	 * This early console is registered using earlyprintk= parameter
	 * that does not call add_preferred_console(). The @options
	 * are passed using a custom buffer.
	 */
	WARN_ON(options);

	return serial_console_setup(co, early_serial_buf);
}

Also we should explain this in the commit message.

Best Regards,
Petr

Re: [PATCH printk v3 00/40] reduce console_lock scope

[Mathieu Desnoyers]

On 2022-11-07 09:15, John Ogness wrote:
[...]
> 
> The base commit for this series is from Paul McKenney's RCU tree
> and provides an NMI-safe SRCU implementation [1]. Without the
> NMI-safe SRCU implementation, this series is not less safe than
> mainline. But we will need the NMI-safe SRCU implementation for
> atomic consoles anyway, so we might as well get it in
> now. Especially since it _does_ increase the reliability for
> mainline in the panic path.

So, your email got me to review the SRCU nmi-safe series:

[1] https://git.kernel.org/pub/scm/linux/kernel/git/paulmck/linux-rcu.git/log/?h=srcunmisafe.2022.10.21a

Especially this commit:

https://git.kernel.org/pub/scm/linux/kernel/git/paulmck/linux-rcu.git/commit/?h=srcunmisafe.2022.10.21a&id=5d0f5953b60f5f7a278085b55ddc73e2932f4c33

I disagree with the overall approach taken there, which is to create
yet another SRCU flavor, this time with explicit "nmi-safe" read-locks.
This adds complexity to the kernel APIs and I think we can be clever
about this and make SRCU nmi-safe without requiring a whole new incompatible
API.

You can find the basic idea needed to achieve this in the libside RCU
user-space implementation. I needed to introduce a split-counter concept
to support rseq vs atomics to keep track of per-cpu grace period counters.
The "rseq" counter is the fast-path, but if rseq fails, the abort handler
uses the atomic counter instead.

https://github.com/compudj/side/blob/main/src/rcu.h#L23

struct side_rcu_percpu_count {
	uintptr_t begin;
	uintptr_t rseq_begin;
	uintptr_t end;
	uintptr_t rseq_end;
}  __attribute__((__aligned__(SIDE_CACHE_LINE_SIZE)));

The idea is to "split" each percpu counter into two counters, one for rseq,
and the other for atomics. When a grace period wants to observe the value of
a percpu counter, it simply sums the two counters:

https://github.com/compudj/side/blob/main/src/rcu.c#L112

The same idea can be applied to SRCU in the kernel: one counter for percpu ops,
and the other counter for nmi context, so basically:

srcu_read_lock()

if (likely(!in_nmi()))
   increment the percpu-ops lock counter
else
   increment the atomic lock counter

srcu_read_unlock()

if (likely(!in_nmi()))
   increment the percpu-ops unlock counter
else
   increment the atomic unlock counter

Then in the grace period sum the percpu-ops and the atomic values whenever
each counter value is read.

This would allow SRCU to be NMI-safe without requiring the callers to
explicitly state whether they need to be nmi-safe or not, and would only
take the overhead of the atomics in the NMI handlers rather than for all
users which happen to use SRCU read locks shared with nmi handlers.

Thoughts ?

Thanks,

Mathieu

-- 
Mathieu Desnoyers
EfficiOS Inc.
https://www.efficios.com

Re: [PATCH printk v3 00/40] reduce console_lock scope

[Mathieu Desnoyers]

On 2022-11-07 09:15, John Ogness wrote:
[...]
> 
> The base commit for this series is from Paul McKenney's RCU tree
> and provides an NMI-safe SRCU implementation [1]. Without the
> NMI-safe SRCU implementation, this series is not less safe than
> mainline. But we will need the NMI-safe SRCU implementation for
> atomic consoles anyway, so we might as well get it in
> now. Especially since it _does_ increase the reliability for
> mainline in the panic path.

So, your email got me to review the SRCU nmi-safe series:

[1] https://git.kernel.org/pub/scm/linux/kernel/git/paulmck/linux-rcu.git/log/?h=srcunmisafe.2022.10.21a

Especially this commit:

https://git.kernel.org/pub/scm/linux/kernel/git/paulmck/linux-rcu.git/commit/?h=srcunmisafe.2022.10.21a&id=5d0f5953b60f5f7a278085b55ddc73e2932f4c33

I disagree with the overall approach taken there, which is to create
yet another SRCU flavor, this time with explicit "nmi-safe" read-locks.
This adds complexity to the kernel APIs and I think we can be clever
about this and make SRCU nmi-safe without requiring a whole new incompatible
API.

You can find the basic idea needed to achieve this in the libside RCU
user-space implementation. I needed to introduce a split-counter concept
to support rseq vs atomics to keep track of per-cpu grace period counters.
The "rseq" counter is the fast-path, but if rseq fails, the abort handler
uses the atomic counter instead.

https://github.com/compudj/side/blob/main/src/rcu.h#L23

struct side_rcu_percpu_count {
	uintptr_t begin;
	uintptr_t rseq_begin;
	uintptr_t end;
	uintptr_t rseq_end;
}  __attribute__((__aligned__(SIDE_CACHE_LINE_SIZE)));

The idea is to "split" each percpu counter into two counters, one for rseq,
and the other for atomics. When a grace period wants to observe the value of
a percpu counter, it simply sums the two counters:

https://github.com/compudj/side/blob/main/src/rcu.c#L112

The same idea can be applied to SRCU in the kernel: one counter for percpu ops,
and the other counter for nmi context, so basically:

srcu_read_lock()

if (likely(!in_nmi()))
   increment the percpu-ops lock counter
else
   increment the atomic lock counter

srcu_read_unlock()

if (likely(!in_nmi()))
   increment the percpu-ops unlock counter
else
   increment the atomic unlock counter

Then in the grace period sum the percpu-ops and the atomic values whenever
each counter value is read.

This would allow SRCU to be NMI-safe without requiring the callers to
explicitly state whether they need to be nmi-safe or not, and would only
take the overhead of the atomics in the NMI handlers rather than for all
users which happen to use SRCU read locks shared with nmi handlers.

Thoughts ?

Thanks,

Mathieu

-- 
Mathieu Desnoyers
EfficiOS Inc.
https://www.efficios.com


_______________________________________________
linux-um mailing list
linux-um@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-um

Re: [PATCH printk v3 00/40] reduce console_lock scope

[Mathieu Desnoyers]

On 2022-11-07 09:15, John Ogness wrote:
[...]
> 
> The base commit for this series is from Paul McKenney's RCU tree
> and provides an NMI-safe SRCU implementation [1]. Without the
> NMI-safe SRCU implementation, this series is not less safe than
> mainline. But we will need the NMI-safe SRCU implementation for
> atomic consoles anyway, so we might as well get it in
> now. Especially since it _does_ increase the reliability for
> mainline in the panic path.

So, your email got me to review the SRCU nmi-safe series:

[1] https://git.kernel.org/pub/scm/linux/kernel/git/paulmck/linux-rcu.git/log/?h=srcunmisafe.2022.10.21a

Especially this commit:

https://git.kernel.org/pub/scm/linux/kernel/git/paulmck/linux-rcu.git/commit/?h=srcunmisafe.2022.10.21a&id=5d0f5953b60f5f7a278085b55ddc73e2932f4c33

I disagree with the overall approach taken there, which is to create
yet another SRCU flavor, this time with explicit "nmi-safe" read-locks.
This adds complexity to the kernel APIs and I think we can be clever
about this and make SRCU nmi-safe without requiring a whole new incompatible
API.

You can find the basic idea needed to achieve this in the libside RCU
user-space implementation. I needed to introduce a split-counter concept
to support rseq vs atomics to keep track of per-cpu grace period counters.
The "rseq" counter is the fast-path, but if rseq fails, the abort handler
uses the atomic counter instead.

https://github.com/compudj/side/blob/main/src/rcu.h#L23

struct side_rcu_percpu_count {
	uintptr_t begin;
	uintptr_t rseq_begin;
	uintptr_t end;
	uintptr_t rseq_end;
}  __attribute__((__aligned__(SIDE_CACHE_LINE_SIZE)));

The idea is to "split" each percpu counter into two counters, one for rseq,
and the other for atomics. When a grace period wants to observe the value of
a percpu counter, it simply sums the two counters:

https://github.com/compudj/side/blob/main/src/rcu.c#L112

The same idea can be applied to SRCU in the kernel: one counter for percpu ops,
and the other counter for nmi context, so basically:

srcu_read_lock()

if (likely(!in_nmi()))
   increment the percpu-ops lock counter
else
   increment the atomic lock counter

srcu_read_unlock()

if (likely(!in_nmi()))
   increment the percpu-ops unlock counter
else
   increment the atomic unlock counter

Then in the grace period sum the percpu-ops and the atomic values whenever
each counter value is read.

This would allow SRCU to be NMI-safe without requiring the callers to
explicitly state whether they need to be nmi-safe or not, and would only
take the overhead of the atomics in the NMI handlers rather than for all
users which happen to use SRCU read locks shared with nmi handlers.

Thoughts ?

Thanks,

Mathieu

-- 
Mathieu Desnoyers
EfficiOS Inc.
https://www.efficios.com

Re: [PATCH printk v3 00/40] reduce console_lock scope

[Mathieu Desnoyers]

On 2022-11-07 09:15, John Ogness wrote:
[...]
> 
> The base commit for this series is from Paul McKenney's RCU tree
> and provides an NMI-safe SRCU implementation [1]. Without the
> NMI-safe SRCU implementation, this series is not less safe than
> mainline. But we will need the NMI-safe SRCU implementation for
> atomic consoles anyway, so we might as well get it in
> now. Especially since it _does_ increase the reliability for
> mainline in the panic path.

So, your email got me to review the SRCU nmi-safe series:

[1] https://git.kernel.org/pub/scm/linux/kernel/git/paulmck/linux-rcu.git/log/?h=srcunmisafe.2022.10.21a

Especially this commit:

https://git.kernel.org/pub/scm/linux/kernel/git/paulmck/linux-rcu.git/commit/?h=srcunmisafe.2022.10.21a&id=5d0f5953b60f5f7a278085b55ddc73e2932f4c33

I disagree with the overall approach taken there, which is to create
yet another SRCU flavor, this time with explicit "nmi-safe" read-locks.
This adds complexity to the kernel APIs and I think we can be clever
about this and make SRCU nmi-safe without requiring a whole new incompatible
API.

You can find the basic idea needed to achieve this in the libside RCU
user-space implementation. I needed to introduce a split-counter concept
to support rseq vs atomics to keep track of per-cpu grace period counters.
The "rseq" counter is the fast-path, but if rseq fails, the abort handler
uses the atomic counter instead.

https://github.com/compudj/side/blob/main/src/rcu.h#L23

struct side_rcu_percpu_count {
	uintptr_t begin;
	uintptr_t rseq_begin;
	uintptr_t end;
	uintptr_t rseq_end;
}  __attribute__((__aligned__(SIDE_CACHE_LINE_SIZE)));

The idea is to "split" each percpu counter into two counters, one for rseq,
and the other for atomics. When a grace period wants to observe the value of
a percpu counter, it simply sums the two counters:

https://github.com/compudj/side/blob/main/src/rcu.c#L112

The same idea can be applied to SRCU in the kernel: one counter for percpu ops,
and the other counter for nmi context, so basically:

srcu_read_lock()

if (likely(!in_nmi()))
   increment the percpu-ops lock counter
else
   increment the atomic lock counter

srcu_read_unlock()

if (likely(!in_nmi()))
   increment the percpu-ops unlock counter
else
   increment the atomic unlock counter

Then in the grace period sum the percpu-ops and the atomic values whenever
each counter value is read.

This would allow SRCU to be NMI-safe without requiring the callers to
explicitly state whether they need to be nmi-safe or not, and would only
take the overhead of the atomics in the NMI handlers rather than for all
users which happen to use SRCU read locks shared with nmi handlers.

Thoughts ?

Thanks,

Mathieu

-- 
Mathieu Desnoyers
EfficiOS Inc.
https://www.efficios.com