diff mbox series

serial: core: Add support for dev_name:0.0 naming for kernel console

Message ID 20230719051525.46494-1-tony@atomide.com
State New
Headers
Series serial: core: Add support for dev_name:0.0 naming for kernel console |

Commit Message

Tony Lindgren July 19, 2023, 5:15 a.m. UTC 
  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(+)

Comments

Jiri Slaby July 19, 2023, 5:25 a.m. UTC | #1 
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,
Tony Lindgren July 19, 2023, 5:28 a.m. UTC | #2 
* 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
Jiri Slaby July 19, 2023, 5:29 a.m. UTC | #3 
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,
Tony Lindgren July 19, 2023, 5:32 a.m. UTC | #4 
* 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
Jiri Slaby July 19, 2023, 5:36 a.m. UTC | #5 
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,
Andy Shevchenko July 19, 2023, 5:37 a.m. UTC | #6 
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.
Tony Lindgren July 19, 2023, 5:49 a.m. UTC | #7 
* 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
kernel test robot July 19, 2023, 3:38 p.m. UTC | #8 
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
kernel test robot July 19, 2023, 6:03 p.m. UTC | #9 
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
Tony Lindgren July 20, 2023, 4:13 a.m. UTC | #10 
* 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 mbox series

Patch

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;