| Message ID | 20240606114149.118633-1-tony.lindgren@linux.intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | [1/2] printk: Track command line console positions to fix console order | expand |
On Thu 2024-06-06 14:41:46, Tony Lindgren wrote: > Recent changes to allow using DEVNAME:0.0 style console names caused a > regression to the kernel command line handling for the console options. Sigh, I have missed that it has already ended in the mainline via the tty tree :-/ Honestly, I would prefer to revert it and implement it a clean way. I see a lot of existing and possible problems: 1. Where and how is DEVNAME:0.0 defined? The only documentation seems to be in Documentation/admin-guide/kernel-parameters.txt for the console= parameter: <paste> <DEVNAME>:<n>.<n>[,options] Use the specified serial port on the serial core bus. The addressing uses DEVNAME of the physical serial port device, followed by the serial core controller instance, and the serial port instance. The options are the same as documented for the ttyS addressing above. The mapping of the serial ports to the tty instances can be viewed with: $ ls -d /sys/bus/serial-base/devices/*:*.*/tty/* /sys/bus/serial-base/devices/00:04:0.0/tty/ttyS0 In the above example, the console can be addressed with console=00:04:0.0. Note that a console addressed this way will only get added when the related device driver is ready. The use of an earlycon parameter in addition to the console may be desired for console output early on. </paste> This seems to be a brand new bus. Is it stable? Is it documented in Documentation/ABI/stable/? It seems that the feature will cover "only" serial consoles. But DEVNAME is a generic name. It might make more sense to call it "SERIAL_BASE_DEVNAME" or "SERIAL_DEVNAME" or "SBASE_DEVNAME" or so. Anyway, console= is an interface with the user space. We will need to maintain the backward compatibility "forever". => we should think twice about the interface !!! 2. On my test system (kvm) I get # ls -d -1 /sys/bus/serial-base/devices/*:*.*/tty/* | sort -t. -k 2n /sys/bus/serial-base/devices/00:00:0.0/tty/ttyS0 /sys/bus/serial-base/devices/serial8250:0.1/tty/ttyS1 /sys/bus/serial-base/devices/serial8250:0.2/tty/ttyS2 /sys/bus/serial-base/devices/serial8250:0.3/tty/ttyS3 [...] /sys/bus/serial-base/devices/serial8250:0.30/tty/ttyS30 /sys/bus/serial-base/devices/serial8250:0.31/tty/ttyS31 So, the DEVNAME:X.Y to ttySZ mapping is: 00:00:0.0 -> ttyS0 serial8250:0.1 -> ttyS1 serial8250:0.2 -> ttyS2 serial8250:0.3 -> ttyS3 [...] serial8250:0.30 -> ttyS30 serial8250:0.31 -> ttyS31 Why is ttyS0 associated with "so ugly" DEVNAME "00:00" while the rest uses "nice" DEVNAME "serial8250"? I would expect: serial8250:0.0 -> ttyS0 serial8250:0.1 -> ttyS1 serial8250:0.2 -> ttyS2 [...] 3. The delimiter between "DEVNAME" and X.Y numbers is ":". But ":" is also part of the sample DEVNAME "00:00" Is it a good idea? Is the current naming scheme a well known historic one or something invented for the new bus? 4. /sys/bus/serial-base/devices/ contains few more entries: # ls -d -1 /sys/bus/serial-base/devices/* | sort -t. -k 2n /sys/bus/serial-base/devices/00:00:0 /sys/bus/serial-base/devices/00:00:0.0 /sys/bus/serial-base/devices/serial8250:0 /sys/bus/serial-base/devices/serial8250:0.1 /sys/bus/serial-base/devices/serial8250:0.2 /sys/bus/serial-base/devices/serial8250:0.3 [...] /sys/bus/serial-base/devices/serial8250:0.30 /sys/bus/serial-base/devices/serial8250:0.31 Are "00:00:0", "00:00:0.0", and "serial8250:0" pointing to the same device? Could all 3 device names be used for console=DEVNAME:0.0? Why the "expected" serial8250:0.0 is missing? 5. The code on the printk side seems to be much more complicated than it might be, see below. > The last preferred console added gets used for init. This is documented > in the comments for add_preferred_console(). Now the kernel command line > options for console=ttyS0,115200 console=tty0 are wrongly handled and > cause the /dev/console to be associated with ttyS0 instead of tty0. > > This happens because we are calling __add_preferred_console() later on > from serial8250_isa_init_ports() after console_setup() and the console > gets treated as the last added preferred console. As the DEVNAME:0.0 style > console device is not known at console_setup() time, and we need to call > __add_preferred_console() later. > > Let's fix the issue by reserving a position in console_cmdline for a > deferred console, and then populate the reserved entry before calling > __add_preferred_console(). Honestly, this looks like a hack. The original patchset added: struct console_option { char name[CONSOLE_NAME_MAX]; char opt[CONSOLE_OPT_MAX]; char brl_opt[CONSOLE_BRL_OPT_MAX]; u8 has_brl_opt:1; }; /* Updated only at console_setup() time, no locking needed */ static struct console_option conopt[MAX_CMDLINECONSOLES]; to keep DEVNAME entries from: struct console_cmdline { char name[16]; /* Name of the driver */ int index; /* Minor dev. to use */ bool user_specified; /* Specified by command line vs. platform */ char *options; /* Options for the driver */ #ifdef CONFIG_A11Y_BRAILLE_CONSOLE char *brl_options; /* Options for braille driver */ #endif }; /* * Array of consoles built from command line options (console=) */ static struct console_cmdline console_cmdline[MAX_CMDLINECONSOLES]; Note that the structures include similar elements except that: + @name in console_option is "DEVNAME" style while @name in console_cmdline is "ttyS" style name => confusing it is even more confusing after this patch which added @devname into console_cmdline + @opt vs @options + @brl_opt vs. @brl_options + @has_brl_opt vs. NULL pointer semantic => many inconsistencies => even more confusion + always vs. CONFIG_A11Y_BRAILLE_CONSOLE => inconsistent, prone to even compilation bugs + nothing vs. @user_specified => inconsistent, another complexity, solved hacky way in this patch OK, it made some sense in the original patchset because you wanted to keep DEVNAME aside. But it seems that it just adds complications now. [ Side note ] I even do not line the part in drivers/tty/serial/serial_base_bus.c I guess that: static int serial_base_device_init(struct uart_port *port, [...] return dev_set_name(dev, "%s:%d.%d", dev_name(port->dev), ctrl_id, port_id); is related to int serial_base_add_preferred_console(struct uart_driver *drv, struct uart_port *port) { port_match = kasprintf(GFP_KERNEL, "%s:%i.%i", dev_name(port->dev), port->ctrl_id, port->port_id); The code should create matching strings => ideally, it should be shared. But there is not even comment. And it does not even use the same integer printf format (%d vs. %i) => it can't be grepped easily. IMHO, all these small details together create a maintenance nightmare[*]. [*] "Nightmare" might look like a too strong word. Well, there were added many "features" to the console registration code over the few decades. And it became a real nightmare to understand the "rules" and maintain it. You see it yourself. Any changes in this area easily create regressions. > --- a/kernel/printk/printk.c > +++ b/kernel/printk/printk.c > @@ -2426,6 +2426,24 @@ static void set_user_specified(struct console_cmdline *c, bool user_specified) > console_set_on_cmdline = 1; > } > > +/* Checks if a console is the last user specified preferred console */ > +static bool is_last_user_prefcon(int position) > +{ > + struct console_cmdline *c = console_cmdline; > + int last_user_specified = -1; > + int i; > + > + for (i = 0; i < MAX_CMDLINECONSOLES; i++, c++) { > + if (!c->name[0] && !c->devname[0]) > + break; > + > + if (c->user_specified || c->devname[0]) What is the logic behind this, please? c->user_specified should be set for all entries which were added by console= parameter. It should be set by reserve_deferred_console() for the allocated slot in console_cmdline[]. And if we set the flag there then this function should not be needed. > + last_user_specified = i; > + } > + > + return position == last_user_specified; > +} > + > static int __add_preferred_console(const char *name, const short idx, char *options, > char *brl_options, bool user_specified) > { > @@ -2542,9 +2581,26 @@ static int __init console_setup(char *str) > __setup("console=", console_setup); > > /* Only called from add_preferred_console_match() */ > -int console_opt_add_preferred_console(const char *name, const short idx, > - char *options, char *brl_options) > +int console_opt_add_preferred_console(const char *devname, const char *name, > + const short idx, char *options, > + char *brl_options) > { > + struct console_cmdline *c = console_cmdline; > + int i; > + > + /* Populate a reserved console based on devname */ > + for (i = 0; i < MAX_CMDLINECONSOLES; i++, c++) { > + if (!c->name[0] && !strcmp(c->devname, devname)) { > + strscpy(c->name, name); > + c->index = idx; > + c->options = options; > +#ifdef CONFIG_A11Y_BRAILLE_CONSOLE > + c->brl_options = brl_options; > +#endif > + break; Is there any advantage to have both console_cmdline[] and conopt[] arrays, please? reserve_deferred_console() already reserved the slot in console_cmdline[] by filling c->devname[]. It could have saved also c->options, c->brl_options there. Then we would not need to copy them here. I would prefer to just delete kernel/printk/conopt.c. Instead, we should re-use __add_preferred_console() also for adding entries where "SERIAL_BASE_DEVNAME" is defined instead of "name", "index". Something like (not even compile tested): diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c index 8b746eeb77fa..c69cd7605836 100644 --- a/kernel/printk/printk.c +++ b/kernel/printk/printk.c @@ -2444,18 +2444,23 @@ static bool is_last_user_prefcon(int position) return position == last_user_specified; } -static int __add_preferred_console(const char *name, const short idx, char *options, +static int __add_preferred_console(const char *name, const short idx, + const char *sbase_devname, char *options, char *brl_options, bool user_specified) { struct console_cmdline *c; int i; + if (!name && !sbase_devname) + return -EINVAL; + /* * We use a signed short index for struct console for device drivers to * indicate a not yet assigned index or port. However, a negative index - * value is not valid for preferred console. + * value is not valid when the console name and index are defined on + * the command line. */ - if (idx < 0) + if (name && idx < 0) return -EINVAL; /* @@ -2463,9 +2468,10 @@ static int __add_preferred_console(const char *name, const short idx, char *opti * if we have a slot free. */ for (i = 0, c = console_cmdline; - i < MAX_CMDLINECONSOLES && (c->name[0]); + i < MAX_CMDLINECONSOLES && (c->name[0] || c->sbase_devname[0]); i++, c++) { - if (strcmp(c->name, name) == 0 && c->index == idx) { + if ((name && strcmp(c->name, name) == 0 && c->index == idx) || + (devname && strcmp(c->name, name) == 0)) { if (!brl_options) preferred_console = i; set_user_specified(c, user_specified); @@ -2476,7 +2482,10 @@ static int __add_preferred_console(const char *name, const short idx, char *opti return -E2BIG; if (!brl_options) preferred_console = i; - strscpy(c->name, name, sizeof(c->name)); + if (name) + strscpy(c->name, name, sizeof(c->name)); + if (sbase_devname) + strscpy(c->sbase_devname, sbase_devname, sizeof(c->sbase_devname)); c->options = options; set_user_specified(c, user_specified); braille_set_options(c, brl_options); The function matching sbase_devname in serial_base_bus.c should iterate over entries in console_cmdline and ignore entries with !c->sbase_devname[0]. On success, the function should just set @name to "ttyS" and @index to the matching one. On the contrary, the cycles in register_console() would ignore entries with !c->name[0]. And we are done. There should not be needed any special changes in the logic for preferred console. If the users defines a non-existing console=SBASE_DEVNAME:X.Y then c->name and c->index will never be set. But it is OK. It will be the same as when user defines a non-existing console=blabla on the command line. Best Regards, Petr PS: I am sorry that I did not pay more attention to the original patchset. I was overloaded, had healthy issues, and it was not easy to balance the priorities.
On Tue, Jun 11, 2024 at 10:17:11AM +0200, Petr Mladek wrote: > On Thu 2024-06-06 14:41:46, Tony Lindgren wrote: > > Recent changes to allow using DEVNAME:0.0 style console names caused a > > regression to the kernel command line handling for the console options. > > Sigh, I have missed that it has already ended in the mainline via > the tty tree :-/ > > Honestly, I would prefer to revert it and implement it a clean way. > I see a lot of existing and possible problems: Naturally no objection from me for reverting if there are issues that are unfixable from the printk point of view. I'll update the fix along the lines you're suggesting below though, let's see if that's enough for the -rc cycle. > 1. Where and how is DEVNAME:0.0 defined? > > The only documentation seems to be in > Documentation/admin-guide/kernel-parameters.txt for the console= > parameter: > > <paste> > <DEVNAME>:<n>.<n>[,options] > Use the specified serial port on the serial core bus. > The addressing uses DEVNAME of the physical serial port > device, followed by the serial core controller instance, > and the serial port instance. The options are the same > as documented for the ttyS addressing above. > > The mapping of the serial ports to the tty instances > can be viewed with: > > $ ls -d /sys/bus/serial-base/devices/*:*.*/tty/* > /sys/bus/serial-base/devices/00:04:0.0/tty/ttyS0 > > In the above example, the console can be addressed with > console=00:04:0.0. Note that a console addressed this > way will only get added when the related device driver > is ready. The use of an earlycon parameter in addition to > the console may be desired for console output early on. > </paste> > > This seems to be a brand new bus. > > Is it stable? > Is it documented in Documentation/ABI/stable/? I think the DEVNAME originates from udev to be used for various rules. Yes it should be documented somewhere though. I'd say it's stable as it's been in use for years :) > It seems that the feature will cover "only" serial consoles. > But DEVNAME is a generic name. It might make more sense to > call it "SERIAL_BASE_DEVNAME" or "SERIAL_DEVNAME" or "SBASE_DEVNAME" or so. > > Anyway, console= is an interface with the user space. We will need to > maintain the backward compatibility "forever". > > => we should think twice about the interface !!! I think we want to keep it generic with DEVNAME, I don't see why we'd want make it serial console specific, at least in the code. For the documentation, I think the kernel parameters example is clear on the serial port usage? > 2. On my test system (kvm) I get > > # ls -d -1 /sys/bus/serial-base/devices/*:*.*/tty/* | sort -t. -k 2n > /sys/bus/serial-base/devices/00:00:0.0/tty/ttyS0 > /sys/bus/serial-base/devices/serial8250:0.1/tty/ttyS1 > /sys/bus/serial-base/devices/serial8250:0.2/tty/ttyS2 > /sys/bus/serial-base/devices/serial8250:0.3/tty/ttyS3 > [...] > /sys/bus/serial-base/devices/serial8250:0.30/tty/ttyS30 > /sys/bus/serial-base/devices/serial8250:0.31/tty/ttyS31 > > So, the DEVNAME:X.Y to ttySZ mapping is: > > 00:00:0.0 -> ttyS0 > serial8250:0.1 -> ttyS1 > serial8250:0.2 -> ttyS2 > serial8250:0.3 -> ttyS3 > [...] > serial8250:0.30 -> ttyS30 > serial8250:0.31 -> ttyS31 > > Why is ttyS0 associated with "so ugly" DEVNAME "00:00" > while the rest uses "nice" DEVNAME "serial8250"? Because ACPI serial driver takes over the serial8250 port. Yes we are recycling the preallocated serial8250 ports for the hardware specific drivers.. That always adds some extra confusion on the top :) > I would expect: > > serial8250:0.0 -> ttyS0 > serial8250:0.1 -> ttyS1 > serial8250:0.2 -> ttyS2 > [...] I believe this would be the situation if you make CONFIG_ACPI is not set in the .config. > 3. The delimiter between "DEVNAME" and X.Y numbers is ":". > But ":" is also part of the sample DEVNAME "00:00" > > Is it a good idea? > > Is the current naming scheme a well known historic one > or something invented for the new bus? PCI and USB use a ":" already for the device naming. Also a "-" can be used by the device names, many platform devices do that. I don't think we can pick any better limiter here, at least out of the ":", "-" and "." options. > 4. /sys/bus/serial-base/devices/ contains few more entries: > > # ls -d -1 /sys/bus/serial-base/devices/* | sort -t. -k 2n > /sys/bus/serial-base/devices/00:00:0 > /sys/bus/serial-base/devices/00:00:0.0 > /sys/bus/serial-base/devices/serial8250:0 > /sys/bus/serial-base/devices/serial8250:0.1 > /sys/bus/serial-base/devices/serial8250:0.2 > /sys/bus/serial-base/devices/serial8250:0.3 > [...] > /sys/bus/serial-base/devices/serial8250:0.30 > /sys/bus/serial-base/devices/serial8250:0.31 > > > Are "00:00:0", "00:00:0.0", and "serial8250:0" pointing to > the same device? Not pointing to the same device, we now have serial port controller devices. Each serial port controller device may have one or more serial port devices. So in your example above, only "00:00:0.0" is a serial port device, the others are serial port controller devices. So above you have these: - 00:00:0 is the first ACPI 8250 serial controller device - 00:00:0.0 is the first serial port instance for controller 00:00:0 above - serial8250:0 is the first serial8250 controller instance On startup (and if ACPI is disabled) ttyS0 maps to the first serial8250 controler port instance as mapped in arch/x86/include/asm/serial.h. Then the ACPI using serial driver (or whatever hardware specific driver) may take over the serial8250 port instance. > Could all 3 device names be used for console=DEVNAME:0.0? No > Why the "expected" serial8250:0.0 is missing? Because the serial8250 port instance got taken over by the ACPI using hardware specific driver :) > 5. The code on the printk side seems to be much more complicated > than it might be, see below. OK thats good to hear. > > The last preferred console added gets used for init. This is documented > > in the comments for add_preferred_console(). Now the kernel command line > > options for console=ttyS0,115200 console=tty0 are wrongly handled and > > cause the /dev/console to be associated with ttyS0 instead of tty0. > > > > This happens because we are calling __add_preferred_console() later on > > from serial8250_isa_init_ports() after console_setup() and the console > > gets treated as the last added preferred console. As the DEVNAME:0.0 style > > console device is not known at console_setup() time, and we need to call > > __add_preferred_console() later. > > > > Let's fix the issue by reserving a position in console_cmdline for a > > deferred console, and then populate the reserved entry before calling > > __add_preferred_console(). > > Honestly, this looks like a hack. > > The original patchset added: > > struct console_option { > char name[CONSOLE_NAME_MAX]; > char opt[CONSOLE_OPT_MAX]; > char brl_opt[CONSOLE_BRL_OPT_MAX]; > u8 has_brl_opt:1; > }; > > /* Updated only at console_setup() time, no locking needed */ > static struct console_option conopt[MAX_CMDLINECONSOLES]; > > > to keep DEVNAME entries from: > > struct console_cmdline > { > char name[16]; /* Name of the driver */ > int index; /* Minor dev. to use */ > bool user_specified; /* Specified by command line vs. platform */ > char *options; /* Options for the driver */ > #ifdef CONFIG_A11Y_BRAILLE_CONSOLE > char *brl_options; /* Options for braille driver */ > #endif > }; > > /* > * Array of consoles built from command line options (console=) > */ > static struct console_cmdline console_cmdline[MAX_CMDLINECONSOLES]; > > Note that the structures include similar elements except that: > > + @name in console_option is "DEVNAME" style while > @name in console_cmdline is "ttyS" style name > > => confusing > > it is even more confusing after this patch which added > @devname into console_cmdline > > > + @opt vs @options > + @brl_opt vs. @brl_options > + @has_brl_opt vs. NULL pointer semantic > > => many inconsistencies => even more confusion OK makes sense to drop the struct console_option now in favor of struct console_cmdline. Will update accordingly. > + always vs. CONFIG_A11Y_BRAILLE_CONSOLE > > => inconsistent, prone to even compilation bugs Maybe we should just drop the ifdef in struct console_cmdline.h to avoid the ifdef. > + nothing vs. @user_specified > > => inconsistent, another complexity, solved hacky way in this patch > > > OK, it made some sense in the original patchset because you wanted > to keep DEVNAME aside. > > But it seems that it just adds complications now. Seems these will go away by dropping the struct console_option. > [ Side note ] > > I even do not line the part in drivers/tty/serial/serial_base_bus.c > I guess that: > > static int serial_base_device_init(struct uart_port *port, > [...] > return dev_set_name(dev, "%s:%d.%d", dev_name(port->dev), > ctrl_id, port_id); > > > is related to > > int serial_base_add_preferred_console(struct uart_driver *drv, > struct uart_port *port) > { > port_match = kasprintf(GFP_KERNEL, "%s:%i.%i", dev_name(port->dev), > port->ctrl_id, port->port_id); > > > The code should create matching strings => ideally, it should be > shared. But there is not even comment. And it does not even use > the same integer printf format (%d vs. %i) => it can't be > grepped easily. OK that should be unified. > IMHO, all these small details together create a maintenance nightmare[*]. > > [*] "Nightmare" might look like a too strong word. Well, there > were added many "features" to the console registration > code over the few decades. And it became a real nightmare > to understand the "rules" and maintain it. You see it yourself. > Any changes in this area easily create regressions. Yes I've seen parts of that nightmare :) Never expected this to spread into printk in the first place.. > > --- a/kernel/printk/printk.c > > +++ b/kernel/printk/printk.c > > @@ -2426,6 +2426,24 @@ static void set_user_specified(struct console_cmdline *c, bool user_specified) > > console_set_on_cmdline = 1; > > } > > > > +/* Checks if a console is the last user specified preferred console */ > > +static bool is_last_user_prefcon(int position) > > +{ > > + struct console_cmdline *c = console_cmdline; > > + int last_user_specified = -1; > > + int i; > > + > > + for (i = 0; i < MAX_CMDLINECONSOLES; i++, c++) { > > + if (!c->name[0] && !c->devname[0]) > > + break; > > + > > + if (c->user_specified || c->devname[0]) > > What is the logic behind this, please? > > c->user_specified should be set for all entries which were > added by console= parameter. It should be set by > reserve_deferred_console() for the allocated slot > in console_cmdline[]. And if we set the flag there > then this function should not be needed. OK makes sense. > > + last_user_specified = i; > > + } > > + > > + return position == last_user_specified; > > +} > > + > > static int __add_preferred_console(const char *name, const short idx, char *options, > > char *brl_options, bool user_specified) > > { > > @@ -2542,9 +2581,26 @@ static int __init console_setup(char *str) > > __setup("console=", console_setup); > > > > /* Only called from add_preferred_console_match() */ > > -int console_opt_add_preferred_console(const char *name, const short idx, > > - char *options, char *brl_options) > > +int console_opt_add_preferred_console(const char *devname, const char *name, > > + const short idx, char *options, > > + char *brl_options) > > { > > + struct console_cmdline *c = console_cmdline; > > + int i; > > + > > + /* Populate a reserved console based on devname */ > > + for (i = 0; i < MAX_CMDLINECONSOLES; i++, c++) { > > + if (!c->name[0] && !strcmp(c->devname, devname)) { > > + strscpy(c->name, name); > > + c->index = idx; > > + c->options = options; > > +#ifdef CONFIG_A11Y_BRAILLE_CONSOLE > > + c->brl_options = brl_options; > > +#endif > > + break; > > Is there any advantage to have both console_cmdline[] and > conopt[] arrays, please? No need for conopt[] if we can use console_cmdline[]. > reserve_deferred_console() already reserved the slot in > console_cmdline[] by filling c->devname[]. It could have > saved also c->options, c->brl_options there. Then we would not need > to copy them here. > > I would prefer to just delete kernel/printk/conopt.c. Yes no need for it if things are handled directly in printk.c. > Instead, we should re-use __add_preferred_console() also for adding > entries where "SERIAL_BASE_DEVNAME" is defined instead of > "name", "index". > > Something like (not even compile tested): > > diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c > index 8b746eeb77fa..c69cd7605836 100644 > --- a/kernel/printk/printk.c > +++ b/kernel/printk/printk.c > @@ -2444,18 +2444,23 @@ static bool is_last_user_prefcon(int position) > return position == last_user_specified; > } > > -static int __add_preferred_console(const char *name, const short idx, char *options, > +static int __add_preferred_console(const char *name, const short idx, > + const char *sbase_devname, char *options, > char *brl_options, bool user_specified) > { > struct console_cmdline *c; > int i; > > + if (!name && !sbase_devname) > + return -EINVAL; > + > /* > * We use a signed short index for struct console for device drivers to > * indicate a not yet assigned index or port. However, a negative index > - * value is not valid for preferred console. > + * value is not valid when the console name and index are defined on > + * the command line. > */ > - if (idx < 0) > + if (name && idx < 0) > return -EINVAL; > > /* > @@ -2463,9 +2468,10 @@ static int __add_preferred_console(const char *name, const short idx, char *opti > * if we have a slot free. > */ > for (i = 0, c = console_cmdline; > - i < MAX_CMDLINECONSOLES && (c->name[0]); > + i < MAX_CMDLINECONSOLES && (c->name[0] || c->sbase_devname[0]); > i++, c++) { > - if (strcmp(c->name, name) == 0 && c->index == idx) { > + if ((name && strcmp(c->name, name) == 0 && c->index == idx) || > + (devname && strcmp(c->name, name) == 0)) { > if (!brl_options) > preferred_console = i; > set_user_specified(c, user_specified); > @@ -2476,7 +2482,10 @@ static int __add_preferred_console(const char *name, const short idx, char *opti > return -E2BIG; > if (!brl_options) > preferred_console = i; > - strscpy(c->name, name, sizeof(c->name)); > + if (name) > + strscpy(c->name, name, sizeof(c->name)); > + if (sbase_devname) > + strscpy(c->sbase_devname, sbase_devname, sizeof(c->sbase_devname)); > c->options = options; > set_user_specified(c, user_specified); > braille_set_options(c, brl_options); > > > The function matching sbase_devname in serial_base_bus.c should > iterate over entries in console_cmdline and ignore entries > with !c->sbase_devname[0]. > > On success, the function should just set @name to "ttyS" and > @index to the matching one. > > On the contrary, the cycles in register_console() would ignore > entries with !c->name[0]. > > And we are done. There should not be needed any special changes in > the logic for preferred console. OK sounds good to me. > If the users defines a non-existing console=SBASE_DEVNAME:X.Y then > c->name and c->index will never be set. But it is OK. It will be > the same as when user defines a non-existing console=blabla > on the command line. OK. I think we should keep it as DEVNAME though as it should not be limited to serial consoles. > PS: I am sorry that I did not pay more attention to the original > patchset. I was overloaded, had healthy issues, and it was > not easy to balance the priorities. No worries, I was wondering what happened after you initial comments. Good to hear your're able to look at these changes now :) Regards, Tony
On Tue, Jun 11, 2024 at 04:10:45PM +0300, Andy Shevchenko wrote: > On Tue, Jun 11, 2024 at 12:56:54PM +0300, Tony Lindgren wrote: > > On Tue, Jun 11, 2024 at 10:17:11AM +0200, Petr Mladek wrote: > > > I would expect: > > > > > > serial8250:0.0 -> ttyS0 > > > serial8250:0.1 -> ttyS1 > > > serial8250:0.2 -> ttyS2 > > > [...] > > > > I believe this would be the situation if you make CONFIG_ACPI is not set > > in the .config. > > This is interesting. So, we still have the DEVNAME differences depending on > the kernel configuration? OTOH, I can't imagine either how we can get the > device names more stable as long as some drivers instantiate the platform > device instead of using 8250 core parts (port, base, core) directly. Seems we can add translation as needed though. For example, we could add console=serial8250:0.0 translation to the PNP port. Or console=0x3f8,115200. Regards, Tony
diff --git a/kernel/printk/conopt.c b/kernel/printk/conopt.c index 9d507bac3657..08dded8fbb3b 100644 --- a/kernel/printk/conopt.c +++ b/kernel/printk/conopt.c @@ -140,7 +140,7 @@ int add_preferred_console_match(const char *match, const char *name, if (con->has_brl_opt) brl_opt = con->brl_opt; - console_opt_add_preferred_console(name, idx, con->opt, brl_opt); + console_opt_add_preferred_console(match, name, idx, con->opt, brl_opt); return 0; } diff --git a/kernel/printk/console_cmdline.h b/kernel/printk/console_cmdline.h index a125e0235589..e22021e3e28b 100644 --- a/kernel/printk/console_cmdline.h +++ b/kernel/printk/console_cmdline.h @@ -5,13 +5,15 @@ #define MAX_CMDLINECONSOLES 8 int console_opt_save(const char *str, const char *brl_opt); -int console_opt_add_preferred_console(const char *name, const short idx, - char *options, char *brl_options); +int console_opt_add_preferred_console(const char *devname, const char *name, + const short idx, char *options, + char *brl_options); struct console_cmdline { char name[16]; /* Name of the driver */ int index; /* Minor dev. to use */ + char devname[32]; /* DEVNAME:0.0 style device name */ bool user_specified; /* Specified by command line vs. platform */ char *options; /* Options for the driver */ #ifdef CONFIG_A11Y_BRAILLE_CONSOLE diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c index 420fd310129d..52bdd7dcdb6f 100644 --- a/kernel/printk/printk.c +++ b/kernel/printk/printk.c @@ -2426,6 +2426,24 @@ static void set_user_specified(struct console_cmdline *c, bool user_specified) console_set_on_cmdline = 1; } +/* Checks if a console is the last user specified preferred console */ +static bool is_last_user_prefcon(int position) +{ + struct console_cmdline *c = console_cmdline; + int last_user_specified = -1; + int i; + + for (i = 0; i < MAX_CMDLINECONSOLES; i++, c++) { + if (!c->name[0] && !c->devname[0]) + break; + + if (c->user_specified || c->devname[0]) + last_user_specified = i; + } + + return position == last_user_specified; +} + static int __add_preferred_console(const char *name, const short idx, char *options, char *brl_options, bool user_specified) { @@ -2445,10 +2463,10 @@ static int __add_preferred_console(const char *name, const short idx, char *opti * if we have a slot free. */ for (i = 0, c = console_cmdline; - i < MAX_CMDLINECONSOLES && c->name[0]; + i < MAX_CMDLINECONSOLES && (c->name[0] || c->devname[0]); i++, c++) { if (strcmp(c->name, name) == 0 && c->index == idx) { - if (!brl_options) + if (!brl_options && (!user_specified || is_last_user_prefcon(i))) preferred_console = i; set_user_specified(c, user_specified); return 0; @@ -2467,6 +2485,25 @@ static int __add_preferred_console(const char *name, const short idx, char *opti return 0; } +/* Reserves a console_cmdline position for a deferred devname console */ +static void reserve_deferred_console(const char *str) +{ + struct console_cmdline *c = console_cmdline; + size_t namelen; + int i; + + namelen = strcspn(str, ","); + if (namelen == 0 || namelen >= sizeof(c->devname)) + return; + + for (i = 0; i < MAX_CMDLINECONSOLES; i++, c++) { + if (c->name[0] || c->devname[0]) + continue; + strscpy(c->devname, str, namelen + 1); + return; + } +} + static int __init console_msg_format_setup(char *str) { if (!strcmp(str, "syslog")) @@ -2508,8 +2545,10 @@ static int __init console_setup(char *str) console_set_on_cmdline = 1; /* Don't attempt to parse a DEVNAME:0.0 style console */ - if (strchr(str, ':')) + if (strchr(str, ':')) { + reserve_deferred_console(str); return 1; + } /* * Decode str into name, index, options. @@ -2542,9 +2581,26 @@ static int __init console_setup(char *str) __setup("console=", console_setup); /* Only called from add_preferred_console_match() */ -int console_opt_add_preferred_console(const char *name, const short idx, - char *options, char *brl_options) +int console_opt_add_preferred_console(const char *devname, const char *name, + const short idx, char *options, + char *brl_options) { + struct console_cmdline *c = console_cmdline; + int i; + + /* Populate a reserved console based on devname */ + for (i = 0; i < MAX_CMDLINECONSOLES; i++, c++) { + if (!c->name[0] && !strcmp(c->devname, devname)) { + strscpy(c->name, name); + c->index = idx; + c->options = options; +#ifdef CONFIG_A11Y_BRAILLE_CONSOLE + c->brl_options = brl_options; +#endif + break; + } + } + return __add_preferred_console(name, idx, options, brl_options, true); } @@ -3333,7 +3389,7 @@ static int try_enable_preferred_console(struct console *newcon, int i, err; for (i = 0, c = console_cmdline; - i < MAX_CMDLINECONSOLES && c->name[0]; + i < MAX_CMDLINECONSOLES && (c->name[0] || c->devname[0]); i++, c++) { if (c->user_specified != user_specified) continue;
Recent changes to allow using DEVNAME:0.0 style console names caused a regression to the kernel command line handling for the console options. The last preferred console added gets used for init. This is documented in the comments for add_preferred_console(). Now the kernel command line options for console=ttyS0,115200 console=tty0 are wrongly handled and cause the /dev/console to be associated with ttyS0 instead of tty0. This happens because we are calling __add_preferred_console() later on from serial8250_isa_init_ports() after console_setup() and the console gets treated as the last added preferred console. As the DEVNAME:0.0 style console device is not known at console_setup() time, and we need to call __add_preferred_console() later. Let's fix the issue by reserving a position in console_cmdline for a deferred console, and then populate the reserved entry before calling __add_preferred_console(). Note that we now need to check for valid preferred console too in __add_preferred_console(). Reported-by: Petr Mladek <pmladek@suse.com> Link: https://lore.kernel.org/linux-serial/ZlC6_Um4P4b-_WQE@pathway.suse.cz/ Fixes: f03e8c1060f8 ("printk: Save console options for add_preferred_console_match()") Signed-off-by: Tony Lindgren <tony.lindgren@linux.intel.com> --- kernel/printk/conopt.c | 2 +- kernel/printk/console_cmdline.h | 6 ++- kernel/printk/printk.c | 68 ++++++++++++++++++++++++++++++--- 3 files changed, 67 insertions(+), 9 deletions(-)