diff mbox

serial: 8250: Make ISA ports optional

Message ID 1420513785-23660-1-git-send-email-peter@hurleysoftware.com (mailing list archive)
State New, archived
Headers show

Commit Message

Peter Hurley Jan. 6, 2015, 3:09 a.m. UTC
Some arches have no need to create unprobed 8250 ports; these phantom
ports are primarily required for ISA ports which have no probe
mechanism or to provide non-operational ports for userspace to
configure (via TIOCSSERIAL and TIOCSERCONFIG ioctls).

Provide CONFIG_SERIAL_8250_PHANTOM_UARTS knob to disable phantom port
registration; ie., CONFIG_SERIAL_8250_PHANTOM_UARTS=N only registers
probed ports (ACPI/PNP, "serial8250" platform devices, PCI, etc).

Cc: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Cc: Tony Lindgren <tony@atomide.com>
Cc: Grant Likely <grant.likely@linaro.org>
Cc: Arnd Bergmann <arnd@arndb.de>
Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
---
 drivers/tty/serial/8250/8250.h      |  6 ++++++
 drivers/tty/serial/8250/8250_core.c | 13 +++++++++++--
 drivers/tty/serial/8250/Kconfig     | 24 +++++++++++++++++++-----
 3 files changed, 36 insertions(+), 7 deletions(-)

Comments

Arnd Bergmann Jan. 6, 2015, 1:13 p.m. UTC | #1
On Monday 05 January 2015 22:09:45 Peter Hurley wrote:
> Some arches have no need to create unprobed 8250 ports; these phantom
> ports are primarily required for ISA ports which have no probe
> mechanism or to provide non-operational ports for userspace to
> configure (via TIOCSSERIAL and TIOCSERCONFIG ioctls).
> 
> Provide CONFIG_SERIAL_8250_PHANTOM_UARTS knob to disable phantom port
> registration; ie., CONFIG_SERIAL_8250_PHANTOM_UARTS=N only registers
> probed ports (ACPI/PNP, "serial8250" platform devices, PCI, etc).
> 
> Cc: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> Cc: Tony Lindgren <tony@atomide.com>
> Cc: Grant Likely <grant.likely@linaro.org>
> Cc: Arnd Bergmann <arnd@arndb.de>
> Signed-off-by: Peter Hurley <peter@hurleysoftware.com>

The intent is definitely right, but I think a better approach is
possible.

I haven't tried it here, but how about moving the serial8250_init
function into a separate module, along with all the other parts
that are only used for ISA devices, but leaving the actual core
(all exported symbols) in this file?

At the same time, the serial8250_pnp_init/serial8250_pnp_exit calls
can be removed from the serial8250_init function and become
standalone initcalls.

	Arnd
Peter Hurley Jan. 6, 2015, 2:32 p.m. UTC | #2
On 01/06/2015 08:13 AM, Arnd Bergmann wrote:
> On Monday 05 January 2015 22:09:45 Peter Hurley wrote:
>> Some arches have no need to create unprobed 8250 ports; these phantom
>> ports are primarily required for ISA ports which have no probe
>> mechanism or to provide non-operational ports for userspace to
>> configure (via TIOCSSERIAL and TIOCSERCONFIG ioctls).
>>
>> Provide CONFIG_SERIAL_8250_PHANTOM_UARTS knob to disable phantom port
>> registration; ie., CONFIG_SERIAL_8250_PHANTOM_UARTS=N only registers
>> probed ports (ACPI/PNP, "serial8250" platform devices, PCI, etc).
>>
>> Cc: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
>> Cc: Tony Lindgren <tony@atomide.com>
>> Cc: Grant Likely <grant.likely@linaro.org>
>> Cc: Arnd Bergmann <arnd@arndb.de>
>> Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
> 
> The intent is definitely right, but I think a better approach is
> possible.
> 
> I haven't tried it here, but how about moving the serial8250_init
> function into a separate module, along with all the other parts
> that are only used for ISA devices, but leaving the actual core
> (all exported symbols) in this file?

Unfortunately, I don't see a way to remove the stacked initialization
without risking tons of breakage.

Since later probes can "find" an already-existing port and
re-initialize it, the probe order is crucial. For example, a PCI
probe can "find" an existing "serial8250" platform device port,
resulting in only one device node.

And the configuration knob will be required on all arches anyway because
that's how user-configurable device nodes are created.

> At the same time, the serial8250_pnp_init/serial8250_pnp_exit calls
> can be removed from the serial8250_init function and become
> standalone initcalls.

PNP probe must occur before the phantom ports are registered.
See commit 835d844d1a28efba81d5aca7385e24c29d3a6db2
("8250_pnp: do pnp probe before legacy probe").

Regards,
Peter Hurley
Arnd Bergmann Jan. 6, 2015, 7:43 p.m. UTC | #3
On Tuesday 06 January 2015 09:32:02 Peter Hurley wrote:
> On 01/06/2015 08:13 AM, Arnd Bergmann wrote:
> > On Monday 05 January 2015 22:09:45 Peter Hurley wrote:
> >> Some arches have no need to create unprobed 8250 ports; these phantom
> >> ports are primarily required for ISA ports which have no probe
> >> mechanism or to provide non-operational ports for userspace to
> >> configure (via TIOCSSERIAL and TIOCSERCONFIG ioctls).
> >>
> >> Provide CONFIG_SERIAL_8250_PHANTOM_UARTS knob to disable phantom port
> >> registration; ie., CONFIG_SERIAL_8250_PHANTOM_UARTS=N only registers
> >> probed ports (ACPI/PNP, "serial8250" platform devices, PCI, etc).
> >>
> >> Cc: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> >> Cc: Tony Lindgren <tony@atomide.com>
> >> Cc: Grant Likely <grant.likely@linaro.org>
> >> Cc: Arnd Bergmann <arnd@arndb.de>
> >> Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
> > 
> > The intent is definitely right, but I think a better approach is
> > possible.
> > 
> > I haven't tried it here, but how about moving the serial8250_init
> > function into a separate module, along with all the other parts
> > that are only used for ISA devices, but leaving the actual core
> > (all exported symbols) in this file?
> 
> Unfortunately, I don't see a way to remove the stacked initialization
> without risking tons of breakage.
> 
> Since later probes can "find" an already-existing port and
> re-initialize it, the probe order is crucial. For example, a PCI
> probe can "find" an existing "serial8250" platform device port,
> resulting in only one device node.

I'm probably missing something important, by why would that
be any different if the PCI driver gets loaded first and the
ISA driver second?

> And the configuration knob will be required on all arches anyway because
> that's how user-configurable device nodes are created.

I think that's fine: The user-configurable ports are the same as
the "ISA" or "phantom" ports we were talking about above, right?

If those are part of a separate (possibly loadable) module, having
a configuration knob is the obvious way to do it. A lot of architectures
can just turn it off because they know exactly which ports are present
and there is no need for user-configurability. The ones that don't know
can load the module.

> > At the same time, the serial8250_pnp_init/serial8250_pnp_exit calls
> > can be removed from the serial8250_init function and become
> > standalone initcalls.
> 
> PNP probe must occur before the phantom ports are registered.
> See commit 835d844d1a28efba81d5aca7385e24c29d3a6db2
> ("8250_pnp: do pnp probe before legacy probe").

Makes sense.

	Arnd
Peter Hurley Jan. 6, 2015, 9:47 p.m. UTC | #4
On 01/06/2015 02:43 PM, Arnd Bergmann wrote:
> On Tuesday 06 January 2015 09:32:02 Peter Hurley wrote:
>> On 01/06/2015 08:13 AM, Arnd Bergmann wrote:
>>> On Monday 05 January 2015 22:09:45 Peter Hurley wrote:
>>>> Some arches have no need to create unprobed 8250 ports; these phantom
>>>> ports are primarily required for ISA ports which have no probe
>>>> mechanism or to provide non-operational ports for userspace to
>>>> configure (via TIOCSSERIAL and TIOCSERCONFIG ioctls).
>>>>
>>>> Provide CONFIG_SERIAL_8250_PHANTOM_UARTS knob to disable phantom port
>>>> registration; ie., CONFIG_SERIAL_8250_PHANTOM_UARTS=N only registers
>>>> probed ports (ACPI/PNP, "serial8250" platform devices, PCI, etc).
>>>>
>>>> Cc: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
>>>> Cc: Tony Lindgren <tony@atomide.com>
>>>> Cc: Grant Likely <grant.likely@linaro.org>
>>>> Cc: Arnd Bergmann <arnd@arndb.de>
>>>> Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
>>>
>>> The intent is definitely right, but I think a better approach is
>>> possible.
>>>
>>> I haven't tried it here, but how about moving the serial8250_init
>>> function into a separate module, along with all the other parts
>>> that are only used for ISA devices, but leaving the actual core
>>> (all exported symbols) in this file?
>>
>> Unfortunately, I don't see a way to remove the stacked initialization
>> without risking tons of breakage.
>>
>> Since later probes can "find" an already-existing port and
>> re-initialize it, the probe order is crucial. For example, a PCI
>> probe can "find" an existing "serial8250" platform device port,
>> resulting in only one device node.
> 
> I'm probably missing something important, by why would that
> be any different if the PCI driver gets loaded first and the
> ISA driver second?

Well, the PCI driver would have the proper irq, for one. So, if the
the platform driver re-initialized the port to the wrong irq...

>> And the configuration knob will be required on all arches anyway because
>> that's how user-configurable device nodes are created.
> 
> I think that's fine: The user-configurable ports are the same as
> the "ISA" or "phantom" ports we were talking about above, right?

Yes.

> If those are part of a separate (possibly loadable) module, having
> a configuration knob is the obvious way to do it. A lot of architectures
> can just turn it off because they know exactly which ports are present
> and there is no need for user-configurability. The ones that don't know
> can load the module.

Let me give this some more thought.

Regards,
Peter Hurley
Arnd Bergmann Jan. 7, 2015, 10:05 a.m. UTC | #5
On Tuesday 06 January 2015 16:47:55 Peter Hurley wrote:
> On 01/06/2015 02:43 PM, Arnd Bergmann wrote:
> > On Tuesday 06 January 2015 09:32:02 Peter Hurley wrote:
> >> On 01/06/2015 08:13 AM, Arnd Bergmann wrote:
> >>> On Monday 05 January 2015 22:09:45 Peter Hurley wrote:
> >>>> Some arches have no need to create unprobed 8250 ports; these phantom
> >>>> ports are primarily required for ISA ports which have no probe
> >>>> mechanism or to provide non-operational ports for userspace to
> >>>> configure (via TIOCSSERIAL and TIOCSERCONFIG ioctls).
> >>>>
> >>>> Provide CONFIG_SERIAL_8250_PHANTOM_UARTS knob to disable phantom port
> >>>> registration; ie., CONFIG_SERIAL_8250_PHANTOM_UARTS=N only registers
> >>>> probed ports (ACPI/PNP, "serial8250" platform devices, PCI, etc).
> >>>>
> >>>> Cc: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> >>>> Cc: Tony Lindgren <tony@atomide.com>
> >>>> Cc: Grant Likely <grant.likely@linaro.org>
> >>>> Cc: Arnd Bergmann <arnd@arndb.de>
> >>>> Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
> >>>
> >>> The intent is definitely right, but I think a better approach is
> >>> possible.
> >>>
> >>> I haven't tried it here, but how about moving the serial8250_init
> >>> function into a separate module, along with all the other parts
> >>> that are only used for ISA devices, but leaving the actual core
> >>> (all exported symbols) in this file?
> >>
> >> Unfortunately, I don't see a way to remove the stacked initialization
> >> without risking tons of breakage.
> >>
> >> Since later probes can "find" an already-existing port and
> >> re-initialize it, the probe order is crucial. For example, a PCI
> >> probe can "find" an existing "serial8250" platform device port,
> >> resulting in only one device node.
> > 
> > I'm probably missing something important, by why would that
> > be any different if the PCI driver gets loaded first and the
> > ISA driver second?
> 
> Well, the PCI driver would have the proper irq, for one. So, if the
> the platform driver re-initialized the port to the wrong irq...

I see. So we must still ensure that the ISA driver either gets loaded
before the PCI driver, or never. It is already closely coupled with
the PNP driver, but I think we can still get to the point where you
don't load the ISA driver at all if you only have platform 8250
ports that are not supposed to be reconfigurable.

On most non-x86 machines, I would think we also want a way to build
the PCI driver without that dependency.

	Arnd
Alan Cox Jan. 8, 2015, 1:10 p.m. UTC | #6
On Mon,  5 Jan 2015 22:09:45 -0500
Peter Hurley <peter@hurleysoftware.com> wrote:

> Some arches have no need to create unprobed 8250 ports; these phantom
> ports are primarily required for ISA ports which have no probe
> mechanism or to provide non-operational ports for userspace to
> configure (via TIOCSSERIAL and TIOCSERCONFIG ioctls).
> 
> Provide CONFIG_SERIAL_8250_PHANTOM_UARTS knob to disable phantom port
> registration; ie., CONFIG_SERIAL_8250_PHANTOM_UARTS=N only registers
> probed ports (ACPI/PNP, "serial8250" platform devices, PCI, etc).

Just #define serial8250_isa_devs to 0 on such platforms. gcc should then
be bright enough to do the rest for you.
Peter Hurley Jan. 8, 2015, 4:11 p.m. UTC | #7
On 01/08/2015 08:10 AM, One Thousand Gnomes wrote:
> On Mon,  5 Jan 2015 22:09:45 -0500
> Peter Hurley <peter@hurleysoftware.com> wrote:
> 
>> Some arches have no need to create unprobed 8250 ports; these phantom
>> ports are primarily required for ISA ports which have no probe
>> mechanism or to provide non-operational ports for userspace to
>> configure (via TIOCSSERIAL and TIOCSERCONFIG ioctls).
>>
>> Provide CONFIG_SERIAL_8250_PHANTOM_UARTS knob to disable phantom port
>> registration; ie., CONFIG_SERIAL_8250_PHANTOM_UARTS=N only registers
>> probed ports (ACPI/PNP, "serial8250" platform devices, PCI, etc).
> 
> Just #define serial8250_isa_devs to 0 on such platforms. gcc should then
> be bright enough to do the rest for you.

Well, this patch has the "advantage" of allowing the user to override
the config and create blank ports anyway.

That said, I'm exploring a completely different approach: I've split the
driver from the port operations. The idea being that the existing
"universal" driver supports everything it does now, while allowing for
a new driver(s) to only support a subset.

At this point, I've only done the actual split. So the "universal" driver
module has:
 * ISA/"serial8250" platform driver
 * PNP init
 * uart driver and console definitions
 * all the existing module parameters
 * all the globals (except the const uart_config[] table)
 * RSA port handling
 * Chained interrupt handling
 * port i/o by timer handling
 * early_serial_setup()
 * sun ttyS shared registration
 * serial8250_ports[] table and how it re-uses existing ports

The "universal" driver also has the sub-driver interface, which is:
 * serial8250_register_8250_port
 * serial8250_unregister_port
 * serial8250_suspend/resume_port
 * serial8250_find_port (hack for 8250_early)
 * serial8250_get_port (hack for runtime pm)

This interface is just storage and minor allocation, since the
port-reuse behavior will be limited to the "universal" driver.
From a sub-driver perspective, the shared storage is actually
a hindrance, so that reduces the requirement to minor allocation.

And that's where I'm stuck at the moment -- how to share ttyS
minor allocation. ttyS console is a related problem.

Regards,
Peter Hurley
Arnd Bergmann Jan. 8, 2015, 10:05 p.m. UTC | #8
On Thursday 08 January 2015 11:11:51 Peter Hurley wrote:
> 
> This interface is just storage and minor allocation, since the
> port-reuse behavior will be limited to the "universal" driver.
> From a sub-driver perspective, the shared storage is actually
> a hindrance, so that reduces the requirement to minor allocation.
> 
> And that's where I'm stuck at the moment -- how to share ttyS
> minor allocation. ttyS console is a related problem.

One idea that has come up in the past but never saw an implementation
is to make the ttyS namespace and minor numbers completely generic and
let any serial port driver use it. This would be a major rework, but
have the added advantage of cleaning up a number of other namespace
issues as well. There also lots of open question, in particular how
to maintain compatibility with existing drivers. One could imagine
that each uart always gets a ttyS device and optionally also gets
a device node for the same port with a driver specific chardev as
most of them do today. Or it could be an either/or decision that is
made at compile time or as a module parameter.

	Arnd
Alan Cox Jan. 8, 2015, 10:36 p.m. UTC | #9
> One idea that has come up in the past but never saw an implementation
> is to make the ttyS namespace and minor numbers completely generic and
> let any serial port driver use it. This would be a major rework, but
> have the added advantage of cleaning up a number of other namespace
> issues as well. There also lots of open question, in particular how
> to maintain compatibility with existing drivers. One could imagine
> that each uart always gets a ttyS device and optionally also gets
> a device node for the same port with a driver specific chardev as
> most of them do today. Or it could be an either/or decision that is
> made at compile time or as a module parameter.

I'm not sure we should use ttyS for it. ttyS means 8250, a lot of
existing code knows what ttyS is and what sort of ioctls and the like are
assumed. A generic namespace needs not to be attached to a legacy naming 

(eg also put all serial ports in /dev/serial/0 /dev/serial/1 etc either
via the tty layer or via systemd/udev)

Alan
Peter Hurley Jan. 8, 2015, 11:25 p.m. UTC | #10
On 01/08/2015 05:36 PM, One Thousand Gnomes wrote:
>> One idea that has come up in the past but never saw an implementation
>> is to make the ttyS namespace and minor numbers completely generic and
>> let any serial port driver use it. This would be a major rework, but
>> have the added advantage of cleaning up a number of other namespace
>> issues as well. There also lots of open question, in particular how
>> to maintain compatibility with existing drivers. One could imagine
>> that each uart always gets a ttyS device and optionally also gets
>> a device node for the same port with a driver specific chardev as
>> most of them do today. Or it could be an either/or decision that is
>> made at compile time or as a module parameter.
> 
> I'm not sure we should use ttyS for it. ttyS means 8250, a lot of
> existing code knows what ttyS is and what sort of ioctls and the like are
> assumed.

That genie has already escaped the bottle.

./arch/powerpc/platforms/powermac/setup.c:	char *devname = "ttyS";
./arch/powerpc/platforms/chrp/setup.c:		add_preferred_console("ttyS", 0, NULL);
./arch/powerpc/kernel/legacy_serial.c:		return add_preferred_console("ttyS", offset, opt);
./arch/xtensa/platforms/iss/console.c:	serial_driver->name = "ttyS";
./arch/mips/sni/setup.c:			add_preferred_console("ttyS", port,
./arch/mips/sgi-ip32/ip32-setup.c:			add_preferred_console("ttyS", *(con + 1) == '2' ? 1 : 0,
./arch/mips/jazz/setup.c:	add_preferred_console("ttyS", 0, "9600");
./arch/mips/sgi-ip22/ip22-setup.c:		add_preferred_console("ttyS", *(ctype + 1) == '2' ? 1 : 0,
./arch/arm/mach-davinci/board-da830-evm.c:	return add_preferred_console("ttyS", 2, "115200");
./arch/arm/mach-davinci/board-mityomapl138.c:	return add_preferred_console("ttyS", 1, "115200");
./arch/arm/mach-davinci/board-da850-evm.c:	return add_preferred_console("ttyS", 2, "115200");
./arch/arm/mach-davinci/board-omapl138-hawk.c:	return add_preferred_console("ttyS", 2, "115200");
./arch/cris/arch-v10/kernel/debugport.c:	name : "ttyS",
./arch/ia64/hp/sim/simserial.c:	hp_simserial_driver->name = "ttyS";
./arch/um/drivers/ssl.c:	.device_name 		= "ttyS",
./arch/s390/kernel/setup.c:			add_preferred_console("ttyS", 1, NULL);
./drivers/tty/amiserial.c:	.name =		"ttyS",
./drivers/tty/serial/serial_txx9.c:#define TXX9_TTY_NAME "ttyS"
./drivers/tty/serial/sunsab.c:	.dev_name		= "ttyS",
./drivers/tty/serial/mcf.c:	.dev_name	= "ttyS",
./drivers/tty/serial/atmel_serial.c:#define ATMEL_DEVICENAME	"ttyS"
./drivers/tty/serial/bcm63xx_uart.c:	.dev_name	= "ttyS",
./drivers/tty/serial/sunsu.c:	.dev_name		= "ttyS",
./drivers/tty/serial/mrst_max3110.c:	.dev_name	= "ttyS",
./drivers/tty/serial/apbuart.c:	.name = "ttyS",
./drivers/tty/serial/apbuart.c:	.dev_name = "ttyS",
./drivers/tty/serial/tilegx.c:#define TILEGX_UART_NAME	"ttyS"
./drivers/tty/serial/pmac_zilog.c:#define PMACZILOG_NAME		"ttyS"
./drivers/tty/serial/pnx8xxx_uart.c:	.dev_name		= "ttyS",
./drivers/tty/serial/m32r_sio.c:	.dev_name		= "ttyS",
./drivers/tty/serial/ip22zilog.c:	.dev_name	= "ttyS",
./drivers/tty/serial/zs.c:	.dev_name		= "ttyS",
./drivers/tty/serial/68328serial.c:	serial_driver->name = "ttyS";
./drivers/tty/serial/crisv10.c:	driver->name = "ttyS";
/drivers/tty/serial/8250/8250_core.c:	.dev_name		= "ttyS",
./drivers/tty/serial/sunzilog.c:	.dev_name	=	"ttyS",
./drivers/tty/serial/pxa.c:	.dev_name	= "ttyS",
./drivers/tty/serial/dz.c:	.dev_name		= "ttyS",
./drivers/tty/serial/sunhv.c:	.dev_name		= "ttyS",
./drivers/s390/char/sclp_vt220.c:#define SCLP_VT220_CONSOLE_NAME		"ttyS"
./drivers/s390/char/sclp_con.c:#define sclp_console_name  "ttyS"
./drivers/s390/char/con3215.c:	driver->name = "ttyS";


Besides, despite its pedigree, omap is no closer to 8250 than anything
else in drivers/tty/serial/

> A generic namespace needs not to be attached to a legacy naming 
> 
> (eg also put all serial ports in /dev/serial/0 /dev/serial/1 etc either
> via the tty layer or via systemd/udev)

This doesn't address pre-init.

Regards,
Peter Hurley
Peter Hurley Jan. 9, 2015, 5:13 a.m. UTC | #11
On 01/08/2015 05:05 PM, Arnd Bergmann wrote:
> On Thursday 08 January 2015 11:11:51 Peter Hurley wrote:
>>
>> This interface is just storage and minor allocation, since the
>> port-reuse behavior will be limited to the "universal" driver.
>> From a sub-driver perspective, the shared storage is actually
>> a hindrance, so that reduces the requirement to minor allocation.
>>
>> And that's where I'm stuck at the moment -- how to share ttyS
>> minor allocation. ttyS console is a related problem.
> 
> One idea that has come up in the past but never saw an implementation
> is to make the ttyS namespace and minor numbers completely generic and
> let any serial port driver use it. This would be a major rework, but
> have the added advantage of cleaning up a number of other namespace
> issues as well. There also lots of open question, in particular how
> to maintain compatibility with existing drivers. One could imagine
> that each uart always gets a ttyS device and optionally also gets
> a device node for the same port with a driver specific chardev as
> most of them do today. Or it could be an either/or decision that is
> made at compile time or as a module parameter.

I'm not totally convinced that sharing minor allocation is the right
solution. ISTM that the main goal is for at least one serial driver
to represent ttyS (assuming that it is a suitable work-alike to the
8250 driver) while other ttyS serial drivers must still be able to
load and operate.

So for example, a UART that is ttyS in a typical configuration, might
be ttyX when another driver has been configured as ttyS instead.
Assuming that every uart driver has a unique, alternate base name,
then the problem is reduced to selecting the appropriate driver for
ttyS.

Some use cases would require selecting the ttyS driver at build time.
Others would probably require the notion of a preferred driver.

What got me thinking about this approach instead of sharing minor
allocation was based on the 'first-come, first-served' question
I asked back in Dec. What happens when probe order varies and
console=ttyS0/login prompt doesn't show up where it's supposed to?
I've had this experience with multiple ethernet adapters and it's
not fun.

Regards,
Peter Hurley
Arnd Bergmann Jan. 9, 2015, 8:48 a.m. UTC | #12
On Friday 09 January 2015 00:13:28 Peter Hurley wrote:
> On 01/08/2015 05:05 PM, Arnd Bergmann wrote:
> > On Thursday 08 January 2015 11:11:51 Peter Hurley wrote:
> >>
> >> This interface is just storage and minor allocation, since the
> >> port-reuse behavior will be limited to the "universal" driver.
> >> From a sub-driver perspective, the shared storage is actually
> >> a hindrance, so that reduces the requirement to minor allocation.
> >>
> >> And that's where I'm stuck at the moment -- how to share ttyS
> >> minor allocation. ttyS console is a related problem.
> > 
> > One idea that has come up in the past but never saw an implementation
> > is to make the ttyS namespace and minor numbers completely generic and
> > let any serial port driver use it. This would be a major rework, but
> > have the added advantage of cleaning up a number of other namespace
> > issues as well. There also lots of open question, in particular how
> > to maintain compatibility with existing drivers. One could imagine
> > that each uart always gets a ttyS device and optionally also gets
> > a device node for the same port with a driver specific chardev as
> > most of them do today. Or it could be an either/or decision that is
> > made at compile time or as a module parameter.
> 
> I'm not totally convinced that sharing minor allocation is the right
> solution. ISTM that the main goal is for at least one serial driver
> to represent ttyS (assuming that it is a suitable work-alike to the
> 8250 driver) while other ttyS serial drivers must still be able to
> load and operate.

Unfortunately, we have already seen a case where we need to have
two drivers enabled at the same time that both use the ttyS
namespace and minor numbers at the moment.

> So for example, a UART that is ttyS in a typical configuration, might
> be ttyX when another driver has been configured as ttyS instead.
> Assuming that every uart driver has a unique, alternate base name,
> then the problem is reduced to selecting the appropriate driver for
> ttyS.
> 
> Some use cases would require selecting the ttyS driver at build time.
> Others would probably require the notion of a preferred driver.

Build-time selection looks problematic to me in the case of an
ARM distro kernel that wants to support dozens of different
uart drivers. At the moment this works for the most part because
the conflicts are rare, but it's getting worse.

> What got me thinking about this approach instead of sharing minor
> allocation was based on the 'first-come, first-served' question
> I asked back in Dec. What happens when probe order varies and
> console=ttyS0/login prompt doesn't show up where it's supposed to?
> I've had this experience with multiple ethernet adapters and it's
> not fun.

In a lot of cases, we can solve the problem for a particular board
by hardcoding the 'aliases' in the board specific dts file, at least
when devicetree is used (as it is for the majority of the cases that
are causing problems now).

Using the aliases correctly at the moment is really awkward when you
have multiple drivers for uarts on the same system and your serial0
and serial1 uart turn into ttyS0 and ttyX1.

	Arnd
Peter Hurley Jan. 9, 2015, 2:14 p.m. UTC | #13
On 01/09/2015 03:48 AM, Arnd Bergmann wrote:
> On Friday 09 January 2015 00:13:28 Peter Hurley wrote:
>> On 01/08/2015 05:05 PM, Arnd Bergmann wrote:
>>> On Thursday 08 January 2015 11:11:51 Peter Hurley wrote:
>>>>
>>>> This interface is just storage and minor allocation, since the
>>>> port-reuse behavior will be limited to the "universal" driver.
>>>> From a sub-driver perspective, the shared storage is actually
>>>> a hindrance, so that reduces the requirement to minor allocation.
>>>>
>>>> And that's where I'm stuck at the moment -- how to share ttyS
>>>> minor allocation. ttyS console is a related problem.
>>>
>>> One idea that has come up in the past but never saw an implementation
>>> is to make the ttyS namespace and minor numbers completely generic and
>>> let any serial port driver use it. This would be a major rework, but
>>> have the added advantage of cleaning up a number of other namespace
>>> issues as well. There also lots of open question, in particular how
>>> to maintain compatibility with existing drivers. One could imagine
>>> that each uart always gets a ttyS device and optionally also gets
>>> a device node for the same port with a driver specific chardev as
>>> most of them do today. Or it could be an either/or decision that is
>>> made at compile time or as a module parameter.
>>
>> I'm not totally convinced that sharing minor allocation is the right
>> solution. ISTM that the main goal is for at least one serial driver
>> to represent ttyS (assuming that it is a suitable work-alike to the
>> 8250 driver) while other ttyS serial drivers must still be able to
>> load and operate.
> 
> Unfortunately, we have already seen a case where we need to have
> two drivers enabled at the same time that both use the ttyS
> namespace and minor numbers at the moment.

Maybe you misunderstood me here. I'm suggesting that every ttyS-capable
driver would be able to load into a pre-determined, unique alternate
namespace. So at least one ttyS driver would be present, without naming
conflicts, and every other ttyS-conflicting driver would be loaded
in their respective alternate namespace.

Unless what you mean is there is some userspace requirement for the
ttyS namespace to be shared. If so, would you please expand on this use
case so I can better understand the requirements.

>> So for example, a UART that is ttyS in a typical configuration, might
>> be ttyX when another driver has been configured as ttyS instead.
>> Assuming that every uart driver has a unique, alternate base name,
>> then the problem is reduced to selecting the appropriate driver for
>> ttyS.
>>
>> Some use cases would require selecting the ttyS driver at build time.
>> Others would probably require the notion of a preferred driver.
> 
> Build-time selection looks problematic to me in the case of an
> ARM distro kernel that wants to support dozens of different
> uart drivers. At the moment this works for the most part because
> the conflicts are rare, but it's getting worse.

A dt property could indicate which uart is the preferred ttyS.


>> What got me thinking about this approach instead of sharing minor
>> allocation was based on the 'first-come, first-served' question
>> I asked back in Dec. What happens when probe order varies and
>> console=ttyS0/login prompt doesn't show up where it's supposed to?
>> I've had this experience with multiple ethernet adapters and it's
>> not fun.
> 
> In a lot of cases, we can solve the problem for a particular board
> by hardcoding the 'aliases' in the board specific dts file, at least
> when devicetree is used (as it is for the majority of the cases that
> are causing problems now).
> 
> Using the aliases correctly at the moment is really awkward when you
> have multiple drivers for uarts on the same system and your serial0
> and serial1 uart turn into ttyS0 and ttyX1.

I'm concerned that this will devolve into some attempt at a
persistent naming scheme from kernel-space; which has repeatedly been
shown in other subsystems to be fruitless. That was my point about
the ethernet adapters.

I have no problem sequentially numbering uarts as ttyS, but it would
suck if that amount of work ultimately proved useless, when udev rules
would have sufficed or became necessary anyway.

Regards,
Peter Hurley
diff mbox

Patch

diff --git a/drivers/tty/serial/8250/8250.h b/drivers/tty/serial/8250/8250.h
index b008368..bda82f7 100644
--- a/drivers/tty/serial/8250/8250.h
+++ b/drivers/tty/serial/8250/8250.h
@@ -96,6 +96,12 @@  struct serial8250_config {
 #define SERIAL8250_SHARE_IRQS 0
 #endif
 
+#ifdef CONFIG_SERIAL_8250_PHANTOM_UARTS
+#define SERIAL8250_PHANTOM_UARTS 1
+#else
+#define SERIAL8250_PHANTOM_UARTS 0
+#endif
+
 static inline int serial_in(struct uart_8250_port *up, int offset)
 {
 	return up->port.serial_in(&up->port, offset);
diff --git a/drivers/tty/serial/8250/8250_core.c b/drivers/tty/serial/8250/8250_core.c
index 3bfcfdb..1b27b2f 100644
--- a/drivers/tty/serial/8250/8250_core.c
+++ b/drivers/tty/serial/8250/8250_core.c
@@ -52,10 +52,13 @@ 
  * Configuration:
  *   share_irqs - whether we pass IRQF_SHARED to request_irq().  This option
  *                is unsafe when used on edge-triggered interrupts.
+ *   nr_uarts - max # of ports which can be registered
+ *   phantom_uarts - whether we pre-register "nr_uarts". Required for ISA ports
+ *		     and providing unprobed ports for userspace to configure.
  */
 static unsigned int share_irqs = SERIAL8250_SHARE_IRQS;
-
 static unsigned int nr_uarts = CONFIG_SERIAL_8250_RUNTIME_UARTS;
+static unsigned int phantom_uarts = SERIAL8250_PHANTOM_UARTS;
 
 static struct uart_driver serial8250_reg;
 
@@ -3155,6 +3158,9 @@  serial8250_register_ports(struct uart_driver *drv, struct device *dev)
 {
 	int i;
 
+	if (!phantom_uarts)
+		return;
+
 	for (i = 0; i < nr_uarts; i++) {
 		struct uart_8250_port *up = &serial8250_ports[i];
 
@@ -3662,7 +3668,7 @@  void serial8250_unregister_port(int line)
 
 	mutex_lock(&serial_mutex);
 	uart_remove_one_port(&serial8250_reg, &uart->port);
-	if (serial8250_isa_devs) {
+	if (serial8250_isa_devs && phantom_uarts) {
 		uart->port.flags &= ~UPF_BOOT_AUTOCONF;
 		uart->port.type = PORT_UNKNOWN;
 		uart->port.dev = &serial8250_isa_devs->dev;
@@ -3769,6 +3775,9 @@  MODULE_PARM_DESC(share_irqs, "Share IRQs with other non-8250/16x50 devices"
 module_param(nr_uarts, uint, 0644);
 MODULE_PARM_DESC(nr_uarts, "Maximum number of UARTs supported. (1-" __MODULE_STRING(CONFIG_SERIAL_8250_NR_UARTS) ")");
 
+module_param(phantom_uarts, uint, 0644);
+MODULE_PARM_DESC(phantom_uarts, "Enable UARTs with no hardware");
+
 module_param(skip_txen_test, uint, 0644);
 MODULE_PARM_DESC(skip_txen_test, "Skip checking for the TXEN bug at init time");
 
diff --git a/drivers/tty/serial/8250/Kconfig b/drivers/tty/serial/8250/Kconfig
index 0fcbcd2..87e649b 100644
--- a/drivers/tty/serial/8250/Kconfig
+++ b/drivers/tty/serial/8250/Kconfig
@@ -145,15 +145,29 @@  config SERIAL_8250_NR_UARTS
 	  via hot-plug, or any ISA multi-port serial cards.
 
 config SERIAL_8250_RUNTIME_UARTS
-	int "Number of 8250/16550 serial ports to register at runtime"
+	int "Maximum number of 8250/16550 serial ports to enable"
 	depends on SERIAL_8250
 	range 0 SERIAL_8250_NR_UARTS
 	default "4"
 	help
-	  Set this to the maximum number of serial ports you want
-	  the kernel to register at boot time.  This can be overridden
-	  with the module parameter "nr_uarts", or boot-time parameter
-	  8250.nr_uarts
+	  Set this to the maximum number of 8250/16550 serial ports you
+	  want the kernel to allow. This can be overridden with the module
+	  parameter "nr_uarts", or boot-time parameter, 8250.nr_uarts,
+	  up to the maximum.
+
+config SERIAL_8250_PHANTOM_UARTS
+	bool
+	prompt "Enable phantom 8250/16550 serial ports" if !ISA
+	depends on SERIAL_8250
+	default y
+	---help---
+	  Say Y here to create all 8250/16550 serial ports at module load time.
+	  Saying N here prevents the creation of unprobed ports, but also
+	  disables userspace probe/configure (since no device node is created
+	  for userspace to open). This can be overridden with the module
+	  parameter "phantom_uarts", or boot-time parameter, 8250.phantom_uarts
+
+	  If unsure, say Y.
 
 config SERIAL_8250_EXTENDED
 	bool "Extended 8250/16550 serial driver options"