* [PATCH] serial: 8250: add option to disable registration of legacy ISA ports
@ 2021-01-28 17:22 Mans Rullgard
[not found] ` <CAHp75VenDC0R3uX4_=Yzii-q5Z-YWcfT2_OO0yJkYehYAHDCew@mail.gmail.com>
[not found] ` <YBam2m2VMowH5Yth@kroah.com>
0 siblings, 2 replies; 6+ messages in thread
From: Mans Rullgard @ 2021-01-28 17:22 UTC (permalink / raw)
To: Greg Kroah-Hartman; +Cc: Jiri Slaby, linux-serial, linux-kernel
On systems that do not have the traditional PC ISA serial ports, the
8250 driver still creates non-functional device nodes. This change
makes only ports that actually exist (PCI, DT, ...) get device nodes.
Signed-off-by: Mans Rullgard <mans@mansr.com>
---
drivers/tty/serial/8250/8250_core.c | 26 ++++++++++++++++++++------
drivers/tty/serial/8250/Kconfig | 5 +++++
2 files changed, 25 insertions(+), 6 deletions(-)
diff --git a/drivers/tty/serial/8250/8250_core.c b/drivers/tty/serial/8250/8250_core.c
index cae61d1ebec5..49695dd3677c 100644
--- a/drivers/tty/serial/8250/8250_core.c
+++ b/drivers/tty/serial/8250/8250_core.c
@@ -555,6 +555,7 @@ static void __init serial8250_isa_init_ports(void)
}
}
+#ifdef CONFIG_SERIAL_8250_ISA
static void __init
serial8250_register_ports(struct uart_driver *drv, struct device *dev)
{
@@ -575,6 +576,7 @@ serial8250_register_ports(struct uart_driver *drv, struct device *dev)
uart_add_one_port(drv, &up->port);
}
}
+#endif
#ifdef CONFIG_SERIAL_8250_CONSOLE
@@ -797,6 +799,7 @@ void serial8250_resume_port(int line)
}
EXPORT_SYMBOL(serial8250_resume_port);
+#ifdef CONFIG_SERIAL_8250_ISA
/*
* Register a set of serial devices attached to a platform device. The
* list is terminated with a zero flags entry, which means we expect
@@ -907,6 +910,7 @@ static struct platform_driver serial8250_isa_driver = {
* in the table in include/asm/serial.h
*/
static struct platform_device *serial8250_isa_devs;
+#endif
/*
* serial8250_register_8250_port and serial8250_unregister_port allows for
@@ -1149,6 +1153,8 @@ void serial8250_unregister_port(int line)
}
uart_remove_one_port(&serial8250_reg, &uart->port);
+ uart->port.dev = NULL;
+#ifdef CONFIG_SERIAL_8250_ISA
if (serial8250_isa_devs) {
uart->port.flags &= ~UPF_BOOT_AUTOCONF;
uart->port.type = PORT_UNKNOWN;
@@ -1156,9 +1162,8 @@ void serial8250_unregister_port(int line)
uart->capabilities = 0;
serial8250_apply_quirks(uart);
uart_add_one_port(&serial8250_reg, &uart->port);
- } else {
- uart->port.dev = NULL;
}
+#endif
mutex_unlock(&serial_mutex);
}
EXPORT_SYMBOL(serial8250_unregister_port);
@@ -1188,6 +1193,7 @@ static int __init serial8250_init(void)
if (ret)
goto unreg_uart_drv;
+#ifdef CONFIG_SERIAL_8250_ISA
serial8250_isa_devs = platform_device_alloc("serial8250",
PLAT8250_DEV_LEGACY);
if (!serial8250_isa_devs) {
@@ -1202,26 +1208,33 @@ static int __init serial8250_init(void)
serial8250_register_ports(&serial8250_reg, &serial8250_isa_devs->dev);
ret = platform_driver_register(&serial8250_isa_driver);
- if (ret == 0)
- goto out;
+ if (ret)
+ goto del_dev;
+#endif
+out:
+ return ret;
+
+#ifdef CONFIG_SERIAL_8250_ISA
+del_dev:
platform_device_del(serial8250_isa_devs);
put_dev:
platform_device_put(serial8250_isa_devs);
unreg_pnp:
serial8250_pnp_exit();
+#endif
unreg_uart_drv:
#ifdef CONFIG_SPARC
sunserial_unregister_minors(&serial8250_reg, UART_NR);
#else
uart_unregister_driver(&serial8250_reg);
#endif
-out:
- return ret;
+ goto out;
}
static void __exit serial8250_exit(void)
{
+#ifdef CONFIG_SERIAL_8250_ISA
struct platform_device *isa_dev = serial8250_isa_devs;
/*
@@ -1233,6 +1246,7 @@ static void __exit serial8250_exit(void)
platform_driver_unregister(&serial8250_isa_driver);
platform_device_unregister(isa_dev);
+#endif
serial8250_pnp_exit();
diff --git a/drivers/tty/serial/8250/Kconfig b/drivers/tty/serial/8250/Kconfig
index 603137da4736..683f81675a77 100644
--- a/drivers/tty/serial/8250/Kconfig
+++ b/drivers/tty/serial/8250/Kconfig
@@ -52,6 +52,11 @@ config SERIAL_8250_DEPRECATED_OPTIONS
If you did not notice yet and/or you have userspace from pre-3.7, it
is safe (and recommended) to say N here.
+config SERIAL_8250_ISA
+ bool "8250/16550 ISA device support" if EXPERT
+ depends on SERIAL_8250
+ default y
+
config SERIAL_8250_PNP
bool "8250/16550 PNP device support" if EXPERT
depends on SERIAL_8250 && PNP
--
2.30.0
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] serial: 8250: add option to disable registration of legacy ISA ports
[not found] ` <YBa0J82FrD6mdP/v@kroah.com>
@ 2021-01-31 15:47 ` Måns Rullgård
2021-01-31 15:53 ` Greg Kroah-Hartman
0 siblings, 1 reply; 6+ messages in thread
From: Måns Rullgård @ 2021-01-31 15:47 UTC (permalink / raw)
To: Greg Kroah-Hartman; +Cc: Jiri Slaby, linux-serial, linux-kernel
Greg Kroah-Hartman <gregkh@linuxfoundation.org> writes:
> On Sun, Jan 31, 2021 at 01:18:47PM +0000, Måns Rullgård wrote:
>> Greg Kroah-Hartman <gregkh@linuxfoundation.org> writes:
>>
>> > On Thu, Jan 28, 2021 at 05:22:44PM +0000, Mans Rullgard wrote:
>> >> On systems that do not have the traditional PC ISA serial ports, the
>> >> 8250 driver still creates non-functional device nodes. This change
>> >> makes only ports that actually exist (PCI, DT, ...) get device nodes.
>> >>
>> >> Signed-off-by: Mans Rullgard <mans@mansr.com>
>> >> ---
>> >> drivers/tty/serial/8250/8250_core.c | 26 ++++++++++++++++++++------
>> >> drivers/tty/serial/8250/Kconfig | 5 +++++
>> >> 2 files changed, 25 insertions(+), 6 deletions(-)
>> >>
>> >> diff --git a/drivers/tty/serial/8250/8250_core.c b/drivers/tty/serial/8250/8250_core.c
>> >> index cae61d1ebec5..49695dd3677c 100644
>> >> --- a/drivers/tty/serial/8250/8250_core.c
>> >> +++ b/drivers/tty/serial/8250/8250_core.c
>> >> @@ -555,6 +555,7 @@ static void __init serial8250_isa_init_ports(void)
>> >> }
>> >> }
>> >>
>> >> +#ifdef CONFIG_SERIAL_8250_ISA
>> >
>> > This is just making a mess of the code.
>>
>> It was already a mess.
>
> True, but don't make it a worse one please.
>
>>
>> > To do this right, pull the isa code out into a separate file and put
>> > the #ifdef in a .h file, so we can properly maintain and support this
>> > code over time. This change as-is is not going to make that any
>> > easier :(
>>
>> I might put in that effort if there's a reasonable chance this change
>> will be accepted. If it's going to be rejected regardless, I'd rather
>> not waste my time.
>>
>> >> +config SERIAL_8250_ISA
>> >> + bool "8250/16550 ISA device support" if EXPERT
>> >
>> > So, no one will set this?
>>
>> I followed the pattern of the existing SERIAL_8250_PNP option. Was that
>> a mistake? How would you prefer it?
>
> I don't know, I'm just asking.
>
>> > What userspace visable change will be caused by this?
>>
>> There won't be /dev/ttyS devices for ports that don't exist.
>>
>> > Will ports get renumbered?
>>
>> Not if they had predictable numbers to begin with.
>
> So that would be "yes"? If so, obviously we couldn't take this, right?
On a Beaglebone Black based system with some of the UARTs enabled, those
keep their numbers such that e.g. ttyS0, ttyS1, and ttyS4 show up in
/dev while ttyS2 and ttyS3 do not since they don't correspond to any
(enabled) ports.
If any of the very many variants of this driver do not assign fixed
numbers, those would possibly be renumbered. Should that be the case,
the numbering was never guaranteed to begin with, so I fail to see any
problem.
--
Måns Rullgård
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] serial: 8250: add option to disable registration of legacy ISA ports
2021-01-31 15:47 ` Måns Rullgård
@ 2021-01-31 15:53 ` Greg Kroah-Hartman
0 siblings, 0 replies; 6+ messages in thread
From: Greg Kroah-Hartman @ 2021-01-31 15:53 UTC (permalink / raw)
To: Måns Rullgård; +Cc: Jiri Slaby, linux-serial, linux-kernel
On Sun, Jan 31, 2021 at 03:47:42PM +0000, Måns Rullgård wrote:
> Greg Kroah-Hartman <gregkh@linuxfoundation.org> writes:
>
> > On Sun, Jan 31, 2021 at 01:18:47PM +0000, Måns Rullgård wrote:
> >> Greg Kroah-Hartman <gregkh@linuxfoundation.org> writes:
> >>
> >> > On Thu, Jan 28, 2021 at 05:22:44PM +0000, Mans Rullgard wrote:
> >> >> On systems that do not have the traditional PC ISA serial ports, the
> >> >> 8250 driver still creates non-functional device nodes. This change
> >> >> makes only ports that actually exist (PCI, DT, ...) get device nodes.
> >> >>
> >> >> Signed-off-by: Mans Rullgard <mans@mansr.com>
> >> >> ---
> >> >> drivers/tty/serial/8250/8250_core.c | 26 ++++++++++++++++++++------
> >> >> drivers/tty/serial/8250/Kconfig | 5 +++++
> >> >> 2 files changed, 25 insertions(+), 6 deletions(-)
> >> >>
> >> >> diff --git a/drivers/tty/serial/8250/8250_core.c b/drivers/tty/serial/8250/8250_core.c
> >> >> index cae61d1ebec5..49695dd3677c 100644
> >> >> --- a/drivers/tty/serial/8250/8250_core.c
> >> >> +++ b/drivers/tty/serial/8250/8250_core.c
> >> >> @@ -555,6 +555,7 @@ static void __init serial8250_isa_init_ports(void)
> >> >> }
> >> >> }
> >> >>
> >> >> +#ifdef CONFIG_SERIAL_8250_ISA
> >> >
> >> > This is just making a mess of the code.
> >>
> >> It was already a mess.
> >
> > True, but don't make it a worse one please.
> >
> >>
> >> > To do this right, pull the isa code out into a separate file and put
> >> > the #ifdef in a .h file, so we can properly maintain and support this
> >> > code over time. This change as-is is not going to make that any
> >> > easier :(
> >>
> >> I might put in that effort if there's a reasonable chance this change
> >> will be accepted. If it's going to be rejected regardless, I'd rather
> >> not waste my time.
> >>
> >> >> +config SERIAL_8250_ISA
> >> >> + bool "8250/16550 ISA device support" if EXPERT
> >> >
> >> > So, no one will set this?
> >>
> >> I followed the pattern of the existing SERIAL_8250_PNP option. Was that
> >> a mistake? How would you prefer it?
> >
> > I don't know, I'm just asking.
> >
> >> > What userspace visable change will be caused by this?
> >>
> >> There won't be /dev/ttyS devices for ports that don't exist.
> >>
> >> > Will ports get renumbered?
> >>
> >> Not if they had predictable numbers to begin with.
> >
> > So that would be "yes"? If so, obviously we couldn't take this, right?
>
> On a Beaglebone Black based system with some of the UARTs enabled, those
> keep their numbers such that e.g. ttyS0, ttyS1, and ttyS4 show up in
> /dev while ttyS2 and ttyS3 do not since they don't correspond to any
> (enabled) ports.
>
> If any of the very many variants of this driver do not assign fixed
> numbers, those would possibly be renumbered. Should that be the case,
> the numbering was never guaranteed to begin with, so I fail to see any
> problem.
Ok, if that's the case, then yes, a cleaned up version of this patch
would be nice, thanks!
greg k-h
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] serial: 8250: add option to disable registration of legacy ISA ports
[not found] ` <CAHp75VenDC0R3uX4_=Yzii-q5Z-YWcfT2_OO0yJkYehYAHDCew@mail.gmail.com>
@ 2021-01-31 19:47 ` Måns Rullgård
2021-02-01 7:38 ` Greg Kroah-Hartman
1 sibling, 0 replies; 6+ messages in thread
From: Måns Rullgård @ 2021-01-31 19:47 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Greg Kroah-Hartman, Jiri Slaby, linux-serial, linux-kernel
Andy Shevchenko <andy.shevchenko@gmail.com> writes:
> On Thursday, January 28, 2021, Mans Rullgard <mans@mansr.com> wrote:
>
>> On systems that do not have the traditional PC ISA serial ports, the
>> 8250 driver still creates non-functional device nodes. This change
>> makes only ports that actually exist (PCI, DT, ...) get device nodes.
>>
>>
>
> This is kinda ABI breakage. At least this will break x86 platforms with
> HSUARTs (all modern ones) that are used in embedded systems.
>
> I think you would rather need an option to disable this and select it by
> the platforms where it is known not to break anything.
What exactly breaks? The new option is enabled by default, just like
the one right beside it (SERIAL_8250_PNP), so nothing at all changes
unless this is actively disabled. On a system that doesn't have those
ports, any attempt to access the device nodes produces some kind of
error. How is it breaking anything to not create device nodes for
hardware that doesn't exist?
--
Måns Rullgård
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] serial: 8250: add option to disable registration of legacy ISA ports
[not found] ` <CAHp75VenDC0R3uX4_=Yzii-q5Z-YWcfT2_OO0yJkYehYAHDCew@mail.gmail.com>
2021-01-31 19:47 ` Måns Rullgård
@ 2021-02-01 7:38 ` Greg Kroah-Hartman
1 sibling, 0 replies; 6+ messages in thread
From: Greg Kroah-Hartman @ 2021-02-01 7:38 UTC (permalink / raw)
To: Andy Shevchenko; +Cc: Mans Rullgard, Jiri Slaby, linux-serial, linux-kernel
On Sun, Jan 31, 2021 at 09:25:33PM +0200, Andy Shevchenko wrote:
> On Thursday, January 28, 2021, Mans Rullgard <mans@mansr.com> wrote:
>
> > On systems that do not have the traditional PC ISA serial ports, the
> > 8250 driver still creates non-functional device nodes. This change
> > makes only ports that actually exist (PCI, DT, ...) get device nodes.
> >
> >
>
> This is kinda ABI breakage. At least this will break x86 platforms with
> HSUARTs (all modern ones) that are used in embedded systems.
>
> I think you would rather need an option to disable this and select it by
> the platforms where it is known not to break anything.
This option is behind CONFIG_EXPERT, if you set it, you know what you
are doing, so that's fine.
thanks,
greg k-h
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] serial: 8250: add option to disable registration of legacy ISA ports
[not found] ` <yw1xwnvtcki0.fsf@mansr.com>
[not found] ` <YBa0J82FrD6mdP/v@kroah.com>
@ 2021-02-18 10:49 ` Maarten Brock
1 sibling, 0 replies; 6+ messages in thread
From: Maarten Brock @ 2021-02-18 10:49 UTC (permalink / raw)
To: Måns Rullgård
Cc: Greg Kroah-Hartman, Jiri Slaby, linux-serial, linux-kernel
On 2021-01-31 14:18, Måns Rullgård wrote:
>> What userspace visable change will be caused by this?
>
> There won't be /dev/ttyS devices for ports that don't exist.
Oh yes, please!
That would mean I can create ttyS2 and upward while keeping ttyPS0 and
ttyPS1 (which invaded the serial<N> alias namespace) at the lower
numbers.
>> Will ports get renumbered?
>
> Not if they had predictable numbers to begin with.
It is hard to make predictable numbers with the backward operating
serial<N> aliases in the device tree. It makes relations in the wrong
direction.
If a system has two ttyPS<N> uarts (xilinx_uartps) and needs them at
ttyPS0 and ttyPS1 (or at least <=ttyPS9, due to another bug in
start_getty) and 10 ttyS<N> (8250) you can configure the kernel for 10
8250 devices and give 8 of them the predictable ttyS2 to ttyS9. The last
two will get the remaining ttyS0 and ttyS1. You cannot assign them their
number, because the serial0 and serial1 alias are required for the
ttyPS0 and ttyPS1.
However with this option it would be possible to configure for 12 8250
devices and not use nor see ttyS0 and ttyS1.
The best option would of course be to not even instantiate kernel
drivers for non-existing devices.
Maarten
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-02-18 13:53 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-01-28 17:22 [PATCH] serial: 8250: add option to disable registration of legacy ISA ports Mans Rullgard
[not found] ` <CAHp75VenDC0R3uX4_=Yzii-q5Z-YWcfT2_OO0yJkYehYAHDCew@mail.gmail.com>
2021-01-31 19:47 ` Måns Rullgård
2021-02-01 7:38 ` Greg Kroah-Hartman
[not found] ` <YBam2m2VMowH5Yth@kroah.com>
[not found] ` <yw1xwnvtcki0.fsf@mansr.com>
[not found] ` <YBa0J82FrD6mdP/v@kroah.com>
2021-01-31 15:47 ` Måns Rullgård
2021-01-31 15:53 ` Greg Kroah-Hartman
2021-02-18 10:49 ` Maarten Brock
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).