[PATCH v2 0/3] Serial core controller port device name fixes

[PATCH v2 0/3] Serial core controller port device name fixes

[Tony Lindgren]

A few issues have been found with device naming for the serial core
controller port device. These issues currently affect mostly the output
for /sys/bus/serial-base/devices, but need to be also fixed to avoid
port addressing issues later on. It would be good to get these fixes into
the -rc cycle to avoid folks possibly trying to use the incorrect sysfs
port device names for finding uart ports for example.

Regards,

Tony

Changes since v1:
- Port id cannot be negative as noted by Jiri

- Controller id cannot be negative as noted by Andy

- Port name is missing the controller instance as noted by Andy

Tony Lindgren (3):
  serial: core: Controller id cannot be negative
  serial: core: Fix serial core port id to not use port->line
  serial: core: Fix serial core controller port name to show controller
    id

 drivers/tty/serial/8250/8250_core.c  |  2 ++
 drivers/tty/serial/serial_base_bus.c | 33 ++++++++++++++++++----------
 include/linux/serial_core.h          |  3 ++-
 3 files changed, 25 insertions(+), 13 deletions(-)

-- 
2.41.0

[PATCH v2 1/3] serial: core: Controller id cannot be negative

[Tony Lindgren]

The controller id cannot be negative.

Fixes: 84a9582fd203 ("serial: core: Start managing serial controllers to enable runtime PM")
Reported-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Closes: https://lore.kernel.org/linux-serial/ZLd154hdaSG2lnue@smile.fi.intel.com/#t
Signed-off-by: Tony Lindgren <tony@atomide.com>
---
 include/linux/serial_core.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
--- a/include/linux/serial_core.h
+++ b/include/linux/serial_core.h
@@ -459,7 +459,7 @@ struct uart_port {
 						struct serial_rs485 *rs485);
 	int			(*iso7816_config)(struct uart_port *,
 						  struct serial_iso7816 *iso7816);
-	int			ctrl_id;		/* optional serial core controller id */
+	unsigned int		ctrl_id;		/* optional serial core controller id */
 	unsigned int		irq;			/* irq number */
 	unsigned long		irqflags;		/* irq flags  */
 	unsigned int		uartclk;		/* base uart clock */
-- 
2.41.0

Re: [PATCH v2 1/3] serial: core: Controller id cannot be negative

[Greg Kroah-Hartman]

On Thu, Jul 20, 2023 at 08:10:14AM +0300, Tony Lindgren wrote:
> The controller id cannot be negative.
> 

What does this mean for a changelog?

And you forgot to cc: linux-serial?

And I never got patch 0/3?

something went wrong here...


> Fixes: 84a9582fd203 ("serial: core: Start managing serial controllers to enable runtime PM")
> Reported-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Closes: https://lore.kernel.org/linux-serial/ZLd154hdaSG2lnue@smile.fi.intel.com/#t

This isn't a bug report to close, is it?

thanks,

greg k-h

Re: [PATCH v2 1/3] serial: core: Controller id cannot be negative

[Tony Lindgren]

* Greg Kroah-Hartman <gregkh@linuxfoundation.org> [230720 19:33]:
> On Thu, Jul 20, 2023 at 08:10:14AM +0300, Tony Lindgren wrote:
> > The controller id cannot be negative.
> > 
> 
> What does this mean for a changelog?

Just let's fix it while at it and adding port_id in the following patch.
If you prefer I can squash the change into the fix adding port_id.

> And you forgot to cc: linux-serial?
> 
> And I never got patch 0/3?
> 
> something went wrong here...

Thanks for letting me know, I'll check what went wrong..
 
> > Fixes: 84a9582fd203 ("serial: core: Start managing serial controllers to enable runtime PM")
> > Reported-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > Closes: https://lore.kernel.org/linux-serial/ZLd154hdaSG2lnue@smile.fi.intel.com/#t
> 
> This isn't a bug report to close, is it?

OK will leave it out. I added it as checkpatch.pl now warns if Reported-by
is added without Closes.

Regards,

Tony

Re: [PATCH v2 1/3] serial: core: Controller id cannot be negative

[Greg Kroah-Hartman]

On Fri, Jul 21, 2023 at 08:43:26AM +0300, Tony Lindgren wrote:
> * Greg Kroah-Hartman <gregkh@linuxfoundation.org> [230720 19:33]:
> > On Thu, Jul 20, 2023 at 08:10:14AM +0300, Tony Lindgren wrote:
> > > The controller id cannot be negative.
> > > 
> > 
> > What does this mean for a changelog?
> 
> Just let's fix it while at it and adding port_id in the following patch.
> If you prefer I can squash the change into the fix adding port_id.

A separate patch like this is fine, just properly document it please :)

thanks,

greg k-h

Re: [PATCH v2 1/3] serial: core: Controller id cannot be negative

[Tony Lindgren]

* Greg Kroah-Hartman <gregkh@linuxfoundation.org> [230721 06:00]:
> On Fri, Jul 21, 2023 at 08:43:26AM +0300, Tony Lindgren wrote:
> > * Greg Kroah-Hartman <gregkh@linuxfoundation.org> [230720 19:33]:
> > > On Thu, Jul 20, 2023 at 08:10:14AM +0300, Tony Lindgren wrote:
> > > > The controller id cannot be negative.
> > > > 
> > > 
> > > What does this mean for a changelog?
> > 
> > Just let's fix it while at it and adding port_id in the following patch.
> > If you prefer I can squash the change into the fix adding port_id.
> 
> A separate patch like this is fine, just properly document it please :)

OK will do.

Looks like linux-serial not getting added is caused by MAINTAINERS
not listing serial_base_bus.c, serial_ctrl.c and serial_port.c. This
causes get_maintainer.pl to not show linux-serial for a patch touching
serial_base_bus.c.. And this will causes git send-email to not pick up
linux-serial.. I'll send a patch for MAINTAINERS file too.

Regards,

Tony

Re: [PATCH v2 1/3] serial: core: Controller id cannot be negative

[Tony Lindgren]

* Tony Lindgren <tony@atomide.com> [230721 06:19]:
> Looks like linux-serial not getting added is caused by MAINTAINERS
> not listing serial_base_bus.c, serial_ctrl.c and serial_port.c. This
> causes get_maintainer.pl to not show linux-serial for a patch touching
> serial_base_bus.c.. And this will causes git send-email to not pick up
> linux-serial.. I'll send a patch for MAINTAINERS file too.

And the TTY LAYER is missing the list entries.. Does something like below
make sense to you guys to include lkml and linux-serial for TTY LAYER?

Regards,

Tony

8< ---------------------
diff --git a/MAINTAINERS b/MAINTAINERS
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -21628,11 +21628,17 @@ F:	Documentation/translations/zh_TW/
 TTY LAYER
 M:	Greg Kroah-Hartman <gregkh@linuxfoundation.org>
 M:	Jiri Slaby <jirislaby@kernel.org>
+L:	linux-kernel@vger.kernel.org
+L:	linux-serial@vger.kernel.org
 S:	Supported
 T:	git git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/tty.git
 F:	Documentation/driver-api/serial/
 F:	drivers/tty/
+F:	drivers/tty/serial/serial_base.h
+F:	drivers/tty/serial/serial_base_bus.c
 F:	drivers/tty/serial/serial_core.c
+F:	drivers/tty/serial/serial_ctrl.c
+F:	drivers/tty/serial/serial_port.c
 F:	include/linux/selection.h
 F:	include/linux/serial.h
 F:	include/linux/serial_core.h
-- 
2.41.0

Re: [PATCH v2 1/3] serial: core: Controller id cannot be negative

[Greg Kroah-Hartman]

On Fri, Jul 21, 2023 at 09:57:01AM +0300, Tony Lindgren wrote:
> * Tony Lindgren <tony@atomide.com> [230721 06:19]:
> > Looks like linux-serial not getting added is caused by MAINTAINERS
> > not listing serial_base_bus.c, serial_ctrl.c and serial_port.c. This
> > causes get_maintainer.pl to not show linux-serial for a patch touching
> > serial_base_bus.c.. And this will causes git send-email to not pick up
> > linux-serial.. I'll send a patch for MAINTAINERS file too.
> 
> And the TTY LAYER is missing the list entries.. Does something like below
> make sense to you guys to include lkml and linux-serial for TTY LAYER?
> 
> Regards,
> 
> Tony
> 
> 8< ---------------------
> diff --git a/MAINTAINERS b/MAINTAINERS
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -21628,11 +21628,17 @@ F:	Documentation/translations/zh_TW/
>  TTY LAYER
>  M:	Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>  M:	Jiri Slaby <jirislaby@kernel.org>
> +L:	linux-kernel@vger.kernel.org
> +L:	linux-serial@vger.kernel.org
>  S:	Supported
>  T:	git git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/tty.git
>  F:	Documentation/driver-api/serial/
>  F:	drivers/tty/
> +F:	drivers/tty/serial/serial_base.h
> +F:	drivers/tty/serial/serial_base_bus.c
>  F:	drivers/tty/serial/serial_core.c
> +F:	drivers/tty/serial/serial_ctrl.c
> +F:	drivers/tty/serial/serial_port.c
>  F:	include/linux/selection.h
>  F:	include/linux/serial.h
>  F:	include/linux/serial_core.h
> -- 
> 2.41.0

Seems sane to me, I've always wondered why some serial patches didn't
end up on the linux-serial list.

thanks,

greg k-h

Re: [PATCH v2 1/3] serial: core: Controller id cannot be negative

[Tony Lindgren]

* Greg Kroah-Hartman <gregkh@linuxfoundation.org> [230721 07:07]:
> On Fri, Jul 21, 2023 at 09:57:01AM +0300, Tony Lindgren wrote:
> > * Tony Lindgren <tony@atomide.com> [230721 06:19]:
> > > Looks like linux-serial not getting added is caused by MAINTAINERS
> > > not listing serial_base_bus.c, serial_ctrl.c and serial_port.c. This
> > > causes get_maintainer.pl to not show linux-serial for a patch touching
> > > serial_base_bus.c.. And this will causes git send-email to not pick up
> > > linux-serial.. I'll send a patch for MAINTAINERS file too.
> > 
> > And the TTY LAYER is missing the list entries.. Does something like below
> > make sense to you guys to include lkml and linux-serial for TTY LAYER?
> > 
> > Regards,
> > 
> > Tony
> > 
> > 8< ---------------------
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -21628,11 +21628,17 @@ F:	Documentation/translations/zh_TW/
> >  TTY LAYER
> >  M:	Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> >  M:	Jiri Slaby <jirislaby@kernel.org>
> > +L:	linux-kernel@vger.kernel.org
> > +L:	linux-serial@vger.kernel.org
> >  S:	Supported
> >  T:	git git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/tty.git
> >  F:	Documentation/driver-api/serial/
> >  F:	drivers/tty/
> > +F:	drivers/tty/serial/serial_base.h
> > +F:	drivers/tty/serial/serial_base_bus.c
> >  F:	drivers/tty/serial/serial_core.c
> > +F:	drivers/tty/serial/serial_ctrl.c
> > +F:	drivers/tty/serial/serial_port.c
> >  F:	include/linux/selection.h
> >  F:	include/linux/serial.h
> >  F:	include/linux/serial_core.h
> > -- 
> > 2.41.0
> 
> Seems sane to me, I've always wondered why some serial patches didn't
> end up on the linux-serial list.

OK will send. Also I noticed that using git send-email --cc-cover does
not work for the cover letter.. It tries to use the first patch that
is the cover letter or something like that. I'm going back to my custom
email scripts for now rather than try to have git handle things
automagically.

Regards,

Tony

Re: [PATCH v2 1/3] serial: core: Controller id cannot be negative

[Andy Shevchenko]

On Fri, Jul 21, 2023 at 10:17:53AM +0300, Tony Lindgren wrote:
> * Greg Kroah-Hartman <gregkh@linuxfoundation.org> [230721 07:07]:

...

> Also I noticed that using git send-email --cc-cover does
> not work for the cover letter.. It tries to use the first patch that
> is the cover letter or something like that. I'm going back to my custom
> email scripts for now rather than try to have git handle things
> automagically.

I have my script [1] that shows good enough results to send patches.
I suggest give it a try :-)

[1]: https://github.com/andy-shev/home-bin-tools/blob/master/ge2maintainer.sh

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 1/3] serial: core: Controller id cannot be negative

[Tony Lindgren]

* Andy Shevchenko <andriy.shevchenko@linux.intel.com> [230721 10:10]:
> On Fri, Jul 21, 2023 at 10:17:53AM +0300, Tony Lindgren wrote:
> > * Greg Kroah-Hartman <gregkh@linuxfoundation.org> [230721 07:07]:
> 
> ...
> 
> > Also I noticed that using git send-email --cc-cover does
> > not work for the cover letter.. It tries to use the first patch that
> > is the cover letter or something like that. I'm going back to my custom
> > email scripts for now rather than try to have git handle things
> > automagically.
> 
> I have my script [1] that shows good enough results to send patches.
> I suggest give it a try :-)

Thanks I'll check if your get_maintainer.pl options help. I was trying
to use .gitconfig [sendemail.linux] style options with git send-email
--identity=linux based on an example Krzysztof posted somewhere a
while back. Sorry could not find it though, maybe Krzysztof has a
link for it.

Regards,

Tony


> [1]: https://github.com/andy-shev/home-bin-tools/blob/master/ge2maintainer.sh

[PATCH v2 2/3] serial: core: Fix serial core port id to not use port->line

[Tony Lindgren]

The serial core port id should be serial core controller specific port
instance, which is not always the port->line index.

For example, 8250 driver maps a number of legacy ports, and when a
hardware specific device driver takes over, we typically have one
driver instance for each port. Let's instead add port->port_id to
keep track serial ports mapped to each serial core controller instance.

Currently this is only a cosmetic issue for the serial core port device
names. The issue can be noticed looking at /sys/bus/serial-base/devices
for example though. Let's fix the issue to avoid port addressing issues
later on.

Fixes: 84a9582fd203 ("serial: core: Start managing serial controllers to enable runtime PM")
Signed-off-by: Tony Lindgren <tony@atomide.com>
---
 drivers/tty/serial/8250/8250_core.c  | 2 ++
 drivers/tty/serial/serial_base_bus.c | 2 +-
 include/linux/serial_core.h          | 1 +
 3 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/serial/8250/8250_core.c b/drivers/tty/serial/8250/8250_core.c
--- a/drivers/tty/serial/8250/8250_core.c
+++ b/drivers/tty/serial/8250/8250_core.c
@@ -497,6 +497,7 @@ static struct uart_8250_port *serial8250_setup_port(int index)
 
 	up = &serial8250_ports[index];
 	up->port.line = index;
+	up->port.port_id = index;
 
 	serial8250_init_port(up);
 	if (!base_ops)
@@ -1040,6 +1041,7 @@ int serial8250_register_8250_port(const struct uart_8250_port *up)
 			uart_remove_one_port(&serial8250_reg, &uart->port);
 
 		uart->port.ctrl_id	= up->port.ctrl_id;
+		uart->port.port_id	= up->port.port_id;
 		uart->port.iobase       = up->port.iobase;
 		uart->port.membase      = up->port.membase;
 		uart->port.irq          = up->port.irq;
diff --git a/drivers/tty/serial/serial_base_bus.c b/drivers/tty/serial/serial_base_bus.c
--- a/drivers/tty/serial/serial_base_bus.c
+++ b/drivers/tty/serial/serial_base_bus.c
@@ -136,7 +136,7 @@ struct serial_port_device *serial_base_port_add(struct uart_port *port,
 	err = serial_base_device_init(port, &port_dev->dev,
 				      &ctrl_dev->dev, &serial_port_type,
 				      serial_base_port_release,
-				      port->line);
+				      port->port_id);
 	if (err)
 		goto err_put_device;
 
diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
--- a/include/linux/serial_core.h
+++ b/include/linux/serial_core.h
@@ -460,6 +460,7 @@ struct uart_port {
 	int			(*iso7816_config)(struct uart_port *,
 						  struct serial_iso7816 *iso7816);
 	unsigned int		ctrl_id;		/* optional serial core controller id */
+	unsigned int		port_id;		/* optional serial core port id */
 	unsigned int		irq;			/* irq number */
 	unsigned long		irqflags;		/* irq flags  */
 	unsigned int		uartclk;		/* base uart clock */
-- 
2.41.0

[PATCH v2 3/3] serial: core: Fix serial core controller port name to show controller id

[Tony Lindgren]

We are missing the serial core controller id for the serial core port
name. Let's fix the issue for sane /sys/bus/serial-core/devices, and to
avoid issues addressing serial ports later on.

Fixes: 84a9582fd203 ("serial: core: Start managing serial controllers to enable runtime PM")
Reported-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Closes: https://lore.kernel.org/linux-serial/20230719051613.46569-1-tony@atomide.com/T/#m0f358e91262f7f56198ba9d0a7100809e9e35cc5
Signed-off-by: Tony Lindgren <tony@atomide.com>
---
 drivers/tty/serial/serial_base_bus.c | 33 ++++++++++++++++++----------
 1 file changed, 21 insertions(+), 12 deletions(-)

diff --git a/drivers/tty/serial/serial_base_bus.c b/drivers/tty/serial/serial_base_bus.c
--- a/drivers/tty/serial/serial_base_bus.c
+++ b/drivers/tty/serial/serial_base_bus.c
@@ -19,6 +19,14 @@
 
 static bool serial_base_initialized;
 
+static const struct device_type serial_ctrl_type = {
+	.name = "ctrl",
+};
+
+static const struct device_type serial_port_type = {
+	.name = "port",
+};
+
 static int serial_base_match(struct device *dev, struct device_driver *drv)
 {
 	int len = strlen(drv->name);
@@ -48,7 +56,8 @@ static int serial_base_device_init(struct uart_port *port,
 				   struct device *parent_dev,
 				   const struct device_type *type,
 				   void (*release)(struct device *dev),
-				   int id)
+				   unsigned int ctrl_id,
+				   unsigned int port_id)
 {
 	device_initialize(dev);
 	dev->type = type;
@@ -61,13 +70,17 @@ static int serial_base_device_init(struct uart_port *port,
 		return -EPROBE_DEFER;
 	}
 
-	return dev_set_name(dev, "%s.%s.%d", type->name, dev_name(port->dev), id);
+	if (type == &serial_ctrl_type)
+		return dev_set_name(dev, "%s.%s.%d", type->name,
+				    dev_name(port->dev), ctrl_id);
+	else if (type == &serial_port_type)
+		return dev_set_name(dev, "%s.%s.%d.%d", type->name,
+				    dev_name(port->dev), ctrl_id,
+				    port_id);
+	else
+		return -EINVAL;
 }
 
-static const struct device_type serial_ctrl_type = {
-	.name = "ctrl",
-};
-
 static void serial_base_ctrl_release(struct device *dev)
 {
 	struct serial_ctrl_device *ctrl_dev = to_serial_base_ctrl_device(dev);
@@ -96,7 +109,7 @@ struct serial_ctrl_device *serial_base_ctrl_add(struct uart_port *port,
 	err = serial_base_device_init(port, &ctrl_dev->dev,
 				      parent, &serial_ctrl_type,
 				      serial_base_ctrl_release,
-				      port->ctrl_id);
+				      port->ctrl_id, 0);
 	if (err)
 		goto err_put_device;
 
@@ -112,10 +125,6 @@ struct serial_ctrl_device *serial_base_ctrl_add(struct uart_port *port,
 	return ERR_PTR(err);
 }
 
-static const struct device_type serial_port_type = {
-	.name = "port",
-};
-
 static void serial_base_port_release(struct device *dev)
 {
 	struct serial_port_device *port_dev = to_serial_base_port_device(dev);
@@ -136,7 +145,7 @@ struct serial_port_device *serial_base_port_add(struct uart_port *port,
 	err = serial_base_device_init(port, &port_dev->dev,
 				      &ctrl_dev->dev, &serial_port_type,
 				      serial_base_port_release,
-				      port->port_id);
+				      port->ctrl_id, port->port_id);
 	if (err)
 		goto err_put_device;
 
-- 
2.41.0