[PATCH] RFC: serial: core: Dynamic minor support

[PATCH] RFC: serial: core: Dynamic minor support

[Sjoerd Simons]

Quite a few serial port drivers use the "Low-density serial ports" major
which is rather packed. And ofcourse as time passes, things get slightly
less dense. For example the SCI serial port driver has an allocation of
4 minors, but in reality it supports up to 11 ports these days. Causing
it to overlap with the allocations for the AMBA PL010 serial port and
firmware console. While some other drivers are simply using minor
regions never allocated to them (both samsung and AMBA PL011 serial user
minor 64 and onwards while those are allocated to the Altix serial card
driver).

Cleary this doesn't scale.

Furthermore some other drivers rely upon the usage of a dynamic major
e.g. the tegra serial port driver. However as there is only a block of
20 major numbers reserved for dynamic assignment that isn't going to
scale either for multiplatform kernels (I can already easily build 18
different serial drivers on an arm64 kernel today).

This patch tries to address the issue adding a new to configuration
option which will let all serial ports use the "Low-density serial
ports" major and dynamically allocate minor ranges out of that rather
then using the driver defined areas. Given there are 20 bits worth of
minors available this should be enough for everyone... This option is
not enabled by default (for now) to not break setups with a static /dev.

This does not try to address device naming by making all serial ports be
ttyS<value> as that would first need a better way of deterministically
specifying serial console devices. While it would be an option to move
to consistent serial device naming while keeping the current console
names, having both not aligned would probably confuse everyone
massively.

Signed-off-by: Sjoerd Simons <sjoerd.simons@collabora.co.uk>

---

 drivers/tty/serial/Kconfig       | 10 ++++++++++
 drivers/tty/serial/serial_core.c | 38 ++++++++++++++++++++++++++++++++++++++
 include/linux/serial_core.h      |  6 ++++++
 3 files changed, 54 insertions(+)

diff --git a/drivers/tty/serial/Kconfig b/drivers/tty/serial/Kconfig
index 6117ac8da48f..65a7261f3060 100644
--- a/drivers/tty/serial/Kconfig
+++ b/drivers/tty/serial/Kconfig
@@ -7,6 +7,16 @@ if TTY
 menu "Serial drivers"
 	depends on HAS_IOMEM
 
+config SERIAL_DYNAMIC_MINORS
+	bool "Dynamic serial minors allocation"
+	help
+	  If you say Y here the serial subsystem will use dynamic minors for all
+	  serial devices, using the Low density serial port major. Note that
+	  this requires the system to a non-static /dev to support a getty on
+	  serial console.
+
+	  If you are unsure about this, say N here.
+
 config SERIAL_EARLYCON
 	bool
 	help
diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index 3fe56894974a..37014c70dd69 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -44,6 +44,32 @@
  */
 static DEFINE_MUTEX(port_mutex);
 
+#ifdef CONFIG_SERIAL_DYNAMIC_MINORS
+/* List of uarts to allow for allocation of dynamic minor ranges*/
+LIST_HEAD(dynamic_uarts);
+DEFINE_MUTEX(dynamic_uarts_mutex);
+
+static void
+uart_setup_major_minor(struct uart_driver *drv)
+{
+	int start = 0;
+	struct uart_driver *d;
+
+	drv->major = LOW_DENSITY_UART_MAJOR;
+	mutex_lock(&dynamic_uarts_mutex);
+
+	list_for_each_entry(d, &dynamic_uarts, dynamic_uarts) {
+		if (start + drv->nr < d->minor)
+			break;
+		start = d->minor + d->nr;
+	}
+	list_add_tail(&drv->dynamic_uarts, &d->dynamic_uarts);
+	drv->minor = start;
+
+	mutex_unlock(&dynamic_uarts_mutex);
+}
+#endif
+
 /*
  * lockdep: port->lock is initialized in two places, but we
  *          want only one lock-class:
@@ -2458,6 +2484,11 @@ int uart_register_driver(struct uart_driver *drv)
 
 	BUG_ON(drv->state);
 
+#ifdef CONFIG_SERIAL_DYNAMIC_MINORS
+	INIT_LIST_HEAD(&drv->dynamic_uarts);
+	uart_setup_major_minor(drv);
+#endif
+
 	/*
 	 * Maybe we should be using a slab cache for this, especially if
 	 * we have a large number of ports to handle.
@@ -2527,6 +2558,13 @@ void uart_unregister_driver(struct uart_driver *drv)
 	put_tty_driver(p);
 	for (i = 0; i < drv->nr; i++)
 		tty_port_destroy(&drv->state[i].port);
+
+#ifdef CONFIG_SERIAL_DYNAMIC_MINORS
+	mutex_lock(&dynamic_uarts_mutex);
+	list_del(&drv->dynamic_uarts);
+	mutex_unlock(&dynamic_uarts_mutex);
+#endif
+
 	kfree(drv->state);
 	drv->state = NULL;
 	drv->tty_driver = NULL;
diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
index 58484fb35cc8..890bbefb3f90 100644
--- a/include/linux/serial_core.h
+++ b/include/linux/serial_core.h
@@ -31,6 +31,8 @@
 #include <linux/sysrq.h>
 #include <uapi/linux/serial_core.h>
 
+#define LOW_DENSITY_UART_MAJOR 204
+
 #ifdef CONFIG_SERIAL_CORE_CONSOLE
 #define uart_console(port) \
 	((port)->cons && (port)->cons->index == (port)->line)
@@ -313,6 +315,10 @@ struct uart_driver {
 	 */
 	struct uart_state	*state;
 	struct tty_driver	*tty_driver;
+
+#ifdef CONFIG_SERIAL_DYNAMIC_MINORS
+	struct list_head	dynamic_uarts;
+#endif
 };
 
 void uart_write_wakeup(struct uart_port *port);
-- 
2.11.0

Re: [PATCH] RFC: serial: core: Dynamic minor support

[Greg Kroah-Hartman]

On Thu, Apr 20, 2017 at 02:03:57PM +0200, Sjoerd Simons wrote:
> diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
> index 3fe56894974a..37014c70dd69 100644
> --- a/drivers/tty/serial/serial_core.c
> +++ b/drivers/tty/serial/serial_core.c
> @@ -44,6 +44,32 @@
>   */
>  static DEFINE_MUTEX(port_mutex);
>  
> +#ifdef CONFIG_SERIAL_DYNAMIC_MINORS
> +/* List of uarts to allow for allocation of dynamic minor ranges*/
> +LIST_HEAD(dynamic_uarts);
> +DEFINE_MUTEX(dynamic_uarts_mutex);

Both of these should be static.

> +
> +static void
> +uart_setup_major_minor(struct uart_driver *drv)
> +{
> +	int start = 0;
> +	struct uart_driver *d;
> +

Why not init the list head in here too?

> +	drv->major = LOW_DENSITY_UART_MAJOR;
> +	mutex_lock(&dynamic_uarts_mutex);
> +
> +	list_for_each_entry(d, &dynamic_uarts, dynamic_uarts) {
> +		if (start + drv->nr < d->minor)
> +			break;
> +		start = d->minor + d->nr;
> +	}
> +	list_add_tail(&drv->dynamic_uarts, &d->dynamic_uarts);
> +	drv->minor = start;
> +
> +	mutex_unlock(&dynamic_uarts_mutex);
> +}
> +#endif
> +
>  /*
>   * lockdep: port->lock is initialized in two places, but we
>   *          want only one lock-class:
> @@ -2458,6 +2484,11 @@ int uart_register_driver(struct uart_driver *drv)
>  
>  	BUG_ON(drv->state);
>  
> +#ifdef CONFIG_SERIAL_DYNAMIC_MINORS
> +	INIT_LIST_HEAD(&drv->dynamic_uarts);

That line should be in the function:

> +	uart_setup_major_minor(drv);
> +#endif

And then provide a "empty" function for this if the option is not
enabled, to keep the #ifdef out of the main flow of the code.

> +
>  	/*
>  	 * Maybe we should be using a slab cache for this, especially if
>  	 * we have a large number of ports to handle.
> @@ -2527,6 +2558,13 @@ void uart_unregister_driver(struct uart_driver *drv)
>  	put_tty_driver(p);
>  	for (i = 0; i < drv->nr; i++)
>  		tty_port_destroy(&drv->state[i].port);
> +
> +#ifdef CONFIG_SERIAL_DYNAMIC_MINORS
> +	mutex_lock(&dynamic_uarts_mutex);
> +	list_del(&drv->dynamic_uarts);
> +	mutex_unlock(&dynamic_uarts_mutex);
> +#endif

Make this a function, like above, and do the same thing to keep #ifdef
out of here.

> +
>  	kfree(drv->state);
>  	drv->state = NULL;
>  	drv->tty_driver = NULL;
> diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
> index 58484fb35cc8..890bbefb3f90 100644
> --- a/include/linux/serial_core.h
> +++ b/include/linux/serial_core.h
> @@ -31,6 +31,8 @@
>  #include <linux/sysrq.h>
>  #include <uapi/linux/serial_core.h>
>  
> +#define LOW_DENSITY_UART_MAJOR 204

Where are you stealing this from?



> +
>  #ifdef CONFIG_SERIAL_CORE_CONSOLE
>  #define uart_console(port) \
>  	((port)->cons && (port)->cons->index == (port)->line)
> @@ -313,6 +315,10 @@ struct uart_driver {
>  	 */
>  	struct uart_state	*state;
>  	struct tty_driver	*tty_driver;
> +
> +#ifdef CONFIG_SERIAL_DYNAMIC_MINORS
> +	struct list_head	dynamic_uarts;
> +#endif

Why not just always have this?

Nice first try though!

thanks,

greg k-h

Re: [PATCH] RFC: serial: core: Dynamic minor support

[Sjoerd Simons]

On Thu, 2017-04-20 at 14:15 +0200, Greg Kroah-Hartman wrote:
> On Thu, Apr 20, 2017 at 02:03:57PM +0200, Sjoerd Simons wrote:
> > --- a/include/linux/serial_core.h
> > +++ b/include/linux/serial_core.h
> > @@ -31,6 +31,8 @@
> >  #include <linux/sysrq.h>
> >  #include <uapi/linux/serial_core.h>
> >  
> > +#define LOW_DENSITY_UART_MAJOR 204
> 
> Where are you stealing this from?

Heh, 204 is defined as the "Low-density serial ports" in
devices.txt. As documented in the commit message, i've repurposed that
for dynamic minors (if configured). Maybe it's better to request a new
major for this purpose? But then again, then just means 204 will go
unused when the option is on so...

Lots of drivers do have it as a hard-coded number, seemed sane to put
it a bit more central for some potential later cleanup in other
drivers.

> 
> > +
> >  #ifdef CONFIG_SERIAL_CORE_CONSOLE
> >  #define uart_console(port) \
> >  	((port)->cons && (port)->cons->index == (port)->line)
> > @@ -313,6 +315,10 @@ struct uart_driver {
> >  	 */
> >  	struct uart_state	*state;
> >  	struct tty_driver	*tty_driver;
> > +
> > +#ifdef CONFIG_SERIAL_DYNAMIC_MINORS
> > +	struct list_head	dynamic_uarts;
> > +#endif
> 
> Why not just always have this?

Trying to save a few bytes if the option is unused; Maybe overdoing it
:)

> Nice first try though!

Thanks, If there are no big comments onthe general approach i'll respin
without RFC soonish addressing your other comments.
-- 
Sjoerd Simons
Collabora Ltd.

Re: [PATCH] RFC: serial: core: Dynamic minor support

[Greg Kroah-Hartman]

On Thu, Apr 20, 2017 at 02:44:18PM +0200, Sjoerd Simons wrote:
> On Thu, 2017-04-20 at 14:15 +0200, Greg Kroah-Hartman wrote:
> > On Thu, Apr 20, 2017 at 02:03:57PM +0200, Sjoerd Simons wrote:
> > > --- a/include/linux/serial_core.h
> > > +++ b/include/linux/serial_core.h
> > > @@ -31,6 +31,8 @@
> > >  #include <linux/sysrq.h>
> > >  #include <uapi/linux/serial_core.h>
> > >  
> > > +#define LOW_DENSITY_UART_MAJOR 204
> > 
> > Where are you stealing this from?
> 
> Heh, 204 is defined as the "Low-density serial ports" in
> devices.txt. As documented in the commit message, i've repurposed that
> for dynamic minors (if configured). Maybe it's better to request a new
> major for this purpose? But then again, then just means 204 will go
> unused when the option is on so...
> 
> Lots of drivers do have it as a hard-coded number, seemed sane to put
> it a bit more central for some potential later cleanup in other
> drivers.

Ah, no, that's fine, didn't realize it wasn't already defined somewhere
already.

> > > +
> > >  #ifdef CONFIG_SERIAL_CORE_CONSOLE
> > >  #define uart_console(port) \
> > >  	((port)->cons && (port)->cons->index == (port)->line)
> > > @@ -313,6 +315,10 @@ struct uart_driver {
> > >  	 */
> > >  	struct uart_state	*state;
> > >  	struct tty_driver	*tty_driver;
> > > +
> > > +#ifdef CONFIG_SERIAL_DYNAMIC_MINORS
> > > +	struct list_head	dynamic_uarts;
> > > +#endif
> > 
> > Why not just always have this?
> 
> Trying to save a few bytes if the option is unused; Maybe overdoing it
> :)
> 
> > Nice first try though!
> 
> Thanks, If there are no big comments onthe general approach i'll respin
> without RFC soonish addressing your other comments.

Wait, can you use an idr for this instead of rolling your own search
logic for a series?  I think the usb-serial core did this same sort of
functionality in the drivers/usb/serial/usb-serial.c:allocate_minors()
function using an idr.  Try that instead here as well.

thanks,

greg k-h

Re: [PATCH] RFC: serial: core: Dynamic minor support

[Alan Cox]

> Furthermore some other drivers rely upon the usage of a dynamic major
> e.g. the tegra serial port driver. However as there is only a block of
> 20 major numbers reserved for dynamic assignment that isn't going to
> scale either for multiplatform kernels (I can already easily build 18
> different serial drivers on an arm64 kernel today).

The patch makes sense to me - we really don't need the static minors any
more.

> This does not try to address device naming by making all serial ports be
> ttyS<value> as that would first need a better way of deterministically
> specifying serial console devices. While it would be an option to move
> to consistent serial device naming while keeping the current console
> names, having both not aligned would probably confuse everyone
> massively.

The existing ttyS being 8250 is ABI and there are lots of things that
know far too much about what each type of ttyFOO is to ever fix that.

What could certainly be done is to do what some other classes of device
do and start also registering

/dev/ttyport/bus-information/0 /1 /2 .. etc

in parallel.

Alan