[v3,3/3] serial: core: Move console character device handling from printk
Commit Message
There's no need for console_setup() to try to figure out the console
character device name for serial ports early on before uart_add_one_port()
has been called. And for early debug console we have earlycon.
Let's handle the serial port specific commandline options in the serial
core subsystem and drop the handling from printk. The serial core
subsystem can just call add_preferred_console() when the serial port is
getting initialized.
Note that eventually we may want to set up driver specific console quirk
handling for the serial port device drivers to use. But we need to figure
out which driver(s) need to call the quirk. So for now, we just move the
sparc quirk and handle it directly.
Signed-off-by: Tony Lindgren <tony@atomide.com>
---
drivers/tty/serial/serial_base_bus.c | 66 ++++++++++++++++++++++++++++
kernel/printk/printk.c | 30 +------------
2 files changed, 67 insertions(+), 29 deletions(-)
Comments
On Tue, Nov 21, 2023 at 01:31:57PM +0200, Tony Lindgren wrote:
> There's no need for console_setup() to try to figure out the console
> character device name for serial ports early on before uart_add_one_port()
> has been called. And for early debug console we have earlycon.
>
> Let's handle the serial port specific commandline options in the serial
> core subsystem and drop the handling from printk. The serial core
> subsystem can just call add_preferred_console() when the serial port is
> getting initialized.
>
> Note that eventually we may want to set up driver specific console quirk
> handling for the serial port device drivers to use. But we need to figure
> out which driver(s) need to call the quirk. So for now, we just move the
> sparc quirk and handle it directly.
...
> + ret = add_preferred_console_match(name, drv->dev_name, port->line);
> + if (ret && ret != -ENOENT)
> + return ret;
> +
> + return 0;
2nd time and so on, perhaps deserves a helper?
static inline int add_preferred_console...(...)
{
int ret;
ret = add_preferred_console_match(name, drv->dev_name, port->line);
if (ret == -ENOENT)
return 0;
return ret;
}
?
* Andy Shevchenko <andriy.shevchenko@intel.com> [231121 18:00]:
> On Tue, Nov 21, 2023 at 01:31:57PM +0200, Tony Lindgren wrote:
> > + ret = add_preferred_console_match(name, drv->dev_name, port->line);
> > + if (ret && ret != -ENOENT)
> > + return ret;
> > +
> > + return 0;
>
> 2nd time and so on, perhaps deserves a helper?
>
> static inline int add_preferred_console...(...)
> {
> int ret;
>
> ret = add_preferred_console_match(name, drv->dev_name, port->line);
> if (ret == -ENOENT)
> return 0;
> return ret;
>
> }
>
> ?
Yes good idea, thanks.
Tony
* Tony Lindgren <tony@atomide.com> [700101 02:00]:
> - __add_preferred_console(buf, idx, options, brl_options, true);
> return 1;
Looks like this can't be dropped yet. We need to keep it for the
brl_options. I'll change it to return early if brl_options is NULL.
Regards,
Tony
* Tony Lindgren <tony@atomide.com> [231122 09:04]:
> * Tony Lindgren <tony@atomide.com> [700101 02:00]:
> > - __add_preferred_console(buf, idx, options, brl_options, true);
> > return 1;
>
> Looks like this can't be dropped yet. We need to keep it for the
> brl_options. I'll change it to return early if brl_options is NULL.
Looks like we can drop the parsing from printk :) In console_setup()
we can call console_opt_save() after _braille_console_setup(), and
then save also the brl_options for the port.
Regards,
Tony
Hi Tony,
kernel test robot noticed the following build warnings:
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Tony-Lindgren/printk-Save-console-options-for-add_preferred_console_match/20231121-193809
base: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git usb-testing
patch link: https://lore.kernel.org/r/20231121113203.61341-4-tony%40atomide.com
patch subject: [PATCH v3 3/3] serial: core: Move console character device handling from printk
config: parisc-randconfig-r081-20231122 (https://download.01.org/0day-ci/archive/20231122/202311221437.5Gil0Pml-lkp@intel.com/config)
compiler: hppa-linux-gcc (GCC) 13.2.0
reproduce: (https://download.01.org/0day-ci/archive/20231122/202311221437.5Gil0Pml-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Reported-by: Dan Carpenter <error27@gmail.com>
| Closes: https://lore.kernel.org/r/202311221437.5Gil0Pml-lkp@intel.com/
smatch warnings:
drivers/tty/serial/serial_base_bus.c:266 serial_base_add_preferred_console() error: uninitialized symbol 'nmbr_match'.
drivers/tty/serial/serial_base_bus.c:265 serial_base_add_preferred_console() error: uninitialized symbol 'char_match'.
vim +/nmbr_match +266 drivers/tty/serial/serial_base_bus.c
e4ebdcd790e0f3 Tony Lindgren 2023-11-21 261 int serial_base_add_preferred_console(struct uart_driver *drv,
e4ebdcd790e0f3 Tony Lindgren 2023-11-21 262 struct uart_port *port)
e4ebdcd790e0f3 Tony Lindgren 2023-11-21 263 {
e4ebdcd790e0f3 Tony Lindgren 2023-11-21 264 const char *port_match __free(kfree);
b1b8726ec3f40b Tony Lindgren 2023-11-21 @265 const char *char_match __free(kfree);
b1b8726ec3f40b Tony Lindgren 2023-11-21 @266 const char *nmbr_match __free(kfree);
These need to be initialized to NULL.
const char *char_match __free(kfree) = NULL;
e4ebdcd790e0f3 Tony Lindgren 2023-11-21 267 int ret;
e4ebdcd790e0f3 Tony Lindgren 2023-11-21 268
e4ebdcd790e0f3 Tony Lindgren 2023-11-21 269 port_match = kasprintf(GFP_KERNEL, "%s:%i.%i", dev_name(port->dev),
e4ebdcd790e0f3 Tony Lindgren 2023-11-21 270 port->ctrl_id, port->port_id);
e4ebdcd790e0f3 Tony Lindgren 2023-11-21 271 if (!port_match)
e4ebdcd790e0f3 Tony Lindgren 2023-11-21 272 return -ENOMEM;
Otherwise in this error path we'll call kfree(char_match) and
kfree(nmbr_match) when the haven't been initialized.
e4ebdcd790e0f3 Tony Lindgren 2023-11-21 273
b1b8726ec3f40b Tony Lindgren 2023-11-21 274 char_match = kasprintf(GFP_KERNEL, "%s%i", drv->dev_name, port->line);
b1b8726ec3f40b Tony Lindgren 2023-11-21 275 if (!char_match)
b1b8726ec3f40b Tony Lindgren 2023-11-21 276 return -ENOMEM;
b1b8726ec3f40b Tony Lindgren 2023-11-21 277
b1b8726ec3f40b Tony Lindgren 2023-11-21 278 /* Handle ttyS specific options */
b1b8726ec3f40b Tony Lindgren 2023-11-21 279 if (!strncmp(drv->dev_name, "ttyS", 4)) {
b1b8726ec3f40b Tony Lindgren 2023-11-21 280 /* No name, just a number */
b1b8726ec3f40b Tony Lindgren 2023-11-21 281 nmbr_match = kasprintf(GFP_KERNEL, "%i", port->line);
b1b8726ec3f40b Tony Lindgren 2023-11-21 282 if (!nmbr_match)
b1b8726ec3f40b Tony Lindgren 2023-11-21 283 return -ENODEV;
b1b8726ec3f40b Tony Lindgren 2023-11-21 284
b1b8726ec3f40b Tony Lindgren 2023-11-21 285 ret = add_preferred_console_match(nmbr_match, drv->dev_name,
b1b8726ec3f40b Tony Lindgren 2023-11-21 286 port->line);
b1b8726ec3f40b Tony Lindgren 2023-11-21 287 if (ret && ret != -ENOENT)
b1b8726ec3f40b Tony Lindgren 2023-11-21 288 return ret;
b1b8726ec3f40b Tony Lindgren 2023-11-21 289
b1b8726ec3f40b Tony Lindgren 2023-11-21 290 /* Sparc ttya and ttyb */
b1b8726ec3f40b Tony Lindgren 2023-11-21 291 ret = serial_base_add_sparc_console(drv, port);
b1b8726ec3f40b Tony Lindgren 2023-11-21 292 if (ret)
b1b8726ec3f40b Tony Lindgren 2023-11-21 293 return ret;
b1b8726ec3f40b Tony Lindgren 2023-11-21 294 }
b1b8726ec3f40b Tony Lindgren 2023-11-21 295
b1b8726ec3f40b Tony Lindgren 2023-11-21 296 /* Handle the traditional character device name style console=ttyS0 */
b1b8726ec3f40b Tony Lindgren 2023-11-21 297 ret = add_preferred_console_match(char_match, drv->dev_name, port->line);
b1b8726ec3f40b Tony Lindgren 2023-11-21 298 if (ret && ret != -ENOENT)
b1b8726ec3f40b Tony Lindgren 2023-11-21 299 return ret;
b1b8726ec3f40b Tony Lindgren 2023-11-21 300
e4ebdcd790e0f3 Tony Lindgren 2023-11-21 301 /* Translate a hardware addressing style console=DEVNAME:0.0 */
e4ebdcd790e0f3 Tony Lindgren 2023-11-21 302 ret = add_preferred_console_match(port_match, drv->dev_name, port->line);
e4ebdcd790e0f3 Tony Lindgren 2023-11-21 303 if (ret && ret != -ENOENT)
e4ebdcd790e0f3 Tony Lindgren 2023-11-21 304 return ret;
e4ebdcd790e0f3 Tony Lindgren 2023-11-21 305
e4ebdcd790e0f3 Tony Lindgren 2023-11-21 306 return 0;
e4ebdcd790e0f3 Tony Lindgren 2023-11-21 307 }
On Thu, Nov 23, 2023 at 10:24:24AM +0300, Dan Carpenter wrote:
> Hi Tony,
>
> kernel test robot noticed the following build warnings:
>
> https://git-scm.com/docs/git-format-patch#_base_tree_information]
>
> url: https://github.com/intel-lab-lkp/linux/commits/Tony-Lindgren/printk-Save-console-options-for-add_preferred_console_match/20231121-193809
> base: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git usb-testing
> patch link: https://lore.kernel.org/r/20231121113203.61341-4-tony%40atomide.com
> patch subject: [PATCH v3 3/3] serial: core: Move console character device handling from printk
> config: parisc-randconfig-r081-20231122 (https://download.01.org/0day-ci/archive/20231122/202311221437.5Gil0Pml-lkp@intel.com/config)
> compiler: hppa-linux-gcc (GCC) 13.2.0
> reproduce: (https://download.01.org/0day-ci/archive/20231122/202311221437.5Gil0Pml-lkp@intel.com/reproduce)
>
> If you fix the issue in a separate patch/commit (i.e. not just a new version of
> the same patch/commit), kindly add following tags
> | Reported-by: kernel test robot <lkp@intel.com>
> | Reported-by: Dan Carpenter <error27@gmail.com>
> | Closes: https://lore.kernel.org/r/202311221437.5Gil0Pml-lkp@intel.com/
>
> smatch warnings:
> drivers/tty/serial/serial_base_bus.c:266 serial_base_add_preferred_console() error: uninitialized symbol 'nmbr_match'.
> drivers/tty/serial/serial_base_bus.c:265 serial_base_add_preferred_console() error: uninitialized symbol 'char_match'.
>
> vim +/nmbr_match +266 drivers/tty/serial/serial_base_bus.c
>
> e4ebdcd790e0f3 Tony Lindgren 2023-11-21 261 int serial_base_add_preferred_console(struct uart_driver *drv,
> e4ebdcd790e0f3 Tony Lindgren 2023-11-21 262 struct uart_port *port)
> e4ebdcd790e0f3 Tony Lindgren 2023-11-21 263 {
> e4ebdcd790e0f3 Tony Lindgren 2023-11-21 264 const char *port_match __free(kfree);
> b1b8726ec3f40b Tony Lindgren 2023-11-21 @265 const char *char_match __free(kfree);
> b1b8726ec3f40b Tony Lindgren 2023-11-21 @266 const char *nmbr_match __free(kfree);
>
> These need to be initialized to NULL.
>
> const char *char_match __free(kfree) = NULL;
>
Let's add a todo to make checkpatch warn about this.
KTODO: make checkpatch warn about __free() functions without an initializer
regards,
dan carpenter
* Tony Lindgren <tony@atomide.com> [231122 10:16]:
> * Tony Lindgren <tony@atomide.com> [231122 09:04]:
> > * Tony Lindgren <tony@atomide.com> [700101 02:00]:
> > > - __add_preferred_console(buf, idx, options, brl_options, true);
> > > return 1;
> >
> > Looks like this can't be dropped yet. We need to keep it for the
> > brl_options. I'll change it to return early if brl_options is NULL.
>
> Looks like we can drop the parsing from printk :) In console_setup()
> we can call console_opt_save() after _braille_console_setup(), and
> then save also the brl_options for the port.
I noticed we have more issues remaining trying to drop the console
parsing completely from console_setup().
If add_preferred_console() gets called later, register_console() can
try to call try_enable_default_console() before we get around to call
try_enable_preferred_console(), and that may lead to no serial console.
To avoid that, setting console_set_on_cmdline = 1 in console_setup(),
and patching register_console() console to check for the flag helps.
But looks like that leads to bootconsole not getting disabled and
more patching for that is needed.. And of course we'd need to check
the other register_console() callers too, not just 8250..
So I think for now, it's best to just drop the 8250 and sparc quirks
from console_setup(), that already simplifies things in printk a bit :)
And for 8250, we should have serial8250_isa_init_ports() call
add_preferred_console_match() to avoid console getting registered
later on when the hardware specific driver takes over for x86, m68k,
and alpha that define SERIAL_PORT_DFNS.
Regards,
Tony
* Dan Carpenter <dan.carpenter@linaro.org> [231123 07:29]:
> On Thu, Nov 23, 2023 at 10:24:24AM +0300, Dan Carpenter wrote:
> > Hi Tony,
> >
> > kernel test robot noticed the following build warnings:
> >
> > https://git-scm.com/docs/git-format-patch#_base_tree_information]
> >
> > url: https://github.com/intel-lab-lkp/linux/commits/Tony-Lindgren/printk-Save-console-options-for-add_preferred_console_match/20231121-193809
> > base: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git usb-testing
> > patch link: https://lore.kernel.org/r/20231121113203.61341-4-tony%40atomide.com
> > patch subject: [PATCH v3 3/3] serial: core: Move console character device handling from printk
> > config: parisc-randconfig-r081-20231122 (https://download.01.org/0day-ci/archive/20231122/202311221437.5Gil0Pml-lkp@intel.com/config)
> > compiler: hppa-linux-gcc (GCC) 13.2.0
> > reproduce: (https://download.01.org/0day-ci/archive/20231122/202311221437.5Gil0Pml-lkp@intel.com/reproduce)
> >
> > If you fix the issue in a separate patch/commit (i.e. not just a new version of
> > the same patch/commit), kindly add following tags
> > | Reported-by: kernel test robot <lkp@intel.com>
> > | Reported-by: Dan Carpenter <error27@gmail.com>
> > | Closes: https://lore.kernel.org/r/202311221437.5Gil0Pml-lkp@intel.com/
> >
> > smatch warnings:
> > drivers/tty/serial/serial_base_bus.c:266 serial_base_add_preferred_console() error: uninitialized symbol 'nmbr_match'.
> > drivers/tty/serial/serial_base_bus.c:265 serial_base_add_preferred_console() error: uninitialized symbol 'char_match'.
> >
> > vim +/nmbr_match +266 drivers/tty/serial/serial_base_bus.c
> >
> > e4ebdcd790e0f3 Tony Lindgren 2023-11-21 261 int serial_base_add_preferred_console(struct uart_driver *drv,
> > e4ebdcd790e0f3 Tony Lindgren 2023-11-21 262 struct uart_port *port)
> > e4ebdcd790e0f3 Tony Lindgren 2023-11-21 263 {
> > e4ebdcd790e0f3 Tony Lindgren 2023-11-21 264 const char *port_match __free(kfree);
> > b1b8726ec3f40b Tony Lindgren 2023-11-21 @265 const char *char_match __free(kfree);
> > b1b8726ec3f40b Tony Lindgren 2023-11-21 @266 const char *nmbr_match __free(kfree);
> >
> > These need to be initialized to NULL.
> >
> > const char *char_match __free(kfree) = NULL;
> >
>
> Let's add a todo to make checkpatch warn about this.
>
> KTODO: make checkpatch warn about __free() functions without an initializer
Yes good idea.
Thanks,
Tony
@@ -206,6 +206,43 @@ void serial_base_port_device_remove(struct serial_port_device *port_dev)
#ifdef CONFIG_SERIAL_CORE_CONSOLE
+#ifdef __sparc__
+
+/* Handle Sparc ttya and ttyb options as done in console_setup() */
+static int serial_base_add_sparc_console(struct uart_driver *drv,
+ struct uart_port *port)
+{
+ const char *name = NULL;
+ int ret;
+
+ switch (port->line) {
+ case 0:
+ name = "ttya";
+ break;
+ case 1:
+ name = "ttyb";
+ break;
+ default:
+ return 0;
+ }
+
+ ret = add_preferred_console_match(name, drv->dev_name, port->line);
+ if (ret && ret != -ENOENT)
+ return ret;
+
+ return 0;
+}
+
+#else
+
+static inline int serial_base_add_sparc_console(struct uart_driver *drv,
+ struct uart_port *port)
+{
+ return 0;
+}
+
+#endif
+
/*
* serial_base_add_preferred_console - Adds a preferred console
* @drv: Serial port device driver
@@ -225,6 +262,8 @@ int serial_base_add_preferred_console(struct uart_driver *drv,
struct uart_port *port)
{
const char *port_match __free(kfree);
+ const char *char_match __free(kfree);
+ const char *nmbr_match __free(kfree);
int ret;
port_match = kasprintf(GFP_KERNEL, "%s:%i.%i", dev_name(port->dev),
@@ -232,6 +271,33 @@ int serial_base_add_preferred_console(struct uart_driver *drv,
if (!port_match)
return -ENOMEM;
+ char_match = kasprintf(GFP_KERNEL, "%s%i", drv->dev_name, port->line);
+ if (!char_match)
+ return -ENOMEM;
+
+ /* Handle ttyS specific options */
+ if (!strncmp(drv->dev_name, "ttyS", 4)) {
+ /* No name, just a number */
+ nmbr_match = kasprintf(GFP_KERNEL, "%i", port->line);
+ if (!nmbr_match)
+ return -ENODEV;
+
+ ret = add_preferred_console_match(nmbr_match, drv->dev_name,
+ port->line);
+ if (ret && ret != -ENOENT)
+ return ret;
+
+ /* Sparc ttya and ttyb */
+ ret = serial_base_add_sparc_console(drv, port);
+ if (ret)
+ return ret;
+ }
+
+ /* Handle the traditional character device name style console=ttyS0 */
+ ret = add_preferred_console_match(char_match, drv->dev_name, port->line);
+ if (ret && ret != -ENOENT)
+ return ret;
+
/* Translate a hardware addressing style console=DEVNAME:0.0 */
ret = add_preferred_console_match(port_match, drv->dev_name, port->line);
if (ret && ret != -ENOENT)
@@ -2438,9 +2438,7 @@ __setup("console_msg_format=", console_msg_format_setup);
*/
static int __init console_setup(char *str)
{
- char buf[sizeof(console_cmdline[0].name) + 4]; /* 4 for "ttyS" */
- char *s, *options, *brl_options = NULL;
- int idx;
+ char *brl_options = NULL;
/*
* Save the console for possible driver subsystem use. Bail out early
@@ -2463,32 +2461,6 @@ static int __init console_setup(char *str)
if (_braille_console_setup(&str, &brl_options))
return 1;
- /*
- * Decode str into name, index, options.
- */
- if (str[0] >= '0' && str[0] <= '9') {
- strcpy(buf, "ttyS");
- strncpy(buf + 4, str, sizeof(buf) - 5);
- } else {
- strncpy(buf, str, sizeof(buf) - 1);
- }
- buf[sizeof(buf) - 1] = 0;
- options = strchr(str, ',');
- if (options)
- *(options++) = 0;
-#ifdef __sparc__
- if (!strcmp(str, "ttya"))
- strcpy(buf, "ttyS0");
- if (!strcmp(str, "ttyb"))
- strcpy(buf, "ttyS1");
-#endif
- for (s = buf; *s; s++)
- if (isdigit(*s) || *s == ',')
- break;
- idx = simple_strtoul(s, NULL, 10);
- *s = 0;
-
- __add_preferred_console(buf, idx, options, brl_options, true);
return 1;
}
__setup("console=", console_setup);