| Message ID | 20230719051525.46494-1-tony@atomide.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | serial: core: Add support for dev_name:0.0 naming for kernel console | expand |
On 19. 07. 23, 7:15, Tony Lindgren wrote: > With the serial core controller related changes we can now start > addressing serial ports with dev_name:0.0 naming. The names are something > like 00:04.0:0.0 on qemu, and 2800000.serial.0:0.0 on ARM for example. > > The dev_name is unique serial port hardware controller device name, also > known as port->dev, and 0.0 are the serial core controller id and port id. > > Typically 0.0 are used for each controller and port instance unless the > serial port hardware controller has multiple controllers or ports. > > Signed-off-by: Tony Lindgren <tony@atomide.com> > --- > > Note that this depends on fix for serial core port ids patch > "[PATCH] serial: core: Fix serial core port id to not use port->line" > > --- > drivers/tty/serial/serial_core.c | 47 ++++++++++++++++++++++++++++++++ > 1 file changed, 47 insertions(+) > > diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c > --- a/drivers/tty/serial/serial_core.c > +++ b/drivers/tty/serial/serial_core.c > @@ -3322,6 +3322,49 @@ static int serial_core_port_device_add(struct serial_ctrl_device *ctrl_dev, > return 0; > } > > +/* > + * Add preferred console if configured on kernel command line with naming > + * "console=dev_name:0.0". > + */ > +static int serial_core_add_preferred_console(struct uart_driver *drv, > + struct uart_port *port) > +{ > + char *port_match, *opt, *name; > + int len, ret = 0; > + > + port_match = kasprintf(GFP_KERNEL, "console=%s:%i.%i", > + dev_name(port->dev), port->ctrl_id, > + port->port_id); > + if (!port_match) > + return -ENOMEM; > + > + opt = strstr(saved_command_line, port_match); > + if (!opt) > + goto free_port_match; > + > + len = strlen(port_match); > + > + if (strlen(opt) > len + 1 && opt[len] == ',') > + opt += len + 1; > + else > + opt = NULL; > + > + name = kstrdup(drv->dev_name, GFP_KERNEL); Why do you dup the name here? > + if (!name) { > + ret = -ENOMEM; > + goto free_port_match; > + } > + > + add_preferred_console(name, port->line, opt); > + > + kfree(name); thanks,
* Jiri Slaby <jirislaby@kernel.org> [230719 05:25]: > On 19. 07. 23, 7:15, Tony Lindgren wrote: > > +/* > > + * Add preferred console if configured on kernel command line with naming > > + * "console=dev_name:0.0". > > + */ > > +static int serial_core_add_preferred_console(struct uart_driver *drv, > > + struct uart_port *port) > > +{ > > + char *port_match, *opt, *name; > > + int len, ret = 0; > > + > > + port_match = kasprintf(GFP_KERNEL, "console=%s:%i.%i", > > + dev_name(port->dev), port->ctrl_id, > > + port->port_id); > > + if (!port_match) > > + return -ENOMEM; > > + > > + opt = strstr(saved_command_line, port_match); > > + if (!opt) > > + goto free_port_match; > > + > > + len = strlen(port_match); > > + > > + if (strlen(opt) > len + 1 && opt[len] == ',') > > + opt += len + 1; > > + else > > + opt = NULL; > > + > > + name = kstrdup(drv->dev_name, GFP_KERNEL); > > Why do you dup the name here? I was getting ignoring const warning, but maybe the right solution is to just use const char *name here.. Let me check. Regards, Tony
On 19. 07. 23, 7:28, Tony Lindgren wrote: > * Jiri Slaby <jirislaby@kernel.org> [230719 05:25]: >> On 19. 07. 23, 7:15, Tony Lindgren wrote: >>> +/* >>> + * Add preferred console if configured on kernel command line with naming >>> + * "console=dev_name:0.0". >>> + */ >>> +static int serial_core_add_preferred_console(struct uart_driver *drv, >>> + struct uart_port *port) >>> +{ >>> + char *port_match, *opt, *name; >>> + int len, ret = 0; >>> + >>> + port_match = kasprintf(GFP_KERNEL, "console=%s:%i.%i", >>> + dev_name(port->dev), port->ctrl_id, >>> + port->port_id); >>> + if (!port_match) >>> + return -ENOMEM; >>> + >>> + opt = strstr(saved_command_line, port_match); >>> + if (!opt) >>> + goto free_port_match; >>> + >>> + len = strlen(port_match); >>> + >>> + if (strlen(opt) > len + 1 && opt[len] == ',') >>> + opt += len + 1; >>> + else >>> + opt = NULL; >>> + >>> + name = kstrdup(drv->dev_name, GFP_KERNEL); >> >> Why do you dup the name here? > > I was getting ignoring const warning, but maybe the right solution is > to just use const char *name here.. Let me check. So fix add_preferred_console() instead ;). thanks,
* Jiri Slaby <jirislaby@kernel.org> [230719 05:29]: > On 19. 07. 23, 7:28, Tony Lindgren wrote: > > * Jiri Slaby <jirislaby@kernel.org> [230719 05:25]: > > > On 19. 07. 23, 7:15, Tony Lindgren wrote: > > > > +/* > > > > + * Add preferred console if configured on kernel command line with naming > > > > + * "console=dev_name:0.0". > > > > + */ > > > > +static int serial_core_add_preferred_console(struct uart_driver *drv, > > > > + struct uart_port *port) > > > > +{ > > > > + char *port_match, *opt, *name; > > > > + int len, ret = 0; > > > > + > > > > + port_match = kasprintf(GFP_KERNEL, "console=%s:%i.%i", > > > > + dev_name(port->dev), port->ctrl_id, > > > > + port->port_id); > > > > + if (!port_match) > > > > + return -ENOMEM; > > > > + > > > > + opt = strstr(saved_command_line, port_match); > > > > + if (!opt) > > > > + goto free_port_match; > > > > + > > > > + len = strlen(port_match); > > > > + > > > > + if (strlen(opt) > len + 1 && opt[len] == ',') > > > > + opt += len + 1; > > > > + else > > > > + opt = NULL; > > > > + > > > > + name = kstrdup(drv->dev_name, GFP_KERNEL); > > > > > > Why do you dup the name here? > > > > I was getting ignoring const warning, but maybe the right solution is > > to just use const char *name here.. Let me check. > > So fix add_preferred_console() instead ;). Let's see what kind of trouble changing it to use const char *name might be. Tony
On 19. 07. 23, 7:32, Tony Lindgren wrote: > * Jiri Slaby <jirislaby@kernel.org> [230719 05:29]: >> On 19. 07. 23, 7:28, Tony Lindgren wrote: >>> * Jiri Slaby <jirislaby@kernel.org> [230719 05:25]: >>>> On 19. 07. 23, 7:15, Tony Lindgren wrote: >>>>> +/* >>>>> + * Add preferred console if configured on kernel command line with naming >>>>> + * "console=dev_name:0.0". >>>>> + */ >>>>> +static int serial_core_add_preferred_console(struct uart_driver *drv, >>>>> + struct uart_port *port) >>>>> +{ >>>>> + char *port_match, *opt, *name; >>>>> + int len, ret = 0; >>>>> + >>>>> + port_match = kasprintf(GFP_KERNEL, "console=%s:%i.%i", >>>>> + dev_name(port->dev), port->ctrl_id, >>>>> + port->port_id); >>>>> + if (!port_match) >>>>> + return -ENOMEM; >>>>> + >>>>> + opt = strstr(saved_command_line, port_match); >>>>> + if (!opt) >>>>> + goto free_port_match; >>>>> + >>>>> + len = strlen(port_match); >>>>> + >>>>> + if (strlen(opt) > len + 1 && opt[len] == ',') >>>>> + opt += len + 1; >>>>> + else >>>>> + opt = NULL; >>>>> + >>>>> + name = kstrdup(drv->dev_name, GFP_KERNEL); >>>> >>>> Why do you dup the name here? >>> >>> I was getting ignoring const warning, but maybe the right solution is >>> to just use const char *name here.. Let me check. >> >> So fix add_preferred_console() instead ;). > > Let's see what kind of trouble changing it to use const char *name > might be. I don't see any, the string is copied internally. So it should be straightforward. Actually all three parameters of __add_preferred_console() should be const, IMO. But that involves changing struct console_cmdline. But that should be straightforward too. Aside from that, why do you parse saved_command_line on your own? Instead of using setup() or other commonly used mechanisms for command line handling? thanks,
On Wed, Jul 19, 2023 at 08:15:23AM +0300, Tony Lindgren wrote: > With the serial core controller related changes we can now start > addressing serial ports with dev_name:0.0 naming. The names are something > like 00:04.0:0.0 on qemu, and 2800000.serial.0:0.0 on ARM for example. > > The dev_name is unique serial port hardware controller device name, also Maybe for the sake of consistency you may use DEVNAME here and everywhere else to link this to the DEVNAME uevent environment variable? > known as port->dev, and 0.0 are the serial core controller id and port id. > > Typically 0.0 are used for each controller and port instance unless the > serial port hardware controller has multiple controllers or ports.
* Jiri Slaby <jirislaby@kernel.org> [230719 05:36]: > On 19. 07. 23, 7:32, Tony Lindgren wrote: > > * Jiri Slaby <jirislaby@kernel.org> [230719 05:29]: > > > On 19. 07. 23, 7:28, Tony Lindgren wrote: > > > > * Jiri Slaby <jirislaby@kernel.org> [230719 05:25]: > > > > > On 19. 07. 23, 7:15, Tony Lindgren wrote: > > > > > > +/* > > > > > > + * Add preferred console if configured on kernel command line with naming > > > > > > + * "console=dev_name:0.0". > > > > > > + */ > > > > > > +static int serial_core_add_preferred_console(struct uart_driver *drv, > > > > > > + struct uart_port *port) > > > > > > +{ > > > > > > + char *port_match, *opt, *name; > > > > > > + int len, ret = 0; > > > > > > + > > > > > > + port_match = kasprintf(GFP_KERNEL, "console=%s:%i.%i", > > > > > > + dev_name(port->dev), port->ctrl_id, > > > > > > + port->port_id); > > > > > > + if (!port_match) > > > > > > + return -ENOMEM; > > > > > > + > > > > > > + opt = strstr(saved_command_line, port_match); > > > > > > + if (!opt) > > > > > > + goto free_port_match; > > > > > > + > > > > > > + len = strlen(port_match); > > > > > > + > > > > > > + if (strlen(opt) > len + 1 && opt[len] == ',') > > > > > > + opt += len + 1; > > > > > > + else > > > > > > + opt = NULL; > > > > > > + > > > > > > + name = kstrdup(drv->dev_name, GFP_KERNEL); > > > > > > > > > > Why do you dup the name here? > > > > > > > > I was getting ignoring const warning, but maybe the right solution is > > > > to just use const char *name here.. Let me check. > > > > > > So fix add_preferred_console() instead ;). > > > > Let's see what kind of trouble changing it to use const char *name > > might be. > > I don't see any, the string is copied internally. So it should be > straightforward. Actually all three parameters of __add_preferred_console() > should be const, IMO. But that involves changing struct console_cmdline. But > that should be straightforward too. Yeah I'll send a patch for that. > Aside from that, why do you parse saved_command_line on your own? Instead of > using setup() or other commonly used mechanisms for command line handling? Hmm that might be easier yeah :) Regards, Tony
Hi Tony, kernel test robot noticed the following build errors: [auto build test ERROR on tty/tty-testing] [also build test ERROR on tty/tty-next linus/master v6.5-rc2 next-20230719] [cannot apply to tty/tty-linus usb/usb-testing usb/usb-next usb/usb-linus] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch#_base_tree_information] url: https://github.com/intel-lab-lkp/linux/commits/Tony-Lindgren/serial-core-Add-support-for-dev_name-0-0-naming-for-kernel-console/20230719-131657 base: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/tty.git tty-testing patch link: https://lore.kernel.org/r/20230719051525.46494-1-tony%40atomide.com patch subject: [PATCH] serial: core: Add support for dev_name:0.0 naming for kernel console config: x86_64-randconfig-r013-20230718 (https://download.01.org/0day-ci/archive/20230719/202307192334.nrgSDnfu-lkp@intel.com/config) compiler: clang version 15.0.7 (https://github.com/llvm/llvm-project.git 8dfdcc7b7bf66834a761bd8de445840ef68e4d1a) reproduce: (https://download.01.org/0day-ci/archive/20230719/202307192334.nrgSDnfu-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> | Closes: https://lore.kernel.org/oe-kbuild-all/202307192334.nrgSDnfu-lkp@intel.com/ All errors (new ones prefixed by >>): >> drivers/tty/serial/serial_core.c:3337:17: error: no member named 'port_id' in 'struct uart_port' port->port_id); ~~~~ ^ 1 error generated. vim +3337 drivers/tty/serial/serial_core.c 3324 3325 /* 3326 * Add preferred console if configured on kernel command line with naming 3327 * "console=dev_name:0.0". 3328 */ 3329 static int serial_core_add_preferred_console(struct uart_driver *drv, 3330 struct uart_port *port) 3331 { 3332 char *port_match, *opt, *name; 3333 int len, ret = 0; 3334 3335 port_match = kasprintf(GFP_KERNEL, "console=%s:%i.%i", 3336 dev_name(port->dev), port->ctrl_id, > 3337 port->port_id); 3338 if (!port_match) 3339 return -ENOMEM; 3340 3341 opt = strstr(saved_command_line, port_match); 3342 if (!opt) 3343 goto free_port_match; 3344 3345 len = strlen(port_match); 3346 3347 if (strlen(opt) > len + 1 && opt[len] == ',') 3348 opt += len + 1; 3349 else 3350 opt = NULL; 3351 3352 name = kstrdup(drv->dev_name, GFP_KERNEL); 3353 if (!name) { 3354 ret = -ENOMEM; 3355 goto free_port_match; 3356 } 3357 3358 add_preferred_console(name, port->line, opt); 3359 3360 kfree(name); 3361 3362 free_port_match: 3363 kfree(port_match); 3364 3365 return ret; 3366 } 3367
Hi Tony, kernel test robot noticed the following build errors: [auto build test ERROR on tty/tty-testing] [also build test ERROR on tty/tty-next tty/tty-linus usb/usb-testing usb/usb-next usb/usb-linus linus/master v6.5-rc2 next-20230719] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch#_base_tree_information] url: https://github.com/intel-lab-lkp/linux/commits/Tony-Lindgren/serial-core-Add-support-for-dev_name-0-0-naming-for-kernel-console/20230719-131657 base: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/tty.git tty-testing patch link: https://lore.kernel.org/r/20230719051525.46494-1-tony%40atomide.com patch subject: [PATCH] serial: core: Add support for dev_name:0.0 naming for kernel console config: x86_64-defconfig (https://download.01.org/0day-ci/archive/20230720/202307200137.Wk1s5BY7-lkp@intel.com/config) compiler: gcc-12 (Debian 12.2.0-14) 12.2.0 reproduce: (https://download.01.org/0day-ci/archive/20230720/202307200137.Wk1s5BY7-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> | Closes: https://lore.kernel.org/oe-kbuild-all/202307200137.Wk1s5BY7-lkp@intel.com/ All errors (new ones prefixed by >>): drivers/tty/serial/serial_core.c: In function 'serial_core_add_preferred_console': >> drivers/tty/serial/serial_core.c:3337:36: error: 'struct uart_port' has no member named 'port_id' 3337 | port->port_id); | ^~ vim +3337 drivers/tty/serial/serial_core.c 3324 3325 /* 3326 * Add preferred console if configured on kernel command line with naming 3327 * "console=dev_name:0.0". 3328 */ 3329 static int serial_core_add_preferred_console(struct uart_driver *drv, 3330 struct uart_port *port) 3331 { 3332 char *port_match, *opt, *name; 3333 int len, ret = 0; 3334 3335 port_match = kasprintf(GFP_KERNEL, "console=%s:%i.%i", 3336 dev_name(port->dev), port->ctrl_id, > 3337 port->port_id); 3338 if (!port_match) 3339 return -ENOMEM; 3340 3341 opt = strstr(saved_command_line, port_match); 3342 if (!opt) 3343 goto free_port_match; 3344 3345 len = strlen(port_match); 3346 3347 if (strlen(opt) > len + 1 && opt[len] == ',') 3348 opt += len + 1; 3349 else 3350 opt = NULL; 3351 3352 name = kstrdup(drv->dev_name, GFP_KERNEL); 3353 if (!name) { 3354 ret = -ENOMEM; 3355 goto free_port_match; 3356 } 3357 3358 add_preferred_console(name, port->line, opt); 3359 3360 kfree(name); 3361 3362 free_port_match: 3363 kfree(port_match); 3364 3365 return ret; 3366 } 3367
* Andy Shevchenko <andriy.shevchenko@intel.com> [230719 05:37]: > On Wed, Jul 19, 2023 at 08:15:23AM +0300, Tony Lindgren wrote: > > With the serial core controller related changes we can now start > > addressing serial ports with dev_name:0.0 naming. The names are something > > like 00:04.0:0.0 on qemu, and 2800000.serial.0:0.0 on ARM for example. > > > > The dev_name is unique serial port hardware controller device name, also > > Maybe for the sake of consistency you may use DEVNAME here and everywhere else > to link this to the DEVNAME uevent environment variable? Yes good idea will do. Regards, Tony
diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c --- a/drivers/tty/serial/serial_core.c +++ b/drivers/tty/serial/serial_core.c @@ -3322,6 +3322,49 @@ static int serial_core_port_device_add(struct serial_ctrl_device *ctrl_dev, return 0; } +/* + * Add preferred console if configured on kernel command line with naming + * "console=dev_name:0.0". + */ +static int serial_core_add_preferred_console(struct uart_driver *drv, + struct uart_port *port) +{ + char *port_match, *opt, *name; + int len, ret = 0; + + port_match = kasprintf(GFP_KERNEL, "console=%s:%i.%i", + dev_name(port->dev), port->ctrl_id, + port->port_id); + if (!port_match) + return -ENOMEM; + + opt = strstr(saved_command_line, port_match); + if (!opt) + goto free_port_match; + + len = strlen(port_match); + + if (strlen(opt) > len + 1 && opt[len] == ',') + opt += len + 1; + else + opt = NULL; + + name = kstrdup(drv->dev_name, GFP_KERNEL); + if (!name) { + ret = -ENOMEM; + goto free_port_match; + } + + add_preferred_console(name, port->line, opt); + + kfree(name); + +free_port_match: + kfree(port_match); + + return ret; +} + /* * Initialize a serial core port device, and a controller device if needed. */ @@ -3358,6 +3401,10 @@ int serial_core_register_port(struct uart_driver *drv, struct uart_port *port) if (ret) goto err_unregister_ctrl_dev; + ret = serial_core_add_preferred_console(drv, port); + if (ret) + goto err_unregister_port_dev; + ret = serial_core_add_one_port(drv, port); if (ret) goto err_unregister_port_dev;
With the serial core controller related changes we can now start addressing serial ports with dev_name:0.0 naming. The names are something like 00:04.0:0.0 on qemu, and 2800000.serial.0:0.0 on ARM for example. The dev_name is unique serial port hardware controller device name, also known as port->dev, and 0.0 are the serial core controller id and port id. Typically 0.0 are used for each controller and port instance unless the serial port hardware controller has multiple controllers or ports. Signed-off-by: Tony Lindgren <tony@atomide.com> --- Note that this depends on fix for serial core port ids patch "[PATCH] serial: core: Fix serial core port id to not use port->line" --- drivers/tty/serial/serial_core.c | 47 ++++++++++++++++++++++++++++++++ 1 file changed, 47 insertions(+)